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 snapshot APIs #748

Merged
merged 8 commits into from
Mar 15, 2022
Merged

[nexus] Add stubs for snapshot APIs #748

merged 8 commits into from
Mar 15, 2022

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Mar 10, 2022

First part of #735

@smklein smklein requested a review from jmpesp March 10, 2022 22:14
@smklein
Copy link
Collaborator Author

smklein commented Mar 10, 2022

FYI, there is a minor divergence from the WIP doc you sent me: this change allows users to add a "name + description" of the snapshot object, and refer to it by a project-scoped name.

This fits the existing scheme for all other objects exposed by the API currently.

@davepacheco
Copy link
Collaborator

Cool! What's the plan for automated tests for the Nexus parts (like the CRUD stuff)?

@smklein
Copy link
Collaborator Author

smklein commented Mar 10, 2022

Cool! What's the plan for automated tests for the Nexus parts (like the CRUD stuff)?

I can't do this quite yet, as a Nexus-level integration test would require a "real DB implementation" of this stuff. However, when that arrives, that'll be up next.

I've added:

[ ] Add support in simulated sled agent / crucible
[ ] Add integration tests in Nexus

to both #734 and #735

Copy link
Contributor

@jmpesp jmpesp left a comment

Choose a reason for hiding this comment

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

I have nothing to add beyond the other comments here.

@smklein smklein merged commit 6db4479 into main Mar 15, 2022
@smklein smklein deleted the snapshot-stubs branch March 15, 2022 23:31
@benjaminleonard
Copy link
Contributor

benjaminleonard commented May 9, 2022

Are there any plans to add state?

GCP has a ready state, and AWS uses completed with a progress percentage.

Same question for images (and any others in that class of resource that might similarly have a ready state).

Also, the snapshot_id on a disk – I gather that's the snapshot used to create the disk, since it's a one-to-one relationship, not a list of a snapshots. And it's fair to see we'll be able to filter snapshots by source disk right?

@smklein
Copy link
Collaborator Author

smklein commented May 9, 2022

@benjaminleonard We could definitely add something like that - as snapshots haven't been implemented yet, I'm not 100% how long we expect they'll take - but adding "state" indicating progress to image creation seems totally reasonable.

You're right, the snapshot_id field on disks is the "snapshot used to create the disk", but I wouldn't describe the relationship as "one-to-one". Each disk will be created from either zero or one snapshot, and a snapshot may be used to create many disks.

And it's fair to see we'll be able to filter snapshots by source disk right?

We definitely could - this would be for an operation like "see all the snapshots of this disk that exist"?

I think we'd want to add an index for this operation (currently, we only have an index to lookup snapshots by (project_id, name)) but that should be possible.

@benjaminleonard
Copy link
Contributor

Great thanks for clarifying Sean!

@leftwo
Copy link
Contributor

leftwo commented May 9, 2022

One little thing to add is that a "source snapshot" can and probably will go away. Once a disk has been created from a snapshot, that disk will have a life of its own and it's possible the original snapshot will be deleted.

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.

7 participants