Add a port index mapper service for sFlow #4794
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
|
This pull request introduces 1 alert when merging f1123c4 into edf3160 - view on LGTM.com new alerts:
|
prsunny
reviewed
Jun 18, 2020
| from pyroute2 import IPDB | ||
| from swsssdk import SonicV2Connector, port_util | ||
|
|
||
| RTM_NEWLINK = 16 |
There was a problem hiding this comment.
Does pyroute2.netlink has these definitions?. I don't think we should define these values explicitly.
| _hash = '{}|{}'.format(PORT_INDEX_TABLE_NAME, ifname) | ||
|
|
||
| if msgtype == RTM_NEWLINK: | ||
| state_db.set(state_db.STATE_DB, _hash, 'index', str(index)) |
There was a problem hiding this comment.
I think its better to put updating STATE DB to a separate function, as it is called from multiple places.
|
This pull request introduces 1 alert when merging 4033024 into 7c2f5a0 - view on LGTM.com new alerts:
|
|
retest vsimage please |
prsunny
approved these changes
Jun 19, 2020
There was a problem hiding this comment.
Code lgtm. Can you add this new stateDb entry PORT_INDEX_TABLE_NAME to swss-schema.md
|
retest vsimage please |
2 similar comments
|
retest vsimage please |
|
retest vsimage please |
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.
- Why I did it
sFlow generates both counter samples and flow samples. While the counter samples contain the ifName, the flow samples have only the interface index - and the index is the Linux ifindex (and not the SONiC port index (as is typically seen in the PORT_TABLE). Any external application (sflow collector / end user) then has to map the index to port index (that's obtained from IF-MIB, say) which is not intuitive.
The SAI driver generates the samples using the Linux indexing scheme. While it is possible to address the issue from SAI driver/SDK perspective, we believe providing a mapping service and letting applications handle the mapping may be a more flexible alternative (which could benefit other applications in future).
- How I did it
Introduce a port_index_mapper script in sflow docker which will:
Note that this currently supports only interfaces supported by port_util.
- How to verify it
Verify that the index/ifindex mapping matches that of "ip link show".
- Description for the changelog
Add a port index mapper service for sFlow
- A picture of a cute animal (not mandatory but encouraged)