Fix config_db.json backup in `test_counterpoll_watermark.py` #16002

arista-nwolfe · 2024-12-11T02:38:21Z

Details of failure described in more detail in #15991

Created a new fixture backup_and_restore_config_db_frontend which uses enum_rand_one_per_hwsku_frontend_hostname so test_counterpoll_watermark.py can run it.

Updated _backup_and_restore_config_db to work on multi-asic LCs

Summary:
Fixes #15991

Type of change

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

Back port request

Also fix '_backup_and_restore_config_db' to work on multi-asic lcs

mssonicbld · 2024-12-11T02:39:36Z

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/common/fixtures/duthost_utils.py:51:23: E711 comparison to None should be 'if cond is None:' tests/common/fixtures/duthost_utils.py:69:23: E711 comparison to None should be 'if cond is None:' flake8...............................................(no files to check)Skipped check conditional mark sort..........................(no files to check)Skipped

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

yxieca · 2024-12-11T16:26:32Z

@arista-nwolfe using config db backup/restore to keep original configuration is a very expensive way of keep the configuration consistent. Because when using this method, when there is diff, the system will go through config reload, which is a 3 minutes+ operation on single ASIC. I don't know the operation time on multi-ASIC/multi-DUT. could be much longer.

@yutongzhang-microsoft can you give some architectural guidance here? I feel that test should try best to revert configuration change to original and use the backup/restore infra to check and make sure that the configuration is restored?

arista-nwolfe · 2024-12-11T17:04:03Z

@arista-nwolfe using config db backup/restore to keep original configuration is a very expensive way of keep the configuration consistent. Because when using this method, when there is diff, the system will go through config reload, which is a 3 minutes+ operation on single ASIC. I don't know the operation time on multi-ASIC/multi-DUT. could be much longer.

@yutongzhang-microsoft can you give some architectural guidance here? I feel that test should try best to revert configuration change to original and use the backup/restore infra to check and make sure that the configuration is restored?

Note that this config reload due to config-mismatch is already happening today before this change.
It's the conftest.core_dump_and_config_check which does that.
The config reload will correct the current running configuration but it won't fix the config_db.json files, that's all this review is fixing.
The copying and restoring of the config_db.json files should be a very quick operation.

I agree we could improve this test to correct/revert it's config within the test but I don't think that needs to block this review as it shouldn't change the duration of the test. It'll just correct the saved config so future tests don't fail due to this.

yutongzhang-microsoft · 2024-12-12T03:11:55Z

@arista-nwolfe using config db backup/restore to keep original configuration is a very expensive way of keep the configuration consistent. Because when using this method, when there is diff, the system will go through config reload, which is a 3 minutes+ operation on single ASIC. I don't know the operation time on multi-ASIC/multi-DUT. could be much longer.

@yutongzhang-microsoft can you give some architectural guidance here? I feel that test should try best to revert configuration change to original and use the backup/restore infra to check and make sure that the configuration is restored?

Hi, @yxieca , @arista-nwolfe . We have the fixture https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2403 to check the running config before and after the test script running. To achieve this, we have already backup the config in /etc/sonic/running_golden_config.json before running. Can we directly use this golden config to cover the config_db.json instead of generating another one?

And I'm also wondering, our fixture core_dump_and_config_check will do config reload if the configurations are modified during testing. As in https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2364, we will copy the golden running config back to config_db.json. So theoretically, we don't need manually restore config_db.json...

arista-nwolfe · 2024-12-12T18:39:19Z

@arista-nwolfe using config db backup/restore to keep original configuration is a very expensive way of keep the configuration consistent. Because when using this method, when there is diff, the system will go through config reload, which is a 3 minutes+ operation on single ASIC. I don't know the operation time on multi-ASIC/multi-DUT. could be much longer.
@yutongzhang-microsoft can you give some architectural guidance here? I feel that test should try best to revert configuration change to original and use the backup/restore infra to check and make sure that the configuration is restored?

Hi, @yxieca , @arista-nwolfe . We have the fixture https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2403 to check the running config before and after the test script running. To achieve this, we have already backup the config in /etc/sonic/running_golden_config.json before running. Can we directly use this golden config to cover the config_db.json instead of generating another one?

