Skip to content
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

EAM API: singular zone declarative app instance post API #316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gainsley
Copy link
Collaborator

@gainsley gainsley commented Nov 6, 2024

What type of PR is this?

  • correction
  • enhancement/feature

What this PR does / why we need it:

  • Deploy an AppInstance to a single Zone instead of multiple zones. This avoids confusion about the behavior if one or more of the multiple zone deployments fails, see EAM: Suggest that POST appinstance targets a single EdgeCloudZone #256.
  • Assign a name to the AppInstance. Without this we cannot have declarative/infrastructure-as-code clients layered on top of the NBI API. See EAM: Suggest to add App Instance Name #255. Also makes it consistent with other objects that also have names, i.e. Apps, EdgeCloudZones.
  • Adds more necessary information to the returned AppInstanceInfo, including the AppId (previously no way to figure out which App the AppInstance was an instance of). Makes the necessary information required. Changed the EdgeCloudZoneName to an EdgeCloudZoneId for consistency, as we always reference other objects by their ID, not their name.

Which issue(s) this PR fixes:

Fixes #256, #255

Special notes for reviewers:

Changelog input

 release-note
- appinstance post API targets a single zone instead of multiple zones and requires a name for declarative and infrastructure-as-code clients, added more information to appinstanceinfo

Additional documentation

docs

@LuisVTazon
Copy link

The original definition of the API is designed to allow the consumer of the API This proposal is intended to allow the user of the /appinstances resource to request the deployment of the same applicationin different zones, with one single call.

This proposal seem to modify that behaviour by only allowing the deployment of an application in one zone per single request.

I think the original behaviour should stull be possible to simplify the user of the API, therefore I identify the following potential options to this

  1. Create a new resource, different than /appinstances to manage a single deployment per request
  2. Leaving the original definition of /appinstances but modify the response so that it provides the status of each of the deployments that are triggered upon the request (zone and status of execution of request)

@JoseMConde
Copy link
Collaborator

I have the same concern than @LuisVTazon commented, the idea is to make things easy to the use of the API, with this change we are obligating to the user to deploy these applications one by one per each zone.

@gainsley
Copy link
Collaborator Author

I'm not sure it would necessarily be easier. For example, if you are deploying through a web UI, the user will probably check a bunch of check boxes for which zones to deploy to, and then hit a deploy button. Whether the UI code runs one API call or many, the user doesn't actually know. In fact, individual calls may be better, in case some finish earlier than others, the user gets feedback sooner, rather than having to wait until all AppInstance requests are processed.

How are you imagining these APIs will be used?

Additionally, the DELETE API is for a single instance, so anyway DELETE must be managed one-by-one. It seems inconsistent that the API allows me to create many at once, but only delete one at a time. Why is this optimization necessary for create, but not for delete?

If we have a multi-zone API call:

  • what is the overall response status code if some AppInsts are accepted (202), some fail (400), and some have conflicts (409) or other statuses?
  • if a target zone already has an instance of the Application, is the intent that we create another instance, or is this a conflict (409), or do we accept and return the ID of the already existing AppInst?

@JoseMConde
Copy link
Collaborator

Hi @gainsley ,
As far as I know, the idea of Camara APIs is not be exposed by web UI, the idea is that APIs should be call directly by the developer without web UI.
In my view, is different when we talk about delete or deploy, I mean, if you by mistake deploy several Instances instead of just one, this doens´t impact in the final user, but if you delete several Instances by mistake, this could impact in the final user, for this reason, we didin't allow the developer to delete several Instances in one call.
About your questions, you are totally wright, we need to improve this.
For the first question, I think the response could be an array of responses, something like this:
One response per AppInsts.

  • 202, Application instantation accepted.
  • 202, Application instantation accepted.
  • 202, Application instantation accepted.
  • 202, Application instantation accepted.
  • 400, bad request.
  • 202, Application instantation accepted.
    For the second question, I think It should return 409, just to let the developer know that this instances is already Instantiated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EAM: Suggest that POST appinstance targets a single EdgeCloudZone
3 participants