Create a test case for testing L2 configuration. #14714

hdwhdw · 2024-09-24T19:57:36Z

Reference: https://github.com/sonic-net/SONiC/wiki/L2-Switch-mode

Description of PR

Create a test case for configuring switch to L2 mode.

Summary:
Add a test case for writing L2 configuration into config DB. This is precursor to a testcase requested in
issue 8777 #8777

Fixes # (issue)

Type of change

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

Back port request

Approach

Add a test case

What is the motivation for this PR?

Address #8777

How did you do it?

How did you verify/test it?

Ran on virtual switch.

Any platform specific information?

No.

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

Any.

Documentation

mssonicbld · 2024-09-26T18:31:52Z

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/l2/test_l2_configure.py:52:1: E302 expected 2 blank lines, found 1 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>

mssonicbld · 2024-09-26T18:39:02Z

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/l2/test_l2_configure.py:52:1: E302 expected 2 blank lines, found 1 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>

qiluo-msft · 2024-09-27T06:15:17Z

tests/l2/test_l2_configure.py

+        " | sudo config load /dev/stdin -y".format(hwsku)
+    duthost.shell(l2_cfg)
+    duthost.shell("sudo config qos reload --no-dynamic-buffer")
+    time.sleep(30)


time.sleep(30)

time.sleep(30)

We should try best to remove sleeping for fixed time:

if too long, waste overall test runtime

if too short, test may be flaky on some weaker hwsku

Could you use function "wait_until" for a special signal? You may use redis-cli monitor to observe what is really happening.

Or maybe you do not need to sleep at all? #Closed

I removed this. This is replaced by a reboot (which is one step in the expected test case).

qiluo-msft · 2024-09-27T06:15:25Z

tests/l2/test_l2_configure.py

+    # Restore from L2
+    logger.info("Restoring DUT into L3 mode.")
+    duthost.shell("sudo config reload -y")
+    time.sleep(30)


time.sleep(30)

time.sleep(30)

The same. #Closed

Removed. config_reload api resolve this issue.

qiluo-msft · 2024-09-27T06:18:47Z

tests/l2/test_l2_configure.py

+    new_vlan = duthost.shell("show vlan config")["stdout"]
+    new_int = duthost.shell("show int status")["stdout"]
+    pytest_expect(orig_vlan != new_vlan, "vlan config not updated.")
+    pytest_expect(orig_int != new_int, "interface status not updated.")


pytest_expect

pytest_expect

I do not understand these expect test.

Since previously we have a bugfix, we should test RESTAPI, TELEMETRY, DEVICE_METADATA tables are not in ConfigDB after reboot
#Closed

ref: sonic-net/sonic-utilities#2887

Sorry please take this commit as a draft. I updated the test expectation to enforce the expectation you said. Please take a look.

ganglyu · 2024-09-27T07:24:35Z

Step 3 is not included in this test.
#8777

Cold reboot a device.
Perform L2 config steps via L2 Switch mode · sonic-net/SONiC Wiki (github.com)
Install target image.
Cold reboot
Check config_db

ganglyu · 2024-09-27T07:34:15Z

tests/l2/test_l2_configure.py

+    duthost.shell(l2_cfg)
+    duthost.shell("sudo config qos reload --no-dynamic-buffer")
+    duthost.shell("sudo config save -y")
+    reboot(duthost, localhost)


After cold rebooting, the image version remains unchanged, and the db_migrator will not function. Consequently, the CONFIG_DB will remain the same.
Therefore, this test cannot verify our bug fix.

Updated test. Please take a look.

wen587 · 2024-09-27T13:55:01Z

tests/l2/test_l2_configure.py

+
+    # Restore from L2
+    logger.info("Restoring DUT into L3 mode.")
+    duthost.shell("sudo config reload -y")


we have config_reload API to guarantee some critical service is up

wen587 · 2024-09-27T13:56:39Z

tests/l2/test_l2_configure.py

+    logger.info("Restoring DUT into L3 mode.")
+    duthost.shell("sudo config reload /tmp/orig_config.json -y")
+    duthost.shell("sudo config save -y")
+    time.sleep(30)


Same as the above comment.

xincunli-sonic · 2024-09-27T16:23:45Z

