Skip to content

[GCU] Add check to ensure operation is valid in RDMA validator#10241

Merged
isabelmsft merged 18 commits intosonic-net:masterfrom
isabelmsft:add_op_check_gcu_rdma
Oct 25, 2023
Merged

[GCU] Add check to ensure operation is valid in RDMA validator#10241
isabelmsft merged 18 commits intosonic-net:masterfrom
isabelmsft:add_op_check_gcu_rdma

Conversation

@isabelmsft
Copy link
Copy Markdown
Contributor

@isabelmsft isabelmsft commented Oct 6, 2023

Description of PR

Summary:
[GCU] Add check to ensure operation is valid in RDMA validator
ADO 25310959

Type of change

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

Back port request

  • 201911
  • 202012
  • 202205
  • 202305

Approach

What is the motivation for this PR?

Ensure that operation is allowed in RDMA config update validator

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

@isabelmsft isabelmsft marked this pull request as draft October 6, 2023 17:50
@bingwang-ms
Copy link
Copy Markdown
Collaborator

The most tricky thing is, GCU will convert add into replace if the entry you are going to add is present.
So, even if add is not supported in the gcu validator, the operation can succeed because add is converted into replace, and replace is supported.
So we may add some logic to check if the entry has been there, and change operation from add to replace if present.

@isabelmsft isabelmsft marked this pull request as ready for review October 13, 2023 03:17
@qiluo-msft qiluo-msft requested a review from neethajohn October 18, 2023 21:01
tmpfile = generate_tmpfile(duthost)
output = apply_patch(duthost, json_data=json_patch, dest_file=tmpfile)
if is_valid_platform_and_version(duthost, "PFC_WD", "PFCWD enable/disable"):
if is_valid_platform_and_version(duthost, "PFC_WD", "PFCWD enable/disable", "add"):
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Oct 18, 2023

Choose a reason for hiding this comment

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

add

If this add depends on above op, better to use variable and do not repeat constant strings. #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test case only tests 'add', I moved add to be in variable op which would help if we extend the test case

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM. Please check with other active reviewers.

@bingwang-ms
Copy link
Copy Markdown
Collaborator

LGTM. Also added the label for 202305 branch.

@isabelmsft isabelmsft merged commit 630d0a5 into sonic-net:master Oct 25, 2023
@bingwang-ms
Copy link
Copy Markdown
Collaborator

MSFT ADO 25264431

@mssonicbld
Copy link
Copy Markdown
Collaborator

@isabelmsft PR conflicts with 202305 branch

@StormLiangMS
Copy link
Copy Markdown
Collaborator

@isabelmsft cherry pick conflict, could you help to check?

isabelmsft added a commit to isabelmsft/sonic-mgmt that referenced this pull request Oct 31, 2023
@isabelmsft
Copy link
Copy Markdown
Contributor Author

isabelmsft commented Oct 31, 2023

@isabelmsft cherry pick conflict, could you help to check?

@StormLiangMS Opened PR #10534 . Cherry-pick conflict was due to #10214 missing from 202305. PR 10534 includes both necessary cherry-picks to 202305

qiluo-msft pushed a commit that referenced this pull request Feb 12, 2024
## Description of PR

Summary:
Fixes # (issue)

### Type of change

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

## Approach
#### What is the motivation for this PR?
ADO 17747379 to cherry-pick sonic-utilities PR to 202205 sonic-net/sonic-utilities#3051 requires 202205 sonic-mgmt test update.

This PR cherry-picks the following PRs: #7981, #9878 (reads RDMA GCU conf file from DUT instead of copy of conf file in sonic-mgmt), #10214 (fix pfcwd test), #10241 (ensure operation is valid), #10756 (empty string edge case)
#### How did you do it?
Ensure the feature and tests are stable on 202305 and master
#### How did you verify/test it?
Monitored test results on 202305 and master for all platforms, manually ran tests on Cisco and Mellanox DUTs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants