Merge pull request #283 from openshift-cherrypick-robot/cherry-pick-2…

…80-to-release-4.14 [release-4.14] OCPBUGS-30014: Never delete a Machine when there's a single Machine in an index
openshift · Mar 4, 2024 · 074a22c · 074a22c
2 parents 0455a74 + 8683c45
commit 074a22c
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 0 deletions.
diff --git a/pkg/controllers/controlplanemachineset/updates.go b/pkg/controllers/controlplanemachineset/updates.go
@@ -346,6 +346,11 @@ func (r *ControlPlaneMachineSetReconciler) waitForRemoveMachine(logger logr.Logg
 }
 
 func (r *ControlPlaneMachineSetReconciler) deleteReplacedMachines(ctx context.Context, logger logr.Logger, machineProvider machineproviders.MachineProvider, machines []machineproviders.MachineInfo) (bool, ctrl.Result, error) {
+	if len(machines) < 2 {
+		// No need to delete any machines if there are less than 2 machines in an index.
+		return false, ctrl.Result{}, nil
+	}
+
 	machinesNeedingReplacement := needReplacementMachines(machines)
 	machinesUpdated := updatedMachines(machines)
 	machinesOutdatedNonReady := nonReadyMachines(machinesNeedingReplacement)

diff --git a/pkg/controllers/controlplanemachineset/updates_test.go b/pkg/controllers/controlplanemachineset/updates_test.go
@@ -397,6 +397,46 @@ var _ = Describe("reconcileMachineUpdates", func() {
 					}
 				},
 			}),
+			Entry("with updates required in a single index, and the outdated index is not ready", rollingUpdateTableInput{
+				cpmsBuilder: cpmsBuilder.WithReplicas(3),
+				machineInfos: map[int32][]machineproviders.MachineInfo{
+					0: {updatedMachineBuilder.WithIndex(0).WithMachineName("machine-0").WithNodeName("node-0").Build()},
+					1: {outdatedNonReadyMachineBuilder.WithIndex(1).WithMachineName("machine-1").WithNodeName("node-1").WithNeedsUpdate(true).
+						WithDiff(instanceDiff).Build()},
+					2: {updatedMachineBuilder.WithIndex(2).WithMachineName("machine-2").WithNodeName("node-2").Build()},
+				},
+				setupMock: func(machineInfos map[int32][]machineproviders.MachineInfo) {
+					mockMachineProvider.EXPECT().WithClient(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockMachineProvider, nil).AnyTimes()
+					mockMachineProvider.EXPECT().GetMachineInfos(gomock.Any(), gomock.Any()).Return(machineInfosMaptoSlice(machineInfos), nil).AnyTimes()
+					mockMachineProvider.EXPECT().CreateMachine(gomock.Any(), gomock.Any(), int32(1)).Return(nil).Times(1)
+					mockMachineProvider.EXPECT().DeleteMachine(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+				},
+				expectedLogsBuilder: func() []testutils.LogEntry {
+					return []testutils.LogEntry{
+						{
+							Level: 2,
+							KeysAndValues: []interface{}{
+								"updateStrategy", machinev1.RollingUpdate,
+								"index", int32(1),
+								"namespace", namespaceName,
+								"name", "machine-1",
+								"diff", instanceDiff,
+							},
+							Message: machineRequiresUpdate,
+						},
+						{
+							Level: 2,
+							KeysAndValues: []interface{}{
+								"updateStrategy", machinev1.RollingUpdate,
+								"index", int32(1),
+								"namespace", namespaceName,
+								"name", "machine-1",
+							},
+							Message: createdReplacement,
+						},
+					}
+				},
+			}),
 			Entry("with updates are required in multiple indexes", rollingUpdateTableInput{
 				cpmsBuilder: cpmsBuilder.WithReplicas(3),
 				machineInfos: map[int32][]machineproviders.MachineInfo{
@@ -516,6 +556,60 @@ var _ = Describe("reconcileMachineUpdates", func() {
 				},
 				expectedResult: ctrl.Result{RequeueAfter: 5 * time.Second},
 			}),
+			Entry("with updates are required in multiple indexes, but the replacement machine is pending, and a later index is not ready", rollingUpdateTableInput{
+				cpmsBuilder: cpmsBuilder.WithReplicas(3),
+				machineInfos: map[int32][]machineproviders.MachineInfo{
+					0: {
+						updatedMachineBuilder.WithIndex(0).WithMachineName("machine-0").WithNodeName("node-0").WithNeedsUpdate(true).Build(),
+						pendingMachineBuilder.WithIndex(0).WithMachineName("machine-replacement-0").Build(),
+					},
+					1: {outdatedNonReadyMachineBuilder.WithIndex(1).WithMachineName("machine-1").WithNodeName("node-1").WithNeedsUpdate(true).
+						WithDiff(instanceDiff).Build()},
+					2: {updatedMachineBuilder.WithIndex(2).WithMachineName("machine-2").WithNodeName("node-2").Build()},
+				},
+				setupMock: func(machineInfos map[int32][]machineproviders.MachineInfo) {
+					// Note, in this case it should not create anything new because we are at surge capacity.
+					mockMachineProvider.EXPECT().CreateMachine(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+					mockMachineProvider.EXPECT().DeleteMachine(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+				},
+				expectedLogsBuilder: func() []testutils.LogEntry {
+					return []testutils.LogEntry{
+						{
+							Level: 2,
+							KeysAndValues: []interface{}{
+								"updateStrategy", machinev1.RollingUpdate,
+								"index", int32(0),
+								"namespace", namespaceName,
+								"name", "machine-0",
+								"replacementName", "machine-replacement-0",
+							},
+							Message: waitingForReplacement,
+						},
+						{
+							Level: 2,
+							KeysAndValues: []interface{}{
+								"updateStrategy", machinev1.RollingUpdate,
+								"index", int32(1),
+								"namespace", namespaceName,
+								"name", "machine-1",
+								"diff", instanceDiff,
+							},
+							Message: machineRequiresUpdate,
+						},
+						{
+							Level: 2,
+							KeysAndValues: []interface{}{
+								"updateStrategy", machinev1.RollingUpdate,
+								"index", int32(1),
+								"namespace", namespaceName,
+								"name", "machine-1",
+							},
+							Message: noCapacityForExpansion,
+						},
+					}
+				},
+				expectedResult: ctrl.Result{RequeueAfter: 5 * time.Second},
+			}),
 			Entry("with updates are required in multiple indexes, and the replacement machine is ready", rollingUpdateTableInput{
 				cpmsBuilder: cpmsBuilder.WithReplicas(3),
 				machineInfos: map[int32][]machineproviders.MachineInfo{