add scheduler cluster deletion and minor fixes

Author: Diaphteiros

api/clusters/v1alpha1/accessrequest_types.go

@@ -38,6 +38,8 @@ type AccessRequestStatus struct {
 	CommonStatus `json:",inline"`
 
 	// Phase is the current phase of the request.
+	// +kubebuilder:default=Pending
+	// +kubebuilder:validation:Enum=Pending;Granted;Denied
 	Phase RequestPhase `json:"phase"`
 
 	// SecretRef holds the reference to the secret that contains the actual credentials.

api/clusters/v1alpha1/cluster_types.go

@@ -6,6 +6,7 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 // ClusterSpec defines the desired state of Cluster
@@ -131,12 +132,19 @@ func (cs *ClusterStatus) SetProviderStatus(from any) error {
 
 // GetTenancyCount returns the number of ClusterRequests currently pointing to this cluster.
 // This is determined by counting the finalizers that have the corresponding prefix.
+// Note that only unique finalizers are counted, so if there are multiple identical request finalizers
+// (which should not happen), this method's return value might not match the actual number of finalizers with the prefix.
 func (c *Cluster) GetTenancyCount() int {
-	count := 0
+	return c.GetRequestUIDs().Len()
+}
+
+// GetRequestUIDs returns the UIDs of all ClusterRequests that have marked this cluster with a corresponding finalizer.
+func (c *Cluster) GetRequestUIDs() sets.Set[string] {
+	res := sets.New[string]()
 	for _, fin := range c.Finalizers {
 		if strings.HasPrefix(fin, RequestFinalizerOnClusterPrefix) {
-			count++
+			res.Insert(strings.TrimPrefix(fin, RequestFinalizerOnClusterPrefix))
 		}
 	}
-	return count
+	return res
 }

api/clusters/v1alpha1/clusterrequest_types.go

@@ -16,6 +16,8 @@ type ClusterRequestStatus struct {
 	CommonStatus `json:",inline"`
 
 	// Phase is the current phase of the request.
+	// +kubebuilder:default=Pending
+	// +kubebuilder:validation:Enum=Pending;Granted;Denied
 	Phase RequestPhase `json:"phase"`
 
 	// Cluster is the reference to the Cluster that was returned as a result of a granted request.

api/crds/manifests/clusters.openmcp.cloud_accessrequests.yaml

@@ -209,7 +209,12 @@ spec:
                 format: int64
                 type: integer
               phase:
+                default: Pending
                 description: Phase is the current phase of the request.
+                enum:
+                - Pending
+                - Granted
+                - Denied
                 type: string
               reason:
                 description: Reason is expected to contain a CamelCased string that

api/crds/manifests/clusters.openmcp.cloud_clusterrequests.yaml

@@ -130,7 +130,12 @@ spec:
                 format: int64
                 type: integer
               phase:
+                default: Pending
                 description: Phase is the current phase of the request.
+                enum:
+                - Pending
+                - Granted
+                - Denied
                 type: string
               reason:
                 description: Reason is expected to contain a CamelCased string that

docs/controller/scheduler.md

@@ -33,11 +33,17 @@ scheduler:
           tenancy: Exclusive
     platform:
       template:
+        metadata:
+          labels:
+            clusters.openmcp.cloud/delete-without-requests: "false"
         spec:
           profile: gcp-large
           tenancy: Shared
     onboarding:
       template:
+        metadata:
+          labels:
+            clusters.openmcp.cloud/delete-without-requests: "false"
         spec:
           profile: gcp-workerless
           tenancy: Shared
@@ -108,3 +114,9 @@ If the cluster template from the configuration for the requested purpose has `me
 For clusters with `Exclusive` tenancy, or for `Shared` ones with a limited tenancy count, the scheduler uses `metadata.generateName` from the cluster template or defaults it to `<purpose>-`, if not set.
 
 For clusters with unlimited tenancy count, `metadata.generateName` takes precedence, if specified in the template, with `metadata.name` being evaluated second. If neither is specified, `<purpose>` is used as `metadata.name` (as there should be only one instance of this cluster in this namespace due to the unlimited tenancy count, there is no need to add a randomized suffix to the name).
+
+## Deletion of Clusters
+
+By default, the scheduler marks every `Cluster` that it has created itself with a `clusters.openmcp.cloud/delete-without-requests: "true"` label. When a `ClusterRequest` is deleted, the scheduler removes the request's finalizers from all clusters and if it was the last request finalizer on that cluster and the cluster has the aforementioned label, the scheduler will delete the cluster.
+
+To prevent the scheduler from deleting a cluster that was created by it after the last request finalizer has been removed from the `Cluster` resource, add the label with any value except `"true"` to the cluster's template in the scheduler configuration.

internal/config/config_scheduler.go

@@ -70,8 +70,8 @@ type SchedulerSelectors struct {
 	Requests *metav1.LabelSelector `json:"requests,omitempty"`
 }
 type CompletedSchedulerSelectors struct {
-	Clusters        labels.Selector
-	ClusterRequests labels.Selector
+	Clusters labels.Selector
+	Requests labels.Selector
 }
 
 func (c *SchedulerConfig) Default(_ *field.Path) error {
@@ -175,7 +175,7 @@ func (c *SchedulerConfig) Complete(fldPath *field.Path) error {
 		}
 		if c.Selectors.Requests != nil {
 			var err error
-			c.CompletedSelectors.ClusterRequests, err = metav1.LabelSelectorAsSelector(c.Selectors.Requests)
+			c.CompletedSelectors.Requests, err = metav1.LabelSelectorAsSelector(c.Selectors.Requests)
 			if err != nil {
 				return field.Invalid(fldPath.Child("selectors").Child("requests"), c.Selectors.Requests, err.Error())
 			}
@@ -184,8 +184,8 @@ func (c *SchedulerConfig) Complete(fldPath *field.Path) error {
 	if c.CompletedSelectors.Clusters == nil {
 		c.CompletedSelectors.Clusters = labels.Everything()
 	}
-	if c.CompletedSelectors.ClusterRequests == nil {
-		c.CompletedSelectors.ClusterRequests = labels.Everything()
+	if c.CompletedSelectors.Requests == nil {
+		c.CompletedSelectors.Requests = labels.Everything()
 	}
 
 	for purpose, definition := range c.PurposeMappings {

internal/controllers/scheduler/controller.go

@@ -81,7 +81,7 @@ func (r *ClusterScheduler) reconcile(ctx context.Context, log logging.Logger, re
 			log.Info("Resource not found")
 			return ReconcileResult{}
 		}
-		return ReconcileResult{ReconcileError: errutils.WithReason(fmt.Errorf("unable to get resource '%s' from cluster: %w", req.NamespacedName.String(), err), cconst.ReasonOnboardingClusterInteractionProblem)}
+		return ReconcileResult{ReconcileError: errutils.WithReason(fmt.Errorf("unable to get resource '%s' from cluster: %w", req.NamespacedName.String(), err), cconst.ReasonPlatformClusterInteractionProblem)}
 	}
 
 	// handle operation annotation
@@ -95,7 +95,7 @@ func (r *ClusterScheduler) reconcile(ctx context.Context, log logging.Logger, re
 			case clustersv1alpha1.OperationAnnotationValueReconcile:
 				log.Debug("Removing reconcile operation annotation from resource")
 				if err := ctrlutils.EnsureAnnotation(ctx, r.PlatformCluster.Client(), cr, clustersv1alpha1.OperationAnnotation, "", true, ctrlutils.DELETE); err != nil {
-					return ReconcileResult{ReconcileError: errutils.WithReason(fmt.Errorf("error removing operation annotation: %w", err), cconst.ReasonOnboardingClusterInteractionProblem)}
+					return ReconcileResult{ReconcileError: errutils.WithReason(fmt.Errorf("error removing operation annotation: %w", err), cconst.ReasonPlatformClusterInteractionProblem)}
 				}
 			}
 		}
@@ -116,7 +116,7 @@ func (r *ClusterScheduler) reconcile(ctx context.Context, log logging.Logger, re
 		if controllerutil.AddFinalizer(cr, clustersv1alpha1.ClusterRequestFinalizer) {
 			log.Info("Adding finalizer")
 			if err := r.PlatformCluster.Client().Patch(ctx, cr, client.MergeFrom(rr.OldObject)); err != nil {
-				rr.ReconcileError = errutils.WithReason(fmt.Errorf("error patching finalizer on resource '%s': %w", req.NamespacedName.String(), err), cconst.ReasonOnboardingClusterInteractionProblem)
+				rr.ReconcileError = errutils.WithReason(fmt.Errorf("error patching finalizer on resource '%s': %w", req.NamespacedName.String(), err), cconst.ReasonPlatformClusterInteractionProblem)
 				return rr
 			}
 		}
@@ -147,7 +147,7 @@ func (r *ClusterScheduler) reconcile(ctx context.Context, log logging.Logger, re
 		}
 		clusterList := &clustersv1alpha1.ClusterList{}
 		if err := r.PlatformCluster.Client().List(ctx, clusterList, client.InNamespace(namespace), client.MatchingLabelsSelector{Selector: cDef.CompletedSelector}); err != nil {
-			rr.ReconcileError = errutils.WithReason(fmt.Errorf("error listing Clusters: %w", err), cconst.ReasonOnboardingClusterInteractionProblem)
+			rr.ReconcileError = errutils.WithReason(fmt.Errorf("error listing Clusters: %w", err), cconst.ReasonPlatformClusterInteractionProblem)
 			return rr
 		}
 		clusters := make([]*clustersv1alpha1.Cluster, len(clusterList.Items))
@@ -233,7 +233,7 @@ func (r *ClusterScheduler) reconcile(ctx context.Context, log logging.Logger, re
 			if controllerutil.AddFinalizer(cluster, fin) {
 				log.Debug("Adding finalizer to cluster", "clusterName", cluster.Name, "clusterNamespace", cluster.Namespace, "finalizer", fin)
 				if err := r.PlatformCluster.Client().Patch(ctx, cluster, client.MergeFrom(oldCluster)); err != nil {
-					rr.ReconcileError = errutils.WithReason(fmt.Errorf("error patching finalizer '%s' on cluster '%s/%s': %w", fin, cluster.Namespace, cluster.Name, err), cconst.ReasonOnboardingClusterInteractionProblem)
+					rr.ReconcileError = errutils.WithReason(fmt.Errorf("error patching finalizer '%s' on cluster '%s/%s': %w", fin, cluster.Namespace, cluster.Name, err), cconst.ReasonPlatformClusterInteractionProblem)
 					return rr
 				}
 			}
@@ -303,7 +303,7 @@ func (r *ClusterScheduler) reconcile(ctx context.Context, log logging.Logger, re
 					rr.ReconcileError = errutils.WithReason(fmt.Errorf("Cluster '%s/%s' already exists, this is not supposed to happen", cluster.Namespace, cluster.Name), cconst.ReasonInternalError)
 					return rr
 				}
-				rr.ReconcileError = errutils.WithReason(fmt.Errorf("error creating cluster '%s/%s': %w", cluster.Namespace, cluster.Name, err), cconst.ReasonOnboardingClusterInteractionProblem)
+				rr.ReconcileError = errutils.WithReason(fmt.Errorf("error creating cluster '%s/%s': %w", cluster.Namespace, cluster.Name, err), cconst.ReasonPlatformClusterInteractionProblem)
 				return rr
 			}
 		}
@@ -322,7 +322,7 @@ func (r *ClusterScheduler) reconcile(ctx context.Context, log logging.Logger, re
 		fin := cr.FinalizerForCluster()
 		clusterList := &clustersv1alpha1.ClusterList{}
 		if err := r.PlatformCluster.Client().List(ctx, clusterList, client.MatchingLabelsSelector{Selector: r.Config.CompletedSelectors.Clusters}); err != nil {
-			rr.ReconcileError = errutils.WithReason(fmt.Errorf("error listing Clusters: %w", err), cconst.ReasonOnboardingClusterInteractionProblem)
+			rr.ReconcileError = errutils.WithReason(fmt.Errorf("error listing Clusters: %w", err), cconst.ReasonPlatformClusterInteractionProblem)
 			return rr
 		}
 		clusters := make([]*clustersv1alpha1.Cluster, len(clusterList.Items))
@@ -344,7 +344,17 @@ func (r *ClusterScheduler) reconcile(ctx context.Context, log logging.Logger, re
 			oldCluster := c.DeepCopy()
 			if controllerutil.RemoveFinalizer(c, fin) {
 				if err := r.PlatformCluster.Client().Patch(ctx, c, client.MergeFrom(oldCluster)); err != nil {
-					errs.Append(errutils.WithReason(fmt.Errorf("error patching finalizer '%s' on cluster '%s/%s': %w", fin, c.Namespace, c.Name, err), cconst.ReasonOnboardingClusterInteractionProblem))
+					errs.Append(errutils.WithReason(fmt.Errorf("error patching finalizer '%s' on cluster '%s/%s': %w", fin, c.Namespace, c.Name, err), cconst.ReasonPlatformClusterInteractionProblem))
+				}
+			}
+			if c.GetTenancyCount() == 0 && ctrlutils.HasLabelWithValue(c, clustersv1alpha1.DeleteWithoutRequestsLabel, "true") {
+				log.Info("Deleting cluster without requests", "clusterName", c.Name, "clusterNamespace", c.Namespace)
+				if err := r.PlatformCluster.Client().Delete(ctx, c); err != nil {
+					if apierrors.IsNotFound(err) {
+						log.Info("Cluster already deleted", "clusterName", c.Name, "clusterNamespace", c.Namespace)
+					} else {
+						errs.Append(errutils.WithReason(fmt.Errorf("error deleting cluster '%s/%s': %w", c.Namespace, c.Name, err), cconst.ReasonPlatformClusterInteractionProblem))
+					}
 				}
 			}
 		}
@@ -357,7 +367,7 @@ func (r *ClusterScheduler) reconcile(ctx context.Context, log logging.Logger, re
 		if controllerutil.RemoveFinalizer(cr, clustersv1alpha1.ClusterRequestFinalizer) {
 			log.Info("Removing finalizer")
 			if err := r.PlatformCluster.Client().Patch(ctx, cr, client.MergeFrom(rr.OldObject)); err != nil {
-				rr.ReconcileError = errutils.WithReason(fmt.Errorf("error patching finalizer on resource '%s': %w", req.NamespacedName.String(), err), cconst.ReasonOnboardingClusterInteractionProblem)
+				rr.ReconcileError = errutils.WithReason(fmt.Errorf("error patching finalizer on resource '%s': %w", req.NamespacedName.String(), err), cconst.ReasonPlatformClusterInteractionProblem)
 				return rr
 			}
 		}
@@ -374,9 +384,10 @@ func (r *ClusterScheduler) SetupWithManager(mgr ctrl.Manager) error {
 		// watch ClusterRequest resources
 		For(&clustersv1alpha1.ClusterRequest{}).
 		WithEventFilter(predicate.And(
-			ctrlutils.LabelSelectorPredicate(r.Config.CompletedSelectors.ClusterRequests),
+			ctrlutils.LabelSelectorPredicate(r.Config.CompletedSelectors.Requests),
 			predicate.Or(
 				predicate.GenerationChangedPredicate{},
+				ctrlutils.DeletionTimestampChangedPredicate{},
 				ctrlutils.GotAnnotationPredicate(clustersv1alpha1.OperationAnnotation, clustersv1alpha1.OperationAnnotationValueReconcile),
 				ctrlutils.LostAnnotationPredicate(clustersv1alpha1.OperationAnnotation, clustersv1alpha1.OperationAnnotationValueIgnore),
 			),

internal/controllers/scheduler/controller_test.go

@@ -8,6 +8,7 @@ import (
 	. "github.com/onsi/gomega"
 	. "github.com/onsi/gomega/gstruct"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/uuid"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -399,4 +400,50 @@ var _ = Describe("Scheduler", func() {
 		Expect(req3.Status.Cluster.Namespace).To(Equal("foo"))
 	})
 
+	FIt("should handle the delete-without-requests label correctly", func() {
+		_, env := defaultTestSetup("testdata", "test-05")
+
+		// should create a new cluster
+		req := &clustersv1alpha1.ClusterRequest{}
+		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("delete", "foo"), req)).To(Succeed())
+		Expect(req.Status.Cluster).To(BeNil())
+
+		env.ShouldReconcile(testutils.RequestFromObject(req))
+		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(req), req)).To(Succeed())
+		Expect(req.Status.Cluster).ToNot(BeNil())
+		cluster := &clustersv1alpha1.Cluster{}
+		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey(req.Status.Cluster.Name, req.Status.Cluster.Namespace), cluster)).To(Succeed())
+		Expect(cluster.Labels).To(HaveKeyWithValue(clustersv1alpha1.DeleteWithoutRequestsLabel, "true"))
+
+		// should delete the cluster
+		Expect(env.Client().Delete(env.Ctx, req)).To(Succeed())
+		env.ShouldReconcile(testutils.RequestFromObject(req))
+		Eventually(func() bool {
+			return apierrors.IsNotFound(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(req), req))
+		}, 3).Should(BeTrue(), "Request should be deleted")
+		Eventually(func() bool {
+			return apierrors.IsNotFound(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(cluster), cluster))
+		}, 3).Should(BeTrue(), "Cluster should be deleted")
+
+		// should create a new cluster
+		req2 := &clustersv1alpha1.ClusterRequest{}
+		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("no-delete", "foo"), req2)).To(Succeed())
+		Expect(req2.Status.Cluster).To(BeNil())
+
+		env.ShouldReconcile(testutils.RequestFromObject(req2))
+		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(req2), req2)).To(Succeed())
+		Expect(req2.Status.Cluster).ToNot(BeNil())
+		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey(req2.Status.Cluster.Name, req2.Status.Cluster.Namespace), cluster)).To(Succeed())
+		Expect(cluster.Labels).To(HaveKeyWithValue(clustersv1alpha1.DeleteWithoutRequestsLabel, "false"))
+
+		// should not delete the cluster
+		Expect(env.Client().Delete(env.Ctx, req2)).To(Succeed())
+		env.ShouldReconcile(testutils.RequestFromObject(req2))
+		Eventually(func() bool {
+			return apierrors.IsNotFound(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(req2), req2))
+		}, 3).Should(BeTrue(), "Request should be deleted")
+		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(cluster), cluster)).To(Succeed(), "Cluster should not be deleted")
+		Expect(cluster.DeletionTimestamp).To(BeZero(), "Cluster should not be marked for deletion")
+	})
+
 })

internal/controllers/scheduler/testdata/test-05/cluster/req-0.yaml

@@ -0,0 +1,8 @@
+apiVersion: clusters.openmcp.cloud/v1alpha1
+kind: ClusterRequest
+metadata:
+  name: delete
+  namespace: foo
+  uid: 4d6aa495-54c0-4df2-bc7b-05b103f02e69
+spec:
+  purpose: delete