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.

Sign up for GitHub

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

Jump to bottom

[copp] Make LLDP disable/enable more robust #2605

Merged
merged 1 commit into from
Dec 1, 2020
Merged

[copp] Make LLDP disable/enable more robust #2605

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 47 additions & 23 deletions tests/copp/test_copp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
_TOR_ONLY_PROTOCOL = ["DHCP"]
_TEST_RATE_LIMIT = 600


class TestCOPP(object):
"""
Tests basic COPP functionality in SONiC.
Expand Down Expand Up @@ -93,10 +94,10 @@ def test_no_policer(self, protocol, duthosts, rand_one_dut_hostname, ptfhost, co
"""
duthost = duthosts[rand_one_dut_hostname]
_copp_runner(duthost,
ptfhost,
protocol,
copp_testbed,
dut_type)
ptfhost,
protocol,
copp_testbed,
dut_type)

@pytest.fixture(scope="class")
def dut_type(duthosts, rand_one_dut_hostname):
Expand All @@ -112,8 +113,15 @@ def dut_type(duthosts, rand_one_dut_hostname):
return dut_type

@pytest.fixture(scope="class")
def copp_testbed(duthosts, rand_one_dut_hostname, creds, ptfhost, tbinfo, request, \
disable_container_autorestart, enable_container_autorestart):
def copp_testbed(
duthosts,
rand_one_dut_hostname,
creds,
ptfhost,
tbinfo,
request,
disable_lldp_for_testing # usefixtures not supported on fixtures
):
"""
Pytest fixture to handle setup and cleanup for the COPP tests.
"""
Expand All @@ -123,14 +131,11 @@ def copp_testbed(duthosts, rand_one_dut_hostname, creds, ptfhost, tbinfo, reques
if test_params.topo not in (_SUPPORTED_PTF_TOPOS + _SUPPORTED_T1_TOPOS):
pytest.skip("Topology not supported by COPP tests")

feature_list = ['lldp']
disable_container_autorestart(duthost, testcase="test_copp", feature_list=feature_list)
_setup_testbed(duthost, creds, ptfhost, test_params)

yield test_params

enable_container_autorestart(duthost, testcase="test_copp", feature_list=feature_list)
_teardown_testbed(duthost, creds, ptfhost, test_params)
try:
_setup_testbed(duthost, creds, ptfhost, test_params)
yield test_params
finally:
_teardown_testbed(duthost, creds, ptfhost, test_params)

@pytest.fixture(autouse=True)
def ignore_expected_loganalyzer_exceptions(loganalyzer):
Expand All @@ -147,11 +152,11 @@ def ignore_expected_loganalyzer_exceptions(loganalyzer):
ignoreRegex = [
".*ERR monit.*'lldpd_monitor' process is not running",
".*ERR monit.* 'lldp\|lldpd_monitor' status failed.*-- 'lldpd:' is not running.",

".*ERR monit.*'lldp_syncd' process is not running",
".*ERR monit.*'lldp\|lldp_syncd' status failed.*'python2 -m lldp_syncd' is not running.",
".*snmp#snmp-subagent.*",
]

if loganalyzer: # Skip if loganalyzer is disabled
loganalyzer.ignore_regex.extend(ignoreRegex)

Expand Down Expand Up @@ -201,16 +206,11 @@ def _gather_test_params(tbinfo, duthost, request):
topo=topo,
bgp_graph=bgp_graph)

@pytest.mark.usefixtures('disable_container_autorestart')
def _setup_testbed(dut, creds, ptf, test_params):
"""
Sets up the testbed to run the COPP tests.
"""

logging.info("Disable LLDP for COPP tests")
dut.command("docker exec lldp supervisorctl stop lldp-syncd")
dut.command("docker exec lldp supervisorctl stop lldpd")

logging.info("Set up the PTF for COPP tests")
copp_utils.configure_ptf(ptf, test_params.nn_target_port)

Expand Down Expand Up @@ -247,6 +247,30 @@ def _teardown_testbed(dut, creds, ptf, test_params):
logging.info("Reloading config and restarting swss...")
config_reload(dut)

logging.info("Restore LLDP")
dut.command("docker exec lldp supervisorctl start lldpd")
dut.command("docker exec lldp supervisorctl start lldp-syncd")

@pytest.fixture(scope="class")
def disable_lldp_for_testing(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this logic not be included in _setup_testbed and _teardown_testbed? If I read it correctly, _teardown_testbed will always be called even if there is a failure as it is now inside finally block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are other steps in teardown that could fail which would cause it to still be missed. I think the rest of the steps should be fixture-ized as much as possible to avoid this, but 1) I can't test this at the moment (because the COPP test itself has issues w/ the recent COPP changes) and 2) it would make this PR a lot more complex.

duthosts,
rand_one_dut_hostname,
disable_container_autorestart,
enable_container_autorestart
):
"""Disables LLDP during testing so that it doesn't interfere with the policer."""
duthost = duthosts[rand_one_dut_hostname]

logging.info("Disabling LLDP for the COPP tests")

feature_list = ['lldp']
disable_container_autorestart(duthost, testcase="test_copp", feature_list=feature_list)

duthost.command("docker exec lldp supervisorctl stop lldp-syncd")
duthost.command("docker exec lldp supervisorctl stop lldpd")

yield

logging.info("Restoring LLDP after the COPP tests")

duthost.command("docker exec lldp supervisorctl start lldpd")
duthost.command("docker exec lldp supervisorctl start lldp-syncd")

enable_container_autorestart(duthost, testcase="test_copp", feature_list=feature_list)