[GCU] Complete RDMA Platform Validation Checks #2791

isabelmsft · 2023-04-13T01:25:31Z

What I did

Add RDMA validator to ensure GCU is being used on allowable platform and version for RDMA use cases.
The allowable platforms and versions are determined by this table:

The last two rows were completed and infrastructure set up in the earlier PR #2619

How I did it

How to verify it

Attempt to use GCU on vs and modify PFC_WD table

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

GCU patch application improperly goes through

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

Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: Modification of /PFC_WD table is illegal due to corresponding validating function rdma_config_update_validator returning false

neethajohn · 2023-04-13T20:54:50Z

generic_config_updater/field_operation_validators.py

-    else:
-        return branch_version >= "20181100"
+    # PFCWD enable/disable scenario
+    if "pfc_wd" in field:


suggest to build a dictionary of the fields and pre-populate it with all the restrictions and write a generic function to evaluate it.
something similar to the eg. below
field = {'pfcwd': {'all': '201811',
'cisco-8000': '202012'
}}
'

Thanks for the suggestion, I added this to the gcu_field_operation_validators.conf.json file

linux-foundation-easycla · 2023-04-26T04:28:01Z

The committers listed above are authorized under a signed CLA.

✅ login: isabelmsft (fc6d3eb, be38e33, 2c6dbd1, c367995, ceecb48, a27827f, f54c0cf, 79ed8df, 065340f, 76a90ce, 5031d8b)

qiluo-msft · 2023-04-26T21:25:29Z

generic_config_updater/gu_common.py

            match = re.search(r'\/([^\/]+)(\/|$)', path) # This matches the table name in the path, eg if path if /PFC_WD/GLOBAL, the match would be PFC_WD
            if match is not None:
-                table = match.group(1)
+                table = match.group(1).lower()


lower()

Why change to lowercases? #Closed

I changed it back to default text which would be upper case, and also changed the config file we are matching against to have table names in upper case

generic_config_updater/gcu_field_operation_validators.conf.json

qiluo-msft · 2023-04-26T22:19:56Z

generic_config_updater/field_operation_validators.py

+        asic = "cisco-8000"
+    elif device_info.get_sonic_version_info()['asic_type'] == 'mellanox':
+        GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku"
+        spc1_hwskus = [ 'ACS-MSN2700', 'ACS-MSN2740', 'ACS-MSN2100', 'ACS-MSN2410', 'ACS-MSN2010', 'Mellanox-SN2700', 'Mellanox-SN2700-D48C8' ]


spc1_hwskus

These string are hard-coded so not future proof. Better to move into gcu_field_operation_validators.conf.json. #Closed

Moved to gcu_field_operation_validators.conf.json.

qiluo-msft · 2023-04-26T22:21:03Z

generic_config_updater/field_operation_validators.py

+        if hwsku.lower() in [spc1_hwsku.lower() for spc1_hwsku in spc1_hwskus]:
+            asic = "spc1"
+    elif device_info.get_sonic_version_info()['asic_type'] == 'broadcom':
+        command = ["sudo", "lspci"]


command

Runtime check is heavy. We could build the mapping offline, and move add the hareware skus into gcu_field_operation_validators.conf.json.

Then in runtime, you just check hwsku. #Closed

Moved mapping to gcu_field_operation_validators.conf.json.

qiluo-msft · 2023-04-26T22:22:31Z

generic_config_updater/field_operation_validators.py

    version_info = device_info.get_sonic_version_info()
    build_version = version_info.get('build_version')
-    asic_type = version_info.get('asic_type')
+    asic = get_asic_name()
+    path = path.lower()


lower

Why change to lowercases? #Closed

For consistency, because all fields/paths other than the table in the conf file are lowercase. But in ConfigDB, some fields are uppercase (like AZURE_LOSSLESS)

qiluo-msft · 2023-04-26T22:23:11Z

generic_config_updater/field_operation_validators.py

-    if (asic_type != 'mellanox' and asic_type != 'broadcom' and asic_type != 'cisco-8000'):
+    # For paths like /BUFFER_PROFILE/pg_lossless_50000_300m_profile/xoff, remove pg_lossless_50000_300m from the path so that we can clearly determine which fields are modifiable
+    cleaned_path = "/".join([part for part in path.split("/") if not any(char.isdigit() for char in part)])
+    if asic == "unknown":


==

You can compare and return earlier. #Closed

Moved comparison to the start of validator function

qiluo-msft · 2023-04-26T23:04:43Z

generic_config_updater/field_operation_validators.py


-    if (asic_type != 'mellanox' and asic_type != 'broadcom' and asic_type != 'cisco-8000'):
+    # For paths like /BUFFER_PROFILE/pg_lossless_50000_300m_profile/xoff, remove pg_lossless_50000_300m from the path so that we can clearly determine which fields are modifiable


