fix: Enable persistentHome only if workspace requires persistent storage

Ensure a persistent home devfile volume will only be added to a devworkspace which actually needs persistent storage. This includes the devworkspace which mount project sources to any containers and have non ephemeral volumes. Signed-off-by: Andrew Obuchowicz <[email protected]>
devfile · Mar 14, 2024 · 7e53503 · 7e53503
1 parent cf19ac6
commit 7e53503
Show file tree

Hide file tree

Showing 7 changed files with 35 additions and 32 deletions.
diff --git a/pkg/library/home/persistentHome.go b/pkg/library/home/persistentHome.go
@@ -20,6 +20,7 @@ import (
 
 	"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
 	devfilevalidation "github.com/devfile/api/v2/pkg/validation"
+	"github.com/devfile/devworkspace-operator/pkg/provision/storage"
 	"k8s.io/utils/pointer"
 
 	"github.com/devfile/devworkspace-operator/pkg/common"
@@ -67,8 +68,10 @@ func StorageStrategySupportsPersistentHome(workspace *common.DevWorkspaceWithCon
 	return storageClass != constants.EphemeralStorageClassType
 }
 
-// Returns true if `persistUserHome` is enabled in the DevWorkspaceOperatorConfig
-// and none of the container components in the DevWorkspace mount a volume to `/home/user/`.
+// Returns true if the following criteria is met:
+// - `persistUserHome` is enabled in the DevWorkspaceOperatorConfig
+// - None of the container components in the DevWorkspace mount a volume to `/home/user/`.
+// - Persistent storage is required for the DevWorkspace
 // Returns false otherwise.
 func NeedsPersistentHomeDirectory(workspace *common.DevWorkspaceWithConfig) bool {
 	if !pointer.BoolDeref(workspace.Config.Workspace.PersistUserHome.Enabled, false) {
@@ -86,5 +89,5 @@ func NeedsPersistentHomeDirectory(workspace *common.DevWorkspaceWithConfig) bool
 			}
 		}
 	}
-	return true
+	return storage.WorkspaceNeedsStorage(&workspace.Spec.Template)
 }
diff --git a/pkg/provision/storage/asyncStorage.go b/pkg/provision/storage/asyncStorage.go
@@ -45,7 +45,7 @@ type AsyncStorageProvisioner struct{}
 var _ Provisioner = (*AsyncStorageProvisioner)(nil)
 
 func (*AsyncStorageProvisioner) NeedsStorage(workspace *dw.DevWorkspaceTemplateSpec) bool {
-	return needsStorage(workspace)
+	return WorkspaceNeedsStorage(workspace)
 }
 
 func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error {

diff --git a/pkg/provision/storage/commonStorage.go b/pkg/provision/storage/commonStorage.go
@@ -41,7 +41,7 @@ type CommonStorageProvisioner struct{}
 var _ Provisioner = (*CommonStorageProvisioner)(nil)
 
 func (*CommonStorageProvisioner) NeedsStorage(workspace *dw.DevWorkspaceTemplateSpec) bool {
-	return needsStorage(workspace)
+	return WorkspaceNeedsStorage(workspace)
 }
 
 func (p *CommonStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error {

diff --git a/pkg/provision/storage/commonStorage_test.go b/pkg/provision/storage/commonStorage_test.go
@@ -288,9 +288,9 @@ func TestNeedsStorage(t *testing.T) {
 			workspace := &dw.DevWorkspaceTemplateSpec{}
 			workspace.Components = tt.Components
 			if tt.NeedsStorage {
-				assert.True(t, needsStorage(workspace), tt.Explanation)
+				assert.True(t, WorkspaceNeedsStorage(workspace), tt.Explanation)
 			} else {
-				assert.False(t, needsStorage(workspace), tt.Explanation)
+				assert.False(t, WorkspaceNeedsStorage(workspace), tt.Explanation)
 			}
 		})
 	}

diff --git a/pkg/provision/storage/perWorkspaceStorage.go b/pkg/provision/storage/perWorkspaceStorage.go
@@ -53,7 +53,7 @@ func (p *PerWorkspaceStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1
 	}
 
 	// If persistent storage is not needed, we're done
-	if !needsStorage(&workspace.Spec.Template) {
+	if !WorkspaceNeedsStorage(&workspace.Spec.Template) {
 		return nil
 	}
 

diff --git a/pkg/provision/storage/perWorkspaceStorage_test.go b/pkg/provision/storage/perWorkspaceStorage_test.go
@@ -60,7 +60,7 @@ func TestRewriteContainerVolumeMountsForPerWorkspaceStorageClass(t *testing.T) {
 				Logger: zap.New(),
 			}
 
-			if needsStorage(&workspace.Spec.Template) {
+			if WorkspaceNeedsStorage(&workspace.Spec.Template) {
 				err := perWorkspaceStorage.ProvisionStorage(tt.Input.PodAdditions.DeepCopy(), workspace, clusterAPI)
 				if !assert.Error(t, err, "Should get a NotReady error when creating PVC") {
 					return

diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go
@@ -37,6 +37,29 @@ import (
 	nsconfig "github.com/devfile/devworkspace-operator/pkg/provision/config"
 )
 
+// WorkspaceNeedsStorage returns true if storage will need to be provisioned for the current workspace. Note that ephemeral volumes
+// do not need to provision storage
+func WorkspaceNeedsStorage(workspace *dw.DevWorkspaceTemplateSpec) bool {
+	projectsVolumeIsEphemeral := false
+	for _, component := range workspace.Components {
+		if component.Volume != nil {
+			// If any non-ephemeral volumes are defined, we need to mount storage
+			if !isEphemeral(component.Volume) {
+				return true
+			}
+			if component.Name == devfileConstants.ProjectsVolumeName {
+				projectsVolumeIsEphemeral = isEphemeral(component.Volume)
+			}
+		}
+	}
+	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)
+}
+
 func getPVCSpec(name, namespace string, storageClass *string, size resource.Quantity) (*corev1.PersistentVolumeClaim, error) {
 
 	return &corev1.PersistentVolumeClaim{
@@ -63,29 +86,6 @@ func isEphemeral(volume *dw.VolumeComponent) bool {
 	return volume.Ephemeral != nil && *volume.Ephemeral
 }
 
-// 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 *dw.DevWorkspaceTemplateSpec) bool {
-	projectsVolumeIsEphemeral := false
-	for _, component := range workspace.Components {
-		if component.Volume != nil {
-			// If any non-ephemeral volumes are defined, we need to mount storage
-			if !isEphemeral(component.Volume) {
-				return true
-			}
-			if component.Name == devfileConstants.ProjectsVolumeName {
-				projectsVolumeIsEphemeral = isEphemeral(component.Volume)
-			}
-		}
-	}
-	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)
-}
-
 func syncCommonPVC(namespace string, config *v1alpha1.OperatorConfiguration, clusterAPI sync.ClusterAPI) (*corev1.PersistentVolumeClaim, error) {
 	namespacedConfig, err := nsconfig.ReadNamespacedConfig(namespace, clusterAPI)
 	if err != nil {