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

[nexus] Add stubs for image APIs #749

Merged
merged 7 commits into from
Apr 4, 2022
Merged

[nexus] Add stubs for image APIs #749

merged 7 commits into from
Apr 4, 2022

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Mar 10, 2022

First part of #734

@smklein smklein changed the title [nexus] Add stubs for images api [nexus] Add stubs for image APIs Mar 10, 2022
Comment on lines 242 to 243
pub identity: IdentityMetadataCreateParams,
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need more here to actually be able to create the image.

Do we want this to be an enum implying "Please provide one of a {file, snapshot UUID, URL}"? Do we want that to all be done through the same endpoint?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, yeah - this maps almost directly to the read only parent Volume. "file" seems dangerous to expose externally, but snapshot UUID and URL could be our start. It may also be that an extra field is only necessary for non-global images (aka you don't have to specify anything for Ubuntu 21.10, Nexus knows what that is)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an enum of "snapshot UUID or URL". We can continue to add extra params as necessary.

time_deleted TIMESTAMPTZ,

project_id UUID,
volume_id UUID NOT NULL,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the image has been constructed, it'll refer to a particular volume.

For the case of e.g. POST-ing an image by URL, would we immediately download + store the image as a part of invoking the POST request?

  • If yes: Cool, this structure should work. We'll create a saga that does that work.
  • If no: This structure may need to be more complex - we'll need to store the URL too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently an unsolved problem, though that answer will likely be the same as others: some block migration task will move blocks from the read only parent to the volume region set(s), then remove the read only parent later. We can't gate the instance boot or image creation on that download though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'll leave this question open as an implementation detail for follow-up PRs

@@ -370,6 +370,26 @@ CREATE INDEX ON omicron.public.disk (
) WHERE
time_deleted IS NULL AND attach_instance_id IS NOT NULL;

CREATE TABLE omicron.public.image (
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFD 4 mentions the following fields which are not yet stored here (nor a part of the API):

  • version
  • digest
  • status

@smklein smklein requested a review from jmpesp March 10, 2022 23:49
#[endpoint {
method = GET,
path = "/organizations/{organization_name}/projects/{project_name}/images",
tags = ["images"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tags = ["images"],
tags = ["projects"],

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there documentation on what tag should be associated with what endpoint? I think I might not be understanding this fully. I thought that an operation acting on:

path = "/organizations/{organization_name}/projects"

Would be tagged with projects, but an operation acting on:

path = "/organizations/{organization_name}/projects/{project_name}/foobar"

Would be tagged with foobar(s).

Are both supposed to be tagged with projects? If we keep nesting, is it just projects all the way down?

#[endpoint {
method = POST,
path = "/organizations/{organization_name}/projects/{project_name}/images",
tags = ["images"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tags = ["images"]
tags = ["projects"]

#[endpoint {
method = GET,
path = "/organizations/{organization_name}/projects/{project_name}/images/{image_name}",
tags = ["images"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tags = ["images"],
tags = ["projects"],

#[endpoint {
method = DELETE,
path = "/organizations/{organization_name}/projects/{project_name}/images/{image_name}",
tags = ["images"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tags = ["images"],
tags = ["projects"],

@jessfraz
Copy link
Contributor

sorry if the fmt is off in my suggestions. Wrote them by hand but will align well with #773 when merged

@smklein
Copy link
Collaborator Author

smklein commented Mar 16, 2022

sorry if the fmt is off in my suggestions. Wrote them by hand but will align well with #773 when merged

@jessfraz , I think these style comments are contradictory to the direction being pursued in #770 ?

@jessfraz
Copy link
Contributor

Yeah that’s fair I missed 770, is there a way to grep replace of I can do by hand, the content is mostly what matters

@smklein
Copy link
Collaborator Author

smklein commented Mar 29, 2022

Hey @jessfraz , I incorporated your recommended comments. Regarding the tags, I added documentation for "images" to tag-config.json - but lemme know if you still think I'm using the wrong tags!

@jessfraz
Copy link
Contributor

Awesome, tags and docs look great!

@smklein smklein enabled auto-merge (squash) April 4, 2022 16:06
@smklein smklein merged commit 589bcdd into main Apr 4, 2022
@smklein smklein deleted the image-stubs branch April 4, 2022 16:09
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.

3 participants