EdgeCloud_LcM.yaml v0.9.2 with EdgeCloudZone and first Linter corrections. Documentation and User Story #208

javierlozallu · 2024-02-16T10:20:43Z

Adopted Edge Cloud Zone name.
Adjusting errors detected in Linter for paths name for /app-instance and duplicated method which has been fixed including Delete method in /app-instance/{appId}. Method /app-instace/{appInstanceId} deleted.

Adopted Edge Cloud Zone name. Adjusting errors detected in Linter for paths name for /app-instance and duplicated method which has been fixed including Delete method in /app-instance/{appId}. Method /app-instace/{appInstanceId} deleted.

gunjald · 2024-02-16T12:28:35Z

One comment,
EdgeCloudZone:
description: An identifier name for an Edge Cloud Zone in the operator domain

The description says that EdgeCloudZone refers to an identifier but the attribute name EdgeCloudZone doesnt look like an identifier and the description itself refer to EdgeCloudZone to explain what EdgeCloudZone is.
May be we can add the definition EdgeCloudZone on top level section and use EdgeCloudZoneID as actual parameter that refer to EdgeCloudZone?

javierlozallu · 2024-02-16T13:25:57Z

Hi @gunjald,

Agree with your comment to make these changes

gunjald · 2024-02-16T15:03:35Z

One suggestion would be to update "Edge Cloud" to "Edge Cloud Zone" so we have a reference for EdgeCloudZone.

gunjald · 2024-02-16T15:06:05Z

Then we may rename EdgeCloudZone to EdgeCloudZoneId and replace the reference to EdgeCloudZone to EdgeCloudZoneId and the description will be fine as we will have the definition available in "Edge Cloud Zone"

gunjald · 2024-02-18T18:00:52Z

On line 122 we should replace "appId" with "appInstanceId" in "/app-instance/{appId}" as the appInstanceId is to be returned in POST /app-instance response

gunjald · 2024-02-18T18:05:05Z

One more point to discuss is if the appInstanceId should be globally unique identifier as described currently or should have a limited scope in context of a given application i.e. appId? But in that case other path may also need to be changed e.g. "/application/{appId}/app-instance", "/application/{appId}/instance/{appInstanceId}" etc

Region and Edge Cloud Zone description updated. Parameters with UUID format have an Id at the end. Parameters with String format do not, that is why we decided to change RegionId to Region.

javierlozallu · 2024-02-19T12:11:06Z

Hi @gunjald

The descriptions have been modified according to the criteria:

Parameters with UUID format end with Id
String format parameters without Id

Definitions at the beginning of the API have been updated.

javierlozallu · 2024-02-19T12:16:11Z

Regarding path changes: /application/{appId}/instance/{appInstanceId}

It might make sense to change it and simplify the API, in the end an Instance belongs to the Application so only one path seems the right one to choose. But as it is a significant change, we could do it for the next version and for now try to clean up the API and have it ready for MWC.

gunjald · 2024-02-19T12:18:14Z

Regarding path changes: /application/{appId}/instance/{appInstanceId}

It might make sense to change it and simplify the API, in the end an Instance belongs to the Application so only one path seems the right one to choose. But as it is a significant change, we could do it for the next version and for now try to clean up the API and have it ready for MWC.

In that case i propose to change appId to appInstanceId in GET /app-instance/{appId}

javierlozallu · 2024-02-19T12:56:11Z

Regarding path changes: /application/{appId}/instance/{appInstanceId}
It might make sense to change it and simplify the API, in the end an Instance belongs to the Application so only one path seems the right one to choose. But as it is a significant change, we could do it for the next version and for now try to clean up the API and have it ready for MWC.

In that case i propose to change appId to appInstanceId in GET /app-instance/{appId}

Ok, but maybe we would lose the functionality of getting a complete list of instances of an app, but that could be covered in future versions with a specific method.

gunjald · 2024-02-19T12:58:43Z

Regarding path changes: /application/{appId}/instance/{appInstanceId}
It might make sense to change it and simplify the API, in the end an Instance belongs to the Application so only one path seems the right one to choose. But as it is a significant change, we could do it for the next version and for now try to clean up the API and have it ready for MWC.

In that case i propose to change appId to appInstanceId in GET /app-instance/{appId}

Ok, but maybe we would lose the functionality of getting a complete list of instances of an app, but that could be covered in future versions with a specific method.

I dont think as we have already defined the /app-instance/{appId} path and just that appId need to change to appInstanceId. Isnt ti?

javierlozallu · 2024-02-19T14:34:00Z

I dont think as we have already defined the /app-instance/{appId} path and just that appId need to change to appInstanceId. Isnt ti?

But if you define the appInstanceId, then you are referring to a specific instance of the application instead, with the /app-instance/{appId} the intention is to retrieve all instances of an application (appId) and if the appInstanceId in the request body is set, then the information of the specific instance is retrieved.

We notice that there is a better way to do it buy maybe for next version. Anyway that capability must be maintained.

gunjald · 2024-02-19T14:56:20Z

I dont think as we have already defined the /app-instance/{appId} path and just that appId need to change to appInstanceId. Isnt ti?

But if you define the appInstanceId, then you are referring to a specific instance of the application instead, with the /app-instance/{appId} the intention is to retrieve all instances of an application (appId) and if the appInstanceId in the request body is set, then the information of the specific instance is retrieved.

We notice that there is a better way to do it buy maybe for next version. Anyway that capability must be maintained.

OK. Now i got it. In that case can we add a query parameter to retrieve a specific instance of the app?

javierlozallu · 2024-02-19T15:15:22Z

I dont think as we have already defined the /app-instance/{appId} path and just that appId need to change to appInstanceId. Isnt ti?

But if you define the appInstanceId, then you are referring to a specific instance of the application instead, with the /app-instance/{appId} the intention is to retrieve all instances of an application (appId) and if the appInstanceId in the request body is set, then the information of the specific instance is retrieved.
We notice that there is a better way to do it buy maybe for next version. Anyway that capability must be maintained.

OK. Now i got it. In that case can we add a query parameter to retrieve a specific instance of the app?

Yes, it is a good option

Reorganization of the API structure to clean it up.

javierlozallu · 2024-02-20T10:39:29Z

PR updated according to previous comments.
We can discuss it at the meeting.

