Repository: gardener/machine-controller-manager (go)

Issue:
[Edge case] Stale `deployment.kubernetes.io/desired-replicas` annotation block rollout
**How to categorize this issue?**
<!--
Please select area, kind, and priority for this issue. This helps the community categorizing it.
Replace below TODOs or exchange the existing identifiers with those that fit best in your opinion.
If multiple identifiers make sense you can also state the commands multiple times, e.g.
  /area control-plane
  /area auto-scaling
  ...

"/area" identifiers:     audit-logging|auto-scaling|backup|certification|control-plane-migration|control-plane|cost|delivery|dev-productivity|disaster-recovery|documentation|high-availability|logging|metering|monitoring|networking|open-source|ops-productivity|os|performance|quality|robustness|scalability|security|storage|testing|usability|user-management
"/kind" identifiers:     api-change|bug|cleanup|discussion|enhancement|epic|impediment|poc|post-mortem|question|regression|task|technical-debt|test
"/priority" identifiers: 1|2|3|4|5 (ordered from greatest to least)
-->
/area robustness
/kind bug
/priority 3

**What happened**:

A case was seen where there was a single machineSet under the machineDeployment with `deployment.kubernetes.io/desired-replicas` not up-to-date with `machinedeloyment.spec.replicas` , but `machineSet.spec.Replicas ==machineDeployment.spec.replicas`

This led to scaling being detected for this machineDeployment in every reconcile but [`scale`](https://github.com/gardener/machine-controller-manager/blob/7b6f3a000d6e2f3f73b798437ba10b899dedcdc7/pkg/controller/deployment_sync.go#L406-L408) not actually happening.

Later when the consumer triggered a rollout (machineDeployment started referring new machineClass) , the rollout never starts as [rollout logic](https://github.com/gardener/machine-controller-manager/blob/7b6f3a000d6e2f3f73b798437ba10b899dedcdc7/pkg/controller/deployment.go#L541) is called after [scaling](https://github.com/gardener/machine-controller-manager/blob/7b6f3a000d6e2f3f73b798437ba10b899dedcdc7/pkg/controller/deployment.go#L532-L535C1) logic 

**What you expected to happen**:

- `deployment.kubernetes.io/desired-replicas` to re-sync 
- rollout not to get blocked

**How to reproduce it (as minimally and precisely as possible)**:

I am not sure how this situation was reached. Ideally , on any machineSet update , all the machineSets are updated, which should update `deployment.kubernetes.io/desired-replicas` also.

But these set of events could lead to this situation:
<img width="1475" alt="image" src="https://github.com/gardener/machine-controller-manager/assets/79965161/84240b45-2969-45d3-9b5a-bdcdcd73b20a">


**Anything else we need to know?**:

**Environment**:
mcm 0.49.0

- Kubernetes version (use `kubectl version`):
- Cloud provider or hardware configuration:
- Others:


Pull request:
Keep `DesiredReplicas` annotation up to date even if no scaling happens
**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
Fixes #815 

**Special notes for your reviewer**:

**Release note**:
<!--  Write your release note:
1. Enter your release note in the below block.
2. If no release note is required, just write "NONE" within the block.

Format of block header: <category> <target_group>
Possible values:
- category:       breaking|feature|bugfix|doc|other
- target_group:   user|operator|developer|dependency
-->
```bugfix user
An edge case where outdated DesiredReplicas annotation blocked a rolling update is fixed.
```


Patch:
diff --git a/pkg/controller/deployment_sync.go b/pkg/controller/deployment_sync.go
index 4c73b1fd5..90e1f1cc0 100644
--- a/pkg/controller/deployment_sync.go
+++ b/pkg/controller/deployment_sync.go
@@ -404,7 +404,10 @@ func (dc *controller) scale(ctx context.Context, deployment *v1alpha1.MachineDep
 	// deployment. If there is no active machine set, then we should scale up the newest machine set.
 	if activeOrLatest := FindActiveOrLatest(newIS, oldISs); activeOrLatest != nil {
 		if (activeOrLatest.Spec.Replicas) == (deployment.Spec.Replicas) {
-			return nil
+			// to deal with the case where the DesiredReplicas annotation is outdated (issue - https://github.com/gardener/machine-controller-manager/issues/815)
+			klog.V(3).Infof("DesiredReplicas annotation possibly outdated for the machineSet %s, updating if needed...", activeOrLatest.Name)
+			_, _, err := dc.scaleMachineSet(ctx, activeOrLatest, deployment.Spec.Replicas, deployment, "no-op")
+			return err
 		}
 		klog.V(3).Infof("Scaling latest/theOnlyActive machineSet %s", activeOrLatest.Name)
 		_, _, err := dc.scaleMachineSetAndRecordEvent(ctx, activeOrLatest, deployment.Spec.Replicas, deployment)


Test patch:
diff --git a/pkg/controller/deployment_sync_test.go b/pkg/controller/deployment_sync_test.go
index 5045fe062..f8e4b7a6b 100644
--- a/pkg/controller/deployment_sync_test.go
+++ b/pkg/controller/deployment_sync_test.go
@@ -455,8 +455,9 @@ var _ = Describe("deployment_sync", func() {
 					controlObjects = append(controlObjects, data.setup.oldISs[i])
 				}
 
-				controlObjects = append(controlObjects, data.setup.newIS)
-
+				if data.setup.newIS != nil {
+					controlObjects = append(controlObjects, data.setup.newIS)
+				}
 				c, trackers := createController(stop, testNamespace, controlObjects, nil, nil)
 				defer trackers.Stop()
 				waitForCacheSync(stop, c)
@@ -840,6 +841,21 @@ var _ = Describe("deployment_sync", func() {
 					err: false,
 				},
 			}),
+			Entry("if no scaling needed but DesiredReplicas Annotation for the only active machineSet is outdated, it should be updated with the correct value", &data{
+				setup: setup{
+					machineDeployment: newMachineDeployment(mDeploymentSpecTemplate, 1, 500, 2, 0, nil, nil, nil, nil),
+					oldISs: []*machinev1.MachineSet{
+						newMachineSet(mSetSpecTemplate, nameMachineSet1, 1, 500, nil, nil, map[string]string{DesiredReplicasAnnotation: "2", MaxReplicasAnnotation: "3"}, nil),
+					},
+					newIS: nil,
+				},
+				expect: expect{
+					err: false,
+					machineSets: []*machinev1.MachineSet{
+						newMachineSet(mSetSpecTemplate, nameMachineSet1, 1, 500, nil, nil, map[string]string{DesiredReplicasAnnotation: "1", MaxReplicasAnnotation: "3"}, nil),
+					},
+				},
+			}),
 		)
 	})
 })