Fix the issue when persistent DVS is used to run pytest which has number of front-panel ports < 32 by abdosi · Pull Request #1373 · sonic-net/sonic-swss

abdosi · 2020-07-31T19:19:49Z

Why/What I did
Fix the issue where persistent DVS is used to run pytest
and ports are created dynamically (ref pr#
sonic-net/sonic-buildimage#4499). There were two
issues:

a) since number of dynamic front port can be < 32 test case fails
   as it expect always 32. Make sure to update persistent DVS to always have
   32 ports/server link as part of test run if forcedvs option provided else abort test case 
   and save the current config db

b) after test is done persistent DVS need to be moved to original state.
   Make sure to remove extra port/server link and restore back config db

How I verified it

a) Verified Persistent DVS < 32 ports without forcedvs option then abort else DVS restarted with 32 ports.
b) Manually Verified VS Docker has correct interface config/port.ini/ip link after test is completed and restored original config db
c) Verified on host ip netns list has expected server link only after is completed

and ports are created dynamically (ref pr: sonic-net/sonic-buildimage#4499). There were two issues: a) since number of dynamic front port can be < 32 test case fails as it expect always 32. Make sure to udpate persitent DVS to always have 32 ports/server link as part of test run and save the current config db b) after test is done persistent DVS need to be moved to original state. Make dure to remove extra port/server link and restore back config db Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

daall · 2020-08-03T14:34:45Z

since number of dynamic front port can be < 32 test case fails as it expect always 32

Which tests are always expecting 32 ports? I am OK with this change as a short-term fix, but I think the real issue here is that the tests are hard-coded for 32 ports instead of checking in the Config DB. We should open an issue and track this.

tests/conftest.py

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

abdosi · 2020-08-03T15:56:51Z

since number of dynamic front port can be < 32 test case fails as it expect always 32

Which tests are always expecting 32 ports? I am OK with this change as a short-term fix, but I think the real issue here is that the tests are hard-coded for 32 ports instead of checking in the Config DB. We should open an issue and track this.

@daall . Many test case are using hard-coded server indexing

For eg: test_fdb_update.py we use dvs.servers[17].runcmd
test_route.py we use dvs.servers[3].runcmd

daall · 2020-08-03T20:24:13Z

since number of dynamic front port can be < 32 test case fails as it expect always 32

Which tests are always expecting 32 ports? I am OK with this change as a short-term fix, but I think the real issue here is that the tests are hard-coded for 32 ports instead of checking in the Config DB. We should open an issue and track this.

@daall . Many test case are using hard-coded server indexing

For eg: test_fdb_update.py we use dvs.servers[17].runcmd
test_route.py we use dvs.servers[3].runcmd

ack, I will open an issue to track this. Thanks for pointing this out!

daall

lgtm, please wait for @lguohan's review

abdosi · 2020-08-03T21:07:56Z

retest this please

abdosi · 2020-08-04T16:26:08Z

retest this please

abdosi · 2020-08-05T15:02:56Z

retest this please

abdosi · 2020-08-06T23:32:26Z

lgtm, please wait for @lguohan's review

@lguohan Can you please review this

abdosi · 2020-08-07T19:20:31Z

retest this please

abdosi · 2020-08-07T23:02:31Z

retest this please

abdosi · 2020-08-08T01:18:31Z

@lguohan can you review this.

abdosi · 2020-08-08T01:55:25Z

retest this please

abdosi · 2020-08-09T17:31:31Z

retest this please

abdosi · 2020-08-11T16:01:36Z

lgtm, please wait for @lguohan's review

@lguohan Can you please review this

@lguohan is it ok to merge ?

lguohan · 2020-08-11T16:24:43Z

maybe we can just check if persistent dvs has 32 ports, if not, then we just abort and say persistent dvs has to be 32 ports instead of reconfiguration here?

abdosi · 2020-08-11T21:41:07Z

maybe we can just check if persistent dvs has 32 ports, if not, then we just abort and say persistent dvs has to be 32 ports instead of reconfiguration here?

@lguohan i think it is better to keep using existing persistent dvs and we can run pytest on top of that
instead to abort it especially for local development/debugging which can have local swss debian package.

lguohan · 2020-08-12T22:18:08Z

@abdosi , can you provide that as an option?

@lguohan Updated with --forcedvs option . Default being abort if ports < 32
@daall

Updated Readme Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

lgtm-com · 2020-08-14T01:52:54Z

This pull request introduces 1 alert and fixes 12 when merging d821218 into 879f45b - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

4 for First parameter of a method is not named 'self'
4 for Unused local variable
2 for Variable defined multiple times
1 for Testing equality to None
1 for Unnecessary 'else' clause in loop

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

lgtm-com · 2020-08-14T02:07:51Z

This pull request introduces 1 alert and fixes 12 when merging 6629b67 into 879f45b - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

4 for First parameter of a method is not named 'self'
4 for Unused local variable
2 for Variable defined multiple times
1 for Testing equality to None
1 for Unnecessary 'else' clause in loop

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

tests/conftest.py

Co-authored-by: Danny Allen <daall@microsoft.com>

daall · 2020-08-17T23:45:50Z

retest vs please

…ber of front-panel ports < 32 (sonic-net#1373) * Fix the issue where persistent DVS is used to run pytest and ports are created dynamically (ref pr: sonic-net/sonic-buildimage#4499). There were two issues: a) since number of dynamic front port can be < 32 test case fails as it expect always 32. Make sure to udpate persitent DVS to always have 32 ports/server link as part of test run and save the current config db b) after test is done persistent DVS need to be moved to original state. Make dure to remove extra port/server link and restore back config db Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Review Comments fix Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Added option for force persitent dvs if port < 32 Updated Readme Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Fix LGTM Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Fix LGTM and compile error Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Update tests/conftest.py Co-authored-by: Danny Allen <daall@microsoft.com> Co-authored-by: Danny Allen <daall@microsoft.com>

abdosi requested review from daall and lguohan July 31, 2020 19:19

Merge branch 'master' into dvs_test

541bffb

daall reviewed Aug 3, 2020

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

tests/conftest.py Show resolved Hide resolved

Review Comments fix

c9f34aa

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

daall previously approved these changes Aug 3, 2020

View reviewed changes

daall added the DVS ⭐ label Aug 3, 2020

daall mentioned this pull request Aug 6, 2020

Many tests are hard-coded to assume 32 ports #1384

Open

abdosi added 2 commits August 13, 2020 18:37

Merge remote-tracking branch 'upstream/master' into dvs_test

ea304d6

Added option for force persitent dvs if port < 32

d821218

Updated Readme Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

abdosi dismissed daall’s stale review via d821218 August 14, 2020 01:46

abdosi force-pushed the dvs_test branch from c48f43f to d821218 Compare August 14, 2020 01:46

Fix LGTM

6629b67

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

Fix LGTM and compile error

70c86df

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

daall mentioned this pull request Aug 17, 2020

[sonic-swsss] Fix the issue of field "next_hop_ip" not getting updated in state DB in ERSPAN Mirror #1375

Merged

daall reviewed Aug 17, 2020

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

Update tests/conftest.py

5bfe5a2

Co-authored-by: Danny Allen <daall@microsoft.com>

daall approved these changes Aug 17, 2020

View reviewed changes

abdosi merged commit 11da264 into sonic-net:master Aug 18, 2020

abdosi deleted the dvs_test branch August 18, 2020 14:58

Conversation

abdosi commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daall commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abdosi commented Aug 3, 2020

Uh oh!

daall commented Aug 3, 2020

Uh oh!

daall left a comment

Choose a reason for hiding this comment

Uh oh!

abdosi commented Aug 3, 2020

Uh oh!

abdosi commented Aug 4, 2020

Uh oh!

abdosi commented Aug 5, 2020

Uh oh!

abdosi commented Aug 6, 2020

Uh oh!

abdosi commented Aug 7, 2020

Uh oh!

abdosi commented Aug 7, 2020

Uh oh!

abdosi commented Aug 8, 2020

Uh oh!

abdosi commented Aug 8, 2020

Uh oh!

abdosi commented Aug 9, 2020

Uh oh!

abdosi commented Aug 11, 2020

Uh oh!

lguohan commented Aug 11, 2020

Uh oh!

abdosi commented Aug 11, 2020

Uh oh!

lguohan commented Aug 12, 2020 • edited by abdosi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Aug 14, 2020

Uh oh!

lgtm-com bot commented Aug 14, 2020

Uh oh!

Uh oh!

daall commented Aug 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abdosi commented Jul 31, 2020 •

edited

Loading

daall commented Aug 3, 2020 •

edited

Loading

lguohan commented Aug 12, 2020 •

edited by abdosi

Loading