Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BGP Convergence Test for Benchmark Performance #3715

Merged
merged 69 commits into from
Mar 17, 2022

Conversation

selldinesh
Copy link
Contributor

@selldinesh selldinesh commented Jun 29, 2021

Description of PR

Summary:BGP Convergence Test for Benchmark Performance
BGP Failover convergence for remote link failure
BGP RIB-IN Convergence
BGP RIB-IN Capacity
BGP Failover convergence for Local Link Failure
Fixes # (issue)

Type of change

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

Back port request

  • 201911

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

https://github.com/Azure/sonic-mgmt/blob/master/docs/testplan/BGP%20Convergence%20Testplan%20for%20single%20DUT.md#test-case--1--convergence-performance-when-remote-link-fails-route-withdraw

@selldinesh selldinesh requested review from wangxin, yxieca and a team as code owners June 29, 2021 00:43
@lgtm-com
Copy link

lgtm-com bot commented Jun 29, 2021

This pull request introduces 3 alerts when merging 13ff5fe18c2fabb7b8f624c54299d10b43bafd8c into ca982e6 - view on LGTM.com

new alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jun 29, 2021

This pull request introduces 6 alerts when merging 8629730 into dff9522 - view on LGTM.com

new alerts:

  • 6 for Unused import

@wangxin
Copy link
Collaborator

wangxin commented Jul 7, 2021

Can you fix the testing failures related with this change? It would be great if you can fix the LGTM alerts as well.

==================================== ERRORS ====================================
__________ ERROR collecting ixia/bgp/test_bgp_local_link_failover.py ___________
ImportError while importing test module '/var/src/s/tests/ixia/bgp/test_bgp_local_link_failover.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
ixia/bgp/test_bgp_local_link_failover.py:2: in <module>
    from tests.common.snappi.snappi_fixtures import cvg_api
common/snappi/snappi_fixtures.py:5: in <module>
    import snappi_convergence
E   ImportError: No module named snappi_convergence
__________ ERROR collecting ixia/bgp/test_bgp_remote_link_failover.py __________
ImportError while importing test module '/var/src/s/tests/ixia/bgp/test_bgp_remote_link_failover.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
ixia/bgp/test_bgp_remote_link_failover.py:1: in <module>
    from tests.common.snappi.snappi_fixtures import cvg_api
common/snappi/snappi_fixtures.py:5: in <module>
    import snappi_convergence
E   ImportError: No module named snappi_convergence
___________ ERROR collecting ixia/bgp/test_bgp_rib_in_convergence.py ___________
ImportError while importing test module '/var/src/s/tests/ixia/bgp/test_bgp_rib_in_convergence.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
ixia/bgp/test_bgp_rib_in_convergence.py:1: in <module>
    from tests.common.snappi.snappi_fixtures import cvg_api
common/snappi/snappi_fixtures.py:5: in <module>
    import snappi_convergence
E   ImportError: No module named snappi_convergence

@selldinesh
Copy link
Contributor Author

@wangxin Raised a PR sonic-net/sonic-buildimage#8030 for snappi_convergence module

@lgtm-com
Copy link

lgtm-com bot commented Jul 12, 2021

This pull request introduces 3 alerts when merging ec60200 into 1b53581 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Variable defined multiple times

@lgtm-com
Copy link

lgtm-com bot commented Jul 13, 2021

This pull request introduces 1 alert when merging 20e20be into 1b53581 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@selldinesh
Copy link
Contributor Author

@wangxin can you please merge sonic-net/sonic-buildimage#9682.
This needs atleast one reviewer to approve, when this is fixed the statisticss import issue will be fixed

@wangxin
Copy link
Collaborator

wangxin commented Jan 25, 2022

PR sonic-net/sonic-buildimage#9682 has been merged, lets wait for a new sonic-mgmt docker image.

@wangxin
Copy link
Collaborator

wangxin commented Feb 7, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@selldinesh
Copy link
Contributor Author

@wangxin I have updated the BGP scripts for the latest snappi version and raised a PR sonic-net/sonic-buildimage#9932 for snappi version 0.7.12 , can you please merge the PR 9932 and create a new sonic-mgmt docker image, after which the pipeline may pass

@lgtm-com
Copy link

lgtm-com bot commented Feb 8, 2022

This pull request introduces 1 alert when merging 89ece91 into 7a8be07 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass

@lgtm-com
Copy link

lgtm-com bot commented Feb 11, 2022

This pull request introduces 1 alert when merging 326464e into 3acec36 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@selldinesh
Copy link
Contributor Author

@wangxin The checks were successful before, now i see some checks are not successful, the results shows the test setup failure. Can this be resolved and merged

@wangxin
Copy link
Collaborator

wangxin commented Feb 23, 2022

@selldinesh I am not able to merge this PR until all tests passed. Lets give it another try.

@wangxin
Copy link
Collaborator

wangxin commented Feb 23, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lgtm-com
Copy link

lgtm-com bot commented Mar 2, 2022

This pull request introduces 2 alerts when merging a4ec6a5 into f28a6c3 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Wrong number of arguments in a call

baiwei0427
baiwei0427 previously approved these changes Mar 11, 2022
@@ -941,5 +941,5 @@ def cleanup_config(duthost):
duthost.command("sudo cp {} {}".format("/etc/sonic/config_db_backup.json","/etc/sonic/config_db.json"))
duthost.shell("sudo config reload -y \n")
logger.info("Wait until all critical services are fully started")
pytest_assert(wait_until(360, 10, 1, duthost.critical_services_fully_started), "Not all critical services are fully started")
pytest_assert(wait_until(360, 10, duthost.critical_services_fully_started), "Not all critical services are fully started")
Copy link
Collaborator

@wangxin wangxin Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid that this change won't work. The LGTM also raised alert "Wrong number of arguments in a call".

The wait_until function has been improved to support initial delay. Unfortunately the change is not backward compatible. The author has made a swept change. But all the open PRs still suffer from this issue.
This is the new signature of the wait_until function: https://github.com/Azure/sonic-mgmt/blob/master/tests/common/utilities.py#L82

For the wait_until to work in your local setup, can you do a rebase to the latest master branch and make a force push

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wangxin i have reverted the changes back for the wait_until function

@selldinesh
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lgtm-com
Copy link

lgtm-com bot commented Mar 16, 2022

This pull request introduces 1 alert when merging 2a47fcf into 5fecae8 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass

@wangxin wangxin merged commit ef24e06 into sonic-net:master Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants