Fix Race conditions in Targeted Deletion of machines by CA #341

rishabh-11 · 2024-12-20T11:12:29Z

What this PR does / why we need it:
This PR fixes the issues noticed in live issues 6120 and 6101. We introduce a mutex in the NodeGroupImpl struct (node group implementation) which must be acquired before performing a scale-down or scale-up operation.

We also introduce an annotation on the MachineDeployment TriggerDeletionByMCM = "node.machine.sapcloud.io/trigger-deletion-by-mcm" whose value denotes the machines CA wants to remove and then atomically update replica count and annotation as one single step. This will help recognize machines for which CA has already reduced the replicas of the MachineDeployment and prevent it from being duplicated - race conditions are avoided when CA scaledowns are occurring in parallel. The MCM is also updated to check this TriggerDeletionByMCM annotation on the MachineDeployment.

The CA no longer directly sets the MachinePriority=1 annotation on the Machine objects. This is now done by the MCM controller when reconciling the updated MachineDeployment.

Which issue(s) this PR fixes:
Fixes #342

Special notes for your reviewer:

Release note:

Fix for data-races in concurrent CA scaledowns and CA restarts

elankath · 2024-12-24T10:49:42Z

cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go

+		klog.Infof("machine deployment %s is under rolling update, skipping", machineDeployment.Name)
+		return nil
+	}
+	markedMachines := sets.New(strings.Split(mcd.Annotations[machinesMarkedByCAForDeletion], ",")...)


Please move this to a small function mcm.getMachinesMarkedByCAForDeletion(mcd) (machineNames sets.Set[string]) which is unit testable and can be consumed by both the mcm_cloud_provider.go and mcm_manager.go

elankath · 2024-12-24T10:50:28Z

cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go

+		klog.Errorf("[Refresh] failed to get machines for machine deployment %s, hence skipping it. Err: %v", machineDeployment.Name, err.Error())
+		return err
+	}
+	var incorrectlyMarkedMachines []*Ref


I believe we can omit the Ref struct and just used types.NamespacedName which is already being used anyways in this file

elankath · 2024-12-26T06:39:47Z

cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go

@@ -539,12 +567,17 @@ func buildMachineDeploymentFromSpec(value string, mcmManager *McmManager) (*Mach
 	return machinedeployment, nil
 }