tests/l2/test_l2_configure.py

+    logger.info("Checking vlan and interface facts.")
+    new_vlan = duthost.shell("show vlan config")["stdout"]
+    new_int = duthost.shell("show int status")["stdout"]
+    pytest_expect(orig_vlan != new_vlan, "vlan config not updated.")


pytest_expect

please use this structure:

try:
pytest assert ...
except:
optional block
Handling of exception (if required)
else:
execute if no exception, in your case may not needed.
finally:
Restore old config (always executed)

xincunli-sonic · 2024-09-27T16:24:31Z

tests/common/helpers/assertions.py

@@ -9,6 +9,16 @@ def pytest_assert(condition, message=None):
        pytest.fail(message)


+def pytest_expect(condition, message=None):


pytest_expect

use try catch finally instead of this way.

xincunli-sonic · 2024-09-27T16:30:10Z

tests/l2/test_l2_configure.py

+    logger.info("Checking vlan and interface facts.")
+    new_vlan = duthost.shell("show vlan config")["stdout"]
+    new_int = duthost.shell("show int status")["stdout"]
+    pytest_expect(orig_vlan != new_vlan, "vlan config not updated.")


orig_vlan != new_vlan

In L2, is there any specific vlan changed? Same to int, also I saw wiki mentioned QoS reload, are you going to check buffer related config as well?

Removed this check. This is not the focus of the test so I was just placing this here as a place holder.

xincunli-sonic · 2024-09-27T16:37:57Z

tests/l2/test_l2_configure.py

