add tests, implement efficiency optimization, refactor

Author: Diaphteiros

api/clusters/v1alpha1/cluster_types.go

@@ -4,9 +4,9 @@ import (
 	"encoding/json"
 	"strings"
 
+	"github.com/openmcp-project/controller-utils/pkg/collections"
 	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
@@ -136,34 +136,40 @@ 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 {
-	return c.GetRequestUIDs().Len()
+	return collections.AggregateMap(c.GetRequestUIDs(), func(_ string, count int, current int) int {
+		return count + current
+	}, 0)
 }
 
-// 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]()
+// GetRequestUIDs returns the UIDs of all ClusterRequests that have marked this cluster with a corresponding finalizer,
+// mapped to their respective counts.
+// Note that a regular request is currently expected to have exactly one finalizer, so the counts for each UID should be 1.
+func (c *Cluster) GetRequestUIDs() map[string]int {
+	res := map[string]int{}
 	for _, fin := range c.Finalizers {
 		if strings.HasPrefix(fin, RequestFinalizerOnClusterPrefix) {
-			res.Insert(strings.TrimPrefix(fin, RequestFinalizerOnClusterPrefix))
+			res[strings.TrimPrefix(fin, RequestFinalizerOnClusterPrefix)]++
 		}
 	}
 	return res
 }
 
-// GetPreemptiveTenancyCount works like GetTenancyCount, but for preemptive ClusterRequests.
+// GetPreemptiveTenancyCount returns the number of PreemptiveClusterRequests currently pointing to this cluster.
+// This is determined by counting the finalizers that have the corresponding prefix.
 func (c *Cluster) GetPreemptiveTenancyCount() int {
-	return c.GetPreemptiveRequestUIDs().Len()
+	return collections.AggregateMap(c.GetPreemptiveRequestUIDs(), func(_ string, count int, current int) int {
+		return count + current
+	}, 0)
 }
 
-// GetPreemptiveRequestUIDs returns the UIDs of all preemptive ClusterRequests that have marked this cluster with a corresponding finalizer.
-func (c *Cluster) GetPreemptiveRequestUIDs() sets.Set[string] {
-	res := sets.New[string]()
+// GetPreemptiveRequestUIDs returns the UIDs of all PreemptiveClusterRequests that have marked this cluster with a corresponding finalizer,
+// mapped to their respective counts.
+func (c *Cluster) GetPreemptiveRequestUIDs() map[string]int {
+	res := map[string]int{}
 	for _, fin := range c.Finalizers {
 		if strings.HasPrefix(fin, PreemptiveRequestFinalizerOnClusterPrefix) {
-			res.Insert(strings.TrimPrefix(fin, PreemptiveRequestFinalizerOnClusterPrefix))
+			res[strings.TrimPrefix(fin, PreemptiveRequestFinalizerOnClusterPrefix)]++
 		}
 	}
 	return res

api/clusters/v1alpha1/constants.go

@@ -70,18 +70,20 @@ const (
 	// For AccessRequests, the generic controller that is part of the openMCP Operator sets it.
 	ProviderLabel = GroupName + "/provider"
 
-	// DeleteWithoutRequestsLabel marks that the corresponding cluster can be deleted if the scheduler removes the last request pointing to it.
-	// Its value must be "true" for the label to take effect.
-	DeleteWithoutRequestsLabel = GroupName + "/delete-without-requests"
 	// ProfileLabel is used to make the profile information easily accessible on AccessRequests.
 	ProfileLabel = GroupName + "/profile"
 )
 
 const (
+	// DeleteWithoutRequestsLabel marks that the corresponding cluster can be deleted if the scheduler removes the last request pointing to it.
+	// Its value must be "true" for the label to take effect.
+	DeleteWithoutRequestsLabel = GroupName + "/delete-without-requests"
 	// ClusterRequestFinalizer is the finalizer used on ClusterRequest resources
 	ClusterRequestFinalizer = GroupName + "/request"
 	// RequestFinalizerOnClusterPrefix is the prefix for the finalizers that mark a Cluster as being referenced by a ClusterRequest.
 	RequestFinalizerOnClusterPrefix = "request." + GroupName + "/"
 	// PreemptiveRequestFinalizerOnClusterPrefix is the prefix for the finalizers that mark a Cluster as being referenced by a preemptive ClusterRequest.
 	PreemptiveRequestFinalizerOnClusterPrefix = "preemptive." + GroupName + "/"
+	// SchedulerCreationIDLabel can be used to track which scheduling operation created a specific Cluster. Mainly for internal purposes.
+	SchedulerCreationIDLabel = GroupName + "/creation-id"
 )

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

@@ -23,7 +23,7 @@ spec:
     - jsonPath: .spec.purpose
       name: Purpose
       type: string
-    - jsonPath: .spec.workloadCount
+    - jsonPath: .spec.workload
       name: Workload
       type: string
     - jsonPath: .status.phase
@@ -59,15 +59,15 @@ spec:
                 x-kubernetes-validations:
                 - message: purpose is immutable
                   rule: self == oldSelf
-              workloadCount:
+              workload:
                 description: |-
-                  WorkloadCount specifies for how many workloads this preemptive cluster request should account.
+                  Workload specifies for how many workloads this preemptive cluster request should account.
                   Must be greater than 0.
                 minimum: 1
                 type: integer
             required:
             - purpose
-            - workloadCount
+            - workload
             type: object
           status:
             properties:

internal/controllers/scheduler/controller.go

@@ -215,48 +215,32 @@ func (r *ClusterScheduler) handleDelete(ctx context.Context, req reconcile.Reque
 
 	log.Info("Deleting resource")
 
-	// fetch all clusters and filter for the ones that have a finalizer from this request
-	fin := cr.FinalizerForCluster()
-	clusterList := &clustersv1alpha1.ClusterList{}
-	if err := r.PlatformCluster.Client().List(ctx, clusterList, client.MatchingLabelsSelector{Selector: r.Config.Selectors.Clusters.Completed()}); err != nil {
-		rr.ReconcileError = errutils.WithReason(fmt.Errorf("error listing Clusters: %w", err), cconst.ReasonPlatformClusterInteractionProblem)
+	// fetch cluster definition
+	purpose := cr.Spec.Purpose
+	cDef, ok := r.Config.PurposeMappings[purpose]
+	if !ok {
+		rr.ReconcileError = errutils.WithReason(fmt.Errorf("no cluster definition found for purpose '%s'", purpose), cconst.ReasonConfigurationProblem)
 		return rr
 	}
-	clusters := make([]*clustersv1alpha1.Cluster, len(clusterList.Items))
-	for i := range clusterList.Items {
-		clusters[i] = &clusterList.Items[i]
+
+	// fetch relevant clusters
+	clusters, rerr := fetchRelevantClusters(ctx, r.PlatformCluster.Client(), r.Config.Scope, cDef, cr.Spec.Purpose, cr.Namespace)
+	if rerr != nil {
+		rr.ReconcileError = rerr
+		return rr
 	}
-	clusters = filters.FilterSlice(clusters, func(args ...any) bool {
-		c, ok := args[0].(*clustersv1alpha1.Cluster)
-		if !ok {
-			return false
-		}
-		return slices.Contains(c.Finalizers, fin)
-	})
 
-	// remove finalizer from all clusters
-	errs := errutils.NewReasonableErrorList()
-	for _, c := range clusters {
-		log.Debug("Removing finalizer from cluster", "clusterName", c.Name, "clusterNamespace", c.Namespace, "finalizer", fin)
-		oldCluster := c.DeepCopy()
-		if RemoveFinalizer(c, fin, true) {
-			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.ReasonPlatformClusterInteractionProblem))
-			}
-		}
-		if c.GetTenancyCount() == 0 && c.GetPreemptiveTenancyCount() == 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))
-				}
-			}
-		}
+	// run the scheduling algorithm to remove the finalizers from this request
+	sr, err := Schedule(ctx, clusters, cDef, r.Config.Strategy, NewSchedulingRequest(string(cr.UID), 0, true, cr.Namespace, cr.Spec.Purpose))
+	if err != nil {
+		rr.ReconcileError = errutils.WithReason(fmt.Errorf("error unscheduling request '%s': %w", req.String(), err), cconst.ReasonSchedulingFailed)
+		return rr
 	}