+func getMachinesMarkedByCAForDeletion(mcd *v1alpha1.MachineDeployment) sets.Set[string] {


Perhaps using the "comma, ok" idiom here for mcd.Annotations[machinesMarkedByCAForDeletion] would be good for clarity to avoid unnecessary split of empty string.

Isn't it allowed to read from a nil map? Only writes will cause panic, right?

getMachinesMarkedByCAForDeletion->getMachineNamesMarkedByCAForDeletion

elankath · 2024-12-26T06:41:52Z

cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go

+	// ignore the machine deployment if it is in rolling update
+	if !isRollingUpdateFinished(mcd) {
+		klog.Infof("machine deployment %s is under rolling update, skipping", machineDeployment.Name)
+		return nil


Shouldn't we return an error here ? We are doing it for other cases of !isRollingUpdateFinished.

I think we should remove this check. Even if a machineDeployment is under rolling update, we should allow the annotation update if needed. wdyt?

elankath · 2024-12-26T06:52:30Z

cluster-autoscaler/cloudprovider/mcm/mcm_manager.go

+// addNodeGroup adds node group defined in string spec. Format:
+// minNodes:maxNodes:namespace.machineDeploymentName
+func (m *McmManager) addNodeGroup(spec string) error {
+	machineDeployment, err := buildMachineDeploymentFromSpec(spec, m)


buildMachineDeploymentFromSpec should also be moved to mcm_manager.go.

elankath · 2024-12-26T06:59:10Z

cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go

+func (machineDeployment *MachineDeployment) Refresh() error {
+	machineDeployment.scalingMutex.Lock()
+	defer machineDeployment.scalingMutex.Unlock()
+	mcd, err := machineDeployment.mcmManager.machineDeploymentLister.MachineDeployments(machineDeployment.Namespace).Get(machineDeployment.Name)


This is repeated nearly a dozen times everywhere including common error handling: machineDeployment.mcmManager.machineDeploymentLister.MachineDeployments(machineDeployment.Namespace).Get(machineDeployment.Name) . Move to a method in mcmManager called GetMachineDeploymentResource which returns a formatted error that can simply be returned, so that error message is fully consistent with all uses. we are already having methods like mcmManager.getMachinesForMachineDeployment so this matches the existing convention.

elankath · 2024-12-26T07:04:50Z

cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go

+		if machine.Status.CurrentStatus.Phase == v1alpha1.MachineTerminating || machine.Status.CurrentStatus.Phase == v1alpha1.MachineFailed {
+			continue
+		}
+		if annotValue, ok := machine.Annotations[machinePriorityAnnotation]; ok && annotValue == priorityValueForCandidateMachines && !markedMachines.Has(machine.Name) {


Out of curiosity, Why is this called priorityValueForCandidateMachines ? Shouldn't it be defined as const PriorityDeletionValueForCandidateMachine = 1 ?

I think we can rename it to PriorityValueForDeletionCandidateMachine. wdyt?

we should also add a comment here to explain what we are doing so next guy making a patch doesn't scratch his head.

elankath · 2024-12-26T07:15:40Z

cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go

+		}
+	}
+	clone := mcd.DeepCopy()
+	clone.Annotations[machinesMarkedByCAForDeletion] = strings.Join(updatedMarkedMachines, ",")


This strings.Join to construct the annotation repeated elsewhere. Make a utility method called getMarkedForDeletionAnnotationValue(machineNames []string) string

elankath · 2024-12-26T07:20:01Z

cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go

+		klog.Errorf("[Refresh] failed to get machines for machine deployment %s, hence skipping it. Err: %v", machineDeployment.Name, err.Error())
+		return err
+	}
+	var incorrectlyMarkedMachines []*types.NamespacedName


minor (need not correct): types.NamespacedName is a simple struct - a value object. Modern programming practices discourages use of pointers to value objects for keeping data inline for processor cache performance. Pointers should be used only for structs holding active resource (like locks/files/etc) or class/service objects. Using a simple []types.NamespacedName and using types.NamespacedName instead of *types.NamespacedName should be followed for newer code.

elankath · 2024-12-26T07:26:32Z

cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go

@@ -539,12 +567,17 @@ func buildMachineDeploymentFromSpec(value string, mcmManager *McmManager) (*Mach
 	return machinedeployment, nil
 }

+func getMachinesMarkedByCAForDeletion(mcd *v1alpha1.MachineDeployment) sets.Set[string] {


Name function to getMachineNamesMarkedByCAForDeletion

elankath · 2024-12-26T07:26:51Z

cluster-autoscaler/cloudprovider/mcm/mcm_manager.go

@@ -508,27 +474,32 @@ func (m *McmManager) DeleteMachines(targetMachineRefs []*Ref) error {
 	if !isRollingUpdateFinished(md) {
 		return fmt.Errorf("MachineDeployment %s is under rolling update , cannot reduce replica count", commonMachineDeployment.Name)
 	}
+	markedMachines := getMachinesMarkedByCAForDeletion(md)


markedMachines -> machineNamesMarkedByCA

elankath · 2025-01-17T08:26:58Z

UPDATE: Testing has shown that the current fix in this PR has a tendency to slow down the CA scaling due to the all the NodeGroup mutexes acquired in CloudProvider.Refresh. We are looking whether this is acceptable or whether we can improve this.

elankath · 2025-01-23T10:51:21Z

Testing has revealed that the current fix unfortunately does not address all data races and also slows down the performance due to the multiple NodeGroup locks needed in the CloudProvider.Refresh.

It has been determined we cannot achieve this as a CA-cloudprovider isolated fix and we need to make MCM changes too to ensure that scaledown works correctly even in cases where CA issues concurrent scaledowns for a NodeGroup and changes its mind later about which node to scale down in a future iteration.

rishabh-11 added 4 commits December 19, 2024 14:59

[WIP] fix CA marking machines for deletion

43cffaf

[WIP] add mutex for machine deployment

0d939b0

initialise machinedeployment map in mcmManager

98f20d3

add Refresh method in nodegrp implementation

56d80ac

gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Dec 20, 2024

rishabh-11 marked this pull request as ready for review December 24, 2024 08:31

rishabh-11 requested review from unmarshall and a team as code owners December 24, 2024 08:31

elankath reviewed Dec 24, 2024

View reviewed changes

address review comments

f3774f4

gardener-robot added size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Dec 24, 2024

rishabh-11 requested a review from elankath December 24, 2024 12:23

gardener-robot removed the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Dec 24, 2024

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 24, 2024

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 24, 2024

elankath reviewed Dec 26, 2024

View reviewed changes

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 16, 2025

fixed broken test after refactor

c515f39

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 16, 2025

fixed broken test

063f07a

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 16, 2025

alternate solution using single annotation for deletion by CA

945c74c

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 28, 2025

fixed use of Go 1.23 functions

b021a3c

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 28, 2025

fixed test

3b02417

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 28, 2025

added godoc for AcquireScalingMutex

22ced54

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 28, 2025

correct godoc for machinesMarkedByCAForDeletionAnnotation

bc85986

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 29, 2025

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 29, 2025

changed to machineutils.TriggerDeletionByMCM

4d33bef

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 29, 2025

removed var shadowing, added TODO to godoc for util fns

6ea649b

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Race conditions in Targeted Deletion of machines by CA #341

Fix Race conditions in Targeted Deletion of machines by CA #341

rishabh-11 commented Dec 20, 2024 •

edited by elankath

Loading

elankath Dec 24, 2024

elankath Dec 24, 2024

elankath Dec 26, 2024 •

edited

Loading

rishabh-11 Dec 26, 2024

elankath Dec 26, 2024

elankath Dec 26, 2024

rishabh-11 Dec 26, 2024

elankath Dec 26, 2024

elankath Dec 26, 2024 •

edited

Loading

elankath Dec 26, 2024

rishabh-11 Dec 26, 2024

elankath Dec 26, 2024

elankath Dec 26, 2024

elankath Dec 26, 2024

elankath Dec 26, 2024

elankath Dec 26, 2024 •

edited

Loading

elankath commented Jan 17, 2025 •

edited

Loading

elankath commented Jan 23, 2025 •

edited

Loading

Fix Race conditions in Targeted Deletion of machines by CA #341

Are you sure you want to change the base?

Fix Race conditions in Targeted Deletion of machines by CA #341

Conversation

rishabh-11 commented Dec 20, 2024 • edited by elankath Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elankath Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elankath Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elankath Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

elankath commented Jan 17, 2025 • edited Loading

elankath commented Jan 23, 2025 • edited Loading

rishabh-11 commented Dec 20, 2024 •

edited by elankath

Loading

elankath Dec 26, 2024 •

edited

Loading

elankath Dec 26, 2024 •

edited

Loading

elankath Dec 26, 2024 •

edited

Loading

elankath commented Jan 17, 2025 •

edited

Loading

elankath commented Jan 23, 2025 •

edited

Loading