[tests/vrf] Submit VRF testcases according to vrf test plan #1065
Conversation
Signed-off-by: Zhiqian Wu <zhiqian.wu@nephosinc.com>
- change the reboot sleep time to a global variable Signed-off-by: Zhiqian Wu <zhiqian.wu@nephosinc.com>
tests/test_vrf.py
Outdated
| duthost.shell("rm /etc/sonic/config_db.json.bak") | ||
|
|
||
| # FIXME use a better way to load config | ||
| duthost.shell("reboot") |
There was a problem hiding this comment.
If you have to use this way I think "config reload" should work, a reboot is too heavy and will cause the test too long.
There was a problem hiding this comment.
Maybe "config reload" is ok here, but I don't think it's a proper way to do that. Since "config reload" does not cleanup kernel devs (Vlans/PortChannels/VRFs) and ips on interfaces, i.e., when _stop_services() not all of services cleanup themselves. It will lead to many garbage in kernel.
For example, consider the situation where there are two cfgs:
- old_cfg(before reload): Vrf1 - Lag1 - IP100
- new_cfg(reload new_cfg): Vrf2 - Lag1 -IP200
After config reload, the result will be "Vrf1-(bind no devs), Vrf2-lag1-IP100+IP200". It is not
what we want.
That's why i choose "reboot". Please let me know what do you think about it.
There was a problem hiding this comment.
sounds like some bug here? Or this is some SONiC common limitation?
There was a problem hiding this comment.
Most managers(e.g. intfmgr, vlanmgr, vrfmgr) have similar behaviors. They are designed to consume add/del msgs from db, but not cleanup when they are shut down. Kernel have no knowledge about the termination. So, I think it's a common limitation. Maybe we need improve config reload , or "config reload" might not design to be used for this purpose.
Anyway, cold, fast and warm reboot are ok here. Not config reload. Since there has a separate test for warm, so here cold reboot is choosen.
tests/test_vrf.py
Outdated
|
|
||
| # FIXME use a better way to load config | ||
| duthost.shell("reboot") | ||
| time.sleep(REBOOT_SLEEP_TIME) |
There was a problem hiding this comment.
There are some utilities to handle this case more nice, you can take a look https://github.com/Azure/sonic-mgmt/blob/master/tests/platform/test_reboot.py#L107
There was a problem hiding this comment.
Add a new reboot function for better handling this.
| verify_no_packet_any(test, masked_exp_pkt, dst_port_list) | ||
|
|
||
|
|
||
| class FibTest(BaseTest): |
There was a problem hiding this comment.
Is it possible to reuse existing ptftest(https://github.com/Azure/sonic-mgmt/tree/master/ansible/roles/test/files/ptftests)? Seems some functions already implemented there, idea is to avoid redundant code.
There was a problem hiding this comment.
Improved fib_test.FibTest and vrf_test.FwdTest
tests/test_vrf.py
Outdated
|
|
||
| def get_cfg_facts(duthost): | ||
| ## use config db contents(running-config) instead of json file(startup-config) | ||
| #tmp_facts = json.loads(duthost.shell("sonic-cfggen -j /etc/sonic/config_db.json --print-data")['stdout']) |
tests/test_vrf.py
Outdated
|
|
||
| # basic check after warm reboot | ||
| duthost.shell("docker exec -i syncd ps aux | grep /usr/bin/syncd") | ||
| duthost.shell("docker exec -i swss ps aux | grep orchagent") |
There was a problem hiding this comment.
maybe you can reuse critical service check function: https://github.com/Azure/sonic-mgmt/blob/master/tests/common/devices.py#L154 same suggestion for below warm reboot test.
There was a problem hiding this comment.
Use wait_until + critical_service_check for better handling this.
tests/test_vrf.py
Outdated
| for ver, ips in g_vars['vrf_intfs']['Vrf1']['PortChannel0001'].iteritems(): | ||
| for ip in ips: | ||
| duthost.shell("config interface ip add PortChannel0001 {}".format(ip)) | ||
| time.sleep(10) # wait for bgp session re-established. |
There was a problem hiding this comment.
I think should check the BGP session status here, since it is critical to the below test.
There was a problem hiding this comment.
Agreed. Check bgp session status after rebind intf(in TestVrfUnbindIntf) and restore vrf(in TestVrfDeletion). In addition, neigh/fib tests are added after rebind/restore.
…tion * use fixture testbed_devices instead of duthost/ptfhost for reuse critical service check function from SonicHost. * extract a function for reboot use `wait_for` instead of sleeping a specified time slot after reboot reuse critical service check function * extract a function for check interface status, use `wait_until` for continue polling interfaces status * remove comment out codes * add a fib/neigh test after rebind intf * add a bgp/fib/neigh test after restore vrf * refactor vrf_test.py
|
@dawnbeauty , can you resolve the conflict?, @keboliu , please approve if comments are addressed |
|
@prsunny, Done. |
|
@keboliu , can you please approve |
Signed-off-by: Zhiqian Wu zhiqian.wu@nephosinc.com
Description of PR
Summary:
Submit VRF testcases according to vrf test plan [sonic-net/SONiC/pull/409].
It works together with below PRs:
Type of change
Approach
How did you verify/test it?
Support options:
Currently, some vendors do not support change attributes of vrf. So just leave it to a separate test file
vrf_test_attr.py.Supported testbed topology if it's a new test case?
create a new topo t0-vrf base on t0 according to vrf test plan.
Documentation
https://github.com/Azure/SONiC/blob/master/doc/vrf/vrf-ansible-test-plan.md