From d1afb2e3039d9dcf83aec6b1798ecae724230cf7 Mon Sep 17 00:00:00 2001 From: jubittajohn Date: Mon, 17 Jun 2024 16:34:19 -0400 Subject: [PATCH] Modified the callback to ensure current revison is rolled out and only new revison is requeued; Added test case for the scenario Signed-off-by: jubittajohn --- .../installer/installer_controller.go | 17 +-- .../installer/installer_controller_test.go | 142 +++++++++++++++--- pkg/operator/staticpod/controllers.go | 6 +- 3 files changed, 132 insertions(+), 33 deletions(-) diff --git a/pkg/operator/staticpod/controller/installer/installer_controller.go b/pkg/operator/staticpod/controller/installer/installer_controller.go index 58730677e2..f47750cadd 100644 --- a/pkg/operator/staticpod/controller/installer/installer_controller.go +++ b/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 diff --git a/pkg/operator/staticpod/controller/installer/installer_controller_test.go b/pkg/operator/staticpod/controller/installer/installer_controller_test.go index 45eb711040..44df87762e 100644 --- a/pkg/operator/staticpod/controller/installer/installer_controller_test.go +++ b/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) diff --git a/pkg/operator/staticpod/controllers.go b/pkg/operator/staticpod/controllers.go index af575aa897..d547f92f09 100644 --- a/pkg/operator/staticpod/controllers.go +++ b/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 }