Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[GCU] Handling non-compliant leaf-list with string values #2174

Merged
merged 3 commits into from
May 20, 2022
Merged

[GCU] Handling non-compliant leaf-list with string values #2174

merged 3 commits into from
May 20, 2022

Conversation

ghooo
Copy link
Contributor

@ghooo ghooo commented May 18, 2022 •
edited
Loading

What I did

Fixes #2170

Converting xpath to path:

  • There is only 1 case where the path is referring to the list itself. Example:
path="/BUFFER_PORT_EGRESS_PROFILE_LIST/Ethernet9/profile_list",
xpath="/sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet9']/profile_list",
  • There is no other case as we the list is just a single string, and we cannot refer to individual elements in string in ConfigDb.

Converting path to xpath, there are 2 cases:

  • Xpath is pointing to leaf-list as a whole, return path of whole list
xpath="/sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet9']/profile_list",
path="/BUFFER_PORT_EGRESS_PROFILE_LIST/Ethernet9/profile_list",
  • Xpath is pointing to an element of the leaf-list, return path of whole list as well since it is not possible to point to specific element -- here we are pointing to the element egress_lossy_profile
xpath="/sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet9']/profile_list[.='egress_lossy_profile']",
path="/BUFFER_PORT_EGRESS_PROFILE_LIST/Ethernet9/profile_list",

How I did it

Only a single change if the xpath is pointing to a specific element, just return whole list token from ConfigDb format.

Please note this solution is future proof, if ConfigDb changes how a list is saved as a string or json list, we don't care.

How to verify it

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)

@ghooo ghooo requested review from qiluo-msft and wen587 May 18, 2022 20:31
qiluo-msft
qiluo-msft reviewed May 18, 2022
View reviewed changes
generic_config_updater/gu_common.py
# xpath: /sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet9']/profile_list[.='egress_lossy_profile']
# path: /BUFFER_PORT_EGRESS_PROFILE_LIST/Ethernet9/profile_list
if isinstance(list_config, str):
return [leaf_list_name]
list_idx = list_config.index(leaf_list_value)
Copy link
Contributor

@qiluo-msft qiluo-msft May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

list_config

