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

fix fixture order issue in ipv6_mgmt tacacs and telemetry cases #12825

Merged
merged 1 commit into from
May 14, 2024

Conversation

sdszhang
Copy link
Contributor

@sdszhang sdszhang commented May 13, 2024

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:

  1. define other fixtures in 'function' scope.
  2. 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

Type of change

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

Back port request

  • 201911
  • 202012
  • 202205
  • 202305
  • 202311

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

How did you verify/test it?

Run it on kvm testbed, all passed.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@sdszhang
Copy link
Contributor Author

sdszhang commented May 13, 2024

Here are some issues observed in the past due to setup/teardown sequence issue:

  1. tacacs_v6 was setup after convert_and_restore_config_db_to_ipv6_only. resulting in test_ro_user_ipv6_only case failure.
    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.

  1. new ansible ssh connection will fail after tacacs_v6 setup, resulting the follow error. Usually, ansible will reuse existing ssh connection to connect to DUT without doing authentication. However, the following error will be seen if ansible tries to use new SSH connection which will require authentication. It's not clear when the new ssh will be triggered by ansible, but changing tacacs_v6 to function scope will help to limit the impact to only tacacs related cases
------------------------------ live log teardown -------------------------------
18:43:08 __init__._fixture_generator_decorator    L0099 ERROR  | 
Host unreachable in the inventory
Traceback (most recent call last):
  File "/azp/_work/25/s/tests/common/plugins/log_section_start/__init__.py", line 95, in _fixture_generator_decorator
    next(it)
  File "/azp/_work/25/s/tests/telemetry/conftest.py", line 128, in setup_streaming_telemetry
    restore_telemetry_forpyclient(duthost, default_client_auth)
  File "/azp/_work/25/s/tests/telemetry/telemetry_utils.py", line 69, in restore_telemetry_forpyclient
    client_auth_out = duthost.shell('sonic-db-cli CONFIG_DB HGET "%s|gnmi" "client_auth"' % (env.gnmi_config_table),
  File "/azp/_work/25/s/tests/common/devices/multi_asic.py", line 128, in _run_on_asics
    return getattr(self.sonichost, self.multi_asic_attr)(*module_args, **complex_args)
  File "/azp/_work/25/s/tests/common/devices/base.py", line 105, in _run
    res = self.module(*module_args, **complex_args)[self.hostname]
  File "/usr/local/lib/python3.8/dist-packages/pytest_ansible/module_dispatcher/v213.py", line 232, in _run
    raise AnsibleConnectionFailure(
pytest_ansible.errors.AnsibleConnectionFailure: Host unreachable in the inventory

@sdszhang sdszhang requested a review from liuh-80 May 13, 2024 07:19
@sdszhang
Copy link
Contributor Author

sdszhang commented May 13, 2024

one of the example issue seen related to telemetry case is:

    def restore_telemetry_forpyclient(duthost, default_client_auth):
        env = GNMIEnvironment(duthost, GNMIEnvironment.TELEMETRY_MODE)
        client_auth_out = duthost.shell('sonic-db-cli CONFIG_DB HGET "%s|gnmi" "client_auth"' % (env.gnmi_config_table),
                                        module_ignore_errors=False)['stdout_lines']
>       client_auth = str(client_auth_out[0])
E       IndexError: list index out of range

client_auth_out = []
default_client_auth = 'true'
duthost    = <MultiAsicSonicHost vlab-01>
env        = <tests.common.helpers.gnmi_utils.GNMIEnvironment object at 0x7f28da4e0a30>

The reason for this failure is slightly different. We see it happened for sonic-db-cli CONFIG_DB HGET "GNMI|gnmi" client_auth, seems more often with 202311 image. The setup sequence is correct, but teardown sequence is NOT expected. This issue 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.

    SETUP    M convert_and_restore_config_db_to_ipv6_only (fixtures used: duthosts)
    SETUP    M setup_streaming_telemetry
...... 
    TEARDOWN M convert_and_restore_config_db_to_ipv6_only   >>>> this is not happening in the reverse order of fixture setup due to pytest issue.
......
    TEARDOWN M setup_streaming_telemetry[True]    

As a workaround for this issue, I duplicated setup_streaming_telemetry at function scope so that the teardown can happen before ipv6_only fixture. As telemetry is only one test case in test_mgmt_ipv6_only module, it's suitable to use function scope as well.

@sdszhang sdszhang requested a review from zbud-msft May 13, 2024 13:46
@sdszhang sdszhang changed the title fix fixture order issue in ipv6_mgmt only cases fix fixture order issue in ipv6_mgmt tacacs and telemetry cases May 14, 2024
Copy link
Collaborator

@yejianquan yejianquan left a comment

Choose a reason for hiding this comment

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

LGTM

@yejianquan yejianquan merged commit ab2cff9 into sonic-net:master May 14, 2024
16 checks passed
@mssonicbld
Copy link
Collaborator

@sdszhang PR conflicts with 202305 branch

@mssonicbld
Copy link
Collaborator

@sdszhang PR conflicts with 202311 branch

sdszhang added a commit to sdszhang/sonic-mgmt that referenced this pull request May 14, 2024
…y cases (sonic-net#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 sonic-net#1 in this PR as the new 'function' scope fixture can be reused by other cases. Option sonic-net#2 has good readability, but will limit the new fixture to be used by ipv6_only cases.

Summary:
Fixes sonic-net#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
sdszhang added a commit to sdszhang/sonic-mgmt that referenced this pull request May 14, 2024
…y cases (sonic-net#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 sonic-net#1 in this PR as the new 'function' scope fixture can be reused by other cases. Option sonic-net#2 has good readability, but will limit the new fixture to be used by ipv6_only cases.

Summary:
Fixes sonic-net#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
yejianquan pushed a commit that referenced this pull request May 15, 2024
…y cases (#12825) (#12845)

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
yejianquan pushed a commit that referenced this pull request May 15, 2024
…y cases (#12825) (#12846)

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
@sdszhang sdszhang deleted the ipv6_only_fixture_reorder branch May 17, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ip/tests_mgmt_ipv6_only.py fails during teardown.
5 participants