[ip/test_mgmt_ipv6_only]: Refactor test to properly cleanup on failure

ipv6 mgmt only tests can fail in setup, after config has been modified,
leading to the teardown to not be run. Refactor the test to split
backing up and restoring the config into another fixture, such that
failures in setting the mgmt IPs wont nuke the testbed.
Also refactor some other aspects to remove linter bypasses

Signed-off-by: Liam Kearney <[email protected]>

tests/common/fixtures/duthost_utils.py

@@ -631,19 +631,17 @@ def wait_bgp_sessions(duthost, timeout=120):
 
 
 @pytest.fixture(scope="module")
-def convert_and_restore_config_db_to_ipv6_only(duthosts):
+def duthosts_ipv6_mgmt_only(duthosts, backup_and_restore_config_db_on_duts):
     """Convert the DUT's mgmt-ip to IPv6 only
 
-    Convert the DUT's mgmt-ip to IPv6 only by removing the IPv4 mgmt-ip,
-    will revert the change after finished.
+    Convert the DUT's mgmt-ip to IPv6 only by removing the IPv4 mgmt-ip.
 
     Since the change commands is distributed by IPv4 mgmt-ip,
     the fixture will detect the IPv6 availability first,
     only remove the IPv4 mgmt-ip when the IPv6 mgmt-ip is available,
     and will re-establish the connection to the DUTs with IPv6 mgmt-ip.
     """
     config_db_file = "/etc/sonic/config_db.json"
-    config_db_bak_file = "/etc/sonic/config_db.json.before_ipv6_only"
 
     # Sample MGMT_INTERFACE:
     #     "MGMT_INTERFACE": {
@@ -702,7 +700,7 @@ def convert_and_restore_config_db_to_ipv6_only(duthosts):
                                            username="WRONG_USER", password="WRONG_PWD", timeout=15)
                     except AuthenticationException:
                         logger.info(f"Host[{duthost.hostname}] IPv6[{ip_addr_without_mask}] mgmt-ip is available")
-                        has_available_ipv6_addr = has_available_ipv6_addr or True
+                        has_available_ipv6_addr = True
                     except BaseException as e:
                         logger.info(f"Host[{duthost.hostname}] IPv6[{ip_addr_without_mask}] mgmt-ip is unavailable, "
                                     f"exception[{type(e)}], msg[{str(e)}]")
@@ -716,8 +714,6 @@ def convert_and_restore_config_db_to_ipv6_only(duthosts):
 
     # Remove IPv4 mgmt-ip
     for duthost in duthosts.nodes:
-        logger.info(f"Backup {config_db_file} to {config_db_bak_file} on {duthost.hostname}")
-        duthost.shell(f"cp {config_db_file} {config_db_bak_file}")
         mgmt_interface = json.loads(duthost.shell(f"jq '.MGMT_INTERFACE' {config_db_file}",
                                                   module_ignore_errors=True)["stdout"])
 
@@ -804,38 +800,7 @@ def convert_and_restore_config_db_to_ipv6_only(duthosts):
                               expect_exists=True, cmd_output=snmp_netstat_output,
                               cmd_desc="netstat")
 
-    yield
-
-    # Recover IPv4 mgmt-ip and other config (SNMP_ADDRESS, etc.)
-    for duthost in duthosts.nodes:
-        if config_db_modified[duthost.hostname]:
-            logger.info(f"Restore {config_db_file} with {config_db_bak_file} on {duthost.hostname}")
-            duthost.shell(f"mv {config_db_bak_file} {config_db_file}")
-            config_reload(duthost, safe_reload=True)
-    duthosts.reset()
-
-    # Verify mgmt-interface status
-    for duthost in duthosts.nodes:
-        logger.info(f"Checking host[{duthost.hostname}] mgmt interface[{mgmt_intf_name}]")
-        mgmt_intf_ifconfig = duthost.shell(f"ifconfig {mgmt_intf_name}", module_ignore_errors=True)["stdout"]
-        assert_addr_in_output(addr_set=ipv4_address, hostname=duthost.hostname,
-                              expect_exists=True, cmd_output=mgmt_intf_ifconfig,
-                              cmd_desc="ifconfig")
-        assert_addr_in_output(addr_set=ipv6_address, hostname=duthost.hostname,
-                              expect_exists=True, cmd_output=mgmt_intf_ifconfig,
-                              cmd_desc="ifconfig")
-
-    # Verify SNMP address status
-    for duthost in duthosts.nodes:
-        logger.info(f"Checking host[{duthost.hostname}] SNMP status in netstat output")
-        snmp_netstat_output = duthost.shell("sudo netstat -tulnpW | grep snmpd",
-                                            module_ignore_errors=True)["stdout"]
-        assert_addr_in_output(addr_set=snmp_ipv4_address, hostname=duthost.hostname,
-                              expect_exists=True, cmd_output=snmp_netstat_output,
-                              cmd_desc="netstat")
-        assert_addr_in_output(addr_set=snmp_ipv6_address, hostname=duthost.hostname,
-                              expect_exists=True, cmd_output=snmp_netstat_output,
-                              cmd_desc="netstat")
+    return duthosts
 
 
 def assert_addr_in_output(addr_set: Dict[str, List], hostname: str,

tests/common/helpers/ntp_helper.py

@@ -6,10 +6,8 @@
 
 
 @contextmanager
-def _context_for_setup_ntp(ptfhost, duthosts, rand_one_dut_hostname, ptf_use_ipv6):
+def setup_ntp_context(ptfhost, duthost, ptf_use_ipv6):
     """setup ntp client and server"""
-    duthost = duthosts[rand_one_dut_hostname]
-
     ptfhost.lineinfile(path="/etc/ntp.conf", line="server 127.127.1.0 prefer")
 
     # restart ntp server
@@ -44,7 +42,7 @@ def _context_for_setup_ntp(ptfhost, duthosts, rand_one_dut_hostname, ptf_use_ipv
 
 @pytest.fixture(scope="function")
 def setup_ntp_func(ptfhost, duthosts, rand_one_dut_hostname, ptf_use_ipv6):
-    with _context_for_setup_ntp(ptfhost, duthosts, rand_one_dut_hostname, ptf_use_ipv6) as result:
+    with setup_ntp_context(ptfhost, duthosts[rand_one_dut_hostname], ptf_use_ipv6) as result:
         yield result
 
 
@@ -55,10 +53,8 @@ def check_ntp_status(host):
     return True
 
 
-def run_ntp(duthosts, rand_one_dut_hostname, setup_ntp):
+def run_ntp(duthost):
     """ Verify that DUT is synchronized with configured NTP server """
-    duthost = duthosts[rand_one_dut_hostname]
-
     ntpsec_conf_stat = duthost.stat(path="/etc/ntpsec/ntp.conf")
     using_ntpsec = ntpsec_conf_stat["stat"]["exists"]
 

tests/common/helpers/tacacs/tacacs_helper.py

@@ -308,8 +308,7 @@ def restore_tacacs_servers(duthost):
 
 
 @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]
+def tacacs_v6_context(ptfhost, duthost, tacacs_creds):
     ptfhost_vars = ptfhost.host.options['inventory_manager'].get_host(ptfhost.hostname).vars
     if 'ansible_hostv6' not in ptfhost_vars:
         pytest.skip("Skip IPv6 test. ptf ansible_hostv6 not configured.")
@@ -324,12 +323,6 @@ def _context_for_check_tacacs_v6(ptfhost, duthosts, enum_rand_one_per_hwsku_host
     restore_tacacs_servers(duthost)
 
 
-@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
-
-
 def tacacs_running(ptfhost):
     out = ptfhost.command("service tacacs_plus status", module_ignore_errors=True)["stdout"]
     return "tacacs+ running" in out

tests/common/helpers/telemetry_helper.py

@@ -1,4 +1,3 @@
-import pytest
 import logging
 from contextlib import contextmanager
 from tests.common.errors import RunAnsibleModuleFail
@@ -82,14 +81,11 @@ def restore_telemetry_forpyclient(duthost, default_client_auth):
 
 
 @contextmanager
-def _context_for_setup_streaming_telemetry(request, duthosts, enum_rand_one_per_hwsku_hostname,
-                                           localhost, ptfhost, gnxi_path):
+def setup_streaming_telemetry_context(is_ipv6, duthost, localhost, ptfhost, gnxi_path):
     """
     @summary: Post setting up the streaming telemetry before running the test.
     """
-    is_ipv6 = request.param
     try:
-        duthost = duthosts[enum_rand_one_per_hwsku_hostname]
         has_gnmi_config = check_gnmi_config(duthost)
         if not has_gnmi_config:
             create_gnmi_config(duthost)
@@ -119,10 +115,3 @@ def _context_for_setup_streaming_telemetry(request, duthosts, enum_rand_one_per_
     restore_telemetry_forpyclient(duthost, default_client_auth)
     if not has_gnmi_config:
         delete_gnmi_config(duthost)
-
-
-@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

tests/ip/test_mgmt_ipv6_only.py

@@ -6,13 +6,11 @@
 from tests.common.utilities import get_mgmt_ipv6, check_output, run_show_features
 from tests.common.helpers.assertions import pytest_assert, pytest_require
 from tests.common.helpers.bgp import run_bgp_facts
-from tests.common.helpers.tacacs.tacacs_helper import ssh_remote_run_retry, check_tacacs_v6_func    # noqa F401
-from tests.common.fixtures.tacacs import tacacs_creds   # noqa F401
-from tests.common.helpers.ntp_helper import run_ntp, setup_ntp_func     # noqa F401
-from tests.common.helpers.telemetry_helper import setup_streaming_telemetry_func    # noqa F401
-from tests.common.helpers.syslog_helpers import run_syslog, check_default_route   # noqa F401
+from tests.common.helpers.tacacs.tacacs_helper import ssh_remote_run_retry, tacacs_v6_context
+from tests.common.helpers.ntp_helper import run_ntp, setup_ntp_context
+from tests.common.helpers.telemetry_helper import setup_streaming_telemetry_context
+from tests.common.helpers.syslog_helpers import run_syslog
 from tests.common.helpers.gnmi_utils import GNMIEnvironment
-from tests.common.fixtures.duthost_utils import convert_and_restore_config_db_to_ipv6_only  # noqa F401
 
 
 pytestmark = [
@@ -82,27 +80,25 @@ def log_tacacs(duthosts, ptfhost):
             logging.debug(f"forced_mgmt_routes: {forced_mgmt_rte}, interface address: {intf_values[2]}")
 
 
-def test_bgp_facts_ipv6_only(duthosts, enum_frontend_dut_hostname, enum_asic_index,
-                             convert_and_restore_config_db_to_ipv6_only): # noqa F811
+def test_bgp_facts_ipv6_only(duthosts_ipv6_mgmt_only, enum_frontend_dut_hostname, enum_asic_index):
     # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later
-    log_eth0_interface_info(duthosts)
-    run_bgp_facts(duthosts, enum_frontend_dut_hostname, enum_asic_index)
+    log_eth0_interface_info(duthosts_ipv6_mgmt_only)
+    run_bgp_facts(duthosts_ipv6_mgmt_only, enum_frontend_dut_hostname, enum_asic_index)
 
 
-def test_show_features_ipv6_only(duthosts, enum_dut_hostname, convert_and_restore_config_db_to_ipv6_only): # noqa F811
+def test_show_features_ipv6_only(duthosts_ipv6_mgmt_only, enum_dut_hostname):
     # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later
-    log_eth0_interface_info(duthosts)
-    run_show_features(duthosts, enum_dut_hostname)
+    log_eth0_interface_info(duthosts_ipv6_mgmt_only)
+    run_show_features(duthosts_ipv6_mgmt_only, enum_dut_hostname)
 
 
-def test_image_download_ipv6_only(creds, duthosts, enum_dut_hostname,
-                                  convert_and_restore_config_db_to_ipv6_only): # noqa F811
+def test_image_download_ipv6_only(creds, duthosts_ipv6_mgmt_only, enum_dut_hostname):
     """
     Test image download in mgmt ipv6 only scenario
     """
     # 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_dut_hostname]
+    log_eth0_interface_info(duthosts_ipv6_mgmt_only)
+    duthost = duthosts_ipv6_mgmt_only[enum_dut_hostname]
     image_url = creds.get("test_image_url", {}).get("ipv6", "")
     pytest_require(len(image_url) != 0, "Cannot get image url")
     cfg_facts = duthost.config_facts(host=duthost.hostname, source="running")['ansible_facts']
@@ -120,18 +116,17 @@ def test_image_download_ipv6_only(creds, duthosts, enum_dut_hostname,
 @pytest.mark.parametrize("dummy_syslog_server_ip_a, dummy_syslog_server_ip_b",
                          [("fd82:b34f:cc99::100", None),
                           ("fd82:b34f:cc99::100", "fd82:b34f:cc99::200")])
-def test_syslog_ipv6_only(duthosts, rand_selected_dut, dummy_syslog_server_ip_a, dummy_syslog_server_ip_b,
-                          check_default_route, convert_and_restore_config_db_to_ipv6_only): # noqa F811
+def test_syslog_ipv6_only(duthosts_ipv6_mgmt_only, rand_selected_dut, dummy_syslog_server_ip_a,
+                          dummy_syslog_server_ip_b, check_default_route):
     # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later
-    log_eth0_interface_info(duthosts)
+    log_eth0_interface_info(duthosts_ipv6_mgmt_only)
     run_syslog(rand_selected_dut, dummy_syslog_server_ip_a, dummy_syslog_server_ip_b, check_default_route)
 
 
-def test_snmp_ipv6_only(duthosts, enum_rand_one_per_hwsku_hostname, localhost, creds_all_duts,
-                        convert_and_restore_config_db_to_ipv6_only): # noqa F811
+def test_snmp_ipv6_only(duthosts_ipv6_mgmt_only, enum_rand_one_per_hwsku_hostname, localhost, creds_all_duts):
     # 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]
+    log_eth0_interface_info(duthosts_ipv6_mgmt_only)
+    duthost = duthosts_ipv6_mgmt_only[enum_rand_one_per_hwsku_hostname]
     hostipv6 = duthost.host.options['inventory_manager'].get_host(
         duthost.hostname).vars['ansible_hostv6']
 
@@ -146,62 +141,58 @@ 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, ptfhost, duthosts, enum_rand_one_per_hwsku_hostname,
-                           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]
-    dutipv6 = get_mgmt_ipv6(duthost)
-    log_tacacs(duthosts, ptfhost)
+def test_ro_user_ipv6_only(localhost, ptfhost, duthosts_ipv6_mgmt_only, enum_rand_one_per_hwsku_hostname,
+                           tacacs_creds):
+    duthost = duthosts_ipv6_mgmt_only[enum_rand_one_per_hwsku_hostname]
+    with tacacs_v6_context(ptfhost, duthost, tacacs_creds):
+        # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later
+        log_eth0_interface_info(duthosts_ipv6_mgmt_only)
+        dutipv6 = get_mgmt_ipv6(duthost)
+        log_tacacs(duthosts_ipv6_mgmt_only, ptfhost)
 
-    res = ssh_remote_run_retry(localhost, dutipv6, ptfhost, tacacs_creds['tacacs_ro_user'],
-                               tacacs_creds['tacacs_ro_user_passwd'], 'cat /etc/passwd')
-    check_output(res, 'test', 'remote_user')
+        res = ssh_remote_run_retry(localhost, dutipv6, ptfhost, tacacs_creds['tacacs_ro_user'],
+                                   tacacs_creds['tacacs_ro_user_passwd'], 'cat /etc/passwd')
+        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, ptfhost, duthosts, enum_rand_one_per_hwsku_hostname,
-                           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]
-    dutipv6 = get_mgmt_ipv6(duthost)
-    log_tacacs(duthosts, ptfhost)
-
-    res = ssh_remote_run_retry(localhost, dutipv6, ptfhost, tacacs_creds['tacacs_rw_user'],
-                               tacacs_creds['tacacs_rw_user_passwd'], "cat /etc/passwd")
-    check_output(res, 'testadmin', 'remote_user_su')
-
-
-# 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_func): # noqa F811
+def test_rw_user_ipv6_only(localhost, ptfhost, duthosts_ipv6_mgmt_only, enum_rand_one_per_hwsku_hostname,
+                           tacacs_creds):
+    duthost = duthosts_ipv6_mgmt_only[enum_rand_one_per_hwsku_hostname]
+    with tacacs_v6_context(ptfhost, duthost, tacacs_creds):
+        # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later
+        log_eth0_interface_info(duthosts_ipv6_mgmt_only)
+        dutipv6 = get_mgmt_ipv6(duthost)
+        log_tacacs(duthosts_ipv6_mgmt_only, ptfhost)
+
+        res = ssh_remote_run_retry(localhost, dutipv6, ptfhost, tacacs_creds['tacacs_rw_user'],
+                                   tacacs_creds['tacacs_rw_user_passwd'], "cat /etc/passwd")
+        check_output(res, 'testadmin', 'remote_user_su')
+
+
+def test_telemetry_output_ipv6_only(duthosts_ipv6_mgmt_only, enum_rand_one_per_hwsku_hostname,
+                                    localhost, ptfhost, gnxi_path):
     # 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]
-    env = GNMIEnvironment(duthost, GNMIEnvironment.TELEMETRY_MODE)
-    if duthost.is_supervisor_node():
-        pytest.skip(
-            "Skipping test as no Ethernet0 frontpanel port on supervisor")
-    dut_ip = get_mgmt_ipv6(duthost)
-    cmd = "~/gnmi_get -xpath_target COUNTERS_DB -xpath COUNTERS/Ethernet0 -target_addr \
-          [%s]:%s -logtostderr -insecure" % (dut_ip, env.gnmi_port)
-    show_gnmi_out = duthost.shell(cmd)['stdout']
-    result = str(show_gnmi_out)
-    inerrors_match = re.search("SAI_PORT_STAT_IF_IN_ERRORS", result)
-    pytest_assert(inerrors_match is not None,
-                  "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
+    duthost = duthosts_ipv6_mgmt_only[enum_rand_one_per_hwsku_hostname]
+    with setup_streaming_telemetry_context(True, duthost, localhost, ptfhost, gnxi_path):
+        log_eth0_interface_info(duthosts_ipv6_mgmt_only)
+        env = GNMIEnvironment(duthost, GNMIEnvironment.TELEMETRY_MODE)
+        if duthost.is_supervisor_node():
+            pytest.skip(
+                "Skipping test as no Ethernet0 frontpanel port on supervisor")
+        dut_ip = get_mgmt_ipv6(duthost)
+        cmd = "~/gnmi_get -xpath_target COUNTERS_DB -xpath COUNTERS/Ethernet0 -target_addr \
+            [%s]:%s -logtostderr -insecure" % (dut_ip, env.gnmi_port)
+        show_gnmi_out = duthost.shell(cmd)['stdout']
+        result = str(show_gnmi_out)
+        inerrors_match = re.search("SAI_PORT_STAT_IF_IN_ERRORS", result)
+        pytest_assert(inerrors_match is not None,
+                      "SAI_PORT_STAT_IF_IN_ERRORS not found in gnmi output")
+
+
+# use function scope fixture so that duthosts_ipv6_mgmt_only will run before setup_ntp_func
+def test_ntp_ipv6_only(duthosts_ipv6_mgmt_only, rand_one_dut_hostname, ptfhost, ptf_use_ipv6):
     # Add a temporary debug log to see if DUTs are reachable via IPv6 mgmt-ip. Will remove later
-    log_eth0_interface_info(duthosts)
-    run_ntp(duthosts, rand_one_dut_hostname, setup_ntp_func)
+    duthost = duthosts_ipv6_mgmt_only[rand_one_dut_hostname]
+    with setup_ntp_context(ptfhost, duthost, ptf_use_ipv6):
+        log_eth0_interface_info(duthosts_ipv6_mgmt_only)
+        run_ntp(duthost)