OCPBUGS-57338: error out on zeroed scheduling interval

as we fail when the scheduling interval isn't set we should also fail
when it is set to an invalid value.

Author: ricardomaraschini

pkg/operator/target_config_reconciler.go

@@ -155,8 +155,8 @@ func (c TargetConfigReconciler) sync() error {
 		return err
 	}
 
-	if descheduler.Spec.DeschedulingIntervalSeconds == nil {
-		return fmt.Errorf("descheduler should have an interval set")
+	if descheduler.Spec.DeschedulingIntervalSeconds == nil || *descheduler.Spec.DeschedulingIntervalSeconds <= 0 {
+		return fmt.Errorf("descheduler should have an interval set and it should be greater than 0")
 	}
 
 	specAnnotations := map[string]string{

pkg/operator/target_config_reconciler_test.go

@@ -1508,6 +1508,29 @@ func TestSync(t *testing.T) {
 				Reason: fmt.Sprintf("profile %v can only be used when KubeVirt is properly deployed", deschedulerv1.RelieveAndMigrate),
 			},
 		},
+		{
+			name: "Zeroed out descheduling interval",
+			targetConfigReconciler: &TargetConfigReconciler{
+				ctx:           context.TODO(),
+				kubeClient:    fake.NewSimpleClientset(),
+				eventRecorder: fakeRecorder,
+				configSchedulerLister: &fakeSchedConfigLister{
+					Items: map[string]*configv1.Scheduler{"cluster": configLowNodeUtilization},
+				},
+			},
+			descheduler: &deschedulerv1.KubeDescheduler{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cluster",
+					Namespace: "openshift-kube-descheduler-operator",
+				},
+				Spec: deschedulerv1.KubeDeschedulerSpec{
+					DeschedulingIntervalSeconds: utilptr.To[int32](0),
+					Profiles:                    []deschedulerv1.DeschedulerProfile{"LifecycleAndUtilization"},
+					ProfileCustomizations:       &deschedulerv1.ProfileCustomizations{ThresholdPriority: utilptr.To[int32](1000), ThresholdPriorityClassName: "className"},
+				},
+			},
+			err: fmt.Errorf("descheduler should have an interval set and it should be greater than 0"),
+		},
 	}
 
 	for _, tt := range tests {

pkg/softtainter/softtainter.go

@@ -102,8 +102,8 @@ func (st *softTainter) Reconcile(ctx context.Context, request reconcile.Request)
 		return reconcile.Result{}, err
 	}
 
