-
Notifications
You must be signed in to change notification settings - Fork 654
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.
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
multicast counters to subinterface, remove physical counters from subinterface #934
Conversation
Major YANG version changes in commit d147c28: |
Ready for review @robshakir One open question for us is: how should multicast IP packets be counted? At the subinterface level or at the ip level or both? It seems counters are duplicated (where applicable) on the subinterface container and IP container. I am not quite sure what the history is here and if I should duplicate the multicast counter at the ip container level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor fix. Did we check that this doesn't remove anything unexpectedly?
Compatibility Report for commit d147c28: |
Fixed index. Merged in latest breaking change detection (which now detects changed and deleted leafs and compares to the new semantic version) and it had findings in wifi ap interfaces model which was impacted by the refactoring of the groupings in openconfig-interfaces. (@wenovus +1) This is now fixed. |
@mike-albano I made a small refactor to the wifi / ap-interfaces model which doesn't change the tree in any way. It was needed to be compatible with the refactoring done on the openconfig-interfaces model in this PR. Would like your approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Change Scope
Platform Implementations
Error counters analysis
The TODO item to investigate if additional interface error counters are needed was removed based on the following analysis:
I'd like to complain a little bit that this is expanding the scope of the PR which is intended to be small and only seeks to
Thank you for reading my complaint. :)
That said, I investigated and I think this TODO can be removed. Here are the error counters which seem to match commonly supported output from 'show interfaces' commands on various implementations. There is variation by This varies by hardware and NOS. Some platforms report only input and output errors without support for additional, more granular errors. Some platforms aggregate errors as input and output errors and also support more granular errors.