+    Args:
+        duthosts: set of DUTs.
+    """
+    # Setup.


Setup

try this way to setup

https://github.com/sonic-net/sonic-mgmt/blob/master/tests/generic_config_updater/test_aaa.py#L33

mssonicbld · 2024-10-01T18:28:33Z

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.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/l2/test_l2_configure.py fix end of files.........................................................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing tests/l2/conftest.py 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 ... [truncated extra lines, please run pre-commit locally to view full check results]

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>

hdwhdw · 2024-10-01T18:34:26Z

Updated the testcase according to comment. The test case should reflect what the issue is looking for.

Reference: https://github.com/sonic-net/SONiC/wiki/L2-Switch-mode

ganglyu · 2024-10-02T00:32:45Z

tests/l2/conftest.py

+# conftest.py for L2 configuration.
+
+
+def pytest_addoption(parser):


Nightly test is used to verify specified sonic version, if we upgrade sonic image in this test, we can't add test to nightly test.

This is what the issue is asking so if we can't upgrade, we need to find another way to trigger db_migrator?

Btw the test will restore the original image. Is that good enough?

Please confirm with Vaibhav, I guess we need to upgrade from 20191130.xx to 20240531.xx and verify L2 configuration.
If we do this in nightly test for 20240531.xx, how do you choose new image to upgrade?

I'll check with him. Maybe we should do a --source_image and --target_image. It will make more sense to upgrade from an older image to 20240531 in the nightly test.

Checked with Vaibhab, this test should be in nightly. I will rewrite part of the test according to test_upgrade_path.py to better accommodate that.

qiluo-msft · 2024-10-03T01:09:46Z

tests/l2/test_l2_configure.py

+    # Step 4: Verifies no config fro minigraph is written into ConfigDB.
+    try:
+        for table in ["TELEMETRY", "RESTAPI", "DEVICE_METADATA"]:
+            count = int(duthost.shell('redis-cli --scan --pattern "{}*" | wc -l'.format(table))['stdout'])


redis-cli

are you checking ConfigDB? by default it is checkin ApplDB.

suggest to use sonic-db-cli instead of redis-cli. #Closed

Looks like this reflect a real issue as I can see those unwanted table all the time, i.e. before l2, after l2, and after installing the image.

admin@vlab-01:~$ sonic-db-cli CONFIG_DB KEYS "TELEMETRY|*"
TELEMETRY|gnmi
TELEMETRY|certs

mssonicbld · 2024-10-08T19:13:25Z

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.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/remote_run_tests.sh 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/l2/test_l2_configure.py:65:52: F821 undefined name 'tbinfo' tests/l2/test_l2_configure.py:66:5: F841 local variable 'mg_ip' is assigned to but never used tests/l2/test_l2_configure.py:67:5: F841 local variable 'init_config' is assigned to but never used flake8...............................................(no files to check)Skipped ... [truncated extra lines, please run pre-commit locally to view full check results]

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>

ganglyu · 2024-10-29T02:16:46Z

tests/l2/test_l2_configure.py

+    for table in ["TELEMETRY", "RESTAPI"]:
+        pytest_assert(
+            is_table_empty(duthost, table), "{} table is not empty!".format(table)
+        )


Need to recover original configuration.

This is already done in the setup_env fixture in the same file.

qiluo-msft · 2024-10-29T21:43:11Z

tests/l2/test_l2_configure.py

+logger = logging.getLogger(__name__)
+
+pytestmark = [
+    pytest.mark.topology("any"),


any

any

we can limit to t0 topo. no need to test l2 mode on t1 topo. #Closed

Thanks! Done.

The test will now simply configure L2 and then check config db. TESTED: passed on 202012.66 (image with db_migrator fix) failed on 202012.76 (image without fix) failed at master head (with fixes, so indicates some other bugs)

hdwhdw · 2024-10-30T00:56:34Z

Force push to fix a commit message.

ganglyu · 2024-10-30T01:15:18Z

Can you check database version before and after config reload?

admin@str-msn2700-20:~$ sonic-db-cli CONFIG_DB hgetall "VERSIONS|DATABASE"
{'VERSION': 'version_202305_01'}

hdwhdw · 2024-10-30T02:16:07Z

@ganglyu done.

hdwhdw · 2024-10-30T02:47:25Z

@ganglyu
daweihuang@sonic-mgmt:/data/sonic-mgmt/tests$ cat logs/test.log | grep "Database version" 30/10/2024 02:09:22 test_l2_configure.test_no_hardcoded_mini L0147 INFO | Database version before L2 configuration reload: version_2_0_0 30/10/2024 02:12:20 test_l2_configure.test_no_hardcoded_mini L0153 INFO | Database version after L2 configuration reload: version_2_0_0

ganglyu

Database version is not changed after config reload, so db migrator should not change anything.
I don't understand why database is changed, but I will approve this PR for further investigation.

qiluo-msft · 2024-10-31T18:31:34Z

tests/l2/test_l2_configure.py

+                "gwaddr": "{}"
+            }}
+        }},
+        "DEVICE_METADATA": {{


DEVICE_METADATA

The step in test does not follow https://github.com/sonic-net/SONiC/wiki/L2-Switch-mode exactly. For example, DualToR is not considered. And the commands used does not exact match. #Closed

For the record, this comes from testbed_set_l2.yml. The wiki way requires running over console and I haven't find a reliable API in sonic-mgmt that pass the test. I'll see if there is any other way to make it work.

Spoke with Gang and Xin yesterday. And it appears the testbed_set_l2.yml method of setting up is a L2 mode is a workaround since ansible library requires the ssh session to be always active, and we don't have library support for doing it through console.

Reference: https://github.com/sonic-net/SONiC/wiki/L2-Switch-mode ### Description of PR Create a test case for configuring switch to L2 mode. Summary: Add a test case for writing L2 configuration into config DB. This is precursor to a testcase requested in issue 8777 sonic-net#8777 ### Approach Add a test case #### What is the motivation for this PR? Address sonic-net#8777 #### How did you do it? #### How did you verify/test it? Ran on virtual switch.

hdwhdw force-pushed the l2-mode branch from 5d88c41 to 4b78bf6 Compare September 26, 2024 18:38

hdwhdw force-pushed the l2-mode branch from 4b78bf6 to 07bf697 Compare September 26, 2024 18:55

qiluo-msft requested review from wen587, ganglyu and xincunli-sonic September 27, 2024 06:04

qiluo-msft reviewed Sep 27, 2024

View reviewed changes

ganglyu reviewed Sep 27, 2024

View reviewed changes

wen587 reviewed Sep 27, 2024

View reviewed changes

xincunli-sonic reviewed Sep 27, 2024

View reviewed changes

hdwhdw force-pushed the l2-mode branch from 07bf697 to 936dc53 Compare October 1, 2024 18:27

hdwhdw force-pushed the l2-mode branch from 936dc53 to 2f7b89d Compare October 1, 2024 18:36

Create test cases for testing L2 configuration.

3a729c4

Reference: https://github.com/sonic-net/SONiC/wiki/L2-Switch-mode

hdwhdw force-pushed the l2-mode branch from 2f7b89d to 3a729c4 Compare October 1, 2024 19:01

ganglyu reviewed Oct 2, 2024

View reviewed changes

qiluo-msft reviewed Oct 3, 2024

View reviewed changes

hdwhdw force-pushed the l2-mode branch 2 times, most recently from 66e89cc to 47d615f Compare October 10, 2024 20:34

hdwhdw requested a review from StormLiangMS as a code owner October 10, 2024 20:39

ganglyu suggested changes Oct 29, 2024

View reviewed changes

qiluo-msft previously approved these changes Oct 29, 2024

View reviewed changes

hdwhdw requested review from wen587, ganglyu, qiluo-msft and xincunli-sonic and removed request for wen587 and xincunli-sonic October 29, 2024 20:56

qiluo-msft reviewed Oct 29, 2024

View reviewed changes

qiluo-msft requested a review from vaibhavhd October 29, 2024 21:45

hdwhdw dismissed qiluo-msft’s stale review via 9aa8e70 October 29, 2024 23:03

hdwhdw requested a review from qiluo-msft October 29, 2024 23:05

hdwhdw added 2 commits October 30, 2024 00:53

Remove upgrade from the test workflow.

c1aebcd

The test will now simply configure L2 and then check config db. TESTED: passed on 202012.66 (image with db_migrator fix) failed on 202012.76 (image without fix) failed at master head (with fixes, so indicates some other bugs)

Limit the test to t0 topology.

56997e5

hdwhdw force-pushed the l2-mode branch from 9aa8e70 to 56997e5 Compare October 30, 2024 00:56

Address comment by adding check for db version.

58cb31c

ganglyu approved these changes Oct 30, 2024

View reviewed changes

qiluo-msft reviewed Oct 31, 2024

View reviewed changes

qiluo-msft approved these changes Nov 4, 2024

View reviewed changes

hdwhdw requested a review from xincunli-sonic November 4, 2024 23:53

qiluo-msft merged commit 9c12485 into sonic-net:master Nov 5, 2024
15 checks passed

qiluo-msft mentioned this pull request Dec 4, 2024

[test-gap] Add a testcase to validate L2 switch config and functionality across reboots/upgrades #8777

Closed

hdwhdw deleted the l2-mode branch January 7, 2025 15:37

		@@ -9,6 +9,16 @@ def pytest_assert(condition, message=None):
		pytest.fail(message)


		def pytest_expect(condition, message=None):

		# conftest.py for L2 configuration.


		def pytest_addoption(parser):

Create a test case for testing L2 configuration. #14714

Create a test case for testing L2 configuration. #14714

Conversation

hdwhdw commented Sep 24, 2024

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

mssonicbld commented Sep 26, 2024

mssonicbld commented Sep 26, 2024

qiluo-msft Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganglyu commented Sep 27, 2024 • edited Loading

ganglyu Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xincunli-sonic Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Setup

mssonicbld commented Oct 1, 2024

hdwhdw commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssonicbld commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdwhdw commented Oct 30, 2024

ganglyu commented Oct 30, 2024

hdwhdw commented Oct 30, 2024

hdwhdw commented Oct 30, 2024

ganglyu left a comment

Choose a reason for hiding this comment

qiluo-msft Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 27, 2024 •

edited

Loading

qiluo-msft Sep 27, 2024 •

edited

Loading

qiluo-msft Sep 27, 2024 •

edited

Loading

ganglyu commented Sep 27, 2024 •

edited

Loading

ganglyu Sep 27, 2024 •

edited

Loading

xincunli-sonic Sep 27, 2024 •

edited

Loading

qiluo-msft Oct 3, 2024 •

edited

Loading

qiluo-msft Oct 29, 2024 •

edited

Loading

qiluo-msft Oct 31, 2024 •

edited

Loading