-
Notifications
You must be signed in to change notification settings - Fork 681
Add two new enums for scheduler-policy priority. #1311
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
base: master
Are you sure you want to change the base?
Conversation
} | ||
enum WRR { | ||
description | ||
"This scheduler term is considered as a deficit weighted |
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.
Presumably "deficit" isn't intended here, and this should just be "weighted round robin"
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.
Done
round robin term - such that packets that arrive in the queue are | ||
serviced based on weighted round robin fashion."; | ||
} | ||
} |
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.
Are there any strong references that provide a strong definition of what WRR or DWRR actually means? My fear is that at least DWRR may end up being quite platform/hardware specific and if that is the case I'm thinking that we may need to pull that out in the description.
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.
+1.
opening a featureprofiles PR with a functional test README would also help clarify the behaviour of these new enum values.
For examples, see the following tests:
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.
Aruba seems to also use the DWRR term: https://arubanetworking.hpe.com/techdocs/AOS-CX/AOSCX-CLI-Bank/cli_832x/Content/QoS_cmds/dwrr-queue.htm
round robin term - such that packets that arrive in the queue are | ||
serviced based on deficit counter with weighted round robin fashion."; | ||
} | ||
enum WRR { |
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.
With the existing model, it seems like setting priority was optional, and at least for our mappings, I think that we defaulted to WRR if STRICT priority wasn't explicitly set. Is the intention with this change that priority should always be set? Would it make sense to mark WRR as the default so that this change is backwards compatible from an existing implementation perspective (with the underlying assumption that other implementations map similarly)?
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.
It seems like OpenConfig prefers not to define defaults, at least according to #1165 (comment).
|
||
revision "2025-05-22" { | ||
description | ||
"Add two new enums for scheduler-policy priority."; |
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.
Can we specify the two new enums in the description? "Add WRR and DWRR enums ..."
revision "2025-05-22" { | ||
description | ||
"Add two new enums for scheduler-policy priority."; | ||
reference "0.11.3"; |
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.
Following https://github.com/openconfig/public/blob/master/doc/semver.md#general-guidelines-for-versioning-openconfig-yang-modules, should this be 0.12.0
?
/gcbrun |
No major YANG version changes in commit a47f0d0 |
Reviewed in the OC Operators Meeting on July 22nd. https://github.com/openconfig/public/pull/1311/files#r2223216393 points out a third vendor-mention of DWRR and WRR. In general, so long as we are able to show that something is used across multiple vendors, we do not require some other standardization for an operational use case. So, the current references are sufficient for demonstrating the terms as vendor-independent (and folks can continue to review the model change, itself). |
Discussed at the OC Operators Meeting on August 5th. Question for the author (@sallylsy): Is WDRR the same as DRR? If so, we could link to this paper (or through some other suitable link) for a definition in the description. Re this comment -- In OpenConfig today, if no priority is set, then there's an assumption that we're not using strict? Perhaps having a leaf that forces the weighted / strict configuration to be mutually exclusive to avoid an issue here? (Or use a |
Change Scope
Platform Implementations