[MASIC] Make test_bgp_bounce.py support for multi-asic platforms by wenyiz2021 · Pull Request #5723 · sonic-net/sonic-mgmt

wenyiz2021 · 2022-05-26T18:03:06Z

Description of PR

for test introduced from #2866, test_bgp_bounce.py calls 'docker cp' cmd from host to bgp(s), via bgp_helpers.py, which does not consider multi-asic scenario, where we want to copy bgp config files to all BGPs.
This PR does the following change:

For multi-asic, copy to all BGPs, as well as restart all BGPs and make sure all BGPs come back. For single-asic remains the same behavior.
Add restart_bgp() in multi_asic.py to do consideration for multi-asic/single-asic all in this file, and leave the wait_until() in bgp_helpers.py to let know if all BGPs are back up.
Original restart_bgp(asic_index) moved to restart_service_on_asic in multi_asic.py to work on one asic.

Summary:
Fixes # (issue)

Type of change

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

Back port request

201911
202012

Approach

What is the motivation for this PR?

Support test_bgp_bounce.py on multi-asic platform and some refactor.

How did you do it?

under sonic-mgmt/tests, do:
./run_tests.sh-n {multi_asic testbed} -i ../ansible/xxx,../ansible/veos -f ../ansible/testbed.csv -u -e "--skip_sanity --disable_loganalyzer" -c bgp/test_bgp_bounce.py

How did you verify/test it?

Before:
2022-05-13T02:18:37.5367742Z E "stderr": "Error: No such container:path: bgp:/usr/share/sonic/templates/bgpd/bgpd.conf.j2",
2022-05-13T02:18:37.5370441Z E "stderr_lines": [
2022-05-13T02:18:37.5373090Z E "Error: No such container:path: bgp:/usr/share/sonic/templates/bgpd/bgpd.conf.j2"
2022-05-13T02:18:37.5375680Z E ],

After:
bgp/test_bgp_bounce.py::test_bgp_bounce PASSED [ 10%]
metadata-scripts/test_metadata_upgrade_path.py::test_cancelled_upgrade_path[warm-svcstr-n-acs-1] SKIPPED [ 20%]
metadata-scripts/test_metadata_upgrade_path.py::test_upgrade_path[warm-svcstr-n-acs-1] SKIPPED [ 30%]
metadata-scripts/test_metadata_upgrade_path.py::test_warm_upgrade_sad_path[warm-sad-svcstr-n-acs-1] SKIPPED [ 40%]
metadata-scripts/test_metadata_upgrade_path.py::test_warm_upgrade_sad_path[warm-multi_sad-svcstr-n-acs-1] SKIPPED [ 50%]
metadata-scripts/test_metadata_upgrade_path.py::test_warm_upgrade_sad_path[warm-sad_bgp-svcstr-n-acs-1] SKIPPED [ 60%]
metadata-scripts/test_metadata_upgrade_path.py::test_warm_upgrade_sad_path[warm-sad_lag_member-svcstr-n-acs-1] SKIPPED [ 70%]
metadata-scripts/test_metadata_upgrade_path.py::test_warm_upgrade_sad_path[warm-sad_lag-svcstr-n-acs-1] SKIPPED [ 80%]
metadata-scripts/test_metadata_upgrade_path.py::test_warm_upgrade_sad_path[warm-sad_vlan_port-svcstr-n=-acs-1] SKIPPED [ 90%]
metadata-scripts/test_metadata_upgrade_path.py::test_warm_upgrade_sad_path[warm-sad_inboot-svcstr-n-acs-1] SKIPPED [100%]

--------------------------------------------------------------------------------------------- generated xml file: /var/src/sonic-mgmt-int/tests/logs/tr.xml ----------------------------------------------------------------------------------------------
================================================================================================================ short test summary info =================================================================================================================
SKIPPED [9] /var/src/sonic-mgmt-int/tests/metadata-scripts/test_metadata_upgrade_path.py:39: base_image_list or target_image_list is empty
========================================================================================================= 1 passed, 9 skipped in 369.93 seconds ==========================================================================================================

