From 08f90822f343fd175b1fe761c99cf5aab133f6ae Mon Sep 17 00:00:00 2001 From: Mykhailo Kuznietsov Date: Wed, 15 May 2024 18:05:52 +0300 Subject: [PATCH] feat: Use labels on PVCs for better reconciling Signed-off-by: Mykhailo Kuznietsov --- controllers/workspace/eventhandlers.go | 53 ++++++++++++++++---- pkg/constants/metadata.go | 3 ++ pkg/provision/storage/perWorkspaceStorage.go | 1 + pkg/provision/storage/shared.go | 4 ++ 4 files changed, 50 insertions(+), 11 deletions(-) diff --git a/controllers/workspace/eventhandlers.go b/controllers/workspace/eventhandlers.go index 14c956527..7b0bd990b 100644 --- a/controllers/workspace/eventhandlers.go +++ b/controllers/workspace/eventhandlers.go @@ -15,6 +15,7 @@ package controllers import ( "context" + "fmt" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" wkspConfig "github.com/devfile/devworkspace-operator/pkg/config" @@ -47,7 +48,14 @@ func dwRelatedPodsHandler(obj client.Object) []reconcile.Request { } func (r *DevWorkspaceReconciler) dwPVCHandler(obj client.Object) []reconcile.Request { + if obj.GetDeletionTimestamp() == nil { + // Do not reconcile unless PVC is being deleted. + return []reconcile.Request{} + } + // Check if PVC is owned by a DevWorkspace (per-workspace storage case) + // TODO: Ensure all new and existing PVC's get the `controller.devfile.io/devworkspace_pvc_type` label. + // See: https://github.com/devfile/devworkspace-operator/issues/1250 for _, ownerref := range obj.GetOwnerReferences() { if ownerref.Kind != "DevWorkspace" { continue @@ -62,26 +70,49 @@ func (r *DevWorkspaceReconciler) dwPVCHandler(obj client.Object) []reconcile.Req } } - // TODO: Label PVCs used for workspace storage so that they can be cleaned up if non-default name is used. - // Otherwise, check if common PVC is deleted to make sure all DevWorkspaces see it happen - if obj.GetName() != wkspConfig.GetGlobalConfig().Workspace.PVCName || obj.GetDeletionTimestamp() == nil { - // We're looking for a deleted common PVC - return []reconcile.Request{} + pvcLabel := obj.GetLabels()[constants.DevWorkspacePVCTypeLabel] + + // TODO: This check is for legacy reasons as existing PVCs might not have the `controller.devfile.io/devworkspace_pvc_type` label. + // Remove once https://github.com/devfile/devworkspace-operator/issues/1250 is resolved + if pvcLabel == "" { + if obj.GetName() != wkspConfig.GetGlobalConfig().Workspace.PVCName { + // No need to reconcile if PVC doesn't have a PVC type label + // and it doesn't have a name of PVC from global config. + return []reconcile.Request{} + } } + dwList := &dw.DevWorkspaceList{} if err := r.Client.List(context.Background(), dwList, &client.ListOptions{Namespace: obj.GetNamespace()}); 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 == constants.PerUserStorageClassType || storageType == "" { - reconciles = append(reconciles, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: workspace.GetName(), - Namespace: workspace.GetNamespace(), - }, - }) + + // Determine workspaces to reconcile that use the current common PVC. + // Workspaces can either use the common PVC where the PVC name + // is coming from the global config, or from an external config the workspace might use + workspacePVCName := wkspConfig.GetGlobalConfig().Workspace.PVCName + + if workspace.Spec.Template.Attributes.Exists(constants.ExternalDevWorkspaceConfiguration) { + externalConfig, err := wkspConfig.ResolveConfigForWorkspace(&workspace, r.Client) + if err != nil { + r.Log.Info(fmt.Sprintf("Couldn't resolve PVC name for workspace '%s' in namespace '%s', using PVC name '%s' from global config instead: %s.", workspace.Name, workspace.Namespace, workspacePVCName, err.Error())) + } else { + workspacePVCName = externalConfig.Workspace.PVCName + } + } + if obj.GetName() == workspacePVCName { + reconciles = append(reconciles, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: workspace.GetName(), + Namespace: workspace.GetNamespace(), + }, + }) + } } } return reconciles diff --git a/pkg/constants/metadata.go b/pkg/constants/metadata.go index 7efeb427c..5fe9a0ba1 100644 --- a/pkg/constants/metadata.go +++ b/pkg/constants/metadata.go @@ -20,6 +20,9 @@ const ( // DevWorkspaceIDLabel is the label key to store workspace identifier DevWorkspaceIDLabel = "controller.devfile.io/devworkspace_id" + // DevWorkspacePVCTypeLabel is the label key to identify PVCs used by DevWorkspaces and indicate their storage strategy. + DevWorkspacePVCTypeLabel = "controller.devfile.io/devworkspace_pvc_type" + // WorkspaceIdOverrideAnnotation is an annotation that can be applied to DevWorkspaces // to override the default DevWorkspace ID assigned by the Operator. Is only respected // when a DevWorkspace is created. Once a DevWorkspace has an ID set, it cannot be changed. diff --git a/pkg/provision/storage/perWorkspaceStorage.go b/pkg/provision/storage/perWorkspaceStorage.go index 5fcd276f4..ef781c778 100644 --- a/pkg/provision/storage/perWorkspaceStorage.go +++ b/pkg/provision/storage/perWorkspaceStorage.go @@ -227,6 +227,7 @@ func syncPerWorkspacePVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sy pvc.Labels = map[string]string{} } pvc.Labels[constants.DevWorkspaceIDLabel] = workspace.Status.DevWorkspaceId + pvc.Labels[constants.DevWorkspacePVCTypeLabel] = constants.PerWorkspaceStorageClassType if err := controllerutil.SetControllerReference(workspace.DevWorkspace, pvc, clusterAPI.Scheme); err != nil { return nil, err diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go index ca7a9c9aa..8ef4617c3 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -103,6 +103,10 @@ func syncCommonPVC(namespace string, config *v1alpha1.OperatorConfiguration, clu if err != nil { return nil, err } + if pvc.Labels == nil { + pvc.Labels = map[string]string{} + } + pvc.Labels[constants.DevWorkspacePVCTypeLabel] = constants.PerUserStorageClassType currObject, err := sync.SyncObjectWithCluster(pvc, clusterAPI) switch t := err.(type) { case nil: