Run test_announce_routes.py only with add-topo by wangxin · Pull Request #2741 · sonic-net/sonic-mgmt

wangxin · 2021-01-06T09:39:40Z

Description of PR

Summary:
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Approach

What is the motivation for this PR?

Although the test_announce_routes.py script is always executed by the run_tests.sh
tool while preparing DUT, not every vendor uses run_tests.sh to trigger testing.
People usually forget to run test_announce_routes.py after a new topology is
deployed. Since announcing routes just need to be executed once after a new topology
is deployed, life could be a little bit easier if we announce routes together with
add-topo.

How did you do it?

Changes in this PR:

Added new ansible module announce_routes.py for announcing routes.
Added new playbook announce_routes.yml for starting exabgp processes on PTF and
call the announce_routes module.
Updated the add-topo.yml to run the new playbook.
Removed the fib at tests/common/plugins/fib.py
Removed the tests/test_announce_routes.py script
The tests/decap/test_decap.py script was dependent on the fib plugin. Updated this test
to generate fib_info file from redis DB information.
Updated the IP_decap_test.py script not to test all the FIB entries to limit
test execution time.

How did you verify/test it?

Test run remove-topo and add-topo on both VS setup and physical setup.
Test run run_tests.sh.
Test run tests/decap/test_decap.py

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

bingwang-ms · 2021-01-06T10:17:39Z

ansible/testbed-cli.sh

run_test.sh will set some environment variable, including ANSIBLE_CONFIG, ANSIBLE_LIBRARY and ANSIBLE_CONNECTION_PLUGINS. Can we run pytest without these variables?

we need to set ANSIBLE_CONFIG for sure. I encountered failure recently when running pytest directly

Good catch! Thanks!
My sonic-mgmt used for testing already has these environment variables exported. That's why the issue was not caught in my testing. Indeed I noticed another issue, currently it only works for VS setup. The reason is that the inventory file used for VS setup veos_vtb has host information of PTF. For physical test setups, the veos file does not have PTF host information, the script would fail. I need to figure out how to make it work for physical test setups as well.

Updated this PR with a new approach. Updated PR description.

lgtm-com · 2021-01-13T05:07:14Z

This pull request introduces 2 alerts when merging 78ae80f41090df9ef262a07026f099dec0a1dd2b into 6feb19a - view on LGTM.com

new alerts:

1 for Testing equality to None
1 for Result of integer division may be truncated

lgtm-com · 2021-01-13T05:37:22Z

This pull request introduces 2 alerts when merging dabda62d720f6ade76c9dc33c5a1e32f9a9ec78e into 6feb19a - view on LGTM.com

new alerts:

1 for Testing equality to None
1 for Result of integer division may be truncated

lgtm-com · 2021-01-13T06:07:19Z

This pull request introduces 1 alert when merging b75dde2a32865b7c76aaf40ce982bb4cdaaf45b7 into 6feb19a - view on LGTM.com

new alerts:

1 for Result of integer division may be truncated

lgtm-com · 2021-01-13T06:54:27Z

This pull request introduces 1 alert when merging 9b7a104053d5cdcd5f7c33ef563e4f7d18a6c2b3 into 6feb19a - view on LGTM.com

new alerts:

1 for Result of integer division may be truncated

lgtm-com · 2021-01-13T07:37:14Z

This pull request introduces 1 alert when merging 23a94db49b3f616835c5cf239ae84136fbf581dc into 6feb19a - view on LGTM.com

new alerts:

1 for Result of integer division may be truncated

wangxin · 2021-01-29T10:20:34Z

@neethajohn @bingwang-ms Can you check again?

yxieca · 2021-01-29T16:17:27Z

Is there a way to keep the route announce code at one place? I am thinking the pytest test_announce_route.py invokes fib plug to announce route. That code is very similar to what you added. Can you refactor the code to a single place, you can use symbolic link if have to. Just so that we won't have these 2 chunk of code out of sync.

Although the test_announce_routes.py script is always executed by the run_tests.sh tool while preparing DUT, not every vendor uses run_tests.sh to trigger testing. People usually forget to run test_announce_routes.py after a new topology is deployed. Since announcing routes just need to be executed once after a new topology is deployed, life could be a little bit easier if we announce routes together with add-topo. Changes in this PR: * Added new ansible module `announce_routes.py` for announcing routes. * Added new playbook `announce_routes.yml` for starting exabgp processes on PTF and call the announce_routes module. * Updated the add-topo.yml to run the new playbook. * Removed the fib at tests/common/plugins/fib.py * Removed the tests/test_announce_routes.py script * The tests/decap/test_decap.py script was dependent on the fib plugin. Updated this test to generate fib_info file from redis DB information. * Updated the IP_decap_test.py script not to test all the FIB entries to limit test execution time. Signed-off-by: Xin Wang <xiwang5@microsoft.com>

wangxin · 2021-02-05T13:13:12Z

I have deprecated the fib plugin. Now there is no code duplication.

lgtm-com · 2021-02-05T14:04:15Z

This pull request fixes 2 alerts when merging 890f45c into 7abdb38 - view on LGTM.com