Any platform specific information?

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

Documentation

To support multi-asic on test_bgp_bounce.py

Move restart_bgp() from bgp_helpers.py to multi_asic.py, to make it work for multi-asic device.

arlakshm · 2022-05-27T14:19:01Z

tests/common/devices/multi_asic.py

            container_name += str(asic_id)
        self.shell("sudo docker cp {}:{} {}".format(container_name, src, dst))
+
+    def restart_bgp(self):


why do we need to do reset_service every time ? Normally we need reset services only if services is not running properly

This is refactored from original code, should I remove it?

arlakshm · 2022-05-27T14:27:49Z

tests/bgp/bgp_helpers.py

+    else:
+        pytest_assert(wait_until(100, 10, 0, duthost.is_service_fully_started, "bgp"), "BGP not started.") 

 def restart_bgp(duthost, asic_index=DEFAULT_ASIC_ID):


in this file in some places we use restart_bgp and in some places we use duthost.restart_bgp. Can we use one function everywhere

I have thought of this, is it ok to check is_fully_started inside multi_asic.py or need to make it in bgp_helpers.py each time?

PR updated, can you please review gain?

Change restart_bgp_on_asic to restart_service_on_asic to make it more flexible to call

Change restart_bgp_on_asic to restart_service_on_asic

lgtm-com · 2022-05-27T20:21:50Z

This pull request introduces 1 alert when merging 1a2b810 into 901ca59 - view on LGTM.com

new alerts:

1 for Unused import

arlakshm · 2022-05-31T20:55:22Z

tests/bgp/bgp_helpers.py

+    if duthost.is_multi_asic:
+        for asic_index in range(duthost.facts["num_asic"]):
+            docker_name = duthost.asic_instance(asic_index).get_docker_name("bgp")
+            pytest_assert(wait_until(100, 10, 0, duthost.is_service_fully_started, docker_name), "BGP not started.")
+    else:
+        pytest_assert(wait_until(100, 10, 0, duthost.is_service_fully_started, "bgp"), "BGP not started.") 
+


Can we add this block as function in multi_asic.py ?

in multi_asic.py it is calling itself or the function in sonic_asic.py, to achieve that will need to add assert in the 2 restart_service() in the 2 files, but other functions do not have these checks, wonder if it is needed here?

def restart_service(self, service): if service in self._DEFAULT_SERVICES: return self.sonichost.restart_service(service, service) for asic in self.asics: asic.restart_service(service)

if we want pyassert as original design, looks like assert in this file is a better solution.

Call is_service_fully_started_per_asic_or_host in pyassert

lgtm-com · 2022-06-01T21:46:39Z

This pull request introduces 1 alert when merging 5d4c47c into a0c926b - view on LGTM.com

new alerts:

1 for Unused import

For test introduced from #2866, test_bgp_bounce.py calls 'docker cp' cmd from host to bgp(s), via bgp_helpers.py, which does not consider multi-asic scenario, where we want to copy bgp config files to all BGPs. This PR does the following change: 1. For multi-asic, copy to all BGPs, as well as restart all BGPs and make sure all BGPs come back. For single-asic remains the same behavior. 2. Add restart_bgp() in multi_asic.py to do consideration for multi-asic/single-asic all in this file, and leave the wait_until() in bgp_helpers.py to let know if all BGPs are back up. 3. Original restart_bgp(asic_index) moved to restart_service_on_asic in multi_asic.py to work on one asic. How did you verify/test it? ./run_tests.sh-n {multi_asic testbed} -i ../ansible/xxx,../ansible/veos -f ../ansible/testbed.csv -u -e "--skip_sanity --disable_loganalyzer" -c bgp/test_bgp_bounce.py

