From ab2cff91af0733b081b2fbe06fa44442d122bc59 Mon Sep 17 00:00:00 2001 From: Dashuai Zhang <164845223+sdszhang@users.noreply.github.com> Date: Tue, 14 May 2024 13:11:39 +1000 Subject: [PATCH] change fixture order to fix the setup sequence issue in ipv6_mgmt only cases (#12825) Description of PR This PR is to address the fixture setup sequence issue, and teardown out of sequence issue. In convert_and_restore_config_db_to_ipv6_only, it will do a "config reload -y" during fixture setup or teardown. For feature test cases where config is not saved into config_db.json, this reload needs to be done before feature fixture setup and after feature teardown, such as: tacacs_v6, setup_streaming_telemetry, or setup_ntp. According to https://docs.pytest.org/en/latest/reference/fixtures.html#reference-fixtures, it only considers the following when deciding the fixture orders: scope dependencies autouse We shouldn't use autouse in this test module. So only two options to let convert_and_restore_config_db_to_ipv6_only runs before other fixtures: define other fixtures in 'function' scope. define the feature fixture to request convert_and_restore_config_db_to_ipv6_only explicit. Using option #1 in this PR as the new 'function' scope fixture can be reused by other cases. Option #2 has good readability, but will limit the new fixture to be used by ipv6_only cases. Summary: Fixes #12705 Approach What is the motivation for this PR? Multiple errors were observed in mgmt_ipv6 are related to fixture setup/teardown sequence. How did you do it? Added two 'function' scope fixture: check_tacacs_v6_func and setup_streaming_telemetry_func. And modified 3 tests cases to use 'function' scope fixture. test_ro_user_ipv6_only test_rw_user_ipv6_only test_telemetry_output_ipv6_only co-authorized by: jianquanye@microsoft.com --- tests/ip/test_mgmt_ipv6_only.py | 19 +++++++++++++------ tests/tacacs/conftest.py | 13 +++++++++++++ tests/telemetry/conftest.py | 16 ++++++++++++++++ 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/tests/ip/test_mgmt_ipv6_only.py b/tests/ip/test_mgmt_ipv6_only.py index 5c1a2eac1d..5081c76307 100644 --- a/tests/ip/test_mgmt_ipv6_only.py +++ b/tests/ip/test_mgmt_ipv6_only.py @@ -11,11 +11,11 @@ from tests.tacacs.test_ro_user import ssh_remote_run from tests.ntp.test_ntp import run_ntp, setup_ntp_func # noqa F401 from tests.common.helpers.assertions import pytest_require -from tests.tacacs.conftest import tacacs_creds, check_tacacs_v6 # noqa F401 +from tests.tacacs.conftest import tacacs_creds, check_tacacs_v6_func # noqa F401 from tests.syslog.test_syslog import run_syslog, check_default_route # noqa F401 from tests.common.fixtures.duthost_utils import convert_and_restore_config_db_to_ipv6_only # noqa F401 from tests.common.helpers.gnmi_utils import GNMIEnvironment -from tests.telemetry.conftest import gnxi_path, setup_streaming_telemetry # noqa F401 +from tests.telemetry.conftest import gnxi_path, setup_streaming_telemetry_func # noqa F401 pytestmark = [ pytest.mark.disable_loganalyzer, @@ -126,8 +126,10 @@ def test_snmp_ipv6_only(duthosts, enum_rand_one_per_hwsku_hostname, localhost, c assert "SONiC Software Version" in result[0], "Sysdescr not found in SNMP result from DUT IPv6 {}".format(hostipv6) +# use function scope fixture so that convert_and_restore_config_db_to_ipv6_only will setup before check_tacacs_v6_func. +# Otherwise, tacacs_v6 config may be lost after config reload in ipv6_only fixture. def test_ro_user_ipv6_only(localhost, duthosts, enum_rand_one_per_hwsku_hostname, - tacacs_creds, convert_and_restore_config_db_to_ipv6_only, check_tacacs_v6): # noqa F811 + tacacs_creds, convert_and_restore_config_db_to_ipv6_only, check_tacacs_v6_func): # noqa F811 # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later log_eth0_interface_info(duthosts) duthost = duthosts[enum_rand_one_per_hwsku_hostname] @@ -138,8 +140,10 @@ def test_ro_user_ipv6_only(localhost, duthosts, enum_rand_one_per_hwsku_hostname check_output(res, 'test', 'remote_user') +# use function scope fixture so that convert_and_restore_config_db_to_ipv6_only will setup before check_tacacs_v6_func. +# Otherwise, tacacs_v6 config may be lost after config reload in ipv6_only fixture. def test_rw_user_ipv6_only(localhost, duthosts, enum_rand_one_per_hwsku_hostname, - tacacs_creds, convert_and_restore_config_db_to_ipv6_only, check_tacacs_v6): # noqa F811 + tacacs_creds, convert_and_restore_config_db_to_ipv6_only, check_tacacs_v6_func): # noqa F811 # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later log_eth0_interface_info(duthosts) duthost = duthosts[enum_rand_one_per_hwsku_hostname] @@ -150,10 +154,12 @@ def test_rw_user_ipv6_only(localhost, duthosts, enum_rand_one_per_hwsku_hostname check_output(res, 'testadmin', 'remote_user_su') -@pytest.mark.parametrize('setup_streaming_telemetry', [True], indirect=True) +# use function scope fixture so that convert_and_restore_config_db_to_ipv6_only will setup before +# setup_streaming_telemetry_func. Otherwise, telemetry config may be lost after config reload in ipv6_only fixture. +@pytest.mark.parametrize('setup_streaming_telemetry_func', [True], indirect=True) def test_telemetry_output_ipv6_only(convert_and_restore_config_db_to_ipv6_only, # noqa F811 duthosts, enum_rand_one_per_hwsku_hostname, - setup_streaming_telemetry): # noqa F811 + setup_streaming_telemetry_func): # noqa F811 # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later log_eth0_interface_info(duthosts) duthost = duthosts[enum_rand_one_per_hwsku_hostname] @@ -171,6 +177,7 @@ def test_telemetry_output_ipv6_only(convert_and_restore_config_db_to_ipv6_only, "SAI_PORT_STAT_IF_IN_ERRORS not found in gnmi output") +# use function scope fixture so that convert_and_restore_config_db_to_ipv6_only will run before setup_ntp_func def test_ntp_ipv6_only(duthosts, rand_one_dut_hostname, convert_and_restore_config_db_to_ipv6_only, setup_ntp_func): # noqa F811 # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later diff --git a/tests/tacacs/conftest.py b/tests/tacacs/conftest.py index 61d05e6fdb..d60a5b7c9e 100644 --- a/tests/tacacs/conftest.py +++ b/tests/tacacs/conftest.py @@ -1,6 +1,7 @@ import logging import pytest from tests.common.fixtures.tacacs import tacacs_creds # noqa F401 +from contextlib import contextmanager from .utils import setup_tacacs_client, setup_tacacs_server,\ cleanup_tacacs, restore_tacacs_servers @@ -23,6 +24,18 @@ def check_tacacs(ptfhost, duthosts, enum_rand_one_per_hwsku_hostname, tacacs_cre @pytest.fixture(scope="module") def check_tacacs_v6(ptfhost, duthosts, enum_rand_one_per_hwsku_hostname, tacacs_creds): # noqa F811 + with _context_for_check_tacacs_v6(ptfhost, duthosts, enum_rand_one_per_hwsku_hostname, tacacs_creds) as result: + yield result + + +@pytest.fixture(scope="function") +def check_tacacs_v6_func(ptfhost, duthosts, enum_rand_one_per_hwsku_hostname, tacacs_creds): # noqa F811 + with _context_for_check_tacacs_v6(ptfhost, duthosts, enum_rand_one_per_hwsku_hostname, tacacs_creds) as result: + yield result + + +@contextmanager +def _context_for_check_tacacs_v6(ptfhost, duthosts, enum_rand_one_per_hwsku_hostname, tacacs_creds): # noqa F811 duthost = duthosts[enum_rand_one_per_hwsku_hostname] ptfhost_vars = ptfhost.host.options['inventory_manager'].get_host(ptfhost.hostname).vars if 'ansible_hostv6' not in ptfhost_vars: diff --git a/tests/telemetry/conftest.py b/tests/telemetry/conftest.py index 9c647534dc..3bb360d5c3 100644 --- a/tests/telemetry/conftest.py +++ b/tests/telemetry/conftest.py @@ -8,6 +8,7 @@ from tests.common.utilities import wait_until, wait_tcp_connection, get_mgmt_ipv6 from tests.common.helpers.gnmi_utils import GNMIEnvironment from tests.telemetry.telemetry_utils import get_list_stdout, setup_telemetry_forpyclient, restore_telemetry_forpyclient +from contextlib import contextmanager EVENTS_TESTS_PATH = "./telemetry/events" sys.path.append(EVENTS_TESTS_PATH) @@ -87,6 +88,21 @@ def delete_gnmi_config(duthost): @pytest.fixture(scope="module") def setup_streaming_telemetry(request, duthosts, enum_rand_one_per_hwsku_hostname, localhost, ptfhost, gnxi_path): + with _context_for_setup_streaming_telemetry(request, duthosts, enum_rand_one_per_hwsku_hostname, + localhost, ptfhost, gnxi_path) as result: + yield result + + +@pytest.fixture(scope="function") +def setup_streaming_telemetry_func(request, duthosts, enum_rand_one_per_hwsku_hostname, localhost, ptfhost, gnxi_path): + with _context_for_setup_streaming_telemetry(request, duthosts, enum_rand_one_per_hwsku_hostname, + localhost, ptfhost, gnxi_path) as result: + yield result + + +@contextmanager +def _context_for_setup_streaming_telemetry(request, duthosts, enum_rand_one_per_hwsku_hostname, + localhost, ptfhost, gnxi_path): """ @summary: Post setting up the streaming telemetry before running the test. """