Update reconciler to use library to decide if storage is needed

- Only set storage finalizer on workspace if workspace needs storage as
defined by storage library
- Only generate and sync PVC if workspace needs storage

Signed-off-by: Angel Misevski <[email protected]>

Author: amisevsk

controllers/workspace/devworkspace_controller.go

@@ -179,21 +179,21 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct
 		reqLogger.Info("DevWorkspace start failed")
 		reconcileStatus.Phase = devworkspace.WorkspaceStatusFailed
 		reconcileStatus.Conditions[devworkspace.WorkspaceFailedStart] = fmt.Sprintf("Error processing devfile: %s", err)
-		return reconcile.Result{}, err
+		return reconcile.Result{}, nil
 	}
 	err = storagelib.RewriteContainerVolumeMounts(workspace.Status.WorkspaceId, devfilePodAdditions, workspace.Spec.Template)
 	if err != nil {
 		reqLogger.Info("DevWorkspace start failed")
 		reconcileStatus.Phase = devworkspace.WorkspaceStatusFailed
-		reconcileStatus.Conditions[devworkspace.WorkspaceFailedStart] = fmt.Sprintf("Error processing devfile persistent storage: %s", err)
-		return reconcile.Result{}, err
+		reconcileStatus.Conditions[devworkspace.WorkspaceFailedStart] = fmt.Sprintf("Error processing devfile volumes: %s", err)
+		return reconcile.Result{}, nil
 	}
 	componentDescriptions, err := shimlib.GetComponentDescriptionsFromPodAdditions(devfilePodAdditions, workspace.Spec.Template)
 	if err != nil {
 		reqLogger.Info("DevWorkspace start failed")
 		reconcileStatus.Phase = devworkspace.WorkspaceStatusFailed
 		reconcileStatus.Conditions[devworkspace.WorkspaceFailedStart] = fmt.Sprintf("Error processing devfile for Theia: %s", err)
-		return reconcile.Result{}, err
+		return reconcile.Result{}, nil
 	}
 	reconcileStatus.Conditions[devworkspace.WorkspaceReady] = ""
 	timing.SetTime(workspace, timing.ComponentsReady)
@@ -211,9 +211,11 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct
 		componentDescriptions = append([]controllerv1alpha1.ComponentDescription{cheRestApisComponent}, componentDescriptions...)
 	}
 
