Merge pull request #1360 from jubittajohn/enable-safe-member-deletion-during-revision-rollout

OCPBUGS-17199: Separated the revision stability check from the bootstrap completeness check

Author: openshift-merge-bot[bot]

pkg/operator/ceohelpers/bootstrap.go

@@ -107,16 +107,24 @@ func CheckSafeToScaleCluster(
 	infraLister configv1listers.InfrastructureLister,
 	etcdClient etcdcli.AllMemberLister) error {
 
-	bootstrapComplete, err := IsBootstrapComplete(configmapLister, staticPodClient, etcdClient)
+	bootstrapComplete, err := IsBootstrapComplete(configmapLister, etcdClient)
 	if err != nil {
 		return fmt.Errorf("CheckSafeToScaleCluster failed to determine bootstrap status: %w", err)
 	}
-
 	// while bootstrapping, scaling should be considered safe always
 	if !bootstrapComplete {
 		return nil
 	}
 
+	revisionStable, err := IsRevisionStable(staticPodClient)
+	if err != nil {
+		return fmt.Errorf("CheckSafeToScaleCluster failed to determine stability of revisions: %w", err)
+	}
+	// when revision is stabilising, scaling should be considered safe always
+	if !revisionStable {
+		return nil
+	}
+
 	scalingStrategy, err := GetBootstrapScalingStrategy(staticPodClient, namespaceLister, infraLister)
 	if err != nil {
 		return fmt.Errorf("CheckSafeToScaleCluster failed to get bootstrap scaling strategy: %w", err)
@@ -151,14 +159,30 @@ func CheckSafeToScaleCluster(
 }
 
 // IsBootstrapComplete returns true if bootstrap has completed.
-func IsBootstrapComplete(configmapLister corev1listers.ConfigMapLister, staticPodClient v1helpers.StaticPodOperatorClient, etcdClient etcdcli.AllMemberLister) (bool, error) {
+func IsBootstrapComplete(configmapLister corev1listers.ConfigMapLister, etcdClient etcdcli.AllMemberLister) (bool, error) {
 	// do a cheap check to see if the installer has marked
 	// bootstrapping as done by creating the configmap first.
 	if isBootstrapComplete, err := bootstrap.IsBootstrapComplete(configmapLister); !isBootstrapComplete || err != nil {
 		return isBootstrapComplete, err
 	}
 
-	// now run check to stability of revisions
+	// check if etcd-bootstrap member is still present within the etcd cluster membership
+	membersList, err := etcdClient.MemberList(context.Background())
+	if err != nil {
+		return false, fmt.Errorf("IsBootstrapComplete couldn't list the etcd cluster members: %w", err)
+	}
+	for _, m := range membersList {
+		if m.Name == "etcd-bootstrap" {
+			klog.V(4).Infof("(etcd-bootstrap) member is still present in the etcd cluster membership")
+			return false, nil
+		}
+	}
+
+	return true, nil
+}
+
+// IsRevisionStable checks the stability of revisions and returns true if the revision is stable
+func IsRevisionStable(staticPodClient v1helpers.StaticPodOperatorClient) (bool, error) {
 	_, status, _, err := staticPodClient.GetStaticPodOperatorState()
 	if err != nil {
 		return false, fmt.Errorf("failed to get static pod operator state: %w", err)
@@ -168,19 +192,7 @@ func IsBootstrapComplete(configmapLister corev1listers.ConfigMapLister, staticPo
 	}
 	for _, curr := range status.NodeStatuses {
 		if curr.CurrentRevision != status.LatestAvailableRevision {
-			klog.V(4).Infof("bootstrap considered incomplete because revision %d is still in progress", status.LatestAvailableRevision)
-			return false, nil
-		}
-	}
-
-	// check if etcd-bootstrap member is still present within the etcd cluster membership
-	membersList, err := etcdClient.MemberList(context.Background())
-	if err != nil {
-		return false, fmt.Errorf("IsBootstrapComplete couldn't list the etcd cluster members: %w", err)
-	}
-	for _, m := range membersList {
-		if m.Name == "etcd-bootstrap" {
-			klog.V(4).Infof("(etcd-bootstrap) member is still present in the etcd cluster membership")
+			klog.V(4).Infof("revision stability check failed because revision %d is still in progress", status.LatestAvailableRevision)
 			return false, nil
 		}
 	}

pkg/operator/ceohelpers/bootstrap_test.go

@@ -164,56 +164,36 @@ func Test_GetBootstrapScalingStrategy(t *testing.T) {
 func Test_IsBootstrapComplete(t *testing.T) {
 	tests := map[string]struct {
 		bootstrapConfigMap *corev1.ConfigMap
-		nodes              []operatorv1.NodeStatus
 		etcdMembers        []*etcdserverpb.Member
 		expectComplete     bool
 		expectError        error
 	}{
-		"bootstrap complete, nodes up to date": {
+		"bootstrap complete, configmap status is complete": {
 			bootstrapConfigMap: bootstrapComplete,
-			nodes:              twoNodesAtCurrentRevision,
 			etcdMembers:        u.DefaultEtcdMembers(),
 			expectComplete:     true,
 			expectError:        nil,
 		},
-		"bootstrap progressing, nodes up to date": {
+		"bootstrap progressing, configmap status is progressing": {
 			bootstrapConfigMap: bootstrapProgressing,
-			nodes:              twoNodesAtCurrentRevision,
 			etcdMembers:        u.DefaultEtcdMembers(),
 			expectComplete:     false,
 			expectError:        nil,
 		},
 		"bootstrap configmap missing": {
 			bootstrapConfigMap: nil,
-			nodes:              twoNodesAtCurrentRevision,
-			etcdMembers:        u.DefaultEtcdMembers(),
-			expectComplete:     false,
-			expectError:        nil,
-		},
-		"bootstrap complete, no recorded revisions": {
-			bootstrapConfigMap: bootstrapComplete,
-			nodes:              zeroNodesAtAnyRevision,
-			etcdMembers:        u.DefaultEtcdMembers(),
-			expectComplete:     true,
-			expectError:        nil,
-		},
-		"bootstrap complete, node progressing": {
-			bootstrapConfigMap: bootstrapComplete,
-			nodes:              twoNodesProgressingTowardsCurrentRevision,
 			etcdMembers:        u.DefaultEtcdMembers(),
 			expectComplete:     false,
 			expectError:        nil,
 		},
 		"bootstrap complete, etcd-bootstrap removed": {
 			bootstrapConfigMap: bootstrapComplete,
-			nodes:              twoNodesAtCurrentRevision,
 			etcdMembers:        u.DefaultEtcdMembers(),
 			expectComplete:     true,
 			expectError:        nil,
 		},
 		"bootstrap complete, etcd-bootstrap exists": {
 			bootstrapConfigMap: bootstrapComplete,
-			nodes:              twoNodesAtCurrentRevision,
 			etcdMembers:        append(u.DefaultEtcdMembers(), u.FakeEtcdBootstrapMember(0)),
 			expectComplete:     false,
 			expectError:        nil,
@@ -230,25 +210,58 @@ func Test_IsBootstrapComplete(t *testing.T) {
 			}
 			fakeConfigMapLister := corev1listers.NewConfigMapLister(indexer)
 
-			operatorStatus := &operatorv1.StaticPodOperatorStatus{
-				OperatorStatus: operatorv1.OperatorStatus{LatestAvailableRevision: 1},
-				NodeStatuses:   test.nodes,
-			}
-			fakeStaticPodClient := v1helpers.NewFakeStaticPodOperatorClient(nil, operatorStatus, nil, nil)
-
 			fakeEtcdClient, err := etcdcli.NewFakeEtcdClient(test.etcdMembers)
 			if err != nil {
 				t.Fatal(err)
 			}
 
-			actualComplete, actualErr := IsBootstrapComplete(fakeConfigMapLister, fakeStaticPodClient, fakeEtcdClient)
+			actualComplete, actualErr := IsBootstrapComplete(fakeConfigMapLister, fakeEtcdClient)
 
 			assert.Equal(t, test.expectComplete, actualComplete)
 			assert.Equal(t, test.expectError, actualErr)
 		})
 	}
 }
 
+func Test_IsRevisionStable(t *testing.T) {
+	tests := map[string]struct {
+		nodes             []operatorv1.NodeStatus
+		expectedStability bool
+		expectedError     error
+	}{
+		"is revision stable, node progressing": {
+			nodes:             twoNodesProgressingTowardsCurrentRevision,
+			expectedStability: false,
+			expectedError:     nil,
+		},
+		"is revision stable, nodes up to date": {
+			nodes:             twoNodesAtCurrentRevision,
+			expectedStability: true,
+			expectedError:     nil,
+		},
+		"bootstrap complete, no recorded revisions": {
+			nodes:             zeroNodesAtAnyRevision,
+			expectedStability: true,
+			expectedError:     nil,
+		},
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			operatorStatus := &operatorv1.StaticPodOperatorStatus{
+				OperatorStatus: operatorv1.OperatorStatus{LatestAvailableRevision: 1},
+				NodeStatuses:   test.nodes,
+			}
+			fakeStaticPodClient := v1helpers.NewFakeStaticPodOperatorClient(nil, operatorStatus, nil, nil)
+
+			actualStability, actualErr := IsRevisionStable(fakeStaticPodClient)
+
+			assert.Equal(t, test.expectedStability, actualStability)
+			assert.Equal(t, test.expectedError, actualErr)
+
+		})
+	}
+}
+
 func Test_CheckSafeToScaleCluster(t *testing.T) {
 	tests := map[string]struct {
 		namespace          *corev1.Namespace

pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go

@@ -101,7 +101,7 @@ func NewClusterMemberRemovalController(
 
 func (c *clusterMemberRemovalController) sync(ctx context.Context, syncCtx factory.SyncContext) error {
 	// skip reconciling this controller, if bootstrapping is not completed
-	bootstrapComplete, err := ceohelpers.IsBootstrapComplete(c.configMapLister, c.operatorClient, c.etcdClient)
+	bootstrapComplete, err := ceohelpers.IsBootstrapComplete(c.configMapLister, c.etcdClient)
 	if err != nil {
 		return fmt.Errorf("IsBootstrapComplete failed to determine bootstrap status: %w", err)
 	}

pkg/operator/etcdendpointscontroller/etcdendpointscontroller.go

@@ -92,13 +92,18 @@ func (c *EtcdEndpointsController) syncConfigMap(ctx context.Context, recorder ev
 	// forward or remove it if possible so clients can forget about it.
 	if existing, err := c.configmapLister.ConfigMaps(operatorclient.TargetNamespace).Get("etcd-endpoints"); err == nil && existing != nil {
 		if existingIP, hasExistingIP := existing.Annotations[etcdcli.BootstrapIPAnnotationKey]; hasExistingIP {
-			bootstrapComplete, err := ceohelpers.IsBootstrapComplete(c.configmapLister, c.operatorClient, c.etcdClient)
+			bootstrapComplete, err := ceohelpers.IsBootstrapComplete(c.configmapLister, c.etcdClient)
 			if err != nil {
 				return fmt.Errorf("couldn't determine bootstrap status: %w", err)
 			}
 
-			if bootstrapComplete {
-				// rename the annotation once we're done with bootstrapping
+			revisionStable, err := ceohelpers.IsRevisionStable(c.operatorClient)
+			if err != nil {
+				return fmt.Errorf("couldn't determine stability of revisions: %w", err)
+			}
+
+			if bootstrapComplete && revisionStable {
+				// rename the annotation once we're done with bootstrapping and the revision is stable
 				required.Annotations[etcdcli.BootstrapIPAnnotationKey+"-"] = existingIP
 			} else {
 				required.Annotations[etcdcli.BootstrapIPAnnotationKey] = existingIP