Member info in SPC status - capacity manager part (#1119)

Simplify the capacity manager to take into account the more elaborate readiness status of the SPC. --------- Co-authored-by: Francisc Munteanu <[email protected]> Co-authored-by: Matous Jobanek <[email protected]>
codeready-toolchain · Jan 20, 2025 · c07ce2e · c07ce2e
1 parent 561d8a7
commit c07ce2e
Show file tree

Hide file tree

Showing 9 changed files with 337 additions and 632 deletions.
diff --git a/controllers/spacecompletion/space_completion_controller.go b/controllers/spacecompletion/space_completion_controller.go
@@ -94,8 +94,7 @@ func (r *Reconciler) ensureFields(ctx context.Context, space *toolchainv1alpha1.
 
 	if space.Spec.TargetCluster == "" {
 		targetCluster, err := r.ClusterManager.GetOptimalTargetCluster(ctx, capacity.OptimalTargetClusterFilter{
-			ToolchainStatusNamespace: space.Namespace,
-			ClusterRoles:             space.Spec.TargetClusterRoles,
+			ClusterRoles: space.Spec.TargetClusterRoles,
 		})
 		if err != nil {
 			return false, errs.Wrapf(err, "unable to get the optimal target cluster")

diff --git a/controllers/usersignup/approval.go b/controllers/usersignup/approval.go
@@ -102,9 +102,8 @@ func getClusterIfApproved(ctx context.Context, cl runtimeclient.Client, userSign
 	clusterRoles := []string{cluster.RoleLabel(cluster.Tenant)}
 
 	clusterName, err := clusterManager.GetOptimalTargetCluster(ctx, capacity.OptimalTargetClusterFilter{
-		PreferredCluster:         preferredCluster,
-		ToolchainStatusNamespace: userSignup.Namespace,
-		ClusterRoles:             clusterRoles,
+		PreferredCluster: preferredCluster,
+		ClusterRoles:     clusterRoles,
 	})
 	if err != nil {
 		return false, unknown, errors.Wrapf(err, "unable to get the optimal target cluster")

diff --git a/controllers/usersignup/approval_test.go b/controllers/usersignup/approval_test.go
@@ -7,14 +7,12 @@ import (
 
 	toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
 	"github.com/codeready-toolchain/host-operator/pkg/capacity"
-	"github.com/codeready-toolchain/host-operator/pkg/metrics"
 	. "github.com/codeready-toolchain/host-operator/test"
 	hspc "github.com/codeready-toolchain/host-operator/test/spaceprovisionerconfig"
 	commonconfig "github.com/codeready-toolchain/toolchain-common/pkg/configuration"
 	"github.com/codeready-toolchain/toolchain-common/pkg/test"
 	commontest "github.com/codeready-toolchain/toolchain-common/pkg/test"
 	testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config"
-	spc "github.com/codeready-toolchain/toolchain-common/pkg/test/spaceprovisionerconfig"
 	commonsignup "github.com/codeready-toolchain/toolchain-common/pkg/test/usersignup"
 
 	"github.com/stretchr/testify/assert"
@@ -26,17 +24,7 @@ func TestGetClusterIfApproved(t *testing.T) {
 	// given
 	ctx := context.TODO()
 	signup := commonsignup.NewUserSignup()
-	toolchainStatus := NewToolchainStatus(
-		WithMetric(toolchainv1alpha1.MasterUserRecordsPerDomainMetricKey, toolchainv1alpha1.Metric{
-			string(metrics.Internal): 100,
-			string(metrics.External): 800,
-		}),
-		WithMetric(toolchainv1alpha1.UserSignupsPerActivationAndDomainMetricKey, toolchainv1alpha1.Metric{
-			"1,internal": 200,
-			"1,external": 700,
-		}),
-		WithMember("member1", WithSpaceCount(700), WithNodeRoleUsage("worker", 68), WithNodeRoleUsage("master", 65)),
-		WithMember("member2", WithSpaceCount(200), WithNodeRoleUsage("worker", 55), WithNodeRoleUsage("master", 60)))
+	toolchainStatus := NewToolchainStatus()
 
 	t.Run("with two clusters available, the second one is defined as the last-used one", func(t *testing.T) {
 		// given
@@ -48,8 +36,8 @@ func TestGetClusterIfApproved(t *testing.T) {
 			testconfig.AutomaticApproval().
 				Enabled(true),
 		)
-		spc1 := hspc.NewEnabledValidTenantSPC("member1", spc.MaxNumberOfSpaces(1000), spc.MaxMemoryUtilizationPercent(70))
-		spc2 := hspc.NewEnabledValidTenantSPC("member2", spc.MaxNumberOfSpaces(1000), spc.MaxMemoryUtilizationPercent(75))
+		spc1 := hspc.NewEnabledValidTenantSPC("member1")
+		spc2 := hspc.NewEnabledValidTenantSPC("member2")
 		fakeClient := commontest.NewFakeClient(t, toolchainStatus, toolchainConfig, spc1, spc2)
 		InitializeCounters(t, toolchainStatus)
 
@@ -244,8 +232,8 @@ func TestGetClusterIfApproved(t *testing.T) {
 
 	t.Run("automatic approval not enabled, user manually approved but no cluster has capacity", func(t *testing.T) {
 		// given
-		spc1 := hspc.NewEnabledValidTenantSPC("member1", spc.MaxMemoryUtilizationPercent(50))
-		spc2 := hspc.NewEnabledValidTenantSPC("member2", spc.MaxMemoryUtilizationPercent(50))
+		spc1 := hspc.NewEnabledTenantSPC("member1")
+		spc2 := hspc.NewEnabledTenantSPC("member2")
 		fakeClient := commontest.NewFakeClient(t, toolchainStatus, spc1, spc2)
 		InitializeCounters(t, toolchainStatus)
 		signup := commonsignup.NewUserSignup(commonsignup.ApprovedManually())
@@ -261,8 +249,8 @@ func TestGetClusterIfApproved(t *testing.T) {
 
 	t.Run("automatic approval not enabled, user manually approved and second cluster has capacity", func(t *testing.T) {
 		// given
-		spc1 := hspc.NewEnabledValidTenantSPC("member1", spc.MaxNumberOfSpaces(2000), spc.MaxMemoryUtilizationPercent(62))
-		spc2 := hspc.NewEnabledValidTenantSPC("member2", spc.MaxMemoryUtilizationPercent(62))
+		spc1 := hspc.NewEnabledTenantSPC("member1")
+		spc2 := hspc.NewEnabledValidTenantSPC("member2")
 		fakeClient := commontest.NewFakeClient(t, toolchainStatus, spc1, spc2)
 		InitializeCounters(t, toolchainStatus)
 		signup := commonsignup.NewUserSignup(commonsignup.ApprovedManually())
@@ -278,7 +266,7 @@ func TestGetClusterIfApproved(t *testing.T) {
 
 	t.Run("automatic approval not enabled, user manually approved, no cluster has capacity but targetCluster is specified", func(t *testing.T) {
 		// given
-		spc1 := hspc.NewEnabledValidTenantSPC("member1", spc.MaxNumberOfSpaces(1000))
+		spc1 := hspc.NewEnabledValidTenantSPC("member1")
 		spc2 := hspc.NewEnabledValidTenantSPC("member2")
 		fakeClient := commontest.NewFakeClient(t, toolchainStatus, spc1, spc2)
 		InitializeCounters(t, toolchainStatus)
@@ -311,26 +299,6 @@ func TestGetClusterIfApproved(t *testing.T) {
 			assert.Equal(t, unknown, clusterName)
 		})
 
-		t.Run("unable to read ToolchainStatus", func(t *testing.T) {
-			// given
-			fakeClient := commontest.NewFakeClient(t, toolchainStatus, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)))
-			fakeClient.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error {
-				if _, ok := obj.(*toolchainv1alpha1.ToolchainStatus); ok {
-					return fmt.Errorf("some error")
-				}
-				return fakeClient.Client.Get(ctx, key, obj, opts...)
-			}
-			InitializeCounters(t, toolchainStatus)
-
-			// when
-			approved, clusterName, err := getClusterIfApproved(ctx, fakeClient, signup, capacity.NewClusterManager(test.HostOperatorNs, fakeClient))
-
-			// then
-			require.EqualError(t, err, "unable to get the optimal target cluster: unable to read ToolchainStatus resource: some error")
-			assert.False(t, approved)
-			assert.Equal(t, unknown, clusterName)
-		})
-
 		t.Run("unable to read SpaceProvisionerConfig", func(t *testing.T) {
 			// given
 			fakeClient := commontest.NewFakeClient(t, toolchainStatus, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)))

diff --git a/controllers/usersignup/usersignup_controller_test.go b/controllers/usersignup/usersignup_controller_test.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"os"
 	"strings"
 	"testing"
 	"time"
@@ -1001,8 +1000,8 @@ func TestUserSignupFailedNoClusterWithCapacityAvailable(t *testing.T) {
 	// given
 	userSignup := commonsignup.NewUserSignup()
 
-	spc1 := hspc.NewEnabledValidTenantSPC("member1", spc.MaxMemoryUtilizationPercent(60))
-	spc2 := hspc.NewEnabledValidTenantSPC("member2", spc.MaxMemoryUtilizationPercent(60))
+	spc1 := hspc.NewEnabledTenantSPC("member1")
+	spc2 := hspc.NewEnabledTenantSPC("member2")
 	config := commonconfig.NewToolchainConfigObjWithReset(t,
 		testconfig.AutomaticApproval().Enabled(true))
 	r, req, _ := prepareReconcile(t, userSignup.Name, spc1, spc2, userSignup, config, baseNSTemplateTier)
@@ -2079,14 +2078,6 @@ func TestUserSignupPropagatedClaimsSynchronizedToMURWhenModified(t *testing.T) {
 	r, req, _ := prepareReconcile(t, userSignup.Name, spc1, userSignup,
 		commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)),
 		baseNSTemplateTier, deactivate30Tier)
-	InitializeCounters(t, NewToolchainStatus(
-		WithMetric(toolchainv1alpha1.UserSignupsPerActivationAndDomainMetricKey, toolchainv1alpha1.Metric{
-			"1,external": 1,
-		}),
-		WithMetric(toolchainv1alpha1.MasterUserRecordsPerDomainMetricKey, toolchainv1alpha1.Metric{
-			string(metrics.External): 1,
-		}),
-	))
 
 	// when - The first reconcile creates the MasterUserRecord
 	res, err := r.Reconcile(context.TODO(), req)
@@ -2961,11 +2952,6 @@ func TestUserSignupDeactivatingNotificationCreated(t *testing.T) {
 
 	r, req, _ := prepareReconcile(t, userSignup.Name, userSignup, mur,
 		commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)), baseNSTemplateTier, deactivate30Tier)
-	InitializeCounters(t, NewToolchainStatus(
-		WithMetric(toolchainv1alpha1.MasterUserRecordsPerDomainMetricKey, toolchainv1alpha1.Metric{
-			string(metrics.External): 1,
-		}),
-	))
 
 	// when
 	_, err := r.Reconcile(context.TODO(), req)
@@ -3500,14 +3486,6 @@ func TestUserSignupDeactivatedButMURDeleteFails(t *testing.T) {
 		mur.Labels = map[string]string{toolchainv1alpha1.MasterUserRecordOwnerLabelKey: userSignup.Name}
 
 		r, req, fakeClient := prepareReconcile(t, userSignup.Name, userSignup, mur, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)), baseNSTemplateTier)
-		InitializeCounters(t, NewToolchainStatus(
-			WithMetric(toolchainv1alpha1.MasterUserRecordsPerDomainMetricKey, toolchainv1alpha1.Metric{
-				string(metrics.External): 1,
-			}),
-			WithMetric(toolchainv1alpha1.UserSignupsPerActivationAndDomainMetricKey, toolchainv1alpha1.Metric{
-				"1,external": 1,
-			}),
-		))
 
 		fakeClient.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error {
 			switch obj.(type) {
@@ -3521,7 +3499,6 @@ func TestUserSignupDeactivatedButMURDeleteFails(t *testing.T) {
 		// when
 		_, err := r.Reconcile(context.TODO(), req)
 		require.NoError(t, err)
-
 	})
 }
 
@@ -3781,8 +3758,6 @@ func TestGenerateUniqueCompliantUsername(t *testing.T) {
 			r, req, _ := prepareReconcile(t, userSignup.Name, spc1, userSignup, baseNSTemplateTier,
 				deactivate30Tier, params.conflictingObject, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)))
 
-			InitializeCounters(t, NewToolchainStatus())
-
 			// when
 			res, err := r.Reconcile(context.TODO(), req)
 
@@ -4078,7 +4053,6 @@ func TestCaptchaAnnotatedWhenUserSignupBanned(t *testing.T) {
 }
 
 func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Object) (*Reconciler, reconcile.Request, *test.FakeClient) {
-	os.Setenv("WATCH_NAMESPACE", test.HostOperatorNs)
 	metrics.Reset()
 
 	s := scheme.Scheme
@@ -4095,10 +4069,11 @@ func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Objec
 			"token": []byte("mycooltoken"),
 		},
 	}
-
 	toolchainStatus := NewToolchainStatus(
 		WithMember("member1", WithNodeRoleUsage("worker", 68), WithNodeRoleUsage("master", 65)))
 
+	InitializeCounters(t, toolchainStatus)
+
 	initObjs = append(initObjs, secret, toolchainStatus)
 
 	fakeClient := test.NewFakeClient(t, initObjs...)
@@ -4150,11 +4125,6 @@ func TestUsernameWithForbiddenPrefix(t *testing.T) {
 			userSignup.Spec.IdentityClaims.PreferredUsername = fmt.Sprintf("%s%s", prefix, name)
 
 			r, req, _ := prepareReconcile(t, userSignup.Name, userSignup, baseNSTemplateTier, deactivate30Tier)
-			InitializeCounters(t, NewToolchainStatus(
-				WithMetric(toolchainv1alpha1.MasterUserRecordsPerDomainMetricKey, toolchainv1alpha1.Metric{
-					string(metrics.External): 1,
-				}),
-			))
 
 			// when
 			_, err := r.Reconcile(context.TODO(), req)
@@ -4195,11 +4165,6 @@ func TestUsernameWithForbiddenSuffixes(t *testing.T) {
 			userSignup.Spec.IdentityClaims.PreferredUsername = fmt.Sprintf("%s%s", name, suffix)
 
 			r, req, _ := prepareReconcile(t, userSignup.Name, userSignup, baseNSTemplateTier, deactivate30Tier)
-			InitializeCounters(t, NewToolchainStatus(
-				WithMetric(toolchainv1alpha1.MasterUserRecordsPerDomainMetricKey, toolchainv1alpha1.Metric{
-					string(metrics.External): 1,
-				}),
-			))
 
 			// when
 			_, err := r.Reconcile(context.TODO(), req)
@@ -4247,11 +4212,6 @@ func TestChangedCompliantUsername(t *testing.T) {
 	}
 	// create the initial resources
 	r, req, _ := prepareReconcile(t, userSignup.Name, userSignup, baseNSTemplateTier, deactivate30Tier)
-	InitializeCounters(t, NewToolchainStatus(
-		WithMetric(toolchainv1alpha1.MasterUserRecordsPerDomainMetricKey, toolchainv1alpha1.Metric{
-			string(metrics.External): 1,
-		}),
-	))
 
 	// 1st reconcile should provision a new MUR
 	res, err := r.Reconcile(context.TODO(), req)
@@ -4330,7 +4290,6 @@ func TestMigrateMur(t *testing.T) {
 	t.Run("mur should be migrated", func(t *testing.T) {
 		// given
 		r, req, _ := prepareReconcile(t, userSignup.Name, userSignup, baseNSTemplateTier, oldMur, deactivate30Tier)
-		InitializeCounters(t, NewToolchainStatus())
 
 		// when
 		_, err := r.Reconcile(context.TODO(), req)
@@ -4543,7 +4502,6 @@ func TestUserSignupLastTargetClusterAnnotation(t *testing.T) {
 		spc1 := hspc.NewEnabledValidTenantSPC("member1")
 		spc2 := hspc.NewEnabledValidTenantSPC("member2")
 		r, req, _ := prepareReconcile(t, userSignup.Name, spc1, spc2, userSignup, baseNSTemplateTier, deactivate30Tier, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)))
-		InitializeCounters(t, NewToolchainStatus())
 
 		// when
 		res, err := r.Reconcile(context.TODO(), req)
@@ -4558,15 +4516,13 @@ func TestUserSignupLastTargetClusterAnnotation(t *testing.T) {
 			HasTargetCluster("member1")
 	})
 
-	t.Run("last target cluster annotation is set but cluster lacks capacity", func(t *testing.T) {
+	t.Run("last target cluster annotation is set but cluster is not ready", func(t *testing.T) {
 		// given
 		userSignup := commonsignup.NewUserSignup()
 		userSignup.Annotations[toolchainv1alpha1.UserSignupLastTargetClusterAnnotationKey] = "member2"
-		spc1 := hspc.NewEnabledValidTenantSPC("member1", spc.MaxMemoryUtilizationPercent(70))
-		// member2 cluster lacks capacity because the prepareReconcile only sets up the resource consumption for member1 so member2 is automatically excluded
-		spc2 := hspc.NewEnabledValidTenantSPC("member2", spc.MaxMemoryUtilizationPercent(75))
+		spc1 := hspc.NewEnabledValidTenantSPC("member1")
+		spc2 := hspc.NewEnabledTenantSPC("member2")
 		r, req, _ := prepareReconcile(t, userSignup.Name, spc1, spc2, userSignup, baseNSTemplateTier, deactivate30Tier, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)))
-		InitializeCounters(t, NewToolchainStatus())
 
 		// when
 		res, err := r.Reconcile(context.TODO(), req)
@@ -4579,22 +4535,19 @@ func TestUserSignupLastTargetClusterAnnotation(t *testing.T) {
 		murtest.AssertThatMasterUserRecord(t, userSignup.Name, r.Client).HasTargetCluster("member1")
 	})
 
-	t.Run("last target cluster annotation is set and cluster has capacity", func(t *testing.T) {
+	t.Run("last target cluster annotation is set and cluster is ready", func(t *testing.T) {
 		// given
 		userSignup := commonsignup.NewUserSignup()
 		userSignup.Annotations[toolchainv1alpha1.UserSignupLastTargetClusterAnnotationKey] = "member2"
 		spc1 := hspc.NewEnabledValidTenantSPC("member1")
 		spc2 := hspc.NewEnabledValidTenantSPC("member2")
 		r, req, _ := prepareReconcile(t, userSignup.Name, spc1, spc2, userSignup, baseNSTemplateTier, deactivate30Tier, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)))
-		InitializeCounters(t, NewToolchainStatus())
 
-		// set acceptable capacity for member2 cluster
-		toolchainStatus := &toolchainv1alpha1.ToolchainStatus{}
-		err := r.Client.Get(context.TODO(), types.NamespacedName{Name: toolchainconfig.ToolchainStatusName, Namespace: req.Namespace}, toolchainStatus)
-		require.NoError(t, err)
-		WithMember("member2", WithNodeRoleUsage("worker", 68), WithNodeRoleUsage("master", 65))(toolchainStatus)
-		err = r.Client.Status().Update(context.TODO(), toolchainStatus)
-		require.NoError(t, err)
+		// make the member2 SPC valid so that we simulate that it now has enough capacity
+		member2Spc := &toolchainv1alpha1.SpaceProvisionerConfig{}
+		require.NoError(t, r.Client.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(spc2), member2Spc))
+		spc.ModifySpaceProvisionerConfig(member2Spc, spc.WithReadyConditionValid())
+		require.NoError(t, r.Client.Status().Update(context.TODO(), member2Spc))
 
 		// when
 		res, err := r.Reconcile(context.TODO(), req)
@@ -4615,7 +4568,6 @@ func TestUserSignupLastTargetClusterAnnotation(t *testing.T) {
 		userSignup.Annotations[toolchainv1alpha1.UserSignupLastTargetClusterAnnotationKey] = "member2"
 		spc1 := hspc.NewEnabledValidTenantSPC("member1")
 		r, req, _ := prepareReconcile(t, userSignup.Name, spc1, userSignup, baseNSTemplateTier, deactivate30Tier, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)))
-		InitializeCounters(t, NewToolchainStatus())
 
 		// when
 		res, err := r.Reconcile(context.TODO(), req)
@@ -4650,7 +4602,6 @@ func TestUserSignupLastTargetClusterAnnotation(t *testing.T) {
 			}
 			return nil
 		}
-		InitializeCounters(t, NewToolchainStatus())
 
 		// when
 		res, err := r.Reconcile(context.TODO(), req)