-	pvcStatus := provision.SyncPVC(workspace, componentDescriptions, r.Client, reqLogger)
-	if pvcStatus.Err != nil || !pvcStatus.Continue {
-		return reconcile.Result{Requeue: true}, pvcStatus.Err
+	if storagelib.NeedsStorage(workspace.Spec.Template) {
+		pvcStatus := provision.SyncPVC(workspace, r.Client, reqLogger)
+		if pvcStatus.Err != nil || !pvcStatus.Continue {
+			return reconcile.Result{Requeue: true}, pvcStatus.Err
+		}
 	}
 
 	rbacStatus := provision.SyncRBAC(workspace, r.Client, reqLogger)
@@ -283,7 +285,7 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct
 
 	// Step six: Create deployment and wait for it to be ready
 	timing.SetTime(workspace, timing.DeploymentCreated)
-	deploymentStatus := provision.SyncDeploymentToCluster(workspace, podAdditions, componentDescriptions, serviceAcctName, clusterAPI)
+	deploymentStatus := provision.SyncDeploymentToCluster(workspace, podAdditions, serviceAcctName, clusterAPI)
 	if !deploymentStatus.Continue {
 		if deploymentStatus.FailStartup {
 			reqLogger.Info("Workspace start failed")

controllers/workspace/finalize.go

@@ -18,6 +18,8 @@ import (
 	"path"
 	"time"
 
+	storagelib "github.com/devfile/devworkspace-operator/pkg/library/storage"
+
 	"github.com/devfile/api/pkg/apis/workspaces/v1alpha2"
 	"github.com/devfile/devworkspace-operator/controllers/workspace/provision"
 	"github.com/devfile/devworkspace-operator/internal/images"
@@ -223,10 +225,8 @@ func (r *DevWorkspaceReconciler) getClusterCleanupJob(ctx context.Context, works
 	return clusterJob, nil
 }
 
-func isFinalizerNecessary(_ *v1alpha2.DevWorkspace) bool {
-	// TODO: Implement checking whether persistent storage is used (once other choices are possible)
-	// Note this could interfere with cloud-shell until this TODO is resolved.
-	return true
+func isFinalizerNecessary(workspace *v1alpha2.DevWorkspace) bool {
+	return storagelib.NeedsStorage(workspace.Spec.Template)
 }
 
 func hasFinalizer(workspace *v1alpha2.DevWorkspace) bool {

controllers/workspace/provision/deployment.go

@@ -59,13 +59,12 @@ var deploymentDiffOpts = cmp.Options{
 func SyncDeploymentToCluster(
 	workspace *devworkspace.DevWorkspace,
 	podAdditions []v1alpha1.PodAdditions,
-	components []v1alpha1.ComponentDescription,
 	saName string,
 	clusterAPI ClusterAPI) DeploymentProvisioningStatus {
 
 	// [design] we have to pass components and routing pod additions separately because we need mountsources from each
 	// component.
-	specDeployment, err := getSpecDeployment(workspace, podAdditions, components, saName, clusterAPI.Scheme)
+	specDeployment, err := getSpecDeployment(workspace, podAdditions, saName, clusterAPI.Scheme)
 	if err != nil {
 		return DeploymentProvisioningStatus{
 			ProvisioningStatus: ProvisioningStatus{
@@ -160,7 +159,6 @@ func checkDeploymentStatus(deployment *appsv1.Deployment) (ready bool) {
 func getSpecDeployment(
 	workspace *devworkspace.DevWorkspace,
 	podAdditionsList []v1alpha1.PodAdditions,
-	components []v1alpha1.ComponentDescription,
 	saName string,
 	scheme *runtime.Scheme) (*appsv1.Deployment, error) {
 	replicas := int32(1)
@@ -225,8 +223,7 @@ func getSpecDeployment(
 		},
 	}
 
-	if IsPVCRequired(components) {
-		deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, getPersistentVolumeClaim())
+	if len(podAdditions.Volumes) > 0 {
 		// Kubernetes creates directories in a PVC to support subpaths such that only the leaf directory has g+rwx permissions.
 		// This means that mounting the subpath e.g. <workspace-id>/plugins will result in the <workspace-id> directory being
 		// created with 755 permissions, requiring the root UID to remove it.
@@ -345,19 +342,6 @@ func mergePodAdditions(toMerge []v1alpha1.PodAdditions) (*v1alpha1.PodAdditions,
 	return podAdditions, nil
 }
 
-func getPersistentVolumeClaim() corev1.Volume {
-	var workspaceClaim = corev1.PersistentVolumeClaimVolumeSource{
-		ClaimName: config.ControllerCfg.GetWorkspacePVCName(),
-	}
-	pvcVolume := corev1.Volume{
-		Name: config.ControllerCfg.GetWorkspacePVCName(),
-		VolumeSource: corev1.VolumeSource{
-			PersistentVolumeClaim: &workspaceClaim,
-		},
-	}
-	return pvcVolume
-}
-
 func getWorkspaceSubpathVolumeMount(workspaceId string) corev1.VolumeMount {
 	volumeName := config.ControllerCfg.GetWorkspacePVCName()
 

controllers/workspace/provision/pvc.go

@@ -14,7 +14,6 @@ package provision
 
 import (
 	devworkspace "github.com/devfile/api/pkg/apis/workspaces/v1alpha2"
-	"github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
 	"github.com/devfile/devworkspace-operator/pkg/config"
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
@@ -23,11 +22,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-func SyncPVC(workspace *devworkspace.DevWorkspace, components []v1alpha1.ComponentDescription, client client.Client, reqLogger logr.Logger) ProvisioningStatus {
-	if !IsPVCRequired(components) {
-		return ProvisioningStatus{Continue: true}
-	}
-
+func SyncPVC(workspace *devworkspace.DevWorkspace, client client.Client, reqLogger logr.Logger) ProvisioningStatus {
 	pvc, err := generatePVC(workspace)
 	if err != nil {
 		return ProvisioningStatus{Err: err}
@@ -61,26 +56,3 @@ func generatePVC(workspace *devworkspace.DevWorkspace) (*corev1.PersistentVolume
 		},
 	}, nil
 }
-
-// IsPVCRequired checks to see if we need a PVC for the given devfile.
-// If there is any Containers with VolumeMounts that have the same name as the workspace PVC name then we need a PVC
-func IsPVCRequired(components []v1alpha1.ComponentDescription) bool {
-	volumeName := config.ControllerCfg.GetWorkspacePVCName()
-	for _, comp := range components {
-		for _, cont := range comp.PodAdditions.Containers {
-			for _, vm := range cont.VolumeMounts {
-				if vm.Name == volumeName {
-					return true
-				}
-			}
-		}
-		for _, cont := range comp.PodAdditions.InitContainers {
-			for _, vm := range cont.VolumeMounts {
-				if vm.Name == volumeName {
-					return true
-				}
-			}
-		}
-	}
-	return false
-}