Create service provider defined QoS Profile #138

RandyLevensalor · 2023-04-19T20:23:05Z

Implememnted issue #125

RandyLevensalor · 2023-04-19T20:26:49Z

@eric-murray I think that I've incorporated your feedback into the PR. But there's a decent chance that I missed something. Thanks!

jlurien

Thanks for the PR. Some initial comments from a first review

code/API_definitions/qod-api.yaml

jlurien · 2023-04-21T10:28:56Z

code/API_definitions/qod-api.yaml

@@ -279,7 +426,9 @@ components:
        applicationServerPorts:
          $ref: "#/components/schemas/PortsSpec"
        qosProfile:
-          $ref: "#/components/schemas/QosProfile"
+          oneOf:


We should consolidate both within the same model. Legacy qosProfile values should correspond also to some profiles returned with the new model, so they should have an Id as well. We may think of defining per profile both an Id (UUID-like) and some short of alias-name, such as previous ones (QOS_L, QOS_E, etc), and allow to use any of them

In general, we should still retain the option of using a qosProfile without doing a GET of available qosProfiles. As we know, a qosProfile string's associated SLAs are agreed outside of the scope of this spec. If an user deciding on using a profile, they know what they are getting. In this case, the mandatory GET option needed to get an id seems unnecessary to me.

@jlurien
I assume the intention is to drop this support for the "legacy" QoS profiles at some stage, so QosProfile would be deleted and qosProfile would then be of type QoSProfileId only. If that is the intention, then we should keep the restriction that QoSProfileId be a uuid (I agree with your comment below) and live with this workaround until it is agreed that the current QoS profiles are no longer useful.

If that is not the intention, and the legacy profiles must always be supported, then simply redefining QoSProfileId to have minLength: 5 would achieve that, albeit at the cost of more complex validation.

@RandyLevensalor @eric-murray @jlurien @hdamker @emil-cheung @ToshiWakayama-KDDI @patrice-conil

I guess we as a community should make a decision now on how to proceed with legacy qosProfile. I made a comment here regarding this. I believe we should be able to combine both legacy and extended qos profiles now as the api is not stable yet.

To me the proposal by @sfnuser makes sense. I don't see why the current predefined qosProfiles should be treated differently once we have the new discovery functionality in place.

We should also decide if API spec enforce some predefined QoS Profile to be offered by all implementations, and what is the best unique identifier for a profile, a UUID or some more friendly label such as current QOS_X. Problem with UUID is that they are not friendly for predefined profiles.

To me the proposal by @sfnuser makes sense. I don't see why the current predefined qosProfiles should be treated differently once we have the new discovery functionality in place.

I can see going down this path of not using a UUID for the QoS profiles and instead use a name as the id. We can define this name to be a unique string that matches this regex ^[a-zA-Z0-9_]+$

The length of the name can be from 1 to 128 characters?

With this, the name would replace the ID as the unique identifier.

I'd also like to add an optional displayName attribute as a string that could be easier to read, and can contain spaces and other special characters. This would replace the current name attribute and would not be unique.

Since the QOS_S etc. can be a part of this, I think that all of the other attributes would become optional without default values. This is due to the fact that QOS_S etc. can be defined by only using the name field.

One more note. Using a name with some constraints is consistent with the way amazon implemented their EC2 instance types. EC2 instance sizing is a rough analogy for the QoSProfiles. https://aws.amazon.com/ec2/instance-types. So this pattern should be familiar to many application developers.

code/API_definitions/qod-api.yaml

jlurien · 2023-04-21T10:44:38Z

code/API_definitions/qod-api.yaml

+          format: string
+        status:
+          $ref: "#/components/schemas/QosProfileStatusEnum"
+          description: The status of the profile.  See QosProfileStatusEnum for description.


In general it is safer to move description to referenced Schema when possible. According to spec any sibling elements of a $ref are ignored, although some visualizers allow them. In cases where a schema is reused by several properties but description is different in each property, we may "take the risk"

code/API_definitions/qod-api.yaml

documentation/API_documentation/QoD_API.md

code/API_definitions/qod-api.yaml

sfnuser · 2023-04-24T10:50:46Z

code/API_definitions/qod-api.yaml

@@ -279,7 +426,9 @@ components:
        applicationServerPorts:
          $ref: "#/components/schemas/PortsSpec"
        qosProfile:
-          $ref: "#/components/schemas/QosProfile"
+          oneOf:


In general, we should still retain the option of using a qosProfile without doing a GET of available qosProfiles. As we know, a qosProfile string's associated SLAs are agreed outside of the scope of this spec. If an user deciding on using a profile, they know what they are getting. In this case, the mandatory GET option needed to get an id seems unnecessary to me.

sfnuser · 2023-04-27T09:13:39Z

code/API_definitions/qod-api.yaml

