diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 0ca3e16ae..a951c5c7f 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -132,13 +132,6 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct return r.stopWorkspace(workspace, reqLogger) } - // Set finalizer on DevWorkspace if necessary - if ok, err := r.setFinalizer(ctx, workspace); err != nil { - return reconcile.Result{}, err - } else if !ok { - return reconcile.Result{Requeue: true}, nil - } - // Prepare handling workspace status and condition reconcileStatus := currentStatus{ Conditions: map[devworkspace.WorkspaceConditionType]string{}, @@ -194,6 +187,15 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct return reconcile.Result{}, nil } workspace.Spec.Template = *flattenedWorkspace + // Set finalizer on DevWorkspace if necessary + // Note: we need to check the flattened workspace to see if a finalizer is needed, as plugins could require storage + if isFinalizerNecessary(workspace) { + if ok, err := r.setFinalizer(ctx, clusterWorkspace); err != nil { + return reconcile.Result{}, err + } else if !ok { + return reconcile.Result{Requeue: true}, nil + } + } devfilePodAdditions, err := containerlib.GetKubeContainersFromDevfile(workspace.Spec.Template) if err != nil { diff --git a/controllers/workspace/finalize.go b/controllers/workspace/finalize.go index 77922e6ba..7a5bc85c0 100644 --- a/controllers/workspace/finalize.go +++ b/controllers/workspace/finalize.go @@ -51,8 +51,10 @@ var ( PVCCleanupPodMemoryLimit = resource.MustParse("32Mi") ) +// setFinalizer sets a finalizer on the workspace and syncs the changes to the cluster. No-op if the workspace already +// has the finalizer set. func (r *DevWorkspaceReconciler) setFinalizer(ctx context.Context, workspace *v1alpha2.DevWorkspace) (ok bool, err error) { - if !isFinalizerNecessary(workspace) || hasFinalizer(workspace) { + if hasFinalizer(workspace) { return true, nil } workspace.SetFinalizers(append(workspace.Finalizers, pvcCleanupFinalizer)) diff --git a/pkg/library/container/mountSources.go b/pkg/library/container/mountSources.go index 6cefd6b6d..864bbae7c 100644 --- a/pkg/library/container/mountSources.go +++ b/pkg/library/container/mountSources.go @@ -34,6 +34,17 @@ func HasMountSources(devfileContainer *devworkspace.ContainerComponent) bool { return mountSources } +// AnyMountSources checks HasMountSources for each container component in a devfile. If a component in the slice +// is not a ContainerComponent, it is ignored. +func AnyMountSources(devfileComponents []devworkspace.Component) bool { + for _, component := range devfileComponents { + if component.Container != nil && HasMountSources(component.Container) { + return true + } + } + return false +} + // handleMountSources adds a volumeMount to a container if the corresponding devfile container has // mountSources enabled. func handleMountSources(k8sContainer *corev1.Container, devfileContainer *devworkspace.ContainerComponent) { diff --git a/pkg/library/storage/commonStorage.go b/pkg/library/storage/commonStorage.go index 0541f6068..3bf7a3a8f 100644 --- a/pkg/library/storage/commonStorage.go +++ b/pkg/library/storage/commonStorage.go @@ -25,6 +25,8 @@ package storage import ( "fmt" + "k8s.io/apimachinery/pkg/api/resource" + "github.com/devfile/devworkspace-operator/pkg/library/constants" containerlib "github.com/devfile/devworkspace-operator/pkg/library/container" corev1 "k8s.io/api/core/v1" @@ -39,16 +41,18 @@ import ( // // Also adds appropriate k8s Volumes to PodAdditions to accomodate the rewritten VolumeMounts. func RewriteContainerVolumeMounts(workspaceId string, podAdditions *v1alpha1.PodAdditions, workspace devworkspace.DevWorkspaceTemplateSpec) error { - if !NeedsStorage(workspace) { - return nil - } devfileVolumes := map[string]devworkspace.VolumeComponent{} + var ephemeralVolumes []devworkspace.Component + for _, component := range workspace.Components { if component.Volume != nil { if _, exists := devfileVolumes[component.Name]; exists { return fmt.Errorf("volume component '%s' is defined multiple times", component.Name) } devfileVolumes[component.Name] = *component.Volume + if component.Volume.Ephemeral { + ephemeralVolumes = append(ephemeralVolumes, component) + } } } if _, exists := devfileVolumes[constants.ProjectsVolumeName]; !exists { @@ -58,56 +62,81 @@ func RewriteContainerVolumeMounts(workspaceId string, podAdditions *v1alpha1.Pod devfileVolumes[constants.ProjectsVolumeName] = projectsVolume } - // TODO: Support more than the common PVC strategy here (storage provisioner interface?) - // TODO: What should we do when a volume isn't explicitly defined? - commonPVCName := config.ControllerCfg.GetWorkspacePVCName() - rewriteVolumeMounts := func(containers []corev1.Container) error { - for cIdx, container := range containers { - for vmIdx, vm := range container.VolumeMounts { - if _, ok := devfileVolumes[vm.Name]; !ok { - return fmt.Errorf("container '%s' references undefined volume '%s'", container.Name, vm.Name) - } - containers[cIdx].VolumeMounts[vmIdx].SubPath = fmt.Sprintf("%s/%s", workspaceId, vm.Name) - containers[cIdx].VolumeMounts[vmIdx].Name = commonPVCName + for _, component := range ephemeralVolumes { + vol := corev1.Volume{ + Name: component.Name, + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + } + if component.Volume.Size != "" { + sizeResource, err := resource.ParseQuantity(component.Volume.Size) + if err != nil { + return fmt.Errorf("failed to parse size for Volume %s: %w", component.Name, err) } + vol.EmptyDir.SizeLimit = &sizeResource } - return nil - } - if err := rewriteVolumeMounts(podAdditions.Containers); err != nil { - return err - } - if err := rewriteVolumeMounts(podAdditions.InitContainers); err != nil { - return err + podAdditions.Volumes = append(podAdditions.Volumes, vol) } - podAdditions.Volumes = append(podAdditions.Volumes, corev1.Volume{ - Name: commonPVCName, - VolumeSource: corev1.VolumeSource{ - PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ - ClaimName: commonPVCName, + if NeedsStorage(workspace) { + // TODO: Support more than the common PVC strategy here (storage provisioner interface?) + // TODO: What should we do when a volume isn't explicitly defined? + commonPVCName := config.ControllerCfg.GetWorkspacePVCName() + rewriteVolumeMounts := func(containers []corev1.Container) error { + for cIdx, container := range containers { + for vmIdx, vm := range container.VolumeMounts { + volume, ok := devfileVolumes[vm.Name] + if !ok { + return fmt.Errorf("container '%s' references undefined volume '%s'", container.Name, vm.Name) + } + if !volume.Ephemeral { + containers[cIdx].VolumeMounts[vmIdx].SubPath = fmt.Sprintf("%s/%s", workspaceId, vm.Name) + containers[cIdx].VolumeMounts[vmIdx].Name = commonPVCName + } + } + } + return nil + } + if err := rewriteVolumeMounts(podAdditions.Containers); err != nil { + return err + } + if err := rewriteVolumeMounts(podAdditions.InitContainers); err != nil { + return err + } + + podAdditions.Volumes = append(podAdditions.Volumes, corev1.Volume{ + Name: commonPVCName, + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: commonPVCName, + }, }, - }, - }) + }) + } + return nil } -// NeedsStorage returns true if storage will need to be provisioned for the current workspace -// TODO: -// - This function is used to decide if we need to create a PVC; need to figure out how to handle -// case of ephemeral storage only +// NeedsStorage returns true if storage will need to be provisioned for the current workspace. Note that ephemeral volumes +// do not need to provision storage func NeedsStorage(workspace devworkspace.DevWorkspaceTemplateSpec) bool { + projectsVolumeIsEphemeral := false for _, component := range workspace.Components { if component.Volume != nil { - return true - } - if component.Container != nil { - if len(component.Container.VolumeMounts) > 0 { + // If any non-ephemeral volumes are defined, we need to mount storage + if !component.Volume.Ephemeral { return true } - if containerlib.HasMountSources(component.Container) { - return true + if component.Name == constants.ProjectsVolumeName { + projectsVolumeIsEphemeral = component.Volume.Ephemeral } } } - return false + if projectsVolumeIsEphemeral { + // No non-ephemeral volumes, and projects volume mount is ephemeral, so all volumes are ephemeral + return false + } + // Implicit projects volume is non-ephemeral, so any container that mounts sources requires storage + return containerlib.AnyMountSources(workspace.Components) } diff --git a/pkg/library/storage/commonStorage_test.go b/pkg/library/storage/commonStorage_test.go index 6ac4a104f..8c73a3822 100644 --- a/pkg/library/storage/commonStorage_test.go +++ b/pkg/library/storage/commonStorage_test.go @@ -13,7 +13,6 @@ package storage import ( - "fmt" "io/ioutil" "path/filepath" "testing" @@ -54,9 +53,8 @@ func setupControllerCfg() { config.SetupConfigForTesting(testControllerCfg) } -func loadTestCaseOrPanic(t *testing.T, testFilename string) testCase { - testPath := filepath.Join("./testdata", testFilename) - bytes, err := ioutil.ReadFile(testPath) +func loadTestCaseOrPanic(t *testing.T, testFilepath string) testCase { + bytes, err := ioutil.ReadFile(testFilepath) if err != nil { t.Fatal(err) } @@ -64,19 +62,26 @@ func loadTestCaseOrPanic(t *testing.T, testFilename string) testCase { if err := yaml.Unmarshal(bytes, &test); err != nil { t.Fatal(err) } - t.Log(fmt.Sprintf("Read file:\n%+v\n\n", test)) return test } -func TestRewriteContainerVolumeMounts(t *testing.T) { - tests := []testCase{ - loadTestCaseOrPanic(t, "does-nothing-for-no-storage-needed.yaml"), - loadTestCaseOrPanic(t, "projects-volume-overriding.yaml"), - loadTestCaseOrPanic(t, "rewrites-volumes-for-common-pvc-strategy.yaml"), - loadTestCaseOrPanic(t, "error-duplicate-volumes.yaml"), - loadTestCaseOrPanic(t, "error-undefined-volume.yaml"), - loadTestCaseOrPanic(t, "error-undefined-volume-init-container.yaml"), +func loadAllTestCasesOrPanic(t *testing.T, fromDir string) []testCase { + files, err := ioutil.ReadDir(fromDir) + if err != nil { + t.Fatal(err) } + var tests []testCase + for _, file := range files { + if file.IsDir() { + continue + } + tests = append(tests, loadTestCaseOrPanic(t, filepath.Join(fromDir, file.Name()))) + } + return tests +} + +func TestRewriteContainerVolumeMounts(t *testing.T) { + tests := loadAllTestCasesOrPanic(t, "testdata") setupControllerCfg() for _, tt := range tests { @@ -97,34 +102,51 @@ func TestRewriteContainerVolumeMounts(t *testing.T) { } func TestNeedsStorage(t *testing.T) { - boolFalse := false boolTrue := true tests := []struct { - Name string - Explanation string - Components []devworkspace.ComponentUnion + Name string + Explanation string + NeedsStorage bool + Components []devworkspace.Component }{ { - Name: "Has volume component", - Explanation: "If the devfile has a volume component, it requires storage", - Components: []devworkspace.ComponentUnion{ + Name: "Has volume component", + Explanation: "If the devfile has a volume component, it requires storage", + NeedsStorage: true, + Components: []devworkspace.Component{ + { + ComponentUnion: devworkspace.ComponentUnion{ + Volume: &devworkspace.VolumeComponent{}, + }, + }, + }, + }, + { + Name: "Has ephemeral volume and does not need storage", + Explanation: "Volumes with ephemeral: true do not require storage", + NeedsStorage: false, + Components: []devworkspace.Component{ { - Volume: &devworkspace.VolumeComponent{}, + ComponentUnion: devworkspace.ComponentUnion{ + Volume: &devworkspace.VolumeComponent{ + Volume: devworkspace.Volume{ + Ephemeral: true, + }, + }, + }, }, }, }, { - Name: "Has container component with volume mounts", - Explanation: "If a devfile container has volumeMounts, it requires storage", - Components: []devworkspace.ComponentUnion{ + Name: "Container has mountSources", + Explanation: "If a devfile container has mountSources set, it requires storage", + NeedsStorage: true, + Components: []devworkspace.Component{ { - Container: &devworkspace.ContainerComponent{ - Container: devworkspace.Container{ - MountSources: &boolFalse, - VolumeMounts: []devworkspace.VolumeMount{ - { - Name: "test-volumeMount", - }, + ComponentUnion: devworkspace.ComponentUnion{ + Container: &devworkspace.ContainerComponent{ + Container: devworkspace.Container{ + MountSources: &boolTrue, }, }, }, @@ -132,25 +154,41 @@ func TestNeedsStorage(t *testing.T) { }, }, { - Name: "Container has mountSources", - Explanation: "If a devfile container has mountSources set, it requires storage", - Components: []devworkspace.ComponentUnion{ + Name: "Container has mountSources but projects is ephemeral", + Explanation: "When a devfile has an explicit, ephemeral projects volume, containers with mountSources do not need storage", + NeedsStorage: false, + Components: []devworkspace.Component{ + { + ComponentUnion: devworkspace.ComponentUnion{ + Container: &devworkspace.ContainerComponent{ + Container: devworkspace.Container{ + MountSources: &boolTrue, + }, + }, + }, + }, { - Container: &devworkspace.ContainerComponent{ - Container: devworkspace.Container{ - MountSources: &boolTrue, + Name: "projects", + ComponentUnion: devworkspace.ComponentUnion{ + Volume: &devworkspace.VolumeComponent{ + Volume: devworkspace.Volume{ + Ephemeral: true, + }, }, }, }, }, }, { - Name: "Container has implicit mountSources", - Explanation: "If a devfile container does not have mountSources set, the default is true", - Components: []devworkspace.ComponentUnion{ + Name: "Container has implicit mountSources", + Explanation: "If a devfile container does not have mountSources set, the default is true", + NeedsStorage: true, + Components: []devworkspace.Component{ { - Container: &devworkspace.ContainerComponent{ - Container: devworkspace.Container{}, + ComponentUnion: devworkspace.ComponentUnion{ + Container: &devworkspace.ContainerComponent{ + Container: devworkspace.Container{}, + }, }, }, }, @@ -159,13 +197,12 @@ func TestNeedsStorage(t *testing.T) { for _, tt := range tests { t.Run(tt.Name, func(t *testing.T) { workspace := devworkspace.DevWorkspaceTemplateSpec{} - for idx, comp := range tt.Components { - workspace.Components = append(workspace.Components, devworkspace.Component{ - Name: fmt.Sprintf("test-component-%d", idx), - ComponentUnion: comp, - }) + workspace.Components = tt.Components + if tt.NeedsStorage { + assert.True(t, NeedsStorage(workspace), tt.Explanation) + } else { + assert.False(t, NeedsStorage(workspace), tt.Explanation) } - assert.True(t, NeedsStorage(workspace), tt.Explanation) }) } } diff --git a/pkg/library/storage/testdata/can-make-projects-ephemeral.yaml b/pkg/library/storage/testdata/can-make-projects-ephemeral.yaml new file mode 100644 index 000000000..a891b4238 --- /dev/null +++ b/pkg/library/storage/testdata/can-make-projects-ephemeral.yaml @@ -0,0 +1,35 @@ +name: "Can make projects volume ephemeral" + +input: + workspaceId: "test-workspaceid" + podAdditions: + containers: + - name: testing-container-1 + image: testing-image + volumeMounts: + - name: "projects" + mountPath: "/projects-mountpath" + + workspace: + components: + - name: testing-container-1 + container: + image: testing-image-1 + sourceMapping: "/plugins-mountpath" + mountSources: true + - name: projects + volume: + ephemeral: true + +output: + podAdditions: + containers: + - name: testing-container-1 + image: testing-image + volumeMounts: + - name: projects + mountPath: "/projects-mountpath" + + volumes: + - name: projects + emptyDir: {} diff --git a/pkg/library/storage/testdata/can-set-ephemeral-volume-size.yaml b/pkg/library/storage/testdata/can-set-ephemeral-volume-size.yaml new file mode 100644 index 000000000..75c923ac1 --- /dev/null +++ b/pkg/library/storage/testdata/can-set-ephemeral-volume-size.yaml @@ -0,0 +1,37 @@ +name: "Can make projects volume ephemeral" + +input: + workspaceId: "test-workspaceid" + podAdditions: + containers: + - name: testing-container-1 + image: testing-image + volumeMounts: + - name: "projects" + mountPath: "/projects-mountpath" + + workspace: + components: + - name: testing-container-1 + container: + image: testing-image-1 + sourceMapping: "/plugins-mountpath" + mountSources: true + - name: projects + volume: + ephemeral: true + size: 512Mi + +output: + podAdditions: + containers: + - name: testing-container-1 + image: testing-image + volumeMounts: + - name: projects + mountPath: "/projects-mountpath" + + volumes: + - name: projects + emptyDir: + sizeLimit: 512Mi diff --git a/pkg/library/storage/testdata/error-unparseable-ephemeral-size.yaml b/pkg/library/storage/testdata/error-unparseable-ephemeral-size.yaml new file mode 100644 index 000000000..84723ab86 --- /dev/null +++ b/pkg/library/storage/testdata/error-unparseable-ephemeral-size.yaml @@ -0,0 +1,26 @@ +name: "Can make projects volume ephemeral" + +input: + workspaceId: "test-workspaceid" + podAdditions: + containers: + - name: testing-container-1 + image: testing-image + volumeMounts: + - name: "projects" + mountPath: "/projects-mountpath" + + workspace: + components: + - name: testing-container-1 + container: + image: testing-image-1 + sourceMapping: "/plugins-mountpath" + mountSources: true + - name: projects + volume: + ephemeral: true + size: 512XX + +output: + errRegexp: "failed to parse size for Volume projects.*" diff --git a/pkg/library/storage/testdata/handles-ephemeral-volumes.yaml b/pkg/library/storage/testdata/handles-ephemeral-volumes.yaml new file mode 100644 index 000000000..8401faffa --- /dev/null +++ b/pkg/library/storage/testdata/handles-ephemeral-volumes.yaml @@ -0,0 +1,35 @@ +name: "Handles ephemeral volumes" + +input: + workspaceId: "test-workspaceid" + podAdditions: + containers: + - name: testing-container-1 + image: testing-image + volumeMounts: + - name: testvol + mountPath: "/projects-mountpath" + + workspace: + components: + - name: testing-container-1 + container: + image: testing-image-1 + mountSources: false + + - name: testvol + volume: + ephemeral: true + +output: + podAdditions: + containers: + - name: testing-container-1 + image: testing-image + volumeMounts: + - name: testvol + mountPath: "/projects-mountpath" + + volumes: + - name: testvol + emptyDir: {} diff --git a/samples/flattened_theia-next.yaml b/samples/flattened_theia-next.yaml index ea46a0985..a5d0ef0ae 100644 --- a/samples/flattened_theia-next.yaml +++ b/samples/flattened_theia-next.yaml @@ -38,7 +38,8 @@ spec: - name: plugins volume: {} - name: remote-endpoint - volume: {} # TODO: Fix this once ephemeral volumes are supported + volume: + ephemeral: true - name: vsx-installer # Mainly reads the container objects and searches for those # with che-theia.eclipse.org/vscode-extensions attributes to get VSX urls # Those found in the dedicated containers components are with a sidecar, diff --git a/samples/flattened_theia-nodejs.yaml b/samples/flattened_theia-nodejs.yaml index 9595d6e31..7eb506b8c 100644 --- a/samples/flattened_theia-nodejs.yaml +++ b/samples/flattened_theia-nodejs.yaml @@ -107,8 +107,8 @@ spec: name: plugins - name: remote-endpoint - volume: {} - # ephemeral: true #### We should add it in the Devfile 2.0 spec ! Not critical to implement at start though + volume: + ephemeral: true - name: remote-runtime-injector attributes: diff --git a/samples/plugins/theia-next.yaml b/samples/plugins/theia-next.yaml index bb6b7197c..55544982e 100644 --- a/samples/plugins/theia-next.yaml +++ b/samples/plugins/theia-next.yaml @@ -7,7 +7,8 @@ spec: - name: plugins volume: {} - name: remote-endpoint - volume: {} # TODO: Fix this once ephemeral volumes are supported + volume: + ephemeral: true - name: vsx-installer plugin: kubernetes: diff --git a/samples/with-k8s-ref/emptyDir-theia.yaml b/samples/with-k8s-ref/emptyDir-theia.yaml new file mode 100644 index 000000000..83ff548ae --- /dev/null +++ b/samples/with-k8s-ref/emptyDir-theia.yaml @@ -0,0 +1,25 @@ +kind: DevWorkspace +apiVersion: workspace.devfile.io/v1alpha2 +metadata: + name: theia +spec: + started: true + template: + components: + - name: theia + plugin: + kubernetes: + name: theia-next + namespace: devworkspace-plugins + components: + - name: plugins + volume: + ephemeral: true + - name: terminal + plugin: + kubernetes: + name: machine-exec + namespace: devworkspace-plugins + - name: projects + volume: + ephemeral: true