Modified the callback to ensure current revison is rolled out and onl…

…y new revison is requeued; Added test case for the scenario

Signed-off-by: jubittajohn <jujohn@redhat.com>

pkg/operator/staticpod/controller/installer/installer_controller.go

@@ -454,16 +454,6 @@ func (c *InstallerController) manageInstallationPods(ctx context.Context, operat
 		return true, requeueAfter, nil
 	}
 
-	// Check if revision should be installed based on if the quorum is about to be violated
-	if c.shouldRevisionInstall != nil {
-		quorumSafe, err := c.shouldRevisionInstall()
-		if !quorumSafe {
-			return true, 30 * time.Second, err
-		} else {
-			return false, 0, nil
-		}
-	}
-
 	for l := 0; l < len(operatorStatus.NodeStatuses); l++ {
 		i := (startNode + l) % len(operatorStatus.NodeStatuses)
 
@@ -869,6 +859,13 @@ func getInstallerPodName(ns *operatorv1.NodeStatus) string {
 
 // ensureInstallerPod creates the installer pod with the secrets required to if it does not exist already
 func (c *InstallerController) ensureInstallerPod(ctx context.Context, operatorSpec *operatorv1.StaticPodOperatorSpec, ns *operatorv1.NodeStatus) error {
+	// checks if new revision should be rolled out
+	if c.shouldRevisionInstall != nil {
+		shouldInstall, err := c.shouldRevisionInstall()
+		if !shouldInstall {
+			return err
+		}
+	}
 	pod := resourceread.ReadPodV1OrDie(podTemplate)
 
 	pod.Namespace = c.targetNamespace

pkg/operator/staticpod/controller/installer/installer_controller_test.go

@@ -2062,15 +2062,16 @@ func TestCreateInstallerPodMultiNode(t *testing.T) {
 
 func TestInstallerController_manageInstallationPods(t *testing.T) {
 	type fields struct {
-		targetNamespace      string
-		staticPodName        string
-		configMaps           []revision.RevisionResource
-		secrets              []revision.RevisionResource
-		command              []string
-		operatorConfigClient v1helpers.StaticPodOperatorClient
-		kubeClient           kubernetes.Interface
-		eventRecorder        events.Recorder
-		installerPodImageFn  func() string
+		targetNamespace       string
+		staticPodName         string
+		configMaps            []revision.RevisionResource
+		secrets               []revision.RevisionResource
+		command               []string
+		operatorConfigClient  v1helpers.StaticPodOperatorClient
+		kubeClient            kubernetes.Interface
+		eventRecorder         events.Recorder
+		installerPodImageFn   func() string
+		shouldRevisionInstall func() (bool, error)
 	}
 	type args struct {
 		operatorSpec           *operatorv1.StaticPodOperatorSpec
@@ -2084,22 +2085,123 @@ func TestInstallerController_manageInstallationPods(t *testing.T) {
 		want    bool
 		wantErr bool
 	}{
-		// TODO: Add test cases.
+		{
+			name: "if the revision shouldn't install and a new revision is to be rolled out, requeue the new installer pod",
+			fields: fields{
+				targetNamespace: "test-namespace",
+				staticPodName:   "test-pod",
+				command:         []string{},
+				kubeClient:      fake.NewSimpleClientset(),
+				eventRecorder:   eventstesting.NewTestingEventRecorder(t),
+				shouldRevisionInstall: func() (bool, error) {
+					return false, fmt.Errorf("revision shouldn't be installed")
+				},
+			},
+			args: args{
+				operatorSpec: &operatorv1.StaticPodOperatorSpec{
+					OperatorSpec: operatorv1.OperatorSpec{},
+				},
+				originalOperatorStatus: &operatorv1.StaticPodOperatorStatus{
+					LatestAvailableRevision: 1,
+					NodeStatuses: []operatorv1.NodeStatus{
+						{
+							NodeName:        "test-node-1",
+							CurrentRevision: 1,
+							TargetRevision:  2,
+							LastFailedCount: 1,
+						},
+					},
+				},
+			},
+			want:    true,
+			wantErr: true,
+		},
+		{
+			name: "if the revision shouldn't install and no new revision is to be rolled out, then no requeue",
+			fields: fields{
+				targetNamespace: "test-namespace",
+				staticPodName:   "test-pod",
+				command:         []string{},
+				kubeClient:      fake.NewSimpleClientset(),
+				shouldRevisionInstall: func() (bool, error) {
+					return false, fmt.Errorf("revision shouldn't be installed")
+				},
+			},
+			args: args{
+				operatorSpec: &operatorv1.StaticPodOperatorSpec{
+					OperatorSpec: operatorv1.OperatorSpec{},
+				},
+				originalOperatorStatus: &operatorv1.StaticPodOperatorStatus{
+					LatestAvailableRevision: 1,
+					NodeStatuses: []operatorv1.NodeStatus{
+						{
+							NodeName:        "test-node-1",
+							CurrentRevision: 1,
+							TargetRevision:  1,
+						},
+					},
+				},
+			},
+			want:    false,
+			wantErr: false,
+		},
+		{
+			name: "if the revision is safe to install and a new revision is to be rolled out, no requeue",
+			fields: fields{
+				targetNamespace: "test-namespace",
+				staticPodName:   "test-pod",
+				configMaps:      []revision.RevisionResource{{Name: "test-config"}},
+				command:         []string{},
+				kubeClient:      fake.NewSimpleClientset(),
+				eventRecorder:   eventstesting.NewTestingEventRecorder(t),
+				installerPodImageFn: func() string {
+					return "docker.io/foo/bar"
+				},
+				shouldRevisionInstall: func() (bool, error) {
+					return true, nil
+				},
+			},
+			args: args{
+				operatorSpec: &operatorv1.StaticPodOperatorSpec{
+					OperatorSpec: operatorv1.OperatorSpec{},
+				},
+				originalOperatorStatus: &operatorv1.StaticPodOperatorStatus{
+					LatestAvailableRevision: 1,
+					NodeStatuses: []operatorv1.NodeStatus{
+						{
+							NodeName:        "test-node-1",
+							CurrentRevision: 1,
+							TargetRevision:  2,
+						},
+					},
+				},
+			},
+			want:    false,
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			c := &InstallerController{
-				targetNamespace:     tt.fields.targetNamespace,
-				staticPodName:       tt.fields.staticPodName,
-				configMaps:          tt.fields.configMaps,
-				secrets:             tt.fields.secrets,
-				command:             tt.fields.command,
-				operatorClient:      tt.fields.operatorConfigClient,
-				configMapsGetter:    tt.fields.kubeClient.CoreV1(),
-				podsGetter:          tt.fields.kubeClient.CoreV1(),
-				eventRecorder:       tt.fields.eventRecorder,
-				installerPodImageFn: tt.fields.installerPodImageFn,
+				targetNamespace:       tt.fields.targetNamespace,
+				staticPodName:         tt.fields.staticPodName,
+				configMaps:            tt.fields.configMaps,
+				secrets:               tt.fields.secrets,
+				command:               tt.fields.command,
+				operatorClient:        tt.fields.operatorConfigClient,
+				configMapsGetter:      tt.fields.kubeClient.CoreV1(),
+				podsGetter:            tt.fields.kubeClient.CoreV1(),
+				eventRecorder:         tt.fields.eventRecorder,
+				installerPodImageFn:   tt.fields.installerPodImageFn,
+				shouldRevisionInstall: tt.fields.shouldRevisionInstall,
 			}
+			c.ownerRefsFn = func(ctx context.Context, revision int32) ([]metav1.OwnerReference, error) {
+				return []metav1.OwnerReference{}, nil
+			}
+			c.startupMonitorEnabled = func() (bool, error) {
+				return false, nil
+			}
+
 			got, _, err := c.manageInstallationPods(context.TODO(), tt.args.operatorSpec, tt.args.originalOperatorStatus)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("InstallerController.manageInstallationPods() error = %v, wantErr %v", err, tt.wantErr)

pkg/operator/staticpod/controllers.go

@@ -117,7 +117,7 @@ type Builder interface {
 	// This can help to drain/maintain a node and recover without a manual intervention when multiple instances of nodes or pods are misbehaving.
 	// Use this with caution, as this option can disrupt perspective pods that have not yet had a chance to become healthy.
 	WithPodDisruptionBudgetGuard(operatorNamespace, operatorName, readyzPort, readyzEndpoint string, pdbUnhealthyPodEvictionPolicy *v1.UnhealthyPodEvictionPolicyType, createConditionalFunc func() (bool, bool, error)) Builder
-	WithShouldRevisionInstall(quorumSafe func() (bool, error)) Builder
+	WithShouldRevisionInstall(shouldInstall func() (bool, error)) Builder
 	ToControllers() (manager.ControllerManager, error)
 }
 
@@ -200,8 +200,8 @@ func (b *staticPodOperatorControllerBuilder) WithPodDisruptionBudgetGuard(operat
 	return b
 }
 
-func (b *staticPodOperatorControllerBuilder) WithShouldRevisionInstall(quorumSafe func() (bool, error)) Builder {
-	b.shouldRevisionInstall = quorumSafe
+func (b *staticPodOperatorControllerBuilder) WithShouldRevisionInstall(shouldInstall func() (bool, error)) Builder {
+	b.shouldRevisionInstall = shouldInstall
 	return b
 }