And I'm also wondering, our fixture core_dump_and_config_check will do config reload if the configurations are modified during testing. As in https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2364, we will copy the golden running config back to config_db.json. So theoretically, we don't need manually restore config_db.json...

It looks like it core_dump_and_config_check only calls __dut_reload to restore the config_db.json on non-modular systems https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2673-L2677

This came from a fairly recent PR: #14368
Tagging @yejianquan in case this was an unintended change:

            is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
            if is_modular_chassis:
                results = recover_chassis(duthosts)
            else:
                results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)

recover_chassis doesn't save to config_db.json but __dut_reload does.

yejianquan · 2024-12-13T01:13:43Z

@arista-nwolfe using config db backup/restore to keep original configuration is a very expensive way of keep the configuration consistent. Because when using this method, when there is diff, the system will go through config reload, which is a 3 minutes+ operation on single ASIC. I don't know the operation time on multi-ASIC/multi-DUT. could be much longer.
@yutongzhang-microsoft can you give some architectural guidance here? I feel that test should try best to revert configuration change to original and use the backup/restore infra to check and make sure that the configuration is restored?

Hi, @yxieca , @arista-nwolfe . We have the fixture https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2403 to check the running config before and after the test script running. To achieve this, we have already backup the config in before running. Can we directly use this golden config to cover the config_db.json instead of generating another one?/etc/sonic/running_golden_config.json
And I'm also wondering, our fixture will do config reload if the configurations are modified during testing. As in https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2364, we will copy the golden running config back to . So theoretically, we don't need manually restore ...core_dump_and_config_check``config_db.json``config_db.json

It looks like it only calls to restore the on non-modular systems https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2673-L2677`core_dump_and_config_check``__dut_reload``config_db.json`

This came from a fairly recent PR: #14368 Tagging @yejianquan in case this was an unintended change:
            is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
            if is_modular_chassis:
                results = recover_chassis(duthosts)
            else:
                results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)
recover_chassis doesn't save to but does.config_db.json``__dut_reload

@arista-nwolfe It's expected behavior.
To have a thorough recovery, we config reload RP and all LC together and it have being working well.
And yes there is mismatch logic between __dut_reload and recover_chassis.
But I suppose it shouldn't make any difference?

The logic of core_dump_and_config_check is

At the start, use current config to
1.a: Generate golden config
1.b: Store in memory json.dumps(duts_data[node.hostname]["pre_running_config"]
After the test, if it's not chassis, it
2.a: Generate config files with memory data
2.b: Config reload with the new generated(2.a) config.
After the test, if it's not chassis, it:
3.a: Config reload with the new generated(1.a) golden config.

@yutongzhang-microsoft please confirm the logic.

@arista-nwolfe The only difference is non-chassis uses the golden config from 2.a, chassis uses the golen config from 1.a.
I suppose the test module won't write golden config, so there shouldn't have any difference, isn't it?

yejianquan · 2024-12-13T01:17:47Z

@arista-nwolfe using config db backup/restore to keep original configuration is a very expensive way of keep the configuration consistent. Because when using this method, when there is diff, the system will go through config reload, which is a 3 minutes+ operation on single ASIC. I don't know the operation time on multi-ASIC/multi-DUT. could be much longer.
@yutongzhang-microsoft can you give some architectural guidance here? I feel that test should try best to revert configuration change to original and use the backup/restore infra to check and make sure that the configuration is restored?

Hi, @yxieca , @arista-nwolfe . We have the fixture https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2403 to check the running config before and after the test script running. To achieve this, we have already backup the config in before running. Can we directly use this golden config to cover the config_db.json instead of generating another one?/etc/sonic/running_golden_config.json
And I'm also wondering, our fixture will do config reload if the configurations are modified during testing. As in https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2364, we will copy the golden running config back to . So theoretically, we don't need manually restore ... core_dump_and_config_checkconfig_db.jsonconfig_db.json

It looks like it only calls to restore the on non-modular systems https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2673-L2677core_dump_and_config_check``__dut_reload``config_db.json
This came from a fairly recent PR: #14368 Tagging @yejianquan in case this was an unintended change:
            is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
            if is_modular_chassis:
                results = recover_chassis(duthosts)
            else:
                results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)
