Keep attribute order in bulk mode #1659
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qiluo-msft
reviewed
Mar 3, 2021
| auto it = rc.first; | ||
|
|
||
| // If inserted new key, assign the attr | ||
| // If found existing key, overwrite the old attr |
There was a problem hiding this comment.
If found existing key, overwrite the old attr [](start = 11, length = 45)
One advantage or map structure is possible to merge setting operation on the same entry and same attribute. Is it still required? #Closed
There was a problem hiding this comment.
I think it does not seem very necessary to merge the operations for the same attribute and the same entry because of the following reasons:
- The same entry should not show up more than once in one bulk.
- For each route entry, there are two attributes that might be set in bulk mode:
PACKET_ACTIONandNEXT_HOP_ID, each gets to set only once in the routeorch logic. - For other types of entries (not supported in bulk yet), the to_sync logic should already merge the same attribute for the same entry. It does not seem necessary to do it again in bulk.
|
what ? order should not matter at all in any way to any component including SAI |
It is reported in sonic-net/sonic-buildimage#6881 that the hardware has a contract that route action must not be DROP when setting next hops (otherwise it gets ignored). The order matters if this is the case since the action must be set first in this scenario. |
|
missing unit test? |
lguohan
previously approved these changes
Mar 5, 2021
qiluo-msft
previously approved these changes
Mar 5, 2021
qiluo-msft
reviewed
Mar 8, 2021
| } | ||
| }; | ||
|
|
||
| TEST_F(BulkerTest, BulkerAttrOrder) |
There was a problem hiding this comment.
BulkerTest [](start = 11, length = 10)
Did this unit test fail old code?
qiluo-msft
approved these changes
Mar 8, 2021
daall
pushed a commit
that referenced
this pull request
Mar 10, 2021
Keep attributes for the same entry in the order that the attributes are set in bulk mode. And add a unit test for the scenario in bulker. Some attributes have a dependency on others in SAI operations (e.g., setting nexthop requires packet action to be forward in route set operation). Without keeping the order of the attributes, the configuration may not be successfully applied to hardware. Therefore, it is necessary to keep the order of attributes in bulk mode.
Junchao-Mellanox
pushed a commit
to Junchao-Mellanox/sonic-swss
that referenced
this pull request
Mar 25, 2021
Keep attributes for the same entry in the order that the attributes are set in bulk mode. And add a unit test for the scenario in bulker. Some attributes have a dependency on others in SAI operations (e.g., setting nexthop requires packet action to be forward in route set operation). Without keeping the order of the attributes, the configuration may not be successfully applied to hardware. Therefore, it is necessary to keep the order of attributes in bulk mode.
raphaelt-nvidia
pushed a commit
to raphaelt-nvidia/sonic-swss
that referenced
this pull request
Oct 5, 2021
Keep attributes for the same entry in the order that the attributes are set in bulk mode. And add a unit test for the scenario in bulker. Some attributes have a dependency on others in SAI operations (e.g., setting nexthop requires packet action to be forward in route set operation). Without keeping the order of the attributes, the configuration may not be successfully applied to hardware. Therefore, it is necessary to keep the order of attributes in bulk mode.
EdenGri
pushed a commit
to EdenGri/sonic-swss
that referenced
this pull request
Feb 28, 2022
- What I did following the change in PR sonic-net/sonic-buildimage#7830, the folder needs to be created when the first time enables the sniffer function. - How I did it check whether the path to store the sniffer file is existing or not, if not, create it. - How to verify it run command "config platform mlnx sniffer sdk enable", can see sniffer function can be enabled successfully.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What I did
Keep attributes for the same entry in the order that the attributes are set in bulk mode.
Fix sonic-net/sonic-buildimage#6881
Why I did it
Some attributes have a dependency on others in SAI operations (e.g., setting nexthop requires packet action to be forward in route set operation). Without keeping the order of the attributes, the configuration may not be successfully applied to hardware. Therefore, it is necessary to keep the order of attributes in bulk mode.
How I verified it
Verify that sairedis applys
SAI_ROUTE_ENTRY_ATTR_PACKET_ACTIONbeforeSAI_ROUTE_ENTRY_ATTR_NEXT_HOP_IDwhen setting default routes.Details if related