First version of Gherkin test feature file for EdgeCloud_LCM including at lis one test with response ok and one test with response nok for all existing POST,GET,DELETE methods in version 0.9.2 (#208)

gunjald · 2024-02-20T11:37:12Z

Changes seems fine to me

Kevsy · 2024-02-21T13:00:49Z

code/API_definitions/EdgeCloud_LcM.yaml

      type: string
-      description:  Human readable name of the geografical zone of the Edge Cloud Node. Defined by the OP.
+      description:  Human readable name of the geografical region of the Edge Cloud. Defined by the OP.


s/geografical/geographical

code/API_definitions/EdgeCloud_LcM.yaml

Kevsy · 2024-02-21T15:35:38Z

My understanding of API Guidelines is that the schema definition (component) is Upper Camel Case, but 'EdgeCloudZone' in the response and parameter name should be camelCase, not Upper CamelCase. (see lines 272, 525, 409)

Kevsy · 2024-02-23T15:51:22Z

edgeCloudZones is a collection, returning an array of edgeCloudZone resources.
However there is no path exposed to read the details of a single edgeCloudZone.

Typically a collection pattern would expose both /collection/ and /{collection}/{item} paths , e.g. in CAMARA QoD there are /sessions/ and /sessions/{id}/ paths.

Is there a plan to add a path to add the individual resource? We would need unique identifiers, to avoid conflict with other edge cloud providers, e.g. urn:vf:abc123. That identifier would need to be added to the EdgeCloudZoneDetails and returned in the GET \edgeCloudZones array response.

So, GET /edgeCloudZones/urn:vf:abc123 would return the details of an individual Edge Cloud Zone.

javierlozallu · 2024-02-26T15:30:13Z

My understanding of API Guidelines is that the schema definition (component) is Upper Camel Case, but 'EdgeCloudZone' in the response and parameter name should be camelCase, not Upper CamelCase. (see lines 272, 525, 409)

Changed. Thanks!

javierlozallu · 2024-02-26T16:32:22Z

edgeCloudZones is a collection, returning an array of edgeCloudZone resources. However there is no path exposed to read the details of a single edgeCloudZone.

Typically a collection pattern would expose both /collection/ and /{collection}/{item} paths , e.g. in CAMARA QoD there are /sessions/ and /sessions/{id}/ paths.

Is there a plan to add a path to add the individual resource? We would need unique identifiers, to avoid conflict with other edge cloud providers, e.g. urn:vf:abc123. That identifier would need to be added to the EdgeCloudZoneDetails and returned in the GET \edgeCloudZones array response.

So, GET /edgeCloudZones/urn:vf:abc123 would return the details of an individual Edge Cloud Zone.

Current GET \edgeCloudZones retrieve a list of zones where the application can be instantiated in POST /app/{appId}/instance. We don't see it useful for the moment to retrieve an specific Zone because is not going to be used but for best practices it will be fine to add it. In next releases we could use the GET /edgeCloudZones/urn:vf:abc123 for extended capabilities.

Added "Provider" parameter to edgeCloudZone and adapted related methods. Spelling and camelCase corrections.

Kevsy · 2024-02-27T09:37:51Z

Thanks @javierlozallu - I think exposing a read operation for a single edgeCloudZone is useful, because it allows to check the status (and other useful information, such as capabilities). We can discuss this in Discussion #215 , including the structure of EdgeCloudZone incorporating the URN Identifier.

javierlozallu · 2024-02-27T10:01:20Z

Thanks @javierlozallu - I think exposing a read operation for a single edgeCloudZone is useful, because it allows to check the status (and other useful information, such as capabilities). We can discuss this in Discussion #215 , including the structure of EdgeCloudZone incorporating the URN Identifier.

I fully agree with this idea.

Sure, we can discuss it there.

First version of the Edge Cloud LCM API documentation.

gainsley

Thanks for adding the App APIs Javier!

code/API_definitions/EdgeCloud_LcM.yaml

gainsley · 2024-03-07T18:16:22Z

code/API_definitions/EdgeCloud_LcM.yaml

        content:
          application/json:
            schema:
-              $ref: '#/components/schemas/AppInstantiation'
-        required: true      
+              $ref: '#/components/schemas/AppManifest'


I suggest App instead of AppManifest, to keep it simple and consistent with the API path /app.

Maybe this will be confusing because an App schema may have more information like appId. But in POST /app Developers are just sending the information of their app in an AppManifest

I don't see an App component definition, though. So perhaps the AppManifest component should be renamed to App? Or is it expected that there is an App component definition which includes AppManifest as part of it? It would be confusing if the /app path creates an AppManifest instead of an App, if the plan is to include an App component.

@gainsley
The approach for POST /app is that developers can send the information about their application to the Operator Platform. We used to name this information artifact but we change it to AppManifest to make it more understandable. So when developers send the application information, Operator Platform checks if all the parameters are suitable (download the image from the repository successfully), and if everything went well, OP store that information and creates appId that is the identifier used to retrieve the information of the application (e.g. Developer are not sure about the version, they can use appId to get the information).

Perhaps, what you are saying makes sense in terms that an App component needs to exist in order to retrieve all the information related to the application not only what the Developers initially sends to the OP but also what OP has done with the information (creating the appId). In that case, we can create an App component that contains AppManifest and AppID, that will be used in the GET /app/{appId} response.

gainsley · 2024-03-07T18:23:44Z

code/API_definitions/EdgeCloud_LcM.yaml

@@ -112,60 +108,38 @@ paths:
              example:
                status: 409
                code: CONFLICT
-                message: "Application already instantiated in the given Edge or Region"
+                message: "App duplicated"


I suggest App already exists as the error message instead of App duplicated, as the latter makes it sound like a duplicate was created.

Agree with you. Take it into account!

What's the criteria for a conflict here? Since this is a POST and the input is only the manifest, I'm not sure a conflict is something that can happen.

Typically the given 'name' of an object is also treated as a unique identifier within the scope of that organization's objects. So the conflict would be that an App with the given name already exists. This is necessary if the API path uses the name, i.e. DELETE /app/{name}. But if we're only using server-generated IDs in the path, i.e. DELETE /app/{appId} then it's not strictly necessary. But I would argue it's a user-friendly feature to ensure names assigned to objects are unique, otherwise it becomes difficult for users to distinguish the differences between objects with the same given name.

I would add we should decide (and document) whether given names will be treated as unique identifiers or not.

Perfect. We will add in the documentation which given names will be treated as unique identifiers.

I'm not sure if the criteria to declare that an application already exists is only the name, I mean that the criteria should be name and version because developers can have the same application name but different versions.

Yes, we use the pair name+version as the unique identifier. It would be easiest for us to continue with that approach, but we are open to discussion on this.

I think we need more clarity on the scope of the identifier where it is unique e.g. globally or within an organization? In that case we also need to define what is an organization in our APIs so the uniqueness can be handled accordingly? In a global scope there can always be conflicts among "name+ version".

@gunjald, @gainsley. I think creating a new Discussion on this topic would help in deciding how to define unique identifiers. Could one of you create one?

code/API_definitions/EdgeCloud_LcM.yaml

gainsley · 2024-03-07T18:32:24Z

code/API_definitions/EdgeCloud_LcM.yaml

      responses:
        '202':
-          description: Request accepted to be processed. It applies for async deletion process
+          description: Request accepted


202 provides the request was accepted, but there's no callback here to be notified of success/failure, or any way to retrieve the error was if the async process had a problem. Since we don't expect deleting an App definition to be a long-running process, I suggest just changing this to 200 and keeping the API simpler.

202 was adopted as @gunjald suggested. And we think is the best option as in CAMARA commonalities indicates that for async process 202 must be showed.

We know that there is no callback and for this MVP the way to know when a process is completed is using the GET method. In next releases callbacks should be added.

Sorry I should have emphasized it more, but I would expect App APIs (which just manage definitions, i.e. database entries) to finish immediately. Do we expect App APIs to be long-running? In our case, the App APIs never modify instantiated objects (clusters/pods/running containers/etc). They just change the definition of the app, which is just a write to the database. In that case we should use 200, since 202 should only be used for long-running APIs.

In our case, we treat Apps like container images. You can update the container image (in the repository) but it doesn't affect any running containers. If you want to apply the change, you'll need to have the instance (re)pull the image from the repository and redeploy it. We keep a revision ID on the App definition (must like container images use tags/sha256 sums) to track the "version" of the App. The revision ID is incremented whenever the App definition is updated. We do not allow the App to be deleted if the are instances of the App running.

App deletion with an app instance running is not allowed too.

I see your point but correct me if I'm wrong, the deletion process means that the image in the repository will be deleted and maybe this process could take some time (repository could be on a centralized node or shared with other MNOs/CSP) but If you think this is not the case and nobody else thinks otherwise I'm agree to change the response to 200

@javierlozallu Actually our LCM APIs and the Federation EWBI APIs have separate APIs for handling binary images, so deleting the App definition wouldn't actually trigger deleting the referenced image. This is primarily because multiple App definitions may want to share the same binary image(s), and especially for VM images, we want to avoid as much as possible having to re-upload large binary images if the user is re-creating an App definition for example.

/app APIs in my opinion could be subjective on how a platform implements the handling. Some of the platform may just store in a central repository all the information from the API or some could even send it to the environments where they may expect it to launch later only on those environments. So if we provide a GET call to retrieve status of the /app API i think we can accommodate more wider platform implementations.

code/API_definitions/EdgeCloud_LcM.yaml

Changes to adapt the API with PR comments

Added EdgeCloudeZone schema to harmonize with SED. Updated version to 0.9.2, as relevant changes are made. Adaptation of the format to the MegalinterWorkflow criteria.

gunjald · 2024-04-08T09:52:28Z

Are there open points that we still need to address for this PR? Reviewers, Please suggest if we need to conclude on to merge this PR.

javierlozallu · 2024-04-10T09:10:41Z

There is an open point, How to define unique identifiers but I think that is a discussion we may address out of this PR. Taken this in to account, @gainsley , @nicolacdnll , @ThomasEdgeXR leave your comments if you have any.

nicolacdnll · 2024-04-11T13:44:34Z

I would suggest we merge this and discuss unique identifier in a separate issue.

Update EdgeCloud_LcM.yaml version 0.9.1

99cc877

Adopted Edge Cloud Zone name. Adjusting errors detected in Linter for paths name for /app-instance and duplicated method which has been fixed including Delete method in /app-instance/{appId}. Method /app-instace/{appInstanceId} deleted.

javierlozallu requested review from gunjald, Kevsy, crissancas, FabrizioMoggio and sergiofranciscoortiz as code owners February 16, 2024 10:20

sergiofranciscoortiz mentioned this pull request Feb 16, 2024

Review of reported errors from local run CAMARA linter to confirm conformance with API Design Guidelines for EdgeCloud_LcM.yaml #192

Closed

Update EdgeCloud_LcM.yaml v0.9.1

09857fe

Region and Edge Cloud Zone description updated. Parameters with UUID format have an Id at the end. Parameters with String format do not, that is why we decided to change RegionId to Region.

Update EdgeCloud_LcM.yaml

51d38cb

Reorganization of the API structure to clean it up.

sergiofranciscoortiz mentioned this pull request Feb 20, 2024

Gherkin Test feature file for EdgeCloud_LCM #211

Merged

Kevsy reviewed Feb 21, 2024

View reviewed changes

code/API_definitions/EdgeCloud_LcM.yaml Outdated Show resolved Hide resolved

Update EdgeCloud_LcM.yaml

aeb5c65

Added "Provider" parameter to edgeCloudZone and adapted related methods. Spelling and camelCase corrections.

Edge Cloud LCM API documentation

c52d99f

First version of the Edge Cloud LCM API documentation.

javierlozallu requested review from nicolacdnll and gainsley as code owners March 7, 2024 17:09

gainsley reviewed Mar 7, 2024

View reviewed changes

nicolacdnll reviewed Mar 10, 2024

View reviewed changes

code/API_definitions/EdgeCloud_LcM.yaml Outdated Show resolved Hide resolved

Edge Cloud LCM API User Story

a305684

javierlozallu changed the title ~~Version 0.9.1 of EdgeCloud_LcM.yaml with EdgeCloudZone and first Linter corrections.~~ EdgeCloud_LcM.yaml v0.9.2 with EdgeCloudZone and first Linter corrections. Documentation and User Story Mar 25, 2024

javierlozallu added 3 commits March 25, 2024 19:53

Update EdgeCloud_LcM.yaml with the comments

2d1e2f4

Changes to adapt the API with PR comments

Merge branch 'main' into patch-3

5a01556

Update EdgeCloud_LcM.yaml to harmonize EdgeCloudZone

9816864

Added EdgeCloudeZone schema to harmonize with SED. Updated version to 0.9.2, as relevant changes are made. Adaptation of the format to the MegalinterWorkflow criteria.

javierlozallu requested review from nicolacdnll and gainsley April 10, 2024 09:04

nicolacdnll approved these changes Apr 11, 2024

View reviewed changes

javierlozallu merged commit eded195 into camaraproject:main Apr 16, 2024
1 check failed

EdgeCloud_LcM.yaml v0.9.2 with EdgeCloudZone and first Linter corrections. Documentation and User Story #208

EdgeCloud_LcM.yaml v0.9.2 with EdgeCloudZone and first Linter corrections. Documentation and User Story #208

Conversation

javierlozallu commented Feb 16, 2024

gunjald commented Feb 16, 2024

javierlozallu commented Feb 16, 2024

gunjald commented Feb 16, 2024

gunjald commented Feb 16, 2024

gunjald commented Feb 18, 2024

gunjald commented Feb 18, 2024

javierlozallu commented Feb 19, 2024

javierlozallu commented Feb 19, 2024

gunjald commented Feb 19, 2024

javierlozallu commented Feb 19, 2024

gunjald commented Feb 19, 2024

javierlozallu commented Feb 19, 2024 • edited Loading

gunjald commented Feb 19, 2024

javierlozallu commented Feb 19, 2024

javierlozallu commented Feb 20, 2024

gunjald commented Feb 20, 2024

Choose a reason for hiding this comment

Kevsy commented Feb 21, 2024

Kevsy commented Feb 23, 2024

javierlozallu commented Feb 26, 2024 • edited Loading

javierlozallu commented Feb 26, 2024

Kevsy commented Feb 27, 2024

javierlozallu commented Feb 27, 2024

gainsley 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

javierlozallu Mar 13, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunjald commented Apr 8, 2024

javierlozallu commented Apr 10, 2024

nicolacdnll commented Apr 11, 2024

javierlozallu commented Feb 19, 2024 •

edited

Loading

javierlozallu commented Feb 26, 2024 •

edited

Loading

javierlozallu Mar 13, 2024 •

edited

Loading