GCU cacl test combination #4835
Conversation
… topo so we can modify directly
|
This pull request introduces 1 alert when merging 3137427 into e95452b - view on LGTM.com new alerts:
|
|
Please fix the LGTM alert. |
|
|
||
| pytestmark = [ | ||
| pytest.mark.topology('any'), | ||
| pytest.mark.topology('t0'), |
There was a problem hiding this comment.
My previous test is to remove all ACL_TABLE and ACL_RULE settings, then test based on that. So it could be applied to any topo.
You mentioned we better test the scenario more practically. So I think it is more applicable that users just modify the change based on one of the topo.
That's why I change to t0 and just make changes above that.
Besides, rollback verification is easier if we have known the topo.
There was a problem hiding this comment.
I see. Then I feel there is also practical test case for t1. Let's discuss and implement in another PR.
| def get_cacl_tables(duthost): | ||
| """Get acl control palne tables | ||
| """ | ||
| cmds = "show acl table | grep -w CTRLPLANE | awk {'print $1'} || true" |
There was a problem hiding this comment.
Removed true to ignore errors.
Have some test on below scenario, it will not report an error because return code is still 0.
The only difference matters is whether the last command succeed or not.
admin@vlab-01:~$ showwwww acl table | grep -w CTRLPLANE | awk {'print $1'}
-bash: showwwww: command not found
admin@vlab-01:~$ echo $?
0
admin@vlab-01:~$ show acl table | greeeeep -w CTRLPLANE | awk {'print $1'}
-bash: greeeeep: command not found
admin@vlab-01:~$ echo $?
0
admin@vlab-01:~$ show acl table | grep -w CTRLPLANE | aaaaawk {'print $1'}
-bash: aaaaawk: command not found
admin@vlab-01:~$ echo $?
127
Also tested in sonic-mgmt, the return code will not be 0 only when the last pipe command fails. So it will not report error even first several commands fail.
{'stderr_lines': [u'/bin/sh: 1: shows: not found'], u'cmd': u"shows acl table | grep -w CTRLPLANE | awk {'print $1'}", u'end': u'2021-12-24 04:23:40.000951', '_ansible_no_log': False, u'stdout': u'', u'changed': True, u'rc': 0, u'start': u'2021-12-24 04:23:39.995670', u'stderr': u'/bin/sh: 1: shows: not found', u'delta': u'0:00:00.005281', u'invocation': {u'module_args': {u'creates': None, u'executable': None, u'_uses_shell': True, u'strip_empty_ends': True, u'_raw_params': u"shows acl table | grep -w CTRLPLANE | awk {'print $1'}", u'removes': None, u'argv': None, u'warn': True, u'chdir': None, u'stdin_add_newline': True, u'stdin': None}}, 'stdout_lines': [], 'failed': False}
Maybe the shell command should report an error based on non-empty stderr instead of non-zero rc ?
There was a problem hiding this comment.
Thanks for the experiment details! LGTM now.
If you would like to investigate further, you may check set -e and set -o pipefail, ref: https://stackoverflow.com/a/4959616/2514803.
Or consider use python to invoke individual commands, check return codes, and chain their stdout/stdin together. This is difficult since it is invoked on duthost.
…ndependently (sonic-net#4835) Summary: Make cacl test run independently What is the motivation for this PR? To avoid testing on one test case that is dependent on the previous test. How did you do it? Setup prerequisite environment for each test and rollback to the original setup How did you verify/test it? Run test of sonic-mgmt/tests/generic_config_updater/test_cacl.py on KVM
Description of PR
Summary: Combine GCU cacl test into one due to dependency among them
Fixes # (issue)
Type of change
Back port request
Approach
What is the motivation for this PR?
To avoid testing on one test case that is dependent on the previous test.
How did you do it?
Combine all test into one
How did you verify/test it?
Run test of sonic-mgmt/tests/generic_config_updater/test_cacl.py on KVM
Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation