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

Make sure PVC is working in dev environment #10017

Closed
Tracked by #7901
sagor999 opened this issue May 13, 2022 · 6 comments · Fixed by #10147
Closed
Tracked by #7901

Make sure PVC is working in dev environment #10017

sagor999 opened this issue May 13, 2022 · 6 comments · Fixed by #10147
Assignees

Comments

@sagor999
Copy link
Contributor

sagor999 commented May 13, 2022

If we create the PVC with spec.storageClassName="", the Kubernetes will search the storage class name "" rather than using the default storage class (which is local-path) in the cluster, ref to https://kubernetes.io/docs/concepts/storage/persistent-volumes/#reserving-a-persistentvolume.

We should update the code then to not set storageClassName in that case. As that would be what users in self hosted would probably do as well, do not set (keep empty) storage class to use default one.

@sagor999
Copy link
Contributor Author

Depends on this being merged first: #9475

@jenting
Copy link
Contributor

jenting commented May 18, 2022

I'd like to handle this one @sagor999
May I assign it to me?

@sagor999 sagor999 assigned jenting and unassigned sagor999 May 18, 2022
@sagor999
Copy link
Contributor Author

@jenting assigned you. Thank you! ❤️

@jenting jenting moved this from Scheduled to In Progress in 🌌 Workspace Team May 19, 2022
@jenting
Copy link
Contributor

jenting commented May 19, 2022

The current code, creating the PVC uses the "default" storageClass (which is empty) in ws-manager config.

According to https://kubernetes.io/docs/concepts/storage/persistent-volumes/#reserving-a-persistentvolume, if we create the PVC with spec.storageClassName="", the Kubernetes will search the storage class name "" rather than using the default storage class (which is local-path) in the cluster.

        PVCConfig := m.Config.WorkspaceClasses[config.DefaultWorkspaceClass].PVC
	if startContext.Class != nil {
		PVCConfig = startContext.Class.PVC
	}
	storageClassName := PVCConfig.StorageClass

	PVC := &corev1.PersistentVolumeClaim{
		ObjectMeta: metav1.ObjectMeta{
			Name:      fmt.Sprintf("%s-%s", prefix, req.Id),
			Namespace: m.Config.Namespace,
		},
		Spec: corev1.PersistentVolumeClaimSpec{
			AccessModes:      []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
			StorageClassName: &storageClassName,
			Resources: corev1.ResourceRequirements{
				Requests: corev1.ResourceList{
					corev1.ResourceName(corev1.ResourceStorage): PVCConfig.Size,
				},
			},
		},
	}

Therefore, we could either update the ws-manager config, "default" storageClass to local-path or we could check if the "default" storageClass is empty, do not set the PVC spec.storageClassName to "".

Probably go with update the ws-manager config way better for different environments (SaaS, Self-hosted).

@sagor999
Copy link
Contributor Author

if we create the PVC with spec.storageClassName="", the Kubernetes will search the storage class name "" rather than using the default storage class (which is local-path) in the cluster

Oh. Good find! I think we should update the code then to not set storageClassName in that case. 🤔
As that would be what users in self hosted would probably do as well, do not set (keep empty) storage class to use default one. I will create a separate issue for this and submit a fix once VolumeSnapshot support PR will get merged.

@jenting
Copy link
Contributor

jenting commented May 19, 2022

I will create a separate issue for this and submit a fix once VolumeSnapshot support PR will get merged.

I think we could use this issue to track.

Repository owner moved this from In Progress to Done in 🌌 Workspace Team May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants