Skip to content

Commit

Permalink
Adding ipv6_mgmt_only test case into PR testing and fix fixture errors (
Browse files Browse the repository at this point in the history
sonic-net#12393)

Summary:
1. Add `ip/test_mgmt_ipv6_only.py` into PR pipeline testing.
2. Rearrange fixture order for two test cases: `ip/test_mgmt_ipv6_only.py::test_rw_user_ipv6_only` and `ip/test_mgmt_ipv6_only.py::test_ro_user_ipv6_only`.
3. Workaround pytest fixture teardown bug affecting `setup_ntp` when run the `ip/test_mgmt_ipv6_only.py` tests.

- [x] Bug fix
- [ ] Testbed and Framework(new/improvement)
- [x] Test case(new/improvement)

```
$ ./run_tests.sh -n vms-kvm-t0 -d vlab-01 -f vtestbed.yaml -i ../ansible/veos_vtb -u -c ip/test_mgmt_ipv6_only.py::test_ro_user_ipv6_only
......
ip/test_mgmt_ipv6_only.py::test_ro_user_ipv6_only[vlab-01] FAILED                                                                                                                  [100%]
......
ip/test_mgmt_ipv6_only.py:138:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

output = {'failed': True, 'changed': True, 'stdout': '', 'stderr': "Warning: Permanently added 'fec0::ffff:afa:1' (RSA) to the ...fec0::ffff:afa:1' (RSA) to the list of known hosts.", 'Permission denied, please try again.'], '_ansible_no_log': None}
exp_val1 = 'test', exp_val2 = 'remote_user'

    def check_output(output, exp_val1, exp_val2):
>       pytest_assert(not output['failed'], output['stderr'])
E       Failed: Warning: Permanently added 'fec0::ffff:afa:1' (RSA) to the list of known hosts.
E       Permission denied, please try again.

exp_val1   = 'test'
exp_val2   = 'remote_user'
output     = {'failed': True, 'changed': True, 'stdout': '', 'stderr': "Warning: Permanently added 'fec0::ffff:afa:1' (RSA) to the ...fec0::ffff:afa:1' (RSA) to the list of known hosts.", 'Permission denied, please try again.'], '_ansible_no_log': None}

tacacs/utils.py:25: Failed
```
The root case is: in current test case definition, the fixture setup sequence is:

1.   `tacacs_v6`       --> `sudo config tacacs add fec0::ffff:afa:2`
2.   `convert_and_restore_config_db_to_ipv6_only`   --> `config reload -y` after removing ipv4 mgmt address

The `sudo config tacacs add fec0::ffff:afa:2` config is lost after the `config reload -y` in step 2. Therefore, causing tacacs authentication failure.

If `convert_and_restore_config_db_to_ipv6_only` is called before `check_tacacs_v6`, there will be no issue.

```
Current definition:
def test_ro_user_ipv6_only(localhost, duthosts, enum_rand_one_per_hwsku_hostname,
                           tacacs_creds, check_tacacs_v6, convert_and_restore_config_db_to_ipv6_only): # noqa F811

Correct definition:
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
```

```
When running the full test cases, we are seeing the following fixture sequence and error.

$./run_tests.sh -n vms-kvm-t0 -d vlab-01 -c ip/test_mgmt_ipv6_only.py -f vtestbed.yaml -i ../ansible/veos_vtb -u -e "--setup-show"

    SETUP    M convert_and_restore_config_db_to_ipv6_only (fixtures used: duthosts)
    SETUP    M setup_ntp (fixtures used: duthosts, ptf_use_ipv6, ptfhost, rand_one_dut_hostname)
......
    TEARDOWN M convert_and_restore_config_db_to_ipv6_only                       ---> This is wrong. setup_ntp should be teardown first.
    TEARDOWN M setup_ntp
......
>           raise RunAnsibleModuleFail("run module {} failed".format(self.module_name), res)
E           tests.common.errors.RunAnsibleModuleFail: run module command failed, Ansible Results =>
E           {"changed": true, "cmd": ["config", "ntp", "del", "fec0::ffff:afa:2"], "delta": "0:00:00.277230", "end": "2024-05-02 11:32:22.404196", "failed": true, "msg": "non-zero return code", "rc": 2, "start": "2024-05-02 11:32:22.126966", "stderr": "Usage: config ntp del [OPTIONS] <ntp_ip_address>\nTry \"config ntp del -h\" for help.\n\nError: NTP server fec0::ffff:afa:2 is not configured.", "stderr_lines": ["Usage: config ntp del [OPTIONS] <ntp_ip_address>", "Try \"config ntp del -h\" for help.", "", "Error: NTP server fec0::ffff:afa:2 is not configured."], "stdout": "", "stdout_lines": []}
......
```

The teardown should be the reverse of fixture setup. The expected setup/teardown order is:
```
    SETUP    M convert_and_restore_config_db_to_ipv6_only (fixtures used: duthosts)
    SETUP    M setup_ntp (fixtures used: duthosts, ptf_use_ipv6, ptfhost, rand_one_dut_hostname)
......
    TEARDOWN M setup_ntp
    TEARDOWN M convert_and_restore_config_db_to_ipv6_only
```
This error is linked to a known issue pytest-dev/pytest#12135 in pytest, and it has been fixed pytest 8.2.0 via pytest-dev/pytest#11833. Currently, SONiC is utilizing pytest version 7.4.0, which does not include the fix for this issue. To address this, a workaround will be necessary until sonic-mgmt is upgraded to pytest version 8.2.0.

1. Add it into the PR test case list.

2.  changed the fixture request sequence, put `convert_and_restore_config_db_to_ipv6_only` to the left of `check_tacacs_v6.` so `convert_and_restore_config_db_to_ipv6_only` fixture will run before `tacacs_v6`.

4.  As upgrading pytest version is not trial change, I duplicated the `setup_ntp` fixture at `function` scope. As ntp is only one case in `test_mgmt_ipv6_only.py`, it makes it more suitable to use a `function` scope fixture instead of `module` scope fixture.

1.  pipeline check included test_mgmt_ipv6_only.py

2.  Run individual test against test_rw_user_ipv6_only, test_ro_user_ipv6_only, test_ntp_ipv6_only. All passed:
```
$./run_tests.sh -n vms-kvm-t0 -d vlab-01 -f vtestbed.yaml -i ../ansible/veos_vtb -u -c ip/test_mgmt_ipv6_only.py::test_ro_user_ipv6_only
....
ip/test_mgmt_ipv6_only.py::test_ro_user_ipv6_only[vlab-01] PASSED                                                                                                                  [100%]

$ ./run_tests.sh -n vms-kvm-t0 -d vlab-01 -f vtestbed.yaml -i ../ansible/veos_vtb -u -c ip/test_mgmt_ipv6_only.py::test_rw_user_ipv6_only
......
ip/test_mgmt_ipv6_only.py::test_rw_user_ipv6_only[vlab-01] PASSED                                                                                                                  [100%]

$./run_tests.sh -n vms-kvm-t0 -d vlab-01 -f vtestbed.yaml -i ../ansible/veos_vtb -u -c ip/test_mgmt_ipv6_only.py::test_ntp_ipv6_only
......
ip/test_mgmt_ipv6_only.py::test_ntp_ipv6_only[True-vlab-01] PASSED                                                                                                                 [100%]
```

3. Full test passed:
```
$./run_tests.sh -n vms-kvm-t0 -d vlab-01 -f vtestbed.yaml -i ../ansible/veos_vtb -u -c ip/test_mgmt_ipv6_only.py
......
ip/test_mgmt_ipv6_only.py::test_bgp_facts_ipv6_only[vlab-01-None] PASSED                                                                                                           [ 10%]
ip/test_mgmt_ipv6_only.py::test_show_features_ipv6_only[vlab-01] PASSED                                                                                                            [ 20%]
ip/test_mgmt_ipv6_only.py::test_image_download_ipv6_only[vlab-01] SKIPPED (Cannot get image url)                                                                                   [ 30%]
ip/test_mgmt_ipv6_only.py::test_syslog_ipv6_only[vlab-01-fd82:b34f:cc99::100-None] PASSED                                                                                          [ 40%]
ip/test_mgmt_ipv6_only.py::test_syslog_ipv6_only[vlab-01-fd82:b34f:cc99::100-fd82:b34f:cc99::200] PASSED                                                                           [ 50%]
ip/test_mgmt_ipv6_only.py::test_ntp_ipv6_only[True-vlab-01] PASSED                                                                                                                 [ 60%]
ip/test_mgmt_ipv6_only.py::test_snmp_ipv6_only[vlab-01] PASSED                                                                                                                     [ 70%]
ip/test_mgmt_ipv6_only.py::test_ro_user_ipv6_only[vlab-01] PASSED                                                                                                                  [ 80%]
ip/test_mgmt_ipv6_only.py::test_rw_user_ipv6_only[vlab-01] PASSED                                                                                                                  [ 90%]
ip/test_mgmt_ipv6_only.py::test_telemetry_output_ipv6_only[vlab-01-True] PASSED                                                                                                    [100%]
==================================================================================== warnings summary ====================================================================================
../../../usr/local/lib/python3.8/dist-packages/paramiko/transport.py:236
  /usr/local/lib/python3.8/dist-packages/paramiko/transport.py:236: CryptographyDeprecationWarning: Blowfish has been deprecated
    "class": algorithms.Blowfish,

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
----------------------------------------------------------------- generated xml file: /data/sonic-mgmt/tests/logs/tr.xml -----------------------------------------------------------------
================================================================================ short test summary info =================================================================================
SKIPPED [1] common/helpers/assertions.py:16: Cannot get image url
================================================================== 9 passed, 1 skipped, 1 warning in 745.28s (0:12:25) ===================================================================
```
  • Loading branch information
sdszhang committed May 16, 2024
1 parent 537cdf7 commit 01efbd9
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 3 deletions.
1 change: 1 addition & 0 deletions .azure-pipelines/pr_test_scripts.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ t0:
- test_procdockerstatsd.py
- database/test_db_scripts.py
- system_health/test_watchdog.py
- ip/test_mgmt_ipv6_only.py

t0-2vlans:
- dhcp_relay/test_dhcp_relay.py
Expand Down
6 changes: 3 additions & 3 deletions tests/ip/test_mgmt_ipv6_only.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from tests.bgp.test_bgp_fact import run_bgp_facts
from tests.test_features import run_show_features
from tests.tacacs.test_ro_user import ssh_remote_run
from tests.ntp.test_ntp import run_ntp, setup_ntp # noqa F401
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_func # noqa F401
from tests.syslog.test_syslog import run_syslog, check_default_route # noqa F401
Expand Down Expand Up @@ -179,5 +179,5 @@ def test_telemetry_output_ipv6_only(convert_and_restore_config_db_to_ipv6_only,

# 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): # noqa F811
run_ntp(duthosts, rand_one_dut_hostname, setup_ntp)
convert_and_restore_config_db_to_ipv6_only, setup_ntp_func): # noqa F811
run_ntp(duthosts, rand_one_dut_hostname, setup_ntp_func)
13 changes: 13 additions & 0 deletions tests/ntp/test_ntp.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging
import time
import pytest
from contextlib import contextmanager

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -46,6 +47,18 @@ def config_long_jump(duthost, enable=False):

@pytest.fixture(scope="module")
def setup_ntp(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:
yield result


@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:
yield result


@contextmanager
def _context_for_setup_ntp(ptfhost, duthosts, rand_one_dut_hostname, ptf_use_ipv6):
"""setup ntp client and server"""
duthost = duthosts[rand_one_dut_hostname]

Expand Down

0 comments on commit 01efbd9

Please sign in to comment.