ctrl: sched: explicit replicas should not exceed nodes number

It's reasonable to have high-availability enabled to have scheduler pods
number matches the control-plane nodes' number or workers on hypershift.
but a number greater than that makes no point. This commit degrades the
scheduler status in case a higher number of replicas is requested.

Signed-off-by: Shereen Haj <shajmakh@redhat.com>

Author: shajmakh

internal/controller/numaresourcesscheduler_controller.go

@@ -244,11 +244,6 @@ func isDeploymentRunning(ctx context.Context, c client.Client, key nropv1.Namesp
 }
 
 func (r *NUMAResourcesSchedulerReconciler) computeSchedulerReplicas(ctx context.Context, schedSpec nropv1.NUMAResourcesSchedulerSpec) (*int32, error) {
-	// do not autodetect if explicitly set by the user
-	if schedSpec.Replicas != nil {
-		return schedSpec.Replicas, nil
-	}
-
 	var labelSelector map[string]string
 	var nodeRoleDescription string
 
@@ -276,6 +271,14 @@ func (r *NUMAResourcesSchedulerReconciler) computeSchedulerReplicas(ctx context.
 
 	replicaCount := int32(len(nodeList.Items))
 
+	if schedSpec.Replicas != nil {
+		// should not allow more replicas than max found nodes
+		if *schedSpec.Replicas > replicaCount {
+			return nil, fmt.Errorf("explicitly set replicas count should not be greater than the number of the nodes with role %s", nodeRoleDescription)
+		}
+		return schedSpec.Replicas, nil
+	}
+
 	// check if no nodes are found for the target role
 	if replicaCount == 0 {
 		return nil, fmt.Errorf("failed to compute scheduler replicas: no %s nodes found for platform %s", nodeRoleDescription, r.PlatformInfo.Platform)

internal/controller/numaresourcesscheduler_controller_test.go

@@ -1105,36 +1105,157 @@ var _ = Describe("Test computeSchedulerReplicas", func() {
 		Entry("HyperShift: 3 control-plane + 5 worker should return 3 replicas (uses worker)", platform.HyperShift, 3, 5, ptr.To(int32(3)), false),
 	)
 
-	Context("when replicas are explicitly set", func() {
-		It("should return the explicitly set replicas without checking nodes", func() {
-			// setup reconciler with HyperShift platform (uses worker nodes)
-			reconciler.PlatformInfo = platforminfo.PlatformInfo{
-				Platform: platform.HyperShift,
-				Version:  "v4.14.0",
-			}
+	When("replicas count is explicitly set", func() {
+		type testCase struct {
+			platform         platform.Platform
+			numControlPlane  int
+			numWorker        int
+			explicitReplicas *int32
+			expectedReplicas *int32
+			expectError      string
+		}
+		DescribeTable(
+			"should not allow more replicas than the number of nodes", func(ctx context.Context, tc testCase) {
+				nrs := testobjs.NewNUMAResourcesScheduler("numaresourcesscheduler", "some/url:latest", testSchedulerName, 11*time.Second)
+				nrs.Spec.Replicas = tc.explicitReplicas
+				initObjects := []runtime.Object{nrs}
+				initObjects = append(initObjects, fakeNodes(tc.numControlPlane, tc.numWorker)...)
+				reconciler, err := NewFakeNUMAResourcesSchedulerReconciler(initObjects...)
+				Expect(err).ToNot(HaveOccurred())
+				reconciler.PlatformInfo = platforminfo.PlatformInfo{
+					Platform: tc.platform,
+					Version:  "v4.14.0",
+				}
 
-			// create scenario with no worker nodes but explicit replicas
-			nodes := fakeNodes(3, 0) // only control-plane nodes
-			reconciler, err := NewFakeNUMAResourcesSchedulerReconciler(nodes...)
+				key := client.ObjectKeyFromObject(nrs)
+				_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+				if tc.expectError == "" {
+					Expect(err).ToNot(HaveOccurred())
+				} else {
+					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(ContainSubstring(tc.expectError))
+				}
+
+				Expect(reconciler.Client.Get(ctx, key, nrs)).To(Succeed())
+
+				if tc.expectError != "" {
+					degradedCondition := getConditionByType(nrs.Status.Conditions, status.ConditionDegraded)
+					Expect(degradedCondition.Status).To(Equal(metav1.ConditionTrue))
+					Expect(degradedCondition.Reason).To(Equal(status.ReasonInternalError))
+					Expect(degradedCondition.Message).To(ContainSubstring(tc.expectError))
+					return
+				}
+
+				dp := &appsv1.Deployment{}
+				dpKey := client.ObjectKey{Namespace: nrs.Status.Deployment.Namespace, Name: nrs.Status.Deployment.Name}
+				Expect(reconciler.Client.Get(ctx, dpKey, dp)).To(Succeed())
+				Expect(*dp.Spec.Replicas).To(Equal(*tc.expectedReplicas))
+			},
+			// openshift scenarios
+			Entry("OpenShift: 3 control-plane with explicit replicas count equal to the number of nodes",
+				testCase{
+					platform:         platform.OpenShift,
+					numControlPlane:  3,
+					numWorker:        5,
+					explicitReplicas: ptr.To(int32(3)),
+					expectedReplicas: ptr.To(int32(3)),
+				}),
+			Entry("OpenShift: 3 control-plane with explicit replicas count less than the number of nodes",
+				testCase{
+					platform:         platform.OpenShift,
+					numControlPlane:  3,
+					numWorker:        0,
+					explicitReplicas: ptr.To(int32(2)),
+					expectedReplicas: ptr.To(int32(2)),
+				}),
+			Entry("OpenShift: 3 control-plane with explicit replicas count greater than the number of nodes",
+				testCase{
+					platform:         platform.OpenShift,
+					numControlPlane:  3,
+					numWorker:        5,
+					explicitReplicas: ptr.To(int32(5)),
+					expectError:      "explicitly set replicas count should not be greater than the number of the nodes with role",
+				}),
+
+			// hypershift scenarios
+			Entry("HyperShift: 3 control-plane with explicit replicas count equal to the number of nodes",
+				testCase{
+					platform:         platform.HyperShift,
+					numControlPlane:  0,
+					numWorker:        3,
+					explicitReplicas: ptr.To(int32(3)),
+					expectedReplicas: ptr.To(int32(3)),
+				}),
+			Entry("HyperShift: 3 control-plane with explicit replicas count less than the number of nodes",
+				testCase{
+					platform:         platform.HyperShift,
+					numControlPlane:  0,
+					numWorker:        3,
+					explicitReplicas: ptr.To(int32(2)),
+					expectedReplicas: ptr.To(int32(2)),
+				}),
+			Entry("HyperShift: 3 control-plane with explicit replicas count greater than the number of nodes",
+				testCase{
+					platform:         platform.HyperShift,
+					numControlPlane:  0,
+					numWorker:        5,
+					explicitReplicas: ptr.To(int32(6)),
+					expectError:      "explicitly set replicas count should not be greater than the number of the nodes with role",
+				}),
+		)
+
+		It("should degrade when nodes are scaled down with higher number of explicit replicas", func(ctx context.Context) {
+			nrs := testobjs.NewNUMAResourcesScheduler("numaresourcesscheduler", "some/url:latest", testSchedulerName, 11*time.Second)
+			initialReplicas := int32(3)
+			nrs.Spec.Replicas = ptr.To(initialReplicas)
+			initObjects := []runtime.Object{nrs}
+			initialNodes := fakeNodes(4, 4)
+			initObjects = append(initObjects, initialNodes...)
+			reconciler, err := NewFakeNUMAResourcesSchedulerReconciler(initObjects...)
 			Expect(err).ToNot(HaveOccurred())
 			reconciler.PlatformInfo = platforminfo.PlatformInfo{
-				Platform: platform.HyperShift,
+				Platform: platform.OpenShift,
 				Version:  "v4.14.0",
 			}
 
-			// create test scheduler spec with explicit replicas
-			explicitReplicas := int32(5)
-			schedSpec := nropv1.NUMAResourcesSchedulerSpec{
-				Replicas: &explicitReplicas,
-			}
+			key := client.ObjectKeyFromObject(nrs)
+			_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+			Expect(err).ToNot(HaveOccurred())
 
-			// call the function under test
-			result, err := reconciler.computeSchedulerReplicas(context.TODO(), schedSpec)
+			Expect(reconciler.Client.Get(ctx, key, nrs)).To(Succeed())
+			dp := &appsv1.Deployment{}
+			dpKey := client.ObjectKey{Namespace: nrs.Status.Deployment.Namespace, Name: nrs.Status.Deployment.Name}
+			Expect(reconciler.Client.Get(ctx, dpKey, dp)).To(Succeed())
+			Expect(*dp.Spec.Replicas).To(Equal(initialReplicas))
 
-			// should return the explicit value without error
+			// scale down the nodes by one but keep it same as the explicit replicas count
+			Eventually(func(g Gomega) {
+				g.Expect(reconciler.Client.Delete(ctx, initialNodes[0].(*corev1.Node))).To(Succeed())
+			}).WithPolling(5 * time.Second).WithTimeout(30 * time.Second).Should(Succeed())
+
+			_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key})
 			Expect(err).ToNot(HaveOccurred())
-			Expect(result).ToNot(BeNil())
-			Expect(*result).To(Equal(explicitReplicas))
+
+			Expect(reconciler.Client.Get(ctx, key, nrs)).To(Succeed())
+			Expect(reconciler.Client.Get(ctx, dpKey, dp)).To(Succeed())
+			Expect(*dp.Spec.Replicas).To(Equal(initialReplicas))
+
+			// scale down the nodes by one but keep it same as the explicit replicas count
+			Eventually(func(g Gomega) {
+				g.Expect(reconciler.Client.Delete(ctx, initialNodes[1].(*corev1.Node))).To(Succeed())
+			}).WithPolling(5 * time.Second).WithTimeout(30 * time.Second).Should(Succeed())
+
+			errMessage := "explicitly set replicas count should not be greater than the number of the nodes with role"
+			_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring(errMessage))
+
+			Expect(reconciler.Client.Get(ctx, key, nrs)).To(Succeed())
+
+			degradedCondition := getConditionByType(nrs.Status.Conditions, status.ConditionDegraded)
+			Expect(degradedCondition.Status).To(Equal(metav1.ConditionTrue))
+			Expect(degradedCondition.Reason).To(Equal(status.ReasonInternalError))
+			Expect(degradedCondition.Message).To(ContainSubstring(errMessage))
 		})
 	})
 })