-
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
New API QOD Provisioning #299
Conversation
First draft of QoD Provision API
Linting complaints
More linting fixes
More trailing spaces
From the minutes of the last meeting:
I took this AP to our Product team, and Eric's suggestion makes sense for this first version. So far, the initial use case is restricted to one provision per device, and affecting all of its traffic. Indeed, in case that it was required to include server side information for provision, we would likely need to use different identifiers for applicationServer, as server side IPs may change or they may be reused or shared by several applications (in a cloud server for example). Using AS domain names would make more sense. Even the original "session" mode may need to review these aspects in the future. So I will simplify the original proposal accordingly. fyi @hdamker |
* Removed support for serveral appliocation flows per device. In this version provision is per device and affects all its traffic. Proiperties `applicationServer`, `devicePorts` and `applicationPorts` are deleted. * Also `messages` in the response is deleted as it had not a requirement * Added support for async deletion process (202 response) in addition to the sync 204
Operation adapted to last changes
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.
The more I read /provisions
, it doesn't seem to capture the intent of this end point. Which if I'm understanding this endpoint correctly to intent is is to apply a persistent QoS profile to a device until it is removed.
"404": | ||
$ref: "#/components/responses/DeviceNotFound404" |
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.
What about adding a second 404 response for when multiple devices are found?
"404":
oneOf:
$ref: "#/components/responses/DeviceNotFound404"
$ref: "#/components/responses/MultipleDevicesFound404"
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.
Device Not Found is the only code considered in Commonalities for 404 related to devices. For cases when the device identifiers identify more than a single device, the response 422 DEVICE_IDENTIFIERS_MISMATCH applies
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.
@jlurien what is then the right code and message if the (single) provided identifier, e.g. Phone Number, belongs to multiple devices (e.g. "MultiCard"/"MultiSIM" features)? "DEVICE_IDENTIFIERS_MISMATCH" would be misleading here ...
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.
@hdamker I think that this case has not been considered yet in Commonalities. It is really a problem we have from the beginning when we identify a device by a MSISDN, which may not be exclusive for a single device, as it identifies a subscriber. IMO it would be a subcase for 422, as the entity is not processable for the service. The problem is also extensible to other APIs such as the sessions mode QoD or Device Location, when a single device is expected.
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.
We still may use the generic 422 UNPROCESSABLE_ENTITY for this sub-case, or even DEVICE_NOT_SUPPORTED, but probably it is worthy to raise an issue to handle properly multi-sim scenarios
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.
If, by "multiSIM" you mean one MSISDN shared between multiple devices/SIMs (e.g. smartphone and smartwatch), this was discussed in Device Identifier, and the conclusion was that the phoneNumber
should always be interpreted as the secondary MSISDN rather than primary.
The secondary MSISDN should be unique to a single device (SIM) - at least for the Vodafone "OneNumber" service it is, but maybe there are services where this is not true. For most devices, the secondary MSISDN will anyway be the same as the primary, and so this interpretation does not change things for such devices. The issue is that, if the secondary MSISDN is different, it is not so easy to find out what it is.
Of course, for Device Identifier this mattered, as the API is returning the IMEI of the UE, so we need to know which UE is intended. For QoS provisioning, maybe we just prioritise all devices that share the primary MSISDN.
- openId: | ||
- "qod-provision:provisions:delete" | ||
|
||
/provisions/retrieve: |
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.
Would something like /provisions/active
be a more intuitive to get the active provision on the current device.
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.
I see that the description is wrong as it returns all provisions regardless the status. I'll have to fix that. And as it is a POST modeled to get information, the path is usually modeled as an action. A POST /resource/active would be seen as an endpoint to create a resource in active status.
"403": | ||
$ref: "#/components/responses/Generic403" | ||
"404": | ||
$ref: "#/components/responses/RetrieveProvision404" |
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.
What about a second response to 404 for device not found? Since this code implies that there isn't an active provision, which is technically true. There are no active provisions on a none existent device, but the implications are different.
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.
The response (schema in components) already considers 2 codes: PROVISION_NOT_FOUND and DEVICE_NOT_FOUND. As I said above there is an error in the description and provision could be in any status.
- name: QoD Provision | ||
description: Manage the provision of QoD | ||
paths: | ||
/provisions: |
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.
Is provisions the best name for this endpoint? As I'm reviewing this, I keep thinking about getting supplies (provisions) for a camping trip.
What about /persistent
or /device-qod
? Something that denotes it's setting a QoS Provide for the device and / or that it's a type of QoD session that will never expire.
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.
I trust a native speaker on this :) We are open to change the name to whatever makes the use case more understandable and remark the difference with respect to the other API (sessions). The concept is related to the provisioning of the QoD persistently.
For API name, maybe qod-provisioning
is more clear? For the resource name it could be device-qos
, and it would imply operations such as POST /qod-provisioning/v0.1/device-qos
and GET /qod-provisioning/v0.1/device-qos/{device_qos_id}
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.
@jlurien /qod-provisioning makes sense to me.
- Replaced "provision" for "provisioning" - Path for /provisions changed to /device-qos - Fix that get operations do not only return active provision
- Trailing space
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.
@jlurien I'm still confused about one aspect of this end point / use case.
Since we are not specifying an application server, target port or any DCSP markings (which is a separate issue on the backlog), would this mean that all traffic would fall under this QoS profile or would the service provider just use this to update a default or predetermined flow/PDU session etc.?
If possible, could we add documentation as to how this impacts the traffic on the given device or just clarify that it's on per provider basis and may change based on type of service. I can't imagine that we'd want to change some of the standard QoS to just flatten the profiles on the device.
Co-authored-by: Randy Levensalor <[email protected]>
The PR description still mentions the optional I can't give a statement if this simplified API (but also the QoD Provisioning API as such) makes sense as product or not ... currently lacking feedback from product side. |
You're right, this means that the QoD profile will be applied to all the traffic sent to or received from the line under consideration. A relevant example would be the line for a point-of-sale terminal that will be configured with a better than best-effort service so that the credit card transactions will succeed even in crowded areas. |
Revert required: true for request Body
- Updated implicit subscriptions model with Commonalities, issue camaraproject#332 - Error examples aligned with Commonalities (removed summary and some messages updated)
Trailing spaces
Hi, @RandyLevensalor please review again, as you requested changes in a past review and now your explicit approval is needed to unblock the merge. |
Fixing review comments
Dedicated 400 response was in wrong place
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
One point which I have seen in my final review: the reference to PROVISIONING_AVAILABLE_WITHOUT_DEVICE made me curios what is behind. Learned then that it is mainly "...WITHOUT_DEVICE_INFORMATION_IN_RESPONSE". Makes sense to have this example for OAS viewers/editors. But it brought me to a point - feel free to create an issue or discussion out of it: is the provisioning really bound to a specific device? Or - in case of phoneNumber as the device identifier - is the provisioning actually bound to a subscription? If the provisioning is bound to a device:
|
I just added the example to illustrate that device can be empty in responses if not provided in the request (because it was implied from the 3-legged access token), but the other questions are relevant
I will share the questions with our Product team to fully discuss the corner cases, but the "happy case" designed so far, to my knowledge, is that device identifiers (explicitly provided or implied from token) are used to identify a valid subscription (typically associated to a SIM), and provisioning is applied to it. If may happen that SIM is not connected to the network at the time of the request if a phone number is provided, and my assumption is that provisioning should be allowed, but it may depend on the implementation. Allowing IP addresses enables the possibility to identify the device in cases where phone numbers are not known by the client, as with the session mode, but if an IP address is provided the device has to be connected. If the SIM is swapped to a different terminal it shouldn't terminate the provisioning (IMO) but again it may be business decision to terminate it, if it is detected. There may be some implications with multi-sim scenarios, but that kind of implications may affect as well the session mode, and it depends also on how multi-sim is implemented internally by operators. For future releases we can open a dedicated issue to discuss all of these implications. |
@RandyLevensalor @eric-murray any last words on the PR? To complete the release PR I would need the API in |
@jlurien Eric had reviewed previously and the comment is addressed, @RandyLevensalor has approved before the last change which was only in the error examples. I think we are good here - would you merge the PR, so that I can finish the Release PR? |
What type of PR is this?
What this PR does / why we need it:
First draft of new QoD Provision API. It is a new API in the family of Quality On Demand. It implements the new mode approved to enhance the current scope:
Which issue(s) this PR fixes:
Fixes #268, #332
Special notes for reviewers:
Name proposal was originally "qod-provision" but it was agreed to use "qod-provisioning". Versions wip/vwip, to be changed to 0.1.0 (and its variants) for first RC
The PR just adds the yaml spec. Other files like README, documentation, etc may need to be updated once the API is reviewed and approved.
It follows closely the model for the sessions mode (renamed to
quality-on-demand
, final name TBD). Main differences are:duration
andexpiresAt
, as provision is assumed to exist indefinitely, until it is revokedapplicationServer
, provisioning affects all device trafficstatus
replacesqosStatus
in sessions, as it is the status of the configuration being applied, not the status of the QoS being in place at the moment. Configuration is active even if the device is temporarily not having a QoS benefit, for example because is disconnected or out of coverage.It already applies the latest alignment with Commonalities/ICM guidelines, and it is also aligned with Align quality-on-demand with Commonalties regarding optional device object and error messages #326
Changelog input