-	rr.ReconcileError = errs.Aggregate()
-	if rr.ReconcileError != nil {
+
+	// apply required changes to the cluster
+	_, err = sr.Apply(ctx, r.PlatformCluster.Client())
+	if err != nil {
+		rr.ReconcileError = errutils.WithReason(fmt.Errorf("error applying scheduling result for request '%s': %w", req.String(), err), cconst.ReasonPlatformClusterInteractionProblem)
 		return rr
 	}
 

internal/controllers/scheduler/controller_preemptive.go

@@ -3,7 +3,6 @@ package scheduler
 import (
 	"context"
 	"fmt"
-	"slices"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -12,7 +11,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/openmcp-project/controller-utils/pkg/clusters"
-	"github.com/openmcp-project/controller-utils/pkg/collections/filters"
 	ctrlutils "github.com/openmcp-project/controller-utils/pkg/controller"
 	errutils "github.com/openmcp-project/controller-utils/pkg/errors"
 	"github.com/openmcp-project/controller-utils/pkg/logging"
@@ -151,48 +149,32 @@ func (r *PreemptiveScheduler) handleDelete(ctx context.Context, req reconcile.Re
 
 	log.Info("Deleting resource")
 
-	// fetch all clusters and filter for the ones that have a finalizer from this request
-	fin := pcr.FinalizerForCluster()
-	clusterList := &clustersv1alpha1.ClusterList{}
-	if err := r.PlatformCluster.Client().List(ctx, clusterList, client.MatchingLabelsSelector{Selector: r.Config.Selectors.Clusters.Completed()}); err != nil {
-		rr.ReconcileError = errutils.WithReason(fmt.Errorf("error listing Clusters: %w", err), cconst.ReasonPlatformClusterInteractionProblem)
+	// fetch cluster definition
+	purpose := pcr.Spec.Purpose
+	cDef, ok := r.Config.PurposeMappings[purpose]
+	if !ok {
+		rr.ReconcileError = errutils.WithReason(fmt.Errorf("no cluster definition found for purpose '%s'", purpose), cconst.ReasonConfigurationProblem)
 		return rr
 	}
-	clusters := make([]*clustersv1alpha1.Cluster, len(clusterList.Items))
-	for i := range clusterList.Items {
-		clusters[i] = &clusterList.Items[i]
+
+	// fetch relevant clusters
+	clusters, rerr := fetchRelevantClusters(ctx, r.PlatformCluster.Client(), r.Config.Scope, cDef, pcr.Spec.Purpose, pcr.Namespace)
+	if rerr != nil {
+		rr.ReconcileError = rerr
+		return rr
 	}
-	clusters = filters.FilterSlice(clusters, func(args ...any) bool {
-		c, ok := args[0].(*clustersv1alpha1.Cluster)
-		if !ok {
-			return false
-		}
-		return slices.Contains(c.Finalizers, fin)
-	})
-
-	// remove finalizer from all clusters
-	errs := errutils.NewReasonableErrorList()
-	for _, c := range clusters {
-		log.Debug("Removing finalizers from cluster", "clusterName", c.Name, "clusterNamespace", c.Namespace, "finalizer", fin)
-		oldCluster := c.DeepCopy()
-		if RemoveFinalizer(c, fin, true) {
-			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.ReasonPlatformClusterInteractionProblem))
-			}
-		}
-		if c.GetTenancyCount() == 0 && c.GetPreemptiveTenancyCount() == 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))
-				}
-			}
-		}
+
+	// run the scheduling algorithm to remove the finalizers from this request
+	sr, err := Schedule(ctx, clusters, cDef, r.Config.Strategy, NewSchedulingRequest(string(pcr.UID), pcr.Spec.Workload, true, pcr.Namespace, pcr.Spec.Purpose))
+	if err != nil {
+		rr.ReconcileError = errutils.WithReason(fmt.Errorf("error unscheduling request '%s': %w", req.String(), err), cconst.ReasonSchedulingFailed)
+		return rr
 	}
-	rr.ReconcileError = errs.Aggregate()
-	if rr.ReconcileError != nil {
+
+	// apply required changes to the cluster
+	_, err = sr.Apply(ctx, r.PlatformCluster.Client())
+	if err != nil {
+		rr.ReconcileError = errutils.WithReason(fmt.Errorf("error applying scheduling result for request '%s': %w", req.String(), err), cconst.ReasonPlatformClusterInteractionProblem)
 		return rr
 	}