@@ -279,7 +426,9 @@ components:
        applicationServerPorts:
          $ref: "#/components/schemas/PortsSpec"
        qosProfile:
-          $ref: "#/components/schemas/QosProfile"
+          oneOf:


@RandyLevensalor @eric-murray @jlurien @hdamker @emil-cheung @ToshiWakayama-KDDI @patrice-conil

I guess we as a community should make a decision now on how to proceed with legacy qosProfile. I made a comment here regarding this. I believe we should be able to combine both legacy and extended qos profiles now as the api is not stable yet.

hdamker · 2023-05-04T18:42:20Z

@RandyLevensalor Could you make a list of the topics we need to discuss tomorrow in our call - best with the options we have? (and try to resolve the others before the call, just if possible)

code/API_definitions/qod-api.yaml

RandyLevensalor · 2023-05-04T23:27:05Z

@hdamker I'll get the updated PR posted this evening.

@eric-murray thanks for all of your comments and feedback. I'm working on incorporating it.

RandyLevensalor · 2023-05-05T04:48:54Z

@hdamker

Open items to discuss:

Behaviour and parameters of querying QoS Profiles. The latest version is very limited and can be updated in a subsequent release.
Is mimumumTargetRate acceptable or would guaranteed be more better.
The latest draft eliminates the unique ID in favor of a unique name with minimal constraints. Is this the approach that we want to take?
Descriptions in the OpenAPI spec have been revised. Which ones need additional clarifications? Do these need to be duplicated in the documents as well?

hdamker · 2023-05-05T06:44:51Z

@RandyLevensalor thanks for the summary of open issues

4. Do these need to be duplicated in the documents as well?

No, see #144 (but we might want to keep the mapping table seperat).

code/API_definitions/qod-api.yaml

These revised description address feedback in the PR and from the sub-project meeting.

Co-authored-by: Jose Luis Urien <[email protected]>

hdamker · 2023-06-04T18:32:21Z

As discussed: @sfnuser @eric-murray @jlurien please do the final check (and) if possible approval until June 6th. Feel free to create issues for later improvements of the descriptions - the goal is to get the PR merged if possible asap.

sfnuser

Thanks @RandyLevensalor for driving this to conclusion.

eric-murray · 2023-06-05T16:09:58Z

@hdamker
It's difficult to review the overall API definition file because Randy's fork is some 80 commits behind the main branch. Whilst much of this is other files, there have been some changes qod-api.yaml that are not included in Randy's PR.

@RandyLevensalor
Can you sync your fork with the main branch (and fix any conflicts that may arise)? This has to be done sometime, so might as well do it now.

RandyLevensalor · 2023-06-05T16:20:47Z

@hdamker It's difficult to review the overall API definition file because Randy's fork is some 80 commits behind the main branch. Whilst much of this is other files, there have been some changes qod-api.yaml that are not included in Randy's PR.

@RandyLevensalor Can you sync your fork with the main branch (and fix any conflicts that may arise)? This has to be done sometime, so might as well do it now.

@eric-murray I've rebased the branch.

Surprisingly enough, there were no conflicts and I didn't see any regressions. Though an additional set of eyes on this will make me feel better.

eric-murray

Thanks @RandyLevensalor

I'm happy with the structure, so am approving. Parameter descriptions can be reviewed as part of the ongoing API documentation effort!

jlurien

Some final suggestions and I give the approval in advance. We can refine parameters and descriptions in subsequent iterations.

Also "pathfile.diff" file should not be part of the PR

code/API_definitions/qod-api.yaml

Co-authored-by: Jose Luis Urien <[email protected]>

RandyLevensalor · 2023-06-06T15:04:57Z

@jlurien Thanks for the additional suggestions. I've merged them into the PR.

code/API_definitions/qod-api.yaml

hdamker · 2023-06-07T06:17:41Z

LGTM - thanks @sfnuser @jlurien @eric-murray for the detailed reviews and your approvals. I will try to delete the patchfile.diff and merge.

Create service provider defined QoS Profile

a7db17f

RandyLevensalor requested review from hdamker and shilpa-padgaonkar as code owners April 19, 2023 20:23

hdamker requested review from jpengar, patrice-conil, bigludo7, akoshunyadi, eric-murray and sfnuser April 20, 2023 08:19

jlurien reviewed Apr 21, 2023

View reviewed changes

sfnuser mentioned this pull request Apr 24, 2023

Adds service provider defined QoS profiles #125

Closed

eric-murray reviewed Apr 27, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

eric-murray reviewed Apr 27, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

eric-murray reviewed Apr 27, 2023

View reviewed changes

documentation/API_documentation/QoD_API.md Outdated Show resolved Hide resolved

eric-murray reviewed Apr 27, 2023

View reviewed changes

documentation/API_documentation/QoD_API.md Outdated Show resolved Hide resolved