-	if des.Spec.DeschedulingIntervalSeconds == nil {
-		return reconcile.Result{}, fmt.Errorf("descheduler should have an interval set")
+	if des.Spec.DeschedulingIntervalSeconds == nil || *des.Spec.DeschedulingIntervalSeconds <= 0 {
+		return reconcile.Result{}, fmt.Errorf("descheduler should have an interval set and it should be greater than 0")
 	}
 	st.resyncPeriod = time.Duration(*des.Spec.DeschedulingIntervalSeconds) * time.Second
 

pkg/softtainter/softtainter_test.go

@@ -2,7 +2,6 @@ package softtainter
 
 import (
 	"context"
-	"errors"
 	"os"
 	"path"
 	"strings"
@@ -75,7 +74,7 @@ func TestReconcile(t *testing.T) {
 		nodes           []*corev1.Node
 		expectednodes   []*corev1.Node
 		nodeUtilization MockNodeUtilization
-		expectedErr     error
+		expectedErr     string
 		kubedescheduler deschedulerv1.KubeDescheduler
 		testfilename    string
 	}{
@@ -99,7 +98,6 @@ func TestReconcile(t *testing.T) {
 				},
 				mockErrors: nil,
 			},
-			expectedErr: nil,
 			kubedescheduler: deschedulerv1.KubeDescheduler{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: operatorclient.OperatorNamespace,
@@ -135,7 +133,6 @@ func TestReconcile(t *testing.T) {
 				},
 				mockErrors: nil,
 			},
-			expectedErr: nil,
 			kubedescheduler: deschedulerv1.KubeDescheduler{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: operatorclient.OperatorNamespace,
@@ -171,7 +168,6 @@ func TestReconcile(t *testing.T) {
 				},
 				mockErrors: nil,
 			},
-			expectedErr: nil,
 			kubedescheduler: deschedulerv1.KubeDescheduler{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: operatorclient.OperatorNamespace,
@@ -213,7 +209,6 @@ func TestReconcile(t *testing.T) {
 				},
 				mockErrors: nil,
 			},
-			expectedErr: nil,
 			kubedescheduler: deschedulerv1.KubeDescheduler{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: operatorclient.OperatorNamespace,
@@ -249,7 +244,6 @@ func TestReconcile(t *testing.T) {
 				},
 				mockErrors: nil,
 			},
-			expectedErr: nil,
 			kubedescheduler: deschedulerv1.KubeDescheduler{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: operatorclient.OperatorNamespace,
@@ -285,7 +279,6 @@ func TestReconcile(t *testing.T) {
 				},
 				mockErrors: nil,
 			},
-			expectedErr: nil,
 			kubedescheduler: deschedulerv1.KubeDescheduler{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: operatorclient.OperatorNamespace,
@@ -321,7 +314,6 @@ func TestReconcile(t *testing.T) {
 				},
 				mockErrors: nil,
 			},
-			expectedErr: nil,
 			kubedescheduler: deschedulerv1.KubeDescheduler{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: operatorclient.OperatorNamespace,
@@ -357,7 +349,6 @@ func TestReconcile(t *testing.T) {
 				},
 				mockErrors: nil,
 			},
-			expectedErr: nil,
 			kubedescheduler: deschedulerv1.KubeDescheduler{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: operatorclient.OperatorNamespace,
@@ -373,6 +364,22 @@ func TestReconcile(t *testing.T) {
 			},
 			testfilename: "policyNoRelieveAndMigrate.yaml",
 		},
+		{
+			description: "zeroed out descheduling interval",
+			nodes:       []*corev1.Node{},
+			expectedErr: "descheduler should have an interval set and it should be greater than 0",
+			kubedescheduler: deschedulerv1.KubeDescheduler{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: operatorclient.OperatorNamespace,
+					Name:      operatorclient.OperatorConfigName,
+				},
+				Spec: deschedulerv1.KubeDeschedulerSpec{
+					DeschedulingIntervalSeconds: pointer.Int32(0),
+					Mode:                        deschedulerv1.Automatic,
+				},
+			},
+			testfilename: "policy.yaml",
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.description, func(t *testing.T) {
@@ -389,8 +396,14 @@ func TestReconcile(t *testing.T) {
 				nodeUtilizationFactory: tc.nodeUtilization.newNodeUtilizationFactory,
 			}
 			result, err := st.Reconcile(ctx, reconcile.Request{})
-			if !errors.Is(err, tc.expectedErr) {
-				t.Errorf("Test %#v failed, test case error: %v, expected error: %v", tc.description, err, tc.expectedErr)
+			if err != nil {
+				if tc.expectedErr == "" {
+					t.Errorf("Test %#v failed, unexpected error: %v", tc.description, err)
+				} else if !strings.Contains(err.Error(), tc.expectedErr) {
+					t.Errorf("Test %#v failed, test case error: %v, expected error to contain: %v", tc.description, err, tc.expectedErr)
+				}
+			} else if tc.expectedErr != "" {
+				t.Errorf("Test %#v failed, expected error: %v, but got no error", tc.description, tc.expectedErr)
 			}
 
 			if tc.kubedescheduler.Spec.DeschedulingIntervalSeconds == nil || result.RequeueAfter != time.Duration(*tc.kubedescheduler.Spec.DeschedulingIntervalSeconds)*time.Second {