diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 786e02065..7ad15e7f6 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/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). diff --git a/controllers/workspace/finalize.go b/controllers/workspace/finalize.go index cffc3baf7..201c176f1 100644 --- a/controllers/workspace/finalize.go +++ b/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 := ¤tStatus{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 diff --git a/pkg/provision/storage/perWorkspaceStorage.go b/pkg/provision/storage/perWorkspaceStorage.go index 4dafe8bdd..db6af9a01 100644 --- a/pkg/provision/storage/perWorkspaceStorage.go +++ b/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{