eric-murray reviewed Apr 27, 2023

View reviewed changes

documentation/API_documentation/QoD_API.md Outdated Show resolved Hide resolved

sfnuser reviewed Apr 27, 2023

View reviewed changes

eric-murray reviewed May 4, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

eric-murray reviewed May 4, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Show resolved Hide resolved

eric-murray reviewed May 4, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

Incorporating feedback from PR

9928c5d

eric-murray reviewed May 5, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

eric-murray reviewed May 5, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

eric-murray reviewed May 5, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

Updated the targetMinRate and packet delay budget

8f67129

These revised description address feedback in the PR and from the sub-project meeting.

RandyLevensalor and others added 2 commits June 2, 2023 05:47

Update code/API_definitions/qod-api.yaml

23bf07b

Co-authored-by: Jose Luis Urien <[email protected]>

Removing descriptions for $ref

9529b12

sfnuser previously approved these changes Jun 5, 2023

View reviewed changes

Merge branch 'camaraproject:main' into sp_defined_qosprofile

9a3ed0e

eric-murray previously approved these changes Jun 5, 2023

View reviewed changes

jlurien previously approved these changes Jun 6, 2023

View reviewed changes

Update code/API_definitions/qod-api.yaml

b7826e1

Co-authored-by: Jose Luis Urien <[email protected]>

RandyLevensalor dismissed stale reviews from jlurien, eric-murray, and sfnuser via b7826e1 June 6, 2023 15:03

RandyLevensalor and others added 4 commits June 6, 2023 09:03

Update code/API_definitions/qod-api.yaml

1265e03

Co-authored-by: Jose Luis Urien <[email protected]>

Update code/API_definitions/qod-api.yaml

bfb1526

Co-authored-by: Jose Luis Urien <[email protected]>

Update code/API_definitions/qod-api.yaml

f89278e

Co-authored-by: Jose Luis Urien <[email protected]>

Update code/API_definitions/qod-api.yaml

d19b5f5

Co-authored-by: Jose Luis Urien <[email protected]>

jlurien previously approved these changes Jun 6, 2023

View reviewed changes

eric-murray reviewed Jun 6, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

Kbps to kbps

5118f39

RandyLevensalor dismissed jlurien’s stale review via 5118f39 June 6, 2023 17:11

eric-murray previously approved these changes Jun 6, 2023

View reviewed changes

Delete patchfile.diff from PR

e550c52

hdamker dismissed eric-murray’s stale review via e550c52 June 7, 2023 06:18

hdamker approved these changes Jun 7, 2023

View reviewed changes

hdamker merged commit 47388f9 into camaraproject:main Jun 7, 2023

hdamker mentioned this pull request Jun 7, 2023

Aligned notifications with guideline #155

Merged

hdamker mentioned this pull request Jun 17, 2023

Pre release v0.9.0 rc #169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create service provider defined QoS Profile #138

Create service provider defined QoS Profile #138

RandyLevensalor commented Apr 19, 2023

RandyLevensalor commented Apr 19, 2023

jlurien left a comment

jlurien Apr 21, 2023

sfnuser Apr 24, 2023

eric-murray Apr 27, 2023

sfnuser Apr 27, 2023

jlurien Apr 27, 2023

RandyLevensalor Apr 27, 2023

RandyLevensalor Apr 27, 2023

jlurien Apr 21, 2023

sfnuser Apr 24, 2023

sfnuser Apr 27, 2023

hdamker commented May 4, 2023

RandyLevensalor commented May 4, 2023

RandyLevensalor commented May 5, 2023

hdamker commented May 5, 2023 •

edited

Loading

hdamker commented Jun 4, 2023

sfnuser left a comment

eric-murray commented Jun 5, 2023

RandyLevensalor commented Jun 5, 2023

eric-murray left a comment

jlurien left a comment •

edited

Loading

RandyLevensalor commented Jun 6, 2023

hdamker commented Jun 7, 2023

Create service provider defined QoS Profile #138

Create service provider defined QoS Profile #138

Conversation

RandyLevensalor commented Apr 19, 2023

RandyLevensalor commented Apr 19, 2023

jlurien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdamker commented May 4, 2023

RandyLevensalor commented May 4, 2023

RandyLevensalor commented May 5, 2023

hdamker commented May 5, 2023 • edited Loading

hdamker commented Jun 4, 2023

sfnuser left a comment

Choose a reason for hiding this comment

eric-murray commented Jun 5, 2023

RandyLevensalor commented Jun 5, 2023

eric-murray left a comment

Choose a reason for hiding this comment

jlurien left a comment • edited Loading

Choose a reason for hiding this comment

RandyLevensalor commented Jun 6, 2023

hdamker commented Jun 7, 2023

hdamker commented May 5, 2023 •

edited

Loading

jlurien left a comment •

edited

Loading