-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add L4S/DSCP support to QoS Profile #384
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Randy Levensalor <[email protected]>
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.
considering your changes, here is some additional verbiage @RandyLevensalor
verbiage updates based on Randy's feedback
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.
Some thoughts from my side
@camaraproject/quality-on-demand_codeowners - this PR should be done, please provide feedback |
@benhepworth would be great if you can consider my comments above ... I'm not yet happy with the PR in the current form. We should also consider to release the API with another initial version instead of declaring it already stable, as the added parameters are more than a minor change and might need some revision in a next version. |
… what L4S is, queue type descriptions, and link to RFC9330
ok @hdamker - thanks for all the feedback. I updated everything based on your comments. Let me know if you have any additional feedback. Thanks! |
For the new proposed properties, as they are optional, we should describe what is the meaning or assumption when they are not included as part of a QoS profile,
If indeed there are default values to be assumed, we may use the |
@jlurien Are you sure that we can assume a default value or behavior. Some operators may not want to expose their queue type or could have an L4S queue, even if the specific profiles doesn't support L4s. Part of making this optional is that we would not impact operators who are not implementing L4S or operators who are only using DSCP for traffic that isn't managed through QoD, such as voice traffic on 5G networks. If we define a default behavior, then we are making assumptions and requirements that may not be true about the underlying network. |
@RandyLevensalor I'm not suggesting to assume any default values, it was just a question. If the answer is that a default value has not to be assumed, which I agree with, we may need to make that more explicit in the description. Do these new parameters have any incompatibility with or impact on other existing parameters? I'm worried that these new parameters are very technical for an average customer and introduce complexity to the API. |
We shouldn't forget that this QoSProfiles are defined by API providers to describe the offered profile, they are not meant to be created or defined by API consumers. It is for sure possible to describe profiles which are contradictions in itself, but to avoid that is the responsibility of the provider (as the provider has actually to deliver the profile according to the description). I share part of the concern that these parameters require deeper network knowledge (and knowledge about the referenced standards), but on the other side is L4S a powerful mechanism for application to adapt to network conditions, which network provider want to expose to application developers. On the ServiceClasses: as they are mainly understood as an indication for which type of traffic the profile is suitable, I'm fine. But they need to be documented in a way that they are not restricting the way of implementation of the profile in different network. But again: the network provider does not need to set the parameter in a profile. |
@hdamker - I updated this PR so that l4sQueueType and serviceClass are experimental 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.
LGTM
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Add L4S/Diffserv support to Qos Profile
Which issue(s) this PR fixes:
Fixes #173
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.