wenyiz2021 added 4 commits May 25, 2022 18:46

Update bgp_helpers.py

b02c83f

To support multi-asic on test_bgp_bounce.py

Update multi_asic.py

3f79301

Move restart_bgp() from bgp_helpers.py to multi_asic.py, to make it work for multi-asic device.

Update bgp_helpers.py

e0ed3ab

Update multi_asic.py

d763815

wenyiz2021 added the Request for 202012 branch label May 26, 2022

wenyiz2021 requested review from arlakshm, rlhui, wangxin and yxieca May 26, 2022 18:03

wenyiz2021 requested a review from a team as a code owner May 26, 2022 18:03

wenyiz2021 added 4 commits May 26, 2022 11:17

Update bgp_helpers.py

3ea8252

Move restart_bgp(asic_index) back to bgp_helpers

c292a8a

Update multi_asic.py

3c80d25

Add back header for restart_bgp(asic_id)

e24add5

arlakshm reviewed May 27, 2022

View reviewed changes

wenyiz2021 added 3 commits May 27, 2022 12:37

Move restart_bgp on an asic to restart_bgp_on_asic

bca41f2

Add restart_bgp_on_asic call for per asic

c4e0024

Add import for wait_until in multi_asic.py

38f66b9

wenyiz2021 requested a review from arlakshm May 27, 2022 19:48

wenyiz2021 added 3 commits May 27, 2022 12:51

Add back reset_service same as original design

a961919

Change restart_bgp_on_asic

64f3e61

Change restart_bgp_on_asic to restart_service_on_asic to make it more flexible to call

Change restart_bgp_on_asic

1a2b810

Change restart_bgp_on_asic to restart_service_on_asic

wenyiz2021 added 4 commits May 27, 2022 14:24

Fix LGMT - unused import

f47c2e2

Fix typo

cb7b528

Update multi_asic.py

9d885c9

Refine assertion message in multi_asic.py

517ae4c

arlakshm reviewed May 31, 2022

View reviewed changes

Update assert message in bgp_helpers.py

bfe7e1c

wenyiz2021 requested a review from arlakshm May 31, 2022 20:56

wenyiz2021 added 4 commits May 31, 2022 13:58

Remove pyassert in bgp_helpers.py

2a4cfb7

Add is_service_fully_started_per_asic_or_host

dfa29db

Call is_service_fully_started_per_asic_or_host

0157d61

Call is_service_fully_started_per_asic_or_host in pyassert

Add comment

5d4c47c

Fix LGMT - unused import

fc45116

arlakshm approved these changes Jun 3, 2022

View reviewed changes

wenyiz2021 merged commit 0b86347 into master Jun 3, 2022

wenyiz2021 deleted the bgp_bounce branch June 3, 2022 23:43

wangxin added the Included in 202012 branch label Jun 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MASIC] Make test_bgp_bounce.py support for multi-asic platforms#5723

[MASIC] Make test_bgp_bounce.py support for multi-asic platforms#5723
wenyiz2021 merged 24 commits intomasterfrom
bgp_bounce

wenyiz2021 commented May 26, 2022 •

edited

Loading

Uh oh!

arlakshm May 27, 2022

Uh oh!

wenyiz2021 May 27, 2022

Uh oh!

wenyiz2021 May 27, 2022

Uh oh!

arlakshm May 27, 2022

Uh oh!

wenyiz2021 May 27, 2022

Uh oh!

wenyiz2021 May 27, 2022

Uh oh!

lgtm-com bot commented May 27, 2022

Uh oh!

arlakshm May 31, 2022

Uh oh!

wenyiz2021 May 31, 2022

Uh oh!

wenyiz2021 May 31, 2022

Uh oh!

lgtm-com bot commented Jun 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

wenyiz2021 commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Back port request

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented May 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jun 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wenyiz2021 commented May 26, 2022 •

edited

Loading