[GCU]Remove GCU unique lane check for duplicate lanes platforms #2343

wen587 · 2022-08-31T06:56:21Z

What I did

Remove unique lane check for specific platforms to unblock GCU nightly test.

How I did it

Add platform check for unique lane validator

How to verify it

Run unit test

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

wen587 · 2022-08-31T07:31:30Z

"DPB_1_TO_4__SUCCESS" in patch_sorter_test will fail after unique lane check removed.
Temporary remove it to pass unit test.

Details below:
After removing the unique lane check, it directly adds duplicate lanes in Ethernet1~3 before removing lanes in Ethernet0.
I doubt if it is a valid move.

Expected change involving unique lane check

[
    [{"op": "replace", "path": "/PORT/Ethernet0/alias", "value": "Eth1/1"}], 
    [{"op": "replace", "path": "/PORT/Ethernet0/description", "value": ""}], 
    [{"op": "replace", "path": "/PORT/Ethernet0/speed", "value": "10000"}], 
    [{"op": "remove", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports"}], 
    [{"op": "remove", "path": "/ACL_TABLE"}], 
    [{"op": "add", "path": "/ACL_TABLE", "value": {"NO-NSW-PACL-V4": {"type": "L3"}}}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", "value": ["Ethernet0"]}], 
    [{"op": "remove", "path": "/VLAN_MEMBER"}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/policy_desc", "value": "NO-NSW-PACL-V4"}], 
    [{"op": "remove", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports"}], [{"op": "remove", "path": "/PORT"}], 
    [{"op": "add", "path": "/PORT", "value": {"Ethernet0": {"alias": "Eth1/1", "lanes": "65", "description": "", "speed": "10000"}}}], 
    [{"op": "add", "path": "/PORT/Ethernet3", "value": {"alias": "Eth1/4", "lanes": "68", "description": "", "speed": "10000"}}], 
    [{"op": "add", "path": "/PORT/Ethernet2", "value": {"alias": "Eth1/3", "lanes": "67", "description": "", "speed": "10000"}}], 
    [{"op": "add", "path": "/PORT/Ethernet1", "value": {"alias": "Eth1/2", "lanes": "66", "description": "", "speed": "10000"}}], 
    [{"op": "add", "path": "/VLAN_MEMBER", "value": {"Vlan100|Ethernet0": {"tagging_mode": "untagged"}}}], 
    [{"op": "add", "path": "/VLAN_MEMBER/Vlan100|Ethernet1", "value": {"tagging_mode": "untagged"}}], 
    [{"op": "add", "path": "/VLAN_MEMBER/Vlan100|Ethernet3", "value": {"tagging_mode": "untagged"}}], 
    [{"op": "add", "path": "/VLAN_MEMBER/Vlan100|Ethernet2", "value": {"tagging_mode": "untagged"}}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", "value": ["Ethernet0"]}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1", "value": "Ethernet1"}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/2", "value": "Ethernet2"}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/3", "value": "Ethernet3"}]
]

Actual change after remove unique lane check:

[
    [{"op": "add", "path": "/PORT/Ethernet3", "value": {"alias": "Eth1/4", "lanes": "68", "description": "", "speed": "10000"}}], 
    [{"op": "add", "path": "/PORT/Ethernet2", "value": {"alias": "Eth1/3", "lanes": "67", "description": "", "speed": "10000"}}], 
    [{"op": "add", "path": "/PORT/Ethernet1", "value": {"alias": "Eth1/2", "lanes": "66", "description": "", "speed": "10000"}}], 
    [{"op": "add", "path": "/VLAN_MEMBER/Vlan100|Ethernet1", "value": {"tagging_mode": "untagged"}}], 
    [{"op": "add", "path": "/VLAN_MEMBER/Vlan100|Ethernet3", "value": {"tagging_mode": "untagged"}}], 
    [{"op": "add", "path": "/VLAN_MEMBER/Vlan100|Ethernet2", "value": {"tagging_mode": "untagged"}}], 
    [{"op": "replace", "path": "/PORT/Ethernet0/alias", "value": "Eth1/1"}], 
    [{"op": "replace", "path": "/PORT/Ethernet0/description", "value": ""}], 
    [{"op": "replace", "path": "/PORT/Ethernet0/speed", "value": "10000"}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1", "value": "Ethernet1"}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/2", "value": "Ethernet2"}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/3", "value": "Ethernet3"}], 
    [{"op": "remove", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/0"}], 
    [{"op": "remove", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports"}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", "value": ["Ethernet1"]}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1", "value": "Ethernet2"}], 
    [{"op": "remove", "path": "/VLAN_MEMBER/Vlan100|Ethernet0"}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/0", "value": "Ethernet0"}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/3", "value": "Ethernet3"}], 
    [{"op": "remove", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/0"}], 
    [{"op": "remove", "path": "/PORT/Ethernet0"}], 
    [{"op": "add", "path": "/PORT/Ethernet0", "value": {"alias": "Eth1/1", "lanes": "65", "description": "", "speed": "10000"}}], 
    [{"op": "add", "path": "/VLAN_MEMBER/Vlan100|Ethernet0", "value": {"tagging_mode": "untagged"}}], 
    [{"op": "add", "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/0", "value": "Ethernet0"}]
]

qiluo-msft · 2022-08-31T18:58:16Z

def validate_lanes(self, config_db):

suggest add an extra parameter unique_lanes=True. and we can keep most of the test.

In reply to: 1233305890

Refers to: generic_config_updater/gu_common.py:136 in 72b8249. [](commit_id = 72b8249, deletion_comment = False)

lgtm-com · 2022-09-01T05:08:21Z

This pull request introduces 1 alert when merging 1c4096b into f0ce586 - view on LGTM.com

new alerts:

1 for Non-callable called

qiluo-msft · 2022-09-02T21:33:34Z

generic_config_updater/gu_common.py

-                success, error = supplemental_yang_validator(config_db_as_json)
+            for supplemental_yang_validator, validator_params in \
+                    zip(supplemental_yang_validators, validators_params):
+                success, error = supplemental_yang_validator(*validator_params)


supplemental_yang_validator

To make code more readable, you may use lambda in supplemental_yang_validators, and no need to zip with another array validators_params. #Closed

qiluo-msft · 2022-09-02T21:35:23Z

tests/generic_config_updater/patch_sorter_test.py

@@ -3034,6 +3034,9 @@ def test_patch_sorter_success(self):
        data = Files.PATCH_SORTER_TEST_SUCCESS
        skip_exact_change_list_match = False
        for test_case_name in data:
+            # DPB_1_TO_4__SUCCESS will fail when unique_lanes is set to False


fail

Why one less validator will make original passing testcase fail? #Closed

You can check above sorting output here #2343 (comment)
When lane validator is removed, the sorting order is changed. That's why it fails.

wen587 · 2022-09-14T01:51:52Z

generic_config_updater/gu_common.py

@@ -116,6 +116,8 @@ def validate_config_db_config(self, config_db_as_json):
        # TODO: Move these validators to YANG models
        supplemental_yang_validators = [self.validate_bgp_peer_group,
                                        self.validate_lanes]


Suggested change

self.validate_lanes]

lambda c: return self.validate_lanes(c, False)]

qiluo-msft · 2022-09-19T06:56:08Z

generic_config_updater/gu_common.py

@@ -133,7 +138,7 @@ def validate_config_db_config(self, config_db_as_json):

        return True, None

-    def validate_lanes(self, config_db):
+    def validate_lanes(self, config_db, dup_lanes_platforms=[]):


[]

This is a common bug. ref: https://stackoverflow.com/a/44272023/2514803 #Closed

qiluo-msft · 2022-09-19T06:58:15Z

generic_config_updater/gu_common.py

@@ -113,9 +113,14 @@ def validate_sonic_yang_config(self, sonic_yang_as_json):
    def validate_config_db_config(self, config_db_as_json):
        sy = self.create_sonic_yang_with_loaded_models()

+        dup_lanes_platforms = [


dup_lanes_platforms

you may move this block to inside validate_lanes function. Add a comment

# TODO: Move this attribute (platform with duplicated lanes in ports) to YANG models ``` #Closed

Could you add the TODO comment in my above comment?

qiluo-msft · 2022-09-19T21:11:47Z

Let's remove blank line only changes, so backport will be easier.

In reply to: 1251553830

Refers to: tests/generic_config_updater/files/patch_sorter_test_success.json:6161 in 6cb3a8c. [](commit_id = 6cb3a8c, deletion_comment = False)

Update sonic-utilities submodule pointer to include the following: * 0a7557b [minigraph] add option to specify golden path in load_minigraph ([sonic-net#2350](sonic-net/sonic-utilities#2350)) * 322aefc [GCU]Remove GCU unique lane check for duplicate lanes platforms ([sonic-net#2343](sonic-net/sonic-utilities#2343)) * 7099fff [fastboot] fastboot enhancement: Use warm-boot infrastructure for fast-boot ([sonic-net#2286](sonic-net/sonic-utilities#2286)) * 09026ed [warm-reboot] fix warm-reboot when /tmp/cache is missing ([sonic-net#2367](sonic-net/sonic-utilities#2367)) * a3c404c Fix typo in platform_sfputil_helper.is_rj45_port ([sonic-net#2374](sonic-net/sonic-utilities#2374)) * 637d834 Vnet_route_check Vxlan tunnel route update. ([sonic-net#2281](sonic-net/sonic-utilities#2281)) * 29a3e51 Added support for tunnel route status in show vnet routes all. ([sonic-net#2341](sonic-net/sonic-utilities#2341)) * 1ac584b Use 'default' VRF when VRF name is not provided ([sonic-net#2368](sonic-net/sonic-utilities#2368)) * 4d377a6 [subinterface]Added additional checks in portchannel and subinterface commands ([sonic-net#2345](sonic-net/sonic-utilities#2345)) * bbcdf2e disk_check: Publish event for RO state ([sonic-net#2320](sonic-net/sonic-utilities#2320)) * 3fd537b Support the bandit check by GitHub Action ([sonic-net#2358](sonic-net/sonic-utilities#2358)) * 491d3d3 [generate dump]Added error message when saisdkdump fails ([sonic-net#2356](sonic-net/sonic-utilities#2356)) * 6830e01 [counterpoll]Fixing counterpoll show for tunnel and acl stats ([sonic-net#2355](sonic-net/sonic-utilities#2355)) * 3be2ad7 [fast-reboot]Avoid stopping masked services during fast-reboot ([sonic-net#2335](sonic-net/sonic-utilities#2335)) * 0e1b0cf [GCU] Fix missing backend in dry run ([sonic-net#2347](sonic-net/sonic-utilities#2347)) * 676c31b Add verification for override ([sonic-net#2305](sonic-net/sonic-utilities#2305)) * 48997c2 Add Password Hardening CLI support ([sonic-net#2338](sonic-net/sonic-utilities#2338)) * 414e239 update unit tests for swap ([#locato](https://github.com/sonic-net/sonic-utilities/pull/locato)) * a91a492 consider swap checking memory in ([#stalle](https://github.com/sonic-net/sonic-utilities/pull/stalle)) * f0ce586 [route_check]: Ignore standalone tunnel routes ([sonic-net#2325](sonic-net/sonic-utilities#2325)) Signed-off-by: dprital <[email protected]>

What I did Remove unique lane check for specific platforms to unblock GCU nightly test. How I did it Add platform check for unique lane validator How to verify it Run unit test

Summary: Duplicate lane check was added in sonic-utilities. Remove that in sonic-mgmt. What is the motivation for this PR? The GCU-related tests were bypassed in duplicate lanes' platforms. We don't want to skip GCU tests on such platforms. So we update the unique lane check and simply bypass the check for such platforms in GCU feature. Now the GCU tests can be tested on such platforms. How did you do it? Fix in GCU feature. sonic-net/sonic-utilities#2343 has been updated to sonic-buildimage. How did you verify/test it? Run e2e test in specific platform.

…c-net#2343) What I did Remove unique lane check for specific platforms to unblock GCU nightly test. How I did it Add platform check for unique lane validator How to verify it Run unit test

Summary: Duplicate lane check was added in sonic-utilities. Remove that in sonic-mgmt. What is the motivation for this PR? The GCU-related tests were bypassed in duplicate lanes' platforms. We don't want to skip GCU tests on such platforms. So we update the unique lane check and simply bypass the check for such platforms in GCU feature. Now the GCU tests can be tested on such platforms. How did you do it? Fix in GCU feature. sonic-net/sonic-utilities#2343 has been updated to sonic-buildimage. How did you verify/test it? Run e2e test in specific platform.

…c-net#2343) What I did Remove unique lane check for specific platforms to unblock GCU nightly test. How I did it Add platform check for unique lane validator How to verify it Run unit test

Summary: Duplicate lane check was added in sonic-utilities. Remove that in sonic-mgmt. What is the motivation for this PR? The GCU-related tests were bypassed in duplicate lanes' platforms. We don't want to skip GCU tests on such platforms. So we update the unique lane check and simply bypass the check for such platforms in GCU feature. Now the GCU tests can be tested on such platforms. How did you do it? Fix in GCU feature. sonic-net/sonic-utilities#2343 has been updated to sonic-buildimage. How did you verify/test it? Run e2e test in specific platform.

…c-net#2343) What I did Remove unique lane check for specific platforms to unblock GCU nightly test. How I did it Add platform check for unique lane validator How to verify it Run unit test

Summary: Duplicate lane check was added in sonic-utilities. Remove that in sonic-mgmt. What is the motivation for this PR? The GCU-related tests were bypassed in duplicate lanes' platforms. We don't want to skip GCU tests on such platforms. So we update the unique lane check and simply bypass the check for such platforms in GCU feature. Now the GCU tests can be tested on such platforms. How did you do it? Fix in GCU feature. sonic-net/sonic-utilities#2343 has been updated to sonic-buildimage. How did you verify/test it? Run e2e test in specific platform.

Remove GCU unique lane check

72b8249

wen587 requested review from isabelmsft and qiluo-msft August 31, 2022 07:31

wen587 marked this pull request as ready for review August 31, 2022 07:31

Add unique_lanes flags and bring most test back

1c4096b

wen587 added 2 commits September 1, 2022 05:21

missing comma

c30e05f

fix test issue

8eab6fa

qiluo-msft reviewed Sep 2, 2022

View reviewed changes

wen587 commented Sep 14, 2022

View reviewed changes

resolve comment:

34bfc37

wen587 added the Request for 202205 Branch label Sep 14, 2022

skip unique lane check for dup_lanes_platforms

1f6d114

qiluo-msft reviewed Sep 19, 2022

View reviewed changes

wen587 added 2 commits September 19, 2022 07:04

resolve comment

6cb3a8c

add todo comment

7fb2c37

wen587 changed the title ~~[GCU]Remove GCU unique lane check~~ [GCU]Remove GCU unique lane check for duplicate lanes platforms Sep 19, 2022

rm unchanged file

b1f7c3a

qiluo-msft approved these changes Sep 20, 2022

View reviewed changes

wen587 merged commit 322aefc into sonic-net:master Sep 20, 2022

dprital mentioned this pull request Sep 21, 2022

[submodule] Advance sonic-utilities pointer sonic-net/sonic-buildimage#12146

Closed

7 tasks

yxieca added the Included in 202205 Branch label Sep 21, 2022

wen587 mentioned this pull request Oct 9, 2022

Remove bypass on duplicate lanes' platforms for GCU tests sonic-net/sonic-mgmt#6487

Merged

6 tasks

mssonicbld mentioned this pull request Aug 28, 2023

[action] [PR:6487] Remove bypass on duplicate lanes' platforms for GCU tests sonic-net/sonic-mgmt#9702

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GCU]Remove GCU unique lane check for duplicate lanes platforms #2343

[GCU]Remove GCU unique lane check for duplicate lanes platforms #2343

wen587 commented Aug 31, 2022 •

edited

Loading

wen587 commented Aug 31, 2022

qiluo-msft commented Aug 31, 2022 •

edited

Loading

lgtm-com bot commented Sep 1, 2022

qiluo-msft Sep 2, 2022 •

edited

Loading

qiluo-msft Sep 2, 2022 •

edited

Loading

wen587 Sep 5, 2022

wen587 Sep 14, 2022

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022

wen587 Sep 20, 2022

qiluo-msft commented Sep 19, 2022 •

edited

Loading

	self.validate_lanes]
	lambda c: return self.validate_lanes(c, False)]

[GCU]Remove GCU unique lane check for duplicate lanes platforms #2343

[GCU]Remove GCU unique lane check for duplicate lanes platforms #2343

Conversation

wen587 commented Aug 31, 2022 • edited Loading

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

wen587 commented Aug 31, 2022

qiluo-msft commented Aug 31, 2022 • edited Loading

lgtm-com bot commented Sep 1, 2022

qiluo-msft Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

wen587 Sep 5, 2022

Choose a reason for hiding this comment

wen587 Sep 14, 2022

Choose a reason for hiding this comment

qiluo-msft Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 19, 2022

Choose a reason for hiding this comment

wen587 Sep 20, 2022

Choose a reason for hiding this comment

qiluo-msft commented Sep 19, 2022 • edited Loading

wen587 commented Aug 31, 2022 •

edited

Loading

qiluo-msft commented Aug 31, 2022 •

edited

Loading

qiluo-msft Sep 2, 2022 •

edited

Loading

qiluo-msft Sep 2, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft commented Sep 19, 2022 •

edited

Loading