recover_chassis doesn't save to but does. config_db.json__dut_reload ``
@arista-nwolfe It's expected behavior. To have a thorough recovery, we config reload RP and all LC together and it have being working well. And yes there is mismatch logic between __dut_reload and recover_chassis. But I suppose it shouldn't make any difference?

The logic of core_dump_and_config_check is

At the start, use current config to
1.a: Generate golden config
1.b: Store in memory json.dumps(duts_data[node.hostname]["pre_running_config"]

After the test, if it's not chassis, it
2.a: Generate golden config with memory data
2.b: Config reload with the new generated(2.a) golden config.

After the test, if it's not chassis, it:
3.a: Config reload with the new generated(1.a) golden config.

@yutongzhang-microsoft please confirm the logic.

@arista-nwolfe The only difference is non-chassis uses the golden config from 2.a, chassis uses the golen config from 1.a. I suppose the test module won't write golden config, so there shouldn't have any difference, isn't it?

But I think yes it's a gap that the chassis_recover should add the logic that save back the config_db*.json
@arista-nwolfe could you enhance the logic here to restore the both running config and config file instead only restore running config ?

sonic-mgmt/tests/conftest.py

Line 2674 in dd14c06

recover_chassis(duthosts)

So that we don't need this PR's fixture to store and restore config files?
Given the functionality is covered by core_dump_and_config_check alraeady

yutongzhang-microsoft · 2024-12-13T01:22:48Z

@arista-nwolfe using config db backup/restore to keep original configuration is a very expensive way of keep the configuration consistent. Because when using this method, when there is diff, the system will go through config reload, which is a 3 minutes+ operation on single ASIC. I don't know the operation time on multi-ASIC/multi-DUT. could be much longer.
@yutongzhang-microsoft can you give some architectural guidance here? I feel that test should try best to revert configuration change to original and use the backup/restore infra to check and make sure that the configuration is restored?

Hi, @yxieca , @arista-nwolfe . We have the fixture https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2403 to check the running config before and after the test script running. To achieve this, we have already backup the config in before running. Can we directly use this golden config to cover the config_db.json instead of generating another one?/etc/sonic/running_golden_config.json
And I'm also wondering, our fixture will do config reload if the configurations are modified during testing. As in https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2364, we will copy the golden running config back to . So theoretically, we don't need manually restore ... core_dump_and_config_checkconfig_db.jsonconfig_db.json

It looks like it only calls to restore the on non-modular systems https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2673-L2677core_dump_and_config_check``__dut_reload``config_db.json
This came from a fairly recent PR: #14368 Tagging @yejianquan in case this was an unintended change:
            is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
            if is_modular_chassis:
                results = recover_chassis(duthosts)
            else:
                results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)
recover_chassis doesn't save to but does. config_db.json__dut_reload ``
@arista-nwolfe It's expected behavior. To have a thorough recovery, we config reload RP and all LC together and it have being working well. And yes there is mismatch logic between __dut_reload and recover_chassis. But I suppose it shouldn't make any difference?

The logic of core_dump_and_config_check is

