[inner hahsing] extension of PBH tests#4848
Conversation
|
@anish-n please a look |
|
@AntonHryshchuk please update the test as discussed |
|
@anish-n just a kind reminder |
There was a problem hiding this comment.
Shouldn't nvgre_tni be self.gre_key itself if it is defined and not a hard coded value?
There was a problem hiding this comment.
changed to use the nvgre_tni parameter instead
tests/ecmp/inner_hashing/conftest.py
Outdated
There was a problem hiding this comment.
Does this logic assume that a port is either a part of a portchannel or a vlan? What if its part of neither, will the test break?
There was a problem hiding this comment.
The test supports t0 topology, there exist the ports in VLAN.
Here, created the map, with all ports in VLAN and newly portchannels: {PortChannel10: port1, PortChannel11: port2..... }
Later, according to this map, will be removed VLAN from the port and added this port to the newly created portchannel.
Not related to already existing portchannels and on t0 topo should be the ports in VLAN.
There was a problem hiding this comment.
Given we do config save here, and we modify device config in the test is there a possibility that we overwrite the base config with something different which will be persisted beyond the test?
There was a problem hiding this comment.
Right. I missed it. Updated.
For "regular" WR test added fixture with "config reload" in a teardown.
For LAG WR tests, it's a part of a fixture "setup_acl_config" teardown.
There was a problem hiding this comment.
I don't get why we need to do a config save, since warm reboot gets the current running config and does not really load config from persisted config we shouldn't need to do a config save at all as far as I see.
There was a problem hiding this comment.
removed the saving of configurations before WR and save configurations as a teardown
|
This pull request introduces 2 alerts when merging a08eb0140f3107453300fc48dbd83fa01648d2b5 into 23a2a96 - view on LGTM.com new alerts:
|
|
@AntonHryshchuk please fix new alerts so we can proceed with the approval flow |
|
@anish-n can you please signoff? |
|
unused imports removed |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Anton <antonh@nvidia.com>
Signed-off-by: Anton <antonh@nvidia.com>
Signed-off-by: Anton <antonh@nvidia.com>
Signed-off-by: Anton <antonh@nvidia.com>
AntonHryshchuk
force-pushed
the
pbh_ports_extension
branch
from
fa2f7a4 to
1b2469c
Compare
|
@anish-n kindly reminder to review the PR changes. If no comments lets merge and add to nightly regression. |
|
@anish-n please help to review & merge as we would like to have PBH edit flows PR on top of these changes |
|
@AntonHryshchuk, this change is causing the issue #5280. |
This reverts commit 9008381. Reverts #4848 PR #4848 depends on pytest-allure package which is only available to user “AzDevOps” in sonic-mgmt docker. The reason is that the package is installed to local path of that user: AzDevOps@2fb15ed04ed5:~$ python Python 2.7.17 (default, Feb 27 2021, 15:10:58) [GCC 7.5.0] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import allure >>> allure.__file__ '/var/AzDevOps/.local/lib/python2.7/site-packages/allure.pyc' AzDevOps@2fb15ed04ed5:~$ sudo su root@2fb15ed04ed5:/var/AzDevOps# pip list | grep allure DEPRECATION: The default format will switch to columns in the future. You can use --format=(legacy|columns) (or define a format=(legacy|columns) in your pip.conf under the [list] section) to disable this warning. root@2fb15ed04ed5:/var/AzDevOps# python Python 2.7.17 (default, Feb 27 2021, 15:10:58) [GCC 7.5.0] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import allure Traceback (most recent call last): File "<stdin>", line 1, in <module> ImportError: No module named allure >>> exit() If use “root” user to run the test, issue #5280 will be hit. I’ll revert #4848 for now. The sonic-mgmt docker file https://github.com/Azure/sonic-buildimage/blob/master/dockers/docker-sonic-mgmt/Dockerfile.j2 needs to be updated to ensure that package pytest-allure is installed to global directory.
…" (sonic-net#5282)" This reverts commit 3244df1.
6 participants
Signed-off-by: Anton antonh@nvidia.com
Description of PR
Summary:
Extension of PBH tests.
Added coverage of multiple interface types(LAG) and all possible match fields in pbh_rule.
added 2 new tests: test_inner_hashing_lag.py, test_wr_inner_hashing_lag.py
Type of change
Back port request
Approach
What is the motivation for this PR?
Full coverage of the feature
How did you do it?
How did you verify/test it?
All tests executed on a testbed with t0 topo
Supported testbed topology if it's a new test case?
t0