-
Notifications
You must be signed in to change notification settings - Fork 45
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
EdgeCloud LifecycleManagement API proposal #154
Conversation
'503': | ||
$ref: '#/components/responses/503' | ||
|
||
/edgeCloudPlatform: |
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.
This duplicates what is in the MEC Exposure and Experience Management API, is that correct?
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 tends to comply with this intent
“I can retrieve a list of the operator’s MECs and their status, ordering the results by location and filtering by status (active/inactive/unknown)” 5SED 5MEE EXRA
Developer Intents - Provisioning Intents 1.
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.
For consistency I would suggest you name this path as /mecplatform. Also note to use all lowercase, as URLs are case-insensitive, so in general paths are in lower case.
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 object of this PR is to cover the intents proposed in this ppt Presentation 28/nov meeting. It is not intended to duplicate the simpleedge discovery definition but to extend its functionality as the current definition only covers intent Developer Intents - Provisioning Intents 21. This way we could have a single API for an MVP instead of multiple APIs coverig this initial intents.
We left the name edge-cloud-platforms
in order to comply with the guidelines that state that the names in the urls need to be human readable and mec-platform may not be understandable by all the users. Anyhow we modify it to comply with the lowecase comonality intent
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.
In the just submitted v0.2.0 of the API, edge-cloud-nodes
is the name we have selected for this parameter oriented by the GSMA naming. It is open to discussion as the final objective is to have the same names for the Simple Edge Discovery and for this set of methods. It is important to follow CAMARA guidelines (reason why we avoid using telco acronyms like mec).
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.
See comments for minor changes (and one requested clarification)
Changes proposed by kevin
Presentation shown on CAMARA EdgeCloud Meeting on Tuestady 28th on November with motivations and explanations for a contribution for a MVP included in #154
|
||
paths: | ||
/app: | ||
post: |
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.
As discussed in the group meeting we should also have a GET method to retrieve the provisioning status of the application.
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.
Agree! That might be included in future versions. For simplicity is not considered in this version with the goal of having a first contribution released soon.
In the future it can be considered if we prefer to have a get method or include callback responses with the progress of the deployment
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 also prefer GET over callbacks, callbacks have a lot of drawbacks and usability issues, especially when the end user is a human.
There should also be "list" equivalent APIs, i.e. GET /app, to return a list of all apps. This is especially important when the end users are humans.
Finally, I don't see any way to retrieve how to access a deployed application instance. Either there should be a GET appinstance API that returns the details of the App instance including what URL/IPs/Ports to use to access it, or we need a discovery API that does the equivalent.
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.
Regarding your last comment, to cover that we proposed the PR #159. This way it would be retrieved the more optimal edge where an instance of the application is deployed, including the URI to access it.
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.
GET appInstance method has been included and the response includes URI and status (also the POST method does). We believe that the Discovery section of the API should also include this (item was under discussion during the meeting but was not closed)
$ref: '#/components/responses/503' | ||
|
||
/app/{appId}: | ||
delete: |
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.
As discussed in the meeting there should be a GET method also to retrieve the termination status of the app instance.
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.
Agree! That might be included in future versions. For simplicity is not considered in this version with the goal of having a first contribution released soon.
In the future it can be considered if we prefer to have a get method or include callback responses with the progress of the deployment
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 finally include this GET methods for app and appinstance in the last v0.2.0
It has been included a status parameter for app instance including the following possible states:
- ready
- instantiating
- failed
- terminating
- unknown
Co-authored-by: Nicola Cadenelli <[email protected]>
Comments and minor changes have been reviewed
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.
Review finished
|
||
paths: | ||
/app: | ||
post: |
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.
Agree! That might be included in future versions. For simplicity is not considered in this version with the goal of having a first contribution released soon.
In the future it can be considered if we prefer to have a get method or include callback responses with the progress of the deployment
$ref: '#/components/responses/503' | ||
|
||
/app/{appId}: | ||
delete: |
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.
Agree! That might be included in future versions. For simplicity is not considered in this version with the goal of having a first contribution released soon.
In the future it can be considered if we prefer to have a get method or include callback responses with the progress of the deployment
|
||
paths: | ||
/app: | ||
post: |
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 also prefer GET over callbacks, callbacks have a lot of drawbacks and usability issues, especially when the end user is a human.
There should also be "list" equivalent APIs, i.e. GET /app, to return a list of all apps. This is especially important when the end users are humans.
Finally, I don't see any way to retrieve how to access a deployed application instance. Either there should be a GET appinstance API that returns the details of the App instance including what URL/IPs/Ports to use to access it, or we need a discovery API that does the equivalent.
'503': | ||
$ref: '#/components/responses/503' | ||
|
||
/edgeCloudPlatform: |
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.
For consistency I would suggest you name this path as /mecplatform. Also note to use all lowercase, as URLs are case-insensitive, so in general paths are in lower case.
description: Ask the operator to provision an application to one or several Edge Application Servers taking into account resources (e.g. vCPU, Memory, network interfaces, storage, GPU) | ||
operationId: appProvisioning | ||
requestBody: | ||
description: Details about application and zones where application instance should be instantiated |
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.
In the description Zones should be replaced with MEC Platforms.
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.
Agree, for now we drop the references to zone in our data model until new outcomes on the discussion are obtained
required: | ||
- geolocation | ||
properties: | ||
edgeResourceName: |
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 suggest renaming the "edgeResourceName" field to just "name", as it's already scoped within the MecDetails object. This is consistent with other objects that just have a "name" field.
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 suggest to have it as it is so to clarify which name the response is refering to (there are more schemas and responses using parameters like name). Also as this is used in the response of a GET method, we find it more clarfying. But we can discuss it.
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.
As simple edge discovery (SED) API may become part of this file itself, we may need to harmonize that if the conceptual meaning of edgeResourceName for this API is same as in SED then we should have a one definition of this parameter
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 have rename the entity of edgeResourceName for edgeCloudNodeName, this is something we have to discuss to harmonize common entities in TI, SED and MVP API.
As discussed in the previous CAMARA - Edge Cloud meeting (12th - Dec) we agree to combine Artifact and Application into a single type, included in this new version. We clarified the difference between Application and Application Instance, defining two methods. Application is HOW to deploy meanwhile Application Instance is WHERE to deploy it. In order to provide a vision of the current configured Applications and for the status of Application Instances, we included GET methods for each one. We have rename the entity of edgeResourceName for edgeCloudNodeName, this is something we have to discuss to harmonize common entities in TI, SED and MVP API.
Update EdgeCloud_LcM.yaml v0.2.0
New commit has been submitted regarding the last meeting we have (Minutes of meeting). Also previous comments have been answered. Hope API is ready to be merged! |
- name: appInstanceId | ||
description: A globally unique identifier associated with a running instance of an application. OP generates this identifier. | ||
in: query | ||
required: false |
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.
Do we need to make "required" as true? Otherwise we need to define if no instance id has been provided then what would be the behavior? To terminate all the instances of this app?
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 intention is that all the instances can be deleted if the instanceID is specificated. This behaviour could be explained in the documentation
$ref: '#/components/schemas/AppInstanceId' | ||
edgeCloudNodeName: | ||
$ref: '#/components/schemas/EdgeCloudNodeName' | ||
uri: |
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 should add some description on what the API user can expect from the "Uri" and how it can be used
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.
Agree! Once the pull request is merged, further documentation can be added
appId: | ||
$ref: '#/components/schemas/AppId' | ||
aplicationInstace: | ||
type: array |
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 describe if more than one "InstantiatedApp" object can be expected in under some conditions as aplicationInstace is defined as an array? And in that case how should information contained in InstantiatedApp objects can be used?
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 is an array because depending whether there is a filter in the request, the app could be instantiated in multiple edge nodes (eg. Region or all the nodes if no filter is parsed)
schema: | ||
$ref: '#/components/schemas/ErrorInfo' | ||
example: | ||
status: 409 |
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.
In the appInstance POST API response we are returning an array of instances but 409 CONFLICT says multiple instances cannot exist in the given edge or region. Is it an anomaly?
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 idea behind is that if a simple node is parsed and that node exists then that general response applies, same with region
'503': | ||
$ref: '#/components/responses/503' | ||
/appInstance/{appId}: | ||
get: |
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.
Should a query parameter also be provided to filter using edge, region etc?
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.
Suggestion accepted
schema: | ||
$ref: "#/components/schemas/AppInstanceId" | ||
responses: | ||
'202': |
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.
As the 202 means that the deletion can some time then should there be a corresponding GET to retrieve the result of the DELETE app instance?
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 GET/appInstance/{appId} it can be checked the status after the DELETE and the POST. For instance if after the DELETE, the GET does not retrieve any response that would mean that the whole instance have been deleted correctly
type: string | ||
minLength: 8 | ||
maxLength: 32 | ||
description: Name of the file with the extension eg. zip, targz, etc. |
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 need to be described what is expected from the app developer from the fileName? Which file this element is referring to?
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 also needs to be described if the fileName has any relation with the repository attribute below? That means the expectation is that OP will try to pull the file(s) from the repository?
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 idea we were thinking was that the combination of both fields FileName
and url
will let the developer to build the correct path to the binary file in their repository.
Agree this needs to be documented.
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.
Deepak comments review
schema: | ||
$ref: "#/components/schemas/AppInstanceId" | ||
responses: | ||
'202': |
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 GET/appInstance/{appId} it can be checked the status after the DELETE and the POST. For instance if after the DELETE, the GET does not retrieve any response that would mean that the whole instance have been deleted correctly
'503': | ||
$ref: '#/components/responses/503' | ||
/appInstance/{appId}: | ||
get: |
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.
Suggestion accepted
schema: | ||
$ref: '#/components/schemas/ErrorInfo' | ||
example: | ||
status: 409 |
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 idea behind is that if a simple node is parsed and that node exists then that general response applies, same with region
appId: | ||
$ref: '#/components/schemas/AppId' | ||
aplicationInstace: | ||
type: array |
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 is an array because depending whether there is a filter in the request, the app could be instantiated in multiple edge nodes (eg. Region or all the nodes if no filter is parsed)
$ref: '#/components/schemas/AppInstanceId' | ||
edgeCloudNodeName: | ||
$ref: '#/components/schemas/EdgeCloudNodeName' | ||
uri: |
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.
Agree! Once the pull request is merged, further documentation can be added
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.
After several iterations/commits with improvements on the API definition, and as it was agreed in 12th December Meeting to advance towards an MVP API, it would help to approve this pull request and merge into main, so further changes proposals ( parameter patterns, descriptions and other "minor" changes) and an eventual merge with Simple Edge Discovery API in a more ordered way in separated PRs.
@Kevsy , @gunjald ,@FabrizioMoggio As agreed during 19th December EdgeCloud Meeting, this PR has evolve to a point where it should be merged as pending conversations could be treated in separated PRs to facilitate management. Please review if there is anything else to consider before 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.
I agree this is good starting point and we can file issues to address outstanding concerns, and address each separately in follow-up PRs.
Hello everyone!
We propose this API definition as a first contribution to the EdgeCloud Lifecycle Management. This includes:
The API is based also in the contributions done by other partners on this CAMARA group. We can further discuss this file in the next meetings.
Contributors: @javierlozallu, @crissancas, @sergiofranciscoortiz
Cheers!