At the start, use current config to
1.a: Generate golden config
1.b: Store in memory json.dumps(duts_data[node.hostname]["pre_running_config"]

After the test, if it's not chassis, it
2.a: Generate golden config with memory data
2.b: Config reload with the new generated(2.a) golden config.

After the test, if it's not chassis, it:
3.a: Config reload with the new generated(1.a) golden config.

@yutongzhang-microsoft please confirm the logic.

@arista-nwolfe The only difference is non-chassis uses the golden config from 2.a, chassis uses the golen config from 1.a. I suppose the test module won't write golden config, so there shouldn't have any difference, isn't it?

Hi, @yejianquan For no chassis, we will copy it back to config_db.json https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2364. Maybe there is a typo of step 2 or 3?

yejianquan · 2024-12-13T01:26:44Z

@arista-nwolfe using config db backup/restore to keep original configuration is a very expensive way of keep the configuration consistent. Because when using this method, when there is diff, the system will go through config reload, which is a 3 minutes+ operation on single ASIC. I don't know the operation time on multi-ASIC/multi-DUT. could be much longer.
@yutongzhang-microsoft can you give some architectural guidance here? I feel that test should try best to revert configuration change to original and use the backup/restore infra to check and make sure that the configuration is restored?

Hi, @yxieca , @arista-nwolfe . We have the fixture https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2403 to check the running config before and after the test script running. To achieve this, we have already backup the config in before running. Can we directly use this golden config to cover the config_db.json instead of generating another one?/etc/sonic/running_golden_config.json
And I'm also wondering, our fixture will do config reload if the configurations are modified during testing. As in https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2364, we will copy the golden running config back to . So theoretically, we don't need manually restore ... core_dump_and_config_checkconfig_db.jsonconfig_db.json

It looks like it only calls to restore the on non-modular systems https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2673-L2677 core_dump_and_config_check__dut_reloadconfig_db.json
This came from a fairly recent PR: #14368 Tagging @yejianquan in case this was an unintended change:
            is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
            if is_modular_chassis:
                results = recover_chassis(duthosts)
            else:
                results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)
recover_chassis doesn't save to but does. config_db.json__dut_reload ``
@arista-nwolfe It's expected behavior. To have a thorough recovery, we config reload RP and all LC together and it have being working well. And yes there is mismatch logic between __dut_reload and recover_chassis. But I suppose it shouldn't make any difference?
The logic of core_dump_and_config_check is

At the start, use current config to
1.a: Generate golden config
1.b: Store in memory json.dumps(duts_data[node.hostname]["pre_running_config"]

After the test, if it's not chassis, it
2.a: Generate golden config with memory data
2.b: Config reload with the new generated(2.a) golden config.

After the test, if it's not chassis, it:
3.a: Config reload with the new generated(1.a) golden config.

@yutongzhang-microsoft please confirm the logic.
@arista-nwolfe The only difference is non-chassis uses the golden config from 2.a, chassis uses the golen config from 1.a. I suppose the test module won't write golden config, so there shouldn't have any difference, isn't it?
Hi, @yejianquan For no chassis, we will copy it back to config_db.json https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2364. Maybe there is a typo of step 2 or 3?

Thanks, updated

arista-nwolfe · 2024-12-13T16:14:04Z

@arista-nwolfe using config db backup/restore to keep original configuration is a very expensive way of keep the configuration consistent. Because when using this method, when there is diff, the system will go through config reload, which is a 3 minutes+ operation on single ASIC. I don't know the operation time on multi-ASIC/multi-DUT. could be much longer.
@yutongzhang-microsoft can you give some architectural guidance here? I feel that test should try best to revert configuration change to original and use the backup/restore infra to check and make sure that the configuration is restored?

Hi, @yxieca , @arista-nwolfe . We have the fixture https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2403 to check the running config before and after the test script running. To achieve this, we have already backup the config in before running. Can we directly use this golden config to cover the config_db.json instead of generating another one?/etc/sonic/running_golden_config.json
And I'm also wondering, our fixture will do config reload if the configurations are modified during testing. As in https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2364, we will copy the golden running config back to . So theoretically, we don't need manually restore ... core_dump_and_config_checkconfig_db.jsonconfig_db.json

It looks like it only calls to restore the on non-modular systems https://github.com/sonic-net/sonic-mgmt/blob/master/tests/conftest.py#L2673-L2677core_dump_and_config_check``__dut_reload``config_db.json
This came from a fairly recent PR: #14368 Tagging @yejianquan in case this was an unintended change:
            is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
            if is_modular_chassis:
                results = recover_chassis(duthosts)
            else:
                results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)
recover_chassis doesn't save to but does. config_db.json__dut_reload ``
@arista-nwolfe It's expected behavior. To have a thorough recovery, we config reload RP and all LC together and it have being working well. And yes there is mismatch logic between __dut_reload and recover_chassis. But I suppose it shouldn't make any difference?