Do you want to add else if block? What is the expected type? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return type is a list of tokens (strings). I don't like adding else to an if that has return. I prefer to avoid indentation when possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. Then add a check of type "list"? and list_config is a misleading name since it could be str.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add description to PR. I think I should enrich the documentation in the code as well adding more examples for each return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@ghooo ghooo merged commit 4fc09b1 into sonic-net:master May 20, 2022
dprital added a commit to dprital/sonic-buildimage that referenced this pull request May 25, 2022
@dprital
[submodule] Update sonic-utilities submodule
657dd81 
Update sonic-utilities submodule pointer to include the following:
* [GCU] Handling type1 lists ([sonic-net#2171](sonic-net/sonic-utilities#2171))
* [yang] extend ConfigMgmt constructor to pass YANG options ([sonic-net#2118](sonic-net/sonic-utilities#2118))
* [dump] implement ACL modules ([sonic-net#2153](sonic-net/sonic-utilities#2153))
* show commands for SYSTEM READY ([sonic-net#1851](sonic-net/sonic-utilities#1851))
* [GCU] Handling non-compliant leaf-list with string values ([sonic-net#2174](sonic-net/sonic-utilities#2174))
* Add sonic-delayed.target to Application Extension .timer file generator ([sonic-net#2176](sonic-net/sonic-utilities#2176))
* [portconfig] Allow to configure interface mtu for physical ports ([#l](https://github.com/Azure/sonic-utilities/pull/l))
* Broadcast Unknown-multicast and Unknown-unicast Storm-control  ([sonic-net#928](sonic-net/sonic-utilities#928))
* sonic-utils: initial support for link-training ([sonic-net#2071](sonic-net/sonic-utilities#2071))
* [portchannel] Added ACL/PBH binding checks to the port before getting added to portchannel ([sonic-net#2151](sonic-net/sonic-utilities#2151))
* Modify override testcase to cover PORT admin_status ([sonic-net#2165](sonic-net/sonic-utilities#2165))
* [GCU] Validate peer_group_range ip_range are correct ([sonic-net#2145](sonic-net/sonic-utilities#2145))
* [auto-ts] add memory check ([sonic-net#2116](sonic-net/sonic-utilities#2116))
* support new interface types CR8/SR8/KR8/LR8 which are brougnt by SAI V.1.10.2 ([sonic-net#2167](sonic-net/sonic-utilities#2167))
* [scripts/fast-reboot] Add option to include ssd-upgrader-part boot option with SONiC partition ([sonic-net#2150](sonic-net/sonic-utilities#2150))
* [config reload] Fix invalid rstrip. ([sonic-net#2157](sonic-net/sonic-utilities#2157))
* Accept 0 for queue and dscp ([sonic-net#2162](sonic-net/sonic-utilities#2162))

Signed-off-by: dprital <drorp@nvidia.com>
@dprital dprital mentioned this pull request May 25, 2022
Closed
6 tasks
@ayurkiv-nvda ayurkiv-nvda mentioned this pull request Jun 10, 2022
Closed
6 tasks
yxieca pushed a commit that referenced this pull request Jun 17, 2022
@ghooo @yxieca
[GCU] Handling non-compliant leaf-list with string values (#2174)
6f4991e 
#### What I did
Fixes #2170

Converting xpath to path:
- There is only 1 case where the path is referring to the list itself. Example:
```
path="/BUFFER_PORT_EGRESS_PROFILE_LIST/Ethernet9/profile_list",
xpath="/sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet9']/profile_list",
```
- There is no other case as we the list is just a single string, and we cannot refer to individual elements in string in ConfigDb.

Converting path to xpath, there are 2 cases:
- Xpath is pointing to leaf-list as a whole, return path of whole list
```
xpath="/sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet9']/profile_list",
path="/BUFFER_PORT_EGRESS_PROFILE_LIST/Ethernet9/profile_list",
```
- Xpath is pointing to an element of the leaf-list, return path of whole list as well since it is not possible to point to specific element -- here we are pointing to the element `egress_lossy_profile`
```
xpath="/sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet9']/profile_list[.='egress_lossy_profile']",
path="/BUFFER_PORT_EGRESS_PROFILE_LIST/Ethernet9/profile_list",
```

#### How I did it
Only a single change if the xpath is pointing to a specific element, just return whole list token from ConfigDb format.

Please note this solution is future proof, if ConfigDb changes how a list is saved as a string or json list, we don't care.

#### How to verify it
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)
@ghooo ghooo mentioned this pull request Jun 24, 2022
Merged
@EdenGri EdenGri mentioned this pull request Sep 14, 2022
Merged
7 tasks
This was referenced Sep 14, 2022
Merged
Merged
@DavidZagury DavidZagury mentioned this pull request Sep 14, 2022
Closed
7 tasks
This was referenced Sep 14, 2022
Merged
Closed
This was referenced Sep 14, 2022
Merged
Merged
@oleksandrivantsiv oleksandrivantsiv mentioned this pull request Sep 14, 2022
Merged
@andywongarista andywongarista mentioned this pull request Sep 14, 2022
Merged
7 tasks
@dgsudharsan dgsudharsan mentioned this pull request Sep 15, 2022
Closed
7 tasks
@jimmyzhai jimmyzhai mentioned this pull request Sep 15, 2022
Merged
7 tasks
@sumukhatv sumukhatv mentioned this pull request Sep 15, 2022
Closed
7 tasks
@nazariig nazariig mentioned this pull request Sep 15, 2022
Closed
7 tasks
@volodymyrsamotiy volodymyrsamotiy mentioned this pull request Oct 3, 2022
Closed
7 tasks
@saiarcot895 saiarcot895 mentioned this pull request Oct 3, 2022
Merged
7 tasks
@andywongarista andywongarista mentioned this pull request Oct 3, 2022
Closed
7 tasks
@dgsudharsan dgsudharsan mentioned this pull request Oct 3, 2022
Merged
7 tasks
@andywongarista andywongarista mentioned this pull request Oct 3, 2022
Merged
7 tasks
@vdahiya12 vdahiya12 mentioned this pull request Oct 3, 2022
Merged
7 tasks
@msosyak msosyak mentioned this pull request Oct 4, 2022
Merged
7 tasks
@zbud-msft zbud-msft mentioned this pull request Oct 4, 2022
Merged
7 tasks
@vivekrnv vivekrnv mentioned this pull request Oct 4, 2022
Merged
7 tasks
@qiluo-msft qiluo-msft mentioned this pull request Oct 4, 2022
Merged
7 tasks
@WeichenChenACCTON WeichenChenACCTON mentioned this pull request Oct 5, 2022
Open
7 tasks
@gechiang gechiang mentioned this pull request Oct 5, 2022
Merged
7 tasks
@stepanblyschak stepanblyschak mentioned this pull request Oct 5, 2022
Closed
7 tasks
@theasianpianist theasianpianist mentioned this pull request Oct 5, 2022
Merged
7 tasks
This was referenced Oct 5, 2022
Merged
Merged
This was referenced Oct 11, 2022
Closed
Closed
kellyyeh pushed a commit to kellyyeh/sonic-buildimage that referenced this pull request Oct 19, 2022
@dgsudharsan @kellyyeh
[201911][mellanox] Extend Mellanox FW utils with CPLD update (sonic-n…
9bb83d2 
…et#12172)

Porting sonic-net#3723 to 201911

#### Why I did it
Extend Mellanox FW utils with CPLD update feature
Added support for CPLD upgrade to Mellanox FW utility


#### How I did it
Updated Mellanox FW utility

#### How to verify it
mlnx-fw-upgrade.sh --upgrade --cpld # Regular CPLD update flow
UPDATE_MLNX_CPLD_FW=1 mlnx-fw-upgrade.sh --upgrade # Force CPLD refresh only

#### Ensure to add label/tag for the feature raised. example - [PR#2174](sonic-net/sonic-utilities#2174) where, Generic Config and Update feature has been labelled as GCU.
This was referenced Oct 25, 2022
Closed
Merged
@praveenraja1 praveenraja1 mentioned this pull request Oct 28, 2022
Merged
7 tasks
liuh-80 added a commit to sonic-net/sonic-buildimage that referenced this pull request Nov 1, 2022
@liuh-80
[TACACS] Send remote address in TACACS+ authorization message. (#12190)
2626573 
Send remote address in TACACS+ authorization message.

#### Why I did it
TACACS+ authorization message not send remote address to server side.

#### How I did it
Send remote address in TACACS+ authorization message.

#### How to verify it
Pass all E2E test.
Create new test case to validate remote address been send to server side.

#### Which release branch to backport (provide reason below if selected)

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111
- [ ] 202205

#### Description for the changelog
Send remote address in TACACS+ authorization message.

#### Ensure to add label/tag for the feature raised. example - [PR#2174](sonic-net/sonic-utilities#2174) where, Generic Config and Update feature has been labelled as GCU.

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md
-->

#### A picture of a cute animal (not mandatory but encouraged)
@Junchao-Mellanox Junchao-Mellanox mentioned this pull request Nov 4, 2022
Merged
7 tasks
yxieca pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Nov 10, 2022
@liuh-80 @yxieca
[TACACS] Send remote address in TACACS+ authorization message. (#12190)
acfa4bb 
Send remote address in TACACS+ authorization message.

#### Why I did it
TACACS+ authorization message not send remote address to server side.

#### How I did it
Send remote address in TACACS+ authorization message.

#### How to verify it
Pass all E2E test.
Create new test case to validate remote address been send to server side.

#### Which release branch to backport (provide reason below if selected)

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111
- [ ] 202205

#### Description for the changelog
Send remote address in TACACS+ authorization message.

#### Ensure to add label/tag for the feature raised. example - [PR#2174](sonic-net/sonic-utilities#2174) where, Generic Config and Update feature has been labelled as GCU.

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md
-->

#### A picture of a cute animal (not mandatory but encouraged)
This was referenced Nov 14, 2022
Merged
Merged
This was referenced Feb 15, 2023
Merged
Merged
@liuh-80 liuh-80 mentioned this pull request Mar 15, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft approved these changes

@wen587 wen587 Awaiting requested review from wen587

Assignees
No one assigned
Labels
Included in 202205 Branch Request for 202205 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[GCU] Handle non-compliant leaf-list with string values
4 participants
@ghooo @qiluo-msft @wen587 @yxieca