-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add persistent volume support for workspaces #9242
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
Conversation
1af7aa8
to
8589a81
Compare
/werft run 👍 started the job as gitpod-build-pavel-9142-pvc2.13 |
b987d4d
to
294bfbc
Compare
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-pavel-9142-pvc2.31 |
5bd7417
to
dbcf9e6
Compare
58f8993
to
918a488
Compare
@csweichel @aledbf @Furisto @gitpod-io/engineering-self-hosted @gitpod-io/engineering-ide @kylos101 for visibility. |
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-pavel-9142-pvc2.44 |
I tried it in the preview environment, but it stopped at the Preparing workspace and did not proceed. staging-pavel-9142-pvc2 ws-db9ba9eb-2670-4f1e-9d68-a17e45816063 Pending 10m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not try, but I looked at supervisor changes and they look backward compatible to how we check for content readiness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thank you for incorporating the feedback.
In a follow-up PR we need to have tests CDWP and status tests for the PVC workspaces.
// persistent_volume_claim means that we use PVC instead of HostPath to mount /workspace folder and content-init | ||
// happens inside workspacekit instead of in ws-daemon. We also use k8s Snapshots to store\restore workspace content | ||
// instead of GCS\tar. | ||
bool persistent_volume_claim = 9; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using enumerations instead of using bool? e.g. VolumeType, StorageType etc
https://developers.google.com/protocol-buffers/docs/proto3#enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is applicable here, since this bool flag is used as feature flag. Once PVC feature is stable and working, we will remove old code and remove this feature flag and this new code will become default code path.
@@ -1221,6 +1221,12 @@ func startContentInit(ctx context.Context, cfg *Config, wg *sync.WaitGroup, cst | |||
}() | |||
|
|||
fn := "/workspace/.gitpod/content.json" | |||
fnReady := "/workspace/.gitpod/ready" | |||
if _, err := os.Stat("/.workspace/.gitpod/content.json"); !os.IsNotExist(err) { | |||
fn = "/.workspace/.gitpod/content.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it explicit that these paths come from a PVC mount, why not make it refer to a variable shared with the following location? This would surely help the code reader.
https://github.com/gitpod-io/gitpod/pull/9242/files#diff-46a025104f556cc7faed4319cb1c9d2bd0b5097231b42707825aaf0df24d7337R477
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will be switching default to always use /.workspace
folder, so that we don't need to do all those checks in the future
I tried it here, but the workspace never gets ready. |
@Furisto @utam0k thank you for reviews! For preview environment, please use this branch: |
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-pavel-9142-pvc2.46 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions:
- What if the PVC storage full, will the workspace hang? Or will it auto increase the PVC size? (I guess no)?
- If I understand correctly, once the workspace stop, it'd still backup the content to the remote like S3, correct?
@@ -227,6 +227,103 @@ func (s *Provider) GetContentLayer(ctx context.Context, owner, workspaceID strin | |||
return nil, nil, xerrors.Errorf("no backup or valid initializer present") | |||
} | |||
|
|||
// GetContentLayerPVC provides the content layer for a workspace that uses PVC feature | |||
func (s *Provider) GetContentLayerPVC(ctx context.Context, owner, workspaceID string, initializer *csapi.WorkspaceInitializer) (l []Layer, manifest *csapi.WorkspaceContentManifest, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: we could refactor these functions GetContentLayer
and GetContentLayerPVC
into one in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Once PVC is stable and we switched over to it, old code will be removed. (so that we will have only one version of that function)
It depends on workspace, but it might experience issues. It is the same behavior as pre PVC though (since workspaces have same 30GB storage quota set on them). And we wouldn't auto increase PVC size.
When workspace stops, we will create snapshot volume out of PVC and that will be our backup. (part of a different PR). We are moving away from S3\Object storage. |
@aledbf just need your approval. |
If I understand correct, create a snapshot volume is leverage the Kubernetes snapshot controller + volumesnapshot/volumesnapshotcontent/volumesnapshotclass CRDs. However, it's the in-cluster CRDs and the volume snapshot could be stored in the external storage disk, then how can we handle a case that we launch a new cluster and shift the traffic to new cluster? If means that the old and new cluster must uses the same external storage, but it also means that the external storage needs to support mounts by multiple node/zone/... |
Not quite. We are using GCP's snapshot feature for this. So when we create volumesnapshot, it actually creates GCP`s snapshot of the disk from which PVC was mounted from. |
Awesome 💯 , then we must make sure the self-hosted works as well 🙏 |
Description
Add support for persistent volume claim use for workspaces instead of using local storage.
Usage is gated behind "persistent volume claim" feature flag.
Related Issue(s)
Fixes #9142
Fixed #9442
How to test
Spin up new workspace preview environment.
go into admin panel and enable feature flag
start new workspace. it will start using dedicated persistent volume.
observe new pvc created (
kubectl get pvc -A
)delete workspace. pvc will be deleted.
image build is using old codepath and not affected by this change.
prebuilds are using old codepath and are not affected by this change.
limitations:
current implementation doesn't support restoring from previous backup. That will be added separately.
Release Notes
Documentation