The logic of core_dump_and_config_check is

At the start, use current config to
1.a: Generate golden config
1.b: Store in memory json.dumps(duts_data[node.hostname]["pre_running_config"]

After the test, if it's not chassis, it
2.a: Generate config files with memory data
2.b: Config reload with the new generated(2.a) config.

After the test, if it's not chassis, it:
3.a: Config reload with the new generated(1.a) golden config.

@yutongzhang-microsoft please confirm the logic.

@arista-nwolfe The only difference is non-chassis uses the golden config from 2.a, chassis uses the golen config from 1.a. I suppose the test module won't write golden config, so there shouldn't have any difference, isn't it?

@yejianquan the fact that the config_db#.json files are no longer being written to by the saved config from the start of the test is a significant difference and is the reason for issues like #15991. If the config_db#.json files aren't corrected the config that the test modified will be persistent into the next test. In the case of this issue, causing future tests which depend on flex counters to fail due to flex counters being disabled by test_counterpoll_watermark.py

yejianquan · 2024-12-13T20:59:31Z

@arista-nwolfe

Per

sonic-mgmt/tests/conftest.py

Line 2364 in 2c28912

    
           node.copy(content=json.dumps(duts_data[node.hostname]["pre_running_config"][None], indent=4),

The config_db.json and config_db{}.json should be overwritten by the logic in

sonic-mgmt/tests/conftest.py

Line 2364 in 2c28912

    
           node.copy(content=json.dumps(duts_data[node.hostname]["pre_running_config"][None], indent=4),

But not included in recover_chassis logic.
recover_chassis is not designed for core_dump_and_config_check originally, but for the sanity check, the purpose of it is to config reload RP and all LC together rather then part of them, to achieve a deeper recover, it was useful in a period that chassis is not stable enough.
As a compare, the sanifty check recover originally only recover the dut that fails on some sanity check.

In core_dump_and_config_check, the functionality seems no much differences, because core_dump_and_config_check will config reload all duthosts at the same time.

So I'm ok to remove using recover_chassis in core_dump_and_config_check if fully tested again and it works on chassis.

But I definitely know some benefits of recover_chassis, it's using SafeThreadPoolExecutor which is multi-thread, for parallel_run, it's multi-process.
SafeThreadPoolExecutor doesn't need to set timeout explicitly, and will have a better log output.
I heard some complaint when using parallel_run on chassis: when the safe check is failing, they can't find the failure log correspondingly.

Regarding next step, my suggestion is to merge them：

is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
                if is_modular_chassis:
                    recover_chassis(duthosts)
                else:
                    results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)
                    logger.debug('Results of dut reload: {}'.format(json.dumps(dict(results))))

With SafeThreadPoolExecutor and save the config in memory back to config_db.json and config_db{}.json

liamkearney-msft · 2024-12-16T00:35:44Z

tests/common/fixtures/duthost_utils.py

+def backup_and_restore_config_db_frontend(duthosts, enum_rand_one_per_hwsku_frontend_hostname):
+    """Back up and restore config DB at the function level."""
+    duthost = duthosts[enum_rand_one_per_hwsku_frontend_hostname]
+    # TODO: Use the neater "yield from _function" syntax when we move to python3


are we not already using python3 ?

yejianquan

Regarding next step, my suggestion is to merge them：

is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
                if is_modular_chassis:
                    recover_chassis(duthosts)
                else:
                    results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)
                    logger.debug('Results of dut reload: {}'.format(json.dumps(dict(results))))

With SafeThreadPoolExecutor and save the config in memory back to config_db.json and config_db{}.json

yutongzhang-microsoft · 2024-12-16T06:49:23Z

Regarding next step, my suggestion is to merge them：

is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
                if is_modular_chassis:
                    recover_chassis(duthosts)
                else:
                    results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)
                    logger.debug('Results of dut reload: {}'.format(json.dumps(dict(results))))

With SafeThreadPoolExecutor and save the config in memory back to config_db.json and config_db{}.json

Yes, I totally agree with you, we can merge the logic of recover_chassis and dut_reload to write the golden config back.