determine

Not quite understand why removing help determine. #Closed

I changed comment to possibly a clearer example: # For paths like /PFC_WD/Ethernet112/action, remove Ethernet112 from the path so that we can clearly determine the relevant field (i.e. action, not Ethernet112)

The conf file only saves action because that's the relevant field. Any part of the path with an integer is not relevant, so I don't save that in the conf file. Alternatively, I could change the conf file to define the regex pattern accepted for paths that include integers.

qiluo-msft · 2023-04-26T23:06:38Z

generic_config_updater/field_operation_validators.py


-    if (asic_type != 'mellanox' and asic_type != 'broadcom' and asic_type != 'cisco-8000'):
+    # For paths like /BUFFER_PROFILE/pg_lossless_50000_300m_profile/xoff, remove pg_lossless_50000_300m from the path so that we can clearly determine which fields are modifiable
+    cleaned_path = "/".join([part for part in path.split("/") if not any(char.isdigit() for char in part)])


split

Manually split or join is dangerous, you do not consider escaping, etc. Suggest leverage some libraries, such as https://pypi.org/project/jsonpointer/. #Closed

Changed to use jsonpointer library

qiluo-msft · 2023-04-26T23:06:59Z

generic_config_updater/field_operation_validators.py

+    else:
+        raise GenericConfigUpdaterError("GCU table modification validators config file not found")
+
+    match = re.search(r'\/([^\/]+)(\/|$)', cleaned_path) # This matches the table name in the path, eg if path if /PFC_WD/GLOBAL, the match would be PFC_WD


search

Suggest use library instead of manually parsing. #Closed

Changed to use jsonpointer library

### Description of PR Summary: There is a dependency between E2E test improvements PR: #8341 and sonic-utilities GCU RDMA feature PR: sonic-net/sonic-utilities#2791 This PR skips the RDMA tests that will fail when the platform GCU RDMA feature PR is merged. After the GCU RDMA feature PR is merged and backported to the appropriate branch, we will revert this PR to stop skipping the E2E tests, and merge the E2E test improvements PR so that the tests will pass once the feature changes take effect.

neethajohn · 2023-05-19T16:32:45Z

generic_config_updater/gcu_field_operation_validators.conf.json

+                            "td2": "20181100",
+                            "th": "20181100",
+                            "th2": "20181100",
+                            "td3": "20181100",


realized there is an error in my doc. this should be 202012. td3 is supported only from that release.

This reverts commit f258e2a.

StormLiangMS · 2023-05-30T14:26:35Z

@isabelmsft revert this PR since it failed the PR test and blocked the submodule advance of sonic-utilities, pls check the issue and re-submit.

This reverts commit f258e2a.

…" (#2854)" This reverts commit 69abbc3.

…" (sonic-net#2854) This reverts commit f258e2a.

### Description of PR Summary: There is a dependency between E2E test improvements PR: sonic-net#8341 and sonic-utilities GCU RDMA feature PR: sonic-net/sonic-utilities#2791 This PR skips the RDMA tests that will fail when the platform GCU RDMA feature PR is merged. After the GCU RDMA feature PR is merged and backported to the appropriate branch, we will revert this PR to stop skipping the E2E tests, and merge the E2E test improvements PR so that the tests will pass once the feature changes take effect.

complete rdma validation checks

fc6d3eb

neethajohn reviewed Apr 13, 2023

View reviewed changes

isabelmsft added 10 commits April 19, 2023 06:05

change json structure

be38e33

cosmetic changes

2c6dbd1

add UT

c367995

remove duplicate tests

ceecb48

clean spacing

a27827f

fix signatures

f54c0cf

add UT

79ed8df

clean UT

065340f

reorganize

76a90ce

reorg

5031d8b

isabelmsft force-pushed the rdma_platform_enforcement_complete branch from 0f90ed2 to 5031d8b Compare April 26, 2023 05:28

isabelmsft added 4 commits April 26, 2023 06:13

fix UT

d9c37f6

make case consistent

a93d1d1

fix code

5a8b5f9

fix UT

81528eb

isabelmsft requested a review from qiluo-msft April 26, 2023 21:06

qiluo-msft reviewed Apr 26, 2023

View reviewed changes

generic_config_updater/gcu_field_operation_validators.conf.json Show resolved Hide resolved

qiluo-msft reviewed Apr 26, 2023

View reviewed changes

isabelmsft requested a review from wen587 May 4, 2023 04:33

address PR comments

505ff3b

isabelmsft dismissed qiluo-msft’s stale review via 94bb81b May 13, 2023 04:20

isabelmsft mentioned this pull request May 17, 2023

[GCU E2E Testing] Update RDMA GCU E2E Tests sonic-net/sonic-mgmt#8341

