Improve handling of errors during PVC cleanup

controllers/workspace/devworkspace_controller.go

@@ -636,28 +636,65 @@ func (r *DevWorkspaceReconciler) getWorkspaceId(ctx context.Context, workspace *
 }
 
 // Mapping the pod to the devworkspace
-func dwRelatedPodsHandler() handler.EventHandler {
-	podToDW := func(obj client.Object) []reconcile.Request {
-		labels := obj.GetLabels()
-		if _, ok := labels[constants.DevWorkspaceNameLabel]; !ok {
-			return nil
-		}
+func dwRelatedPodsHandler(obj client.Object) []reconcile.Request {
+	labels := obj.GetLabels()
+	if _, ok := labels[constants.DevWorkspaceNameLabel]; !ok {
+		return []reconcile.Request{}
+	}
 
-		//If the dewworkspace label does not exist, do no reconcile
-		if _, ok := labels[constants.DevWorkspaceIDLabel]; !ok {
-			return nil
-		}
+	//If the dewworkspace label does not exist, do no reconcile
+	if _, ok := labels[constants.DevWorkspaceIDLabel]; !ok {
+		return []reconcile.Request{}
+	}
+
+	return []reconcile.Request{
+		{
+			NamespacedName: types.NamespacedName{
+				Name:      labels[constants.DevWorkspaceNameLabel],
+				Namespace: obj.GetNamespace(),
+			},
+		},
+	}
+}
 
+func (r *DevWorkspaceReconciler) dwPVCHandler(obj client.Object) []reconcile.Request {
+	// Check if PVC is owned by a DevWorkspace (per-workspace storage case)
+	for _, ownerref := range obj.GetOwnerReferences() {
+		if ownerref.Kind != "DevWorkspace" {
+			continue
+		}
 		return []reconcile.Request{
 			{
 				NamespacedName: types.NamespacedName{
-					Name:      labels[constants.DevWorkspaceNameLabel],
+					Name:      ownerref.Name,
 					Namespace: obj.GetNamespace(),
 				},
 			},
 		}
 	}
-	return handler.EnqueueRequestsFromMapFunc(podToDW)
+
+	// Otherwise, check if common PVC is deleted to make sure all DevWorkspaces see it happen
+	if obj.GetName() != config.Workspace.PVCName || obj.GetDeletionTimestamp() == nil {
+		// We're looking for a deleted common PVC
+		return []reconcile.Request{}
+	}
+	dwList := &dw.DevWorkspaceList{}
+	if err := r.Client.List(context.Background(), dwList); err != nil {
+		return []reconcile.Request{}
+	}
+	var reconciles []reconcile.Request
+	for _, workspace := range dwList.Items {
+		storageType := workspace.Spec.Template.Attributes.GetString(constants.DevWorkspaceStorageTypeAttribute, nil)
+		if storageType == constants.CommonStorageClassType || storageType == "" {
+			reconciles = append(reconciles, reconcile.Request{
+				NamespacedName: types.NamespacedName{
+					Name:      workspace.GetName(),
+					Namespace: workspace.GetNamespace(),
+				},
+			})
+		}
+	}
+	return reconciles
 }
 
 func (r *DevWorkspaceReconciler) SetupWithManager(mgr ctrl.Manager) error {
@@ -688,7 +725,8 @@ func (r *DevWorkspaceReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Owns(&corev1.ConfigMap{}).
 		Owns(&corev1.Secret{}).
 		Owns(&corev1.ServiceAccount{}).
-		Watches(&source.Kind{Type: &corev1.Pod{}}, dwRelatedPodsHandler()).
+		Watches(&source.Kind{Type: &corev1.Pod{}}, handler.EnqueueRequestsFromMapFunc(dwRelatedPodsHandler)).
+		Watches(&source.Kind{Type: &corev1.PersistentVolumeClaim{}}, handler.EnqueueRequestsFromMapFunc(r.dwPVCHandler)).
 		Watches(&source.Kind{Type: &controllerv1alpha1.DevWorkspaceOperatorConfig{}}, handler.EnqueueRequestsFromMapFunc(emptyMapper), configWatcher).
 		WithEventFilter(predicates).
 		WithEventFilter(podPredicates).

controllers/workspace/finalize.go

@@ -18,6 +18,7 @@ package controllers
 import (
 	"context"
 
+	"github.com/devfile/devworkspace-operator/pkg/conditions"
 	"github.com/devfile/devworkspace-operator/pkg/constants"
 
 	dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
@@ -26,7 +27,6 @@ import (
 	"github.com/go-logr/logr"
 	coputil "github.com/redhat-cop/operator-utils/pkg/util"
 	corev1 "k8s.io/api/core/v1"
-	k8sErrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -46,28 +46,35 @@ func (r *DevWorkspaceReconciler) workspaceNeedsFinalize(workspace *dw.DevWorkspa
 	return false
 }
 
-func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace) (reconcile.Result, error) {
-	if workspace.Status.Phase != dw.DevWorkspaceStatusError {
-		workspace.Status.Message = "Cleaning up resources for deletion"
-		workspace.Status.Phase = devworkspacePhaseTerminating
-		err := r.Client.Status().Update(ctx, workspace)
-		if err != nil && !k8sErrors.IsConflict(err) {
-			return reconcile.Result{}, err
+func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace) (finalizeResult reconcile.Result, finalizeErr error) {
+	// Tracked state for the finalize process; we update the workspace status in a deferred function (and pass the
+	// named return value for finalize()) to update the workspace's status with whatever is in finalizeStatus
+	// when this function returns.
+	finalizeStatus := &currentStatus{phase: devworkspacePhaseTerminating}
+	finalizeStatus.setConditionTrue(conditions.Started, "Cleaning up resources for deletion")
+	defer func() (reconcile.Result, error) {
+		if len(workspace.Finalizers) == 0 {
+			// If there are no finalizers on the workspace, the workspace may be garbage collected before we get to update
+			// its status. This avoids potentially logging a confusing error due to trying to set the status on a deleted
+			// workspace. This check has to be in the deferred function since updateWorkspaceStatus will be called after the
+			// client.Update() call that removes the last finalizer.
+			return finalizeResult, finalizeErr
 		}
+		return r.updateWorkspaceStatus(workspace, log, finalizeStatus, finalizeResult, finalizeErr)
+	}()
 
-		for _, finalizer := range workspace.Finalizers {
-			switch finalizer {
-			case constants.StorageCleanupFinalizer:
-				return r.finalizeStorage(ctx, log, workspace)
-			case constants.ServiceAccountCleanupFinalizer:
-				return r.finalizeServiceAccount(ctx, log, workspace)
-			}
+	for _, finalizer := range workspace.Finalizers {
+		switch finalizer {
+		case constants.StorageCleanupFinalizer:
+			return r.finalizeStorage(ctx, log, workspace, finalizeStatus)
+		case constants.ServiceAccountCleanupFinalizer:
+			return r.finalizeServiceAccount(ctx, log, workspace, finalizeStatus)
 		}
 	}
 	return reconcile.Result{}, nil
 }
 
-func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace) (reconcile.Result, error) {
+func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace, finalizeStatus *currentStatus) (reconcile.Result, error) {
 	// Need to make sure Deployment is cleaned up before starting job to avoid mounting issues for RWO PVCs
 	wait, err := wsprovision.DeleteWorkspaceDeployment(ctx, workspace, r.Client)
 	if err != nil {
@@ -90,9 +97,9 @@ func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.L
 	storageProvisioner, err := storage.GetProvisioner(workspace)
 	if err != nil {
 		log.Error(err, "Failed to clean up DevWorkspace storage")
-		failedStatus := currentStatus{phase: dw.DevWorkspaceStatusError}
-		failedStatus.setConditionTrue(dw.DevWorkspaceError, err.Error())
-		return r.updateWorkspaceStatus(workspace, r.Log, &failedStatus, reconcile.Result{}, nil)
+		finalizeStatus.phase = dw.DevWorkspaceStatusError
+		finalizeStatus.setConditionTrue(dw.DevWorkspaceError, err.Error())
+		return reconcile.Result{}, nil
 	}
 	err = storageProvisioner.CleanupWorkspaceStorage(workspace, sync.ClusterAPI{
 		Ctx:    ctx,
@@ -106,10 +113,13 @@ func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.L
 			log.Info(storageErr.Message)
 			return reconcile.Result{RequeueAfter: storageErr.RequeueAfter}, nil
 		case *storage.ProvisioningError:
-			log.Error(storageErr, "Failed to clean up DevWorkspace storage")
-			failedStatus := currentStatus{phase: dw.DevWorkspaceStatusError}
-			failedStatus.setConditionTrue(dw.DevWorkspaceError, err.Error())
-			return r.updateWorkspaceStatus(workspace, r.Log, &failedStatus, reconcile.Result{}, nil)
+			if workspace.Status.Phase != dw.DevWorkspaceStatusError {
+				// Avoid repeatedly logging error unless it's relevant
+				log.Error(storageErr, "Failed to clean up DevWorkspace storage")
+			}
+			finalizeStatus.phase = dw.DevWorkspaceStatusError
+			finalizeStatus.setConditionTrue(dw.DevWorkspaceError, err.Error())
+			return reconcile.Result{}, nil
 		default:
 			return reconcile.Result{}, storageErr
 		}
@@ -119,13 +129,13 @@ func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.L
 	return reconcile.Result{}, r.Update(ctx, workspace)
 }
 
-func (r *DevWorkspaceReconciler) finalizeServiceAccount(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace) (reconcile.Result, error) {
+func (r *DevWorkspaceReconciler) finalizeServiceAccount(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace, finalizeStatus *currentStatus) (reconcile.Result, error) {
 	retry, err := wsprovision.FinalizeServiceAccount(workspace, ctx, r.NonCachingClient)
 	if err != nil {
 		log.Error(err, "Failed to finalize workspace ServiceAccount")
-		failedStatus := currentStatus{phase: dw.DevWorkspaceStatusError}
-		failedStatus.setConditionTrue(dw.DevWorkspaceError, err.Error())
-		return r.updateWorkspaceStatus(workspace, r.Log, &failedStatus, reconcile.Result{}, nil)
+		finalizeStatus.phase = dw.DevWorkspaceStatusError
+		finalizeStatus.setConditionTrue(dw.DevWorkspaceError, err.Error())
+		return reconcile.Result{}, nil
 	}
 	if retry {
 		return reconcile.Result{Requeue: true}, nil

pkg/provision/storage/perWorkspaceStorage.go

@@ -63,6 +63,13 @@ func (p *PerWorkspaceStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1
 	}
 	pvcName := perWorkspacePVC.Name
 
+	// If PVC is being deleted, we need to fail workspace startup as a running pod will block deletion.
+	if perWorkspacePVC.DeletionTimestamp != nil {
+		return &ProvisioningError{
+			Message: "DevWorkspace PVC is being deleted",
+		}
+	}
+
 	// Rewrite container volume mounts
 	if err := p.rewriteContainerVolumeMounts(workspace.Status.DevWorkspaceId, pvcName, podAdditions, &workspace.Spec.Template); err != nil {
 		return &ProvisioningError{