Created a new function restore_config_db_and_config_reload which merges the recover_chassis and __dut_reload function so that we both save the config_db.json files and execute a config reload if the config differs before and after the test (checked in core_dump_and_config_check)

mssonicbld · 2024-12-19T01:05:17Z

/azp run

azure-pipelines · 2024-12-19T01:05:28Z

Azure Pipelines successfully started running 1 pipeline(s).

arista-nwolfe · 2024-12-19T01:10:40Z

Regarding next step, my suggestion is to merge them：

is_modular_chassis = duthosts[0].get_facts().get("modular_chassis")
                if is_modular_chassis:
                    recover_chassis(duthosts)
                else:
                    results = parallel_run(__dut_reload, (), {"duts_data": duts_data}, duthosts, timeout=360)
                    logger.debug('Results of dut reload: {}'.format(json.dumps(dict(results))))

With SafeThreadPoolExecutor and save the config in memory back to config_db.json and config_db{}.json

@yejianquan @yutongzhang-microsoft could you check the latest change to see if that's what you both were thinking?
Note that this is a more wide-spread change as it could be run on any test whereas the change on _backup_and_restore_config_db only impacted the tests which used that fixture, so please take a careful look and consider if this could cause problems on different topologies or platforms.

Also, in case there are performance concerns, I benchmarked the config_db file copying on a T2 and it takes 15s for all the duthost.copy comands to run, so that shouldn't be a concern.

mssonicbld · 2024-12-19T01:12:56Z

/azp run

azure-pipelines · 2024-12-19T01:13:06Z

Azure Pipelines successfully started running 1 pipeline(s).

tests/conftest.py

mssonicbld · 2024-12-19T06:36:12Z

/azp run

azure-pipelines · 2024-12-19T06:36:23Z

Azure Pipelines successfully started running 1 pipeline(s).

yejianquan

LGTM

arista-nwolfe · 2024-12-19T17:05:08Z

/azpw run Azure.sonic-mgmt

mssonicbld · 2024-12-19T17:05:12Z

/AzurePipelines run Azure.sonic-mgmt

azure-pipelines · 2024-12-19T17:05:24Z

Azure Pipelines successfully started running 1 pipeline(s).

…et#16002) Details of failure described in more detail in sonic-net#15991 Created a new fixture backup_and_restore_config_db_frontend which uses enum_rand_one_per_hwsku_frontend_hostname so test_counterpoll_watermark.py can run it. Updated _backup_and_restore_config_db to work on multi-asic LCs Summary: Fixes sonic-net#15991 co-authorized by: [email protected]

mssonicbld · 2024-12-27T21:35:11Z

Cherry-pick PR to 202405: #16253

mssonicbld · 2024-12-27T21:35:12Z

Cherry-pick PR to 202411: #16254

Details of failure described in more detail in #15991 Created a new fixture backup_and_restore_config_db_frontend which uses enum_rand_one_per_hwsku_frontend_hostname so test_counterpoll_watermark.py can run it. Updated _backup_and_restore_config_db to work on multi-asic LCs Summary: Fixes #15991 co-authorized by: [email protected]

Fix config_db backup in test_counterpoll_watermark.py

239af90

Also fix '_backup_and_restore_config_db' to work on multi-asic lcs

arista-nwolfe requested a review from prgeor as a code owner December 11, 2024 02:38

Fix conditional failing linter

fb72287

rlhui requested a review from liamkearney-msft December 14, 2024 05:58

rlhui assigned arista-nwolfe Dec 14, 2024

liamkearney-msft reviewed Dec 16, 2024

View reviewed changes

liamkearney-msft approved these changes Dec 16, 2024

View reviewed changes

yejianquan requested changes Dec 16, 2024

View reviewed changes

Removed unused includes

725eedd

yutongzhang-microsoft reviewed Dec 19, 2024

View reviewed changes

tests/conftest.py Show resolved Hide resolved

arista-nwolfe requested a review from yejianquan December 19, 2024 06:31

Adding extra spaces to fix linter issues

b6fb4a9

yejianquan approved these changes Dec 19, 2024