Merged

6 tasks

qiluo-msft previously approved these changes May 17, 2023

View reviewed changes

isabelmsft mentioned this pull request May 18, 2023

Skip RDMA tests on specific hwsku based on support sonic-net/sonic-mgmt#8343

Merged

6 tasks

wen587 previously approved these changes May 18, 2023

View reviewed changes

neethajohn reviewed May 19, 2023

View reviewed changes

Update gcu_field_operation_validators.conf.json

f595095

isabelmsft dismissed stale reviews from wen587 and qiluo-msft via f595095 May 19, 2023 18:35

isabelmsft marked this pull request as ready for review May 23, 2023 21:49

qiluo-msft approved these changes May 24, 2023

View reviewed changes

isabelmsft merged commit f258e2a into sonic-net:master May 24, 2023

StormLiangMS added a commit that referenced this pull request May 30, 2023

Revert "[GCU] Complete RDMA Platform Validation Checks (#2791)"

1be0c30

This reverts commit f258e2a.

StormLiangMS added a commit that referenced this pull request May 30, 2023

Revert "[GCU] Complete RDMA Platform Validation Checks (#2791)"

263b6fb

This reverts commit f258e2a.

StormLiangMS mentioned this pull request May 30, 2023

Revert "[GCU] Complete RDMA Platform Validation Checks" #2854

Merged

StormLiangMS added a commit that referenced this pull request May 30, 2023

Revert "[GCU] Complete RDMA Platform Validation Checks (#2791)" (#2854)

69abbc3

This reverts commit f258e2a.

isabelmsft added a commit that referenced this pull request May 31, 2023

Revert "Revert "[GCU] Complete RDMA Platform Validation Checks (#2791)…

836496b

…" (#2854)" This reverts commit 69abbc3.

isabelmsft added Request for 202205 Branch and removed Request for 202205 Branch labels Jun 17, 2023

isabelmsft mentioned this pull request Jun 23, 2023

[GCU E2E Testing] Update RDMA GCU E2E Tests- MMU Config sonic-net/sonic-mgmt#8703

Open

6 tasks

pdhruv-marvell pushed a commit to pdhruv-marvell/sonic-utilities that referenced this pull request Aug 23, 2023

[GCU] Complete RDMA Platform Validation Checks (sonic-net#2791)

60c535e

pdhruv-marvell pushed a commit to pdhruv-marvell/sonic-utilities that referenced this pull request Aug 23, 2023

Revert "[GCU] Complete RDMA Platform Validation Checks (sonic-net#2791)…

1c67342

…" (sonic-net#2854) This reverts commit f258e2a.

rbpittman mentioned this pull request Oct 11, 2023

GCU support for Cisco-8000 features #3010

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GCU] Complete RDMA Platform Validation Checks #2791

[GCU] Complete RDMA Platform Validation Checks #2791

isabelmsft commented Apr 13, 2023 •

edited

Loading

neethajohn Apr 13, 2023

isabelmsft Apr 20, 2023

linux-foundation-easycla bot commented Apr 26, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading

isabelmsft May 10, 2023

qiluo-msft Apr 26, 2023 •

edited

Loading

isabelmsft May 10, 2023

qiluo-msft Apr 26, 2023 •

edited

Loading

isabelmsft May 10, 2023

qiluo-msft Apr 26, 2023 •

edited

Loading

isabelmsft May 10, 2023

qiluo-msft Apr 26, 2023 •

edited

Loading

isabelmsft May 10, 2023

qiluo-msft Apr 26, 2023 •

edited

Loading

isabelmsft May 10, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading

isabelmsft May 10, 2023

qiluo-msft Apr 26, 2023 •

edited

Loading

isabelmsft May 10, 2023

neethajohn May 19, 2023

isabelmsft May 19, 2023

StormLiangMS commented May 30, 2023


		if (asic_type != 'mellanox' and asic_type != 'broadcom' and asic_type != 'cisco-8000'):
		# For paths like /BUFFER_PROFILE/pg_lossless_50000_300m_profile/xoff, remove pg_lossless_50000_300m from the path so that we can clearly determine which fields are modifiable

[GCU] Complete RDMA Platform Validation Checks #2791

[GCU] Complete RDMA Platform Validation Checks #2791

Conversation

isabelmsft commented Apr 13, 2023 • 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)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Apr 26, 2023 • edited Loading

qiluo-msft Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

isabelmsft May 10, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Apr 26, 2023 • 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

StormLiangMS commented May 30, 2023

isabelmsft commented Apr 13, 2023 •

edited

Loading

linux-foundation-easycla bot commented Apr 26, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading

isabelmsft May 10, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading

qiluo-msft Apr 26, 2023 •

edited

Loading