fixed alerts:

2 for Unused import

tests/decap/test_decap.py

ansible/roles/test/files/ptftests/IP_decap_test.py

bingwang-ms · 2021-02-07T02:10:33Z

ansible/roles/vm_set/tasks/announce_routes.yml

+
+  when:
+    - ptf_host_user is defined
+    - ptf_host_pass is defined


Why do we need this check?

when: - ptf_host_user is defined - ptf_host_pass is defined

Indeed it is unnecessary. I'll remove this check.

lgtm-com · 2021-02-07T06:39:49Z

This pull request fixes 2 alerts when merging b65b5d8 into 42ca406 - view on LGTM.com

fixed alerts:

2 for Unused import

In PR #2741, the fib plugin for announcing routes have been deprecated. Announcing routes is done together with add-topo. The test_dir_bcast.py script dependent on the fib plugin was missed in PR #2741. Since the routes have already been announced during add-topo, there is no need to do that for test_dir_bcast.py again. To fix the issue, we can simply removes the dependency of the fib plugin for test_dir_bcast.py. Signed-off-by: Xin Wang <xiwang5@microsoft.com>

In PR #2741, test script for announcing IP routes to PTF docker container was deprecated. The function of announcing IP routes is moved to 'testbed-cli.sh add-topo'. Then we do not need to run the test script to announce IP routes after a new topology is deployed. However, there are still occasions that people may want to re-announce the IP routes to PTF docker containers. Because announcing routes is part of add-topo, then the only possible way to re-announce routes is to run 'testbed-cli.sh remove-topo' and 'testbed-cli.sh add-topo'. This is inconvenient and time consuming. This change added a new 'announce-routes' sub-command to the testbed-cli.sh tool. We can now just re-announce IP routes using command like below: ./testbed-cli.sh -t vtestbed.csv vms-kvm-t0 password.txt Other changes: * Corrected the inv_name field in vtestbed.csv file. * Moved the 'announce_routes.py' module from ansible/roles/vm_set/library to ansible/library Signed-off-by: Xin Wang <xiwang5@microsoft.com>

…net#14309) Why I did it src/sonic-dbsyncd * 4dcfb61 - (HEAD -> 202205, origin/202205) Handled the error case of negative age (sonic-net#57) (2 days ago) [Vivek] src/sonic-swss * 142abdf - (HEAD -> 202205, origin/202205) swss: Fix egress queue counters in voq systems. (sonic-net#2705) (2 days ago) [Sambath Kumar Balasubramanian] src/sonic-utilities * 42a57f4c - (HEAD -> 202205, origin/202205) [202205] Update the ref guide to reflect the vlan brief output (sonic-net#2741) (2 days ago) [Vivek]

wangxin requested a review from a team January 6, 2021 09:39

bingwang-ms reviewed Jan 6, 2021

View reviewed changes

Remove the importing from deprecated fib plugin

890f45c

bingwang-ms reviewed Feb 7, 2021

View reviewed changes

tests/decap/test_decap.py Show resolved Hide resolved

bingwang-ms reviewed Feb 7, 2021

View reviewed changes

ansible/roles/test/files/ptftests/IP_decap_test.py Outdated Show resolved Hide resolved

bingwang-ms reviewed Feb 7, 2021

View reviewed changes

Remove unnecessary ptf_host_user/pass check and improve covered_range

b65b5d8

bingwang-ms approved these changes Feb 7, 2021

View reviewed changes

wangxin merged commit dee1b38 into sonic-net:master Feb 8, 2021

wangxin mentioned this pull request Feb 19, 2021

Remove dependency of deprecated fib plugin for test_dir_bcast.py #3000

Merged

3 tasks

shlomibitton mentioned this pull request Feb 28, 2021

[announce_routes] announce_routes.yml is broken for ptf32/ptf64 topology #3061

Closed

wangxin deleted the add-topo-ann-routes branch March 5, 2021 00:51

wangxin mentioned this pull request Mar 11, 2021

Add 'testbed-cli.sh announce-routes' #3127

Merged

3 tasks

mkim-upscaleai mentioned this pull request Mar 1, 2026

Bug: topo_t1-64.yml "bp_interfaces" not found error during add-topo #22677

Open

Conversation

wangxin commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Uh oh!

bingwang-ms Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

neethajohn Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

wangxin Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

wangxin Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jan 13, 2021

Uh oh!

lgtm-com bot commented Jan 13, 2021

Uh oh!

lgtm-com bot commented Jan 13, 2021

Uh oh!

lgtm-com bot commented Jan 13, 2021

Uh oh!

lgtm-com bot commented Jan 13, 2021

Uh oh!

wangxin commented Jan 29, 2021

Uh oh!

yxieca commented Jan 29, 2021

Uh oh!

wangxin commented Feb 5, 2021

Uh oh!

lgtm-com bot commented Feb 5, 2021

Uh oh!

Uh oh!

Uh oh!

bingwang-ms Feb 7, 2021

Choose a reason for hiding this comment

Uh oh!

wangxin Feb 7, 2021

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Feb 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wangxin commented Jan 6, 2021 •

edited

Loading