View reviewed changes

yejianquan merged commit 3ab400f into sonic-net:master Dec 27, 2024
16 checks passed

yejianquan added Approved for 202405 branch Request for 202405 branch Request for 202411 branch Approved for 202411 branch labels Dec 27, 2024

mssonicbld added the Created PR to 202405 branch label Dec 27, 2024

mssonicbld added the Created PR to 202411 branch label Dec 27, 2024

This was referenced Dec 27, 2024

[action] [PR:16002] Fix config_db.json backup in test_counterpoll_watermark.py #16253

Merged

[action] [PR:16002] Fix config_db.json backup in test_counterpoll_watermark.py #16254

Merged

mssonicbld added Included in 202405 branch and removed Created PR to 202405 branch labels Dec 28, 2024

mssonicbld added Included in 202411 branch and removed Created PR to 202411 branch labels Jan 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix config_db.json backup in `test_counterpoll_watermark.py` #16002

Fix config_db.json backup in `test_counterpoll_watermark.py` #16002

arista-nwolfe commented Dec 11, 2024

mssonicbld commented Dec 11, 2024

yxieca commented Dec 11, 2024

arista-nwolfe commented Dec 11, 2024

yutongzhang-microsoft commented Dec 12, 2024 •

edited

Loading

arista-nwolfe commented Dec 12, 2024

yejianquan commented Dec 13, 2024 •

edited

Loading

yejianquan commented Dec 13, 2024 •

edited

Loading

yutongzhang-microsoft commented Dec 13, 2024 •

edited

Loading

yejianquan commented Dec 13, 2024

arista-nwolfe commented Dec 13, 2024 •

edited

Loading

yejianquan commented Dec 13, 2024 •

edited

Loading

liamkearney-msft Dec 16, 2024

yejianquan left a comment •

edited

Loading

yutongzhang-microsoft commented Dec 16, 2024

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

arista-nwolfe commented Dec 19, 2024

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

yejianquan left a comment

arista-nwolfe commented Dec 19, 2024

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

mssonicbld commented Dec 27, 2024

mssonicbld commented Dec 27, 2024

Fix config_db.json backup in test_counterpoll_watermark.py #16002

Fix config_db.json backup in test_counterpoll_watermark.py #16002

Conversation

arista-nwolfe commented Dec 11, 2024

Type of change

Back port request

mssonicbld commented Dec 11, 2024

yxieca commented Dec 11, 2024

arista-nwolfe commented Dec 11, 2024

yutongzhang-microsoft commented Dec 12, 2024 • edited Loading

arista-nwolfe commented Dec 12, 2024

yejianquan commented Dec 13, 2024 • edited Loading

yejianquan commented Dec 13, 2024 • edited Loading

yutongzhang-microsoft commented Dec 13, 2024 • edited Loading

yejianquan commented Dec 13, 2024

arista-nwolfe commented Dec 13, 2024 • edited Loading

yejianquan commented Dec 13, 2024 • edited Loading

liamkearney-msft Dec 16, 2024

Choose a reason for hiding this comment

yejianquan left a comment • edited Loading

Choose a reason for hiding this comment

yutongzhang-microsoft commented Dec 16, 2024

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

arista-nwolfe commented Dec 19, 2024

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

yejianquan left a comment

Choose a reason for hiding this comment

arista-nwolfe commented Dec 19, 2024

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

mssonicbld commented Dec 27, 2024

mssonicbld commented Dec 27, 2024

Fix config_db.json backup in `test_counterpoll_watermark.py` #16002

Fix config_db.json backup in `test_counterpoll_watermark.py` #16002

yutongzhang-microsoft commented Dec 12, 2024 •

edited

Loading

yejianquan commented Dec 13, 2024 •

edited

Loading

yejianquan commented Dec 13, 2024 •

edited

Loading

yutongzhang-microsoft commented Dec 13, 2024 •

edited

Loading

arista-nwolfe commented Dec 13, 2024 •

edited

Loading

yejianquan commented Dec 13, 2024 •

edited

Loading

yejianquan left a comment •

edited

Loading