Skip to content

[content-service-api] add new snapshot initializer #9808

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

Closed
wants to merge 1 commit into from

Conversation

sagor999
Copy link
Contributor

@sagor999 sagor999 commented May 5, 2022

Description

This just adds a new from snapshot volume initializer that is used by #9475 to add support for initializing workspace from snapshot volume.

Related Issue(s)

Part of #9475

How to test

Release Notes

none

Documentation

Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

The rest LGTM

@sagor999 sagor999 force-pushed the pavel/snapshot-init branch from 69c0db1 to d69a9af Compare May 9, 2022 00:41
@jenting
Copy link
Contributor

jenting commented May 9, 2022

/werft run

👍 started the job as gitpod-build-pavel-snapshot-init.2
(with .werft/ from main)

@csweichel
Copy link
Contributor

How would this be used? I.e. who would decide how when to use this initializer? Server can hardly make that decision, because workspace content storage is opaque to webapp.

IMHO we should try and stick to the backup/prebuild/snapshot initializer we have today which rely on a "snapshot URL" (or in case of the backup initializer use their own convention to discover the backup).

@sagor999
Copy link
Contributor Author

sagor999 commented May 9, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-pavel-snapshot-init.3
(with .werft/ from main)

@geropl
Copy link
Member

geropl commented May 9, 2022

How would this be used? I.e. who would decide how when to use this initializer? Server can hardly make that decision, because workspace content storage is opaque to webapp.

IMHO we should try and stick to the backup/prebuild/snapshot initializer we have today which rely on a "snapshot URL" (or in case of the backup initializer use their own convention to discover the backup).

@sagor999 Would be interested in this as well ☝️ So far I'm not aware WebApp is going to store information about how workspace content is stored; that was handled inside content-service. Is that going to change in any of the following PRs? Or is this a workspace-internal change, and just happens to expand the API surface? 🤔

@sagor999
Copy link
Contributor Author

sagor999 commented May 9, 2022

@csweichel this is used by #9475, if you want to see how decision is being made when to use it.
It will be very cumbersome (I think) to add this into existing initializer (and possibly confusing)

@sagor999
Copy link
Contributor Author

sagor999 commented May 9, 2022

@csweichel @geropl I extracted this from original PR at #9475 to make it easier to review those changes piece by piece, but I guess this was not very helpful. You can look at that PR to see how this initializer is used.

@sagor999
Copy link
Contributor Author

sagor999 commented May 9, 2022

@csweichel @geropl just to add, if there is a concern that there are too many initializers, once PVC approach is adopted, FromBackup and Snapshot initializers will be removed, as they will be obsolete.

@csweichel
Copy link
Contributor

@sagor999 I've put a call in for tomorrow - I reckon we should catch up and go through those changes sync.

@sagor999
Copy link
Contributor Author

After a sync with @csweichel decided it will be best to re-use existing initializers so that it is easier to add PVC support into prebuilds later on.

@sagor999 sagor999 closed this May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants