fix: ensure Machines with delete-machine annotation are deleted first

[mweibel]

What type of PR is this?
/kind bug

What this PR does / why we need it:
As outlined in the linked issue: when a Machine has the delete annotation, CAPZ needs to prioritize those Machines or AzureMachinePoolMachines for downscaling. This is achieved by fetching the DeleteMachine annotation from the owner Machine before starting to look at scaling decisions.

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

Special notes for your reviewer:

• cherry-pick candidate

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

ensure Machines with delete-machine annotation are deleted first

[k8s-ci-robot]

Hi @mweibel. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[jackfrancis]

/ok-to-test

[codecov]

Codecov Report

Attention: Patch coverage is 84.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 51.17%. Comparing base (711d861) to head (0b71e08).
Report is 53 commits behind head on main.

Files with missing lines	Patch %	Lines
azure/scope/machinepool.go	71.42%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4949      +/-   ##
==========================================
+ Coverage   51.14%   51.17%   +0.02%     
==========================================
  Files         274      274              
  Lines       24669    24690      +21     
==========================================
+ Hits        12617    12634      +17     
- Misses      11264    11267       +3     
- Partials      788      789       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Jont828]

Thanks for looking into this. The logic for sorting the delete priority is actually here, and it's worth noting that it will prioritize deleting failed Machines or Machines without the latest model over the delete annotation. Here's the part that sorts the AzureMachines as well if that's helpful.

[nawazkh]

Thanks for looking into this. The logic for sorting the delete priority is actually here, and it's worth noting that it will prioritize deleting failed Machines or Machines without the latest model over the delete annotation. Here's the part that sorts the AzureMachines as well if that's helpful.

Looks good to me overall, but I am curious to know if your approach changes with Jonathan's comment👆🏼 , @mweibel

[mweibel]

Thanks for the reviews!

Thanks for looking into this. The logic for sorting the delete priority is actually here, and it's worth noting that it will prioritize deleting failed Machines or Machines without the latest model over the delete annotation. Here's the part that sorts the AzureMachines as well if that's helpful.

That's a very good question.

prioritize failed machines: That's debatable. It can make sense, however e.g. in the case of windows nodes we sometimes have temporarily failed machines if they reboot for some reason but they come back online. Maybe prioritize marked as delete machines would make more sense.

prioritize machines without the latest model: This is another question. In our use case it's even debatable if we want to delete those at all, because we run batch workloads (each on their own machine). This means that machines without the latest model aren't bothering us and we'd rather keep them online. That sounds like a separate issue which needs consideration, though.

I'd happily adjust the code to prioritize machines with the delete annotations - what are your opinions about this, given the additional context I provided?

[mweibel]

about my previous comment:

This means that machines without the latest model aren't bothering us and we'd rather keep them online.

Just figured out that it actually does bother us, we're currently facing an issue with machines not getting updated and the VMSS rejecting to scale because we have too many VM models at the same time (10 is the limit apparently). Will file a separate issue for that .

[mweibel]

Thanks for looking into this. The logic for sorting the delete priority is actually here, and it's worth noting that it will prioritize deleting failed Machines or Machines without the latest model over the delete annotation. Here's the part that sorts the AzureMachines as well if that's helpful.

reading the code again, I believe this is not what the code does, or do I misunderstand something?

cluster-api-provider-azure/azure/scope/strategies/machinepool_deployments/machinepool_deployment_strategy.go

Lines 146 to 151 in 52df930

	// Order AzureMachinePoolMachines with the clutserv1.DeleteMachineAnnotation to the front so that they have delete priority.
	// This allows MachinePool Machines to work with the autoscaler.
	failedMachines = orderByDeleteMachineAnnotation(failedMachines)
	deletingMachines = orderByDeleteMachineAnnotation(deletingMachines)
	readyMachines = orderByDeleteMachineAnnotation(readyMachines)
	machinesWithoutLatestModel = orderByDeleteMachineAnnotation(machinesWithoutLatestModel)

This code ensures that for each of those machine groups (failed, deleting, ready, without latest model), it'll prioritize those with the delete machine annotation over not annotated ones. This is because each machine group is sorted using orderByDeleteMachineAnnotation:

cluster-api-provider-azure/azure/scope/strategies/machinepool_deployments/machinepool_deployment_strategy.go

Lines 307 to 317 in 52df930

	// orderByDeleteMachineAnnotation will sort AzureMachinePoolMachines with the clusterv1.DeleteMachineAnnotation to the front of the list.
	// It will preserve the existing order of the list otherwise so that it respects the existing delete priority otherwise.
	func orderByDeleteMachineAnnotation(machines []infrav1exp.AzureMachinePoolMachine) []infrav1exp.AzureMachinePoolMachine {
	sort.SliceStable(machines, func(i, j int) bool {
	_, iHasAnnotation := machines[i].Annotations[clusterv1.DeleteMachineAnnotation]
	return iHasAnnotation
	})
	return machines
	}

Though indeed, we could think about adjusting this code to first delete machines with the annotations, regardless of the group, and then only afterwards potentially delete other machines (maybe in the next reconcile iteration only).

Would that be what we want? I think that would make sense.

[mweibel]

/retest

./scripts/../hack/../hack/ensure-azcli.sh: line 28: AZURE_CLIENT_SECRET: unbound variable is not related to this change.

[mweibel]

I assume this needs #4939 to be merged to make all tests work.

[nawazkh]

Though indeed, we could think about adjusting this code to first delete machines with the annotations, regardless of the group, and then only afterwards potentially delete other machines (maybe in the next reconcile iteration only).

Would that be what we want? I think that would make sense.

Coming to this thought; it makes sense to me that "delete annotations" should be respected over cleanup. I vote for that change. Will you be adding that change via this PR?

[nawazkh]

I assume this needs #4939 to be merged to make all tests work.

We are migrating CAPZ tests onto community clusters, and as a part of that effort pull-cluster-api-provider-azure-e2e-with-wi-optional was created to test the migration. Please disregard any failures of pull-cluster-api-provider-azure-e2e-with-wi-optional for the time being. I will also put out a post on the community channel to conveying the same.

[nawazkh]

I assume this needs #4939 to be merged to make all tests work.

We are migrating CAPZ tests onto community clusters, and as a part of that effort pull-cluster-api-provider-azure-e2e-with-wi-optional was created to test the migration. Please disregard any failures of pull-cluster-api-provider-azure-e2e-with-wi-optional for the time being. I will also put out a post on the community channel to conveying the same.

Created a PR kubernetes/test-infra#32926 to remove pull-cluster-api-provider-azure-e2e-with-wi-optional from automatically running on CAPZ PRs.

[nawazkh]

/test pull-cluster-api-provider-azure-e2e-aks

[k8s-ci-robot]

@mweibel: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-e2e-with-wi-optional	eb033f2	link	false	/test pull-cluster-api-provider-azure-e2e-with-wi-optional

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[mweibel]

I wasn't sure if we should include the annotated machines in this condition (instead of making a separate one).

I decided against this approach because it would require a bit more code (filter out duplicates) and the benefit didn't seem huge - in the next reconciliation the failed/deleting machines would be sorted out, too.

Please let me know what you think about this, thanks!

[mweibel]

@nawazkh @Jont828 Please review again.

I updated the implementation to always prefer delete machine annotated machines and removes the ordering of the other kind of machines.

Some e2e tests fail currently but to me it looks like they're a bit flaky today. I'll wait until you reviewed before I retest and potentially investigate deeper into if my changes could have an impact on e2e tests.

[Jont828]

Suggested change

	// if we have machines annotated with delete machine, remove them
	// If we have machines with the delete machine annotation, remove them.

[Jont828]

@nawazkh @Jont828 Please review again.

I updated the implementation to always prefer delete machine annotated machines and removes the ordering of the other kind of machines.

Some e2e tests fail currently but to me it looks like they're a bit flaky today. I'll wait until you reviewed before I retest and potentially investigate deeper into if my changes could have an impact on e2e tests.

Overall I think the PR looks solid to me, I think the only thing would be how to prioritize machines with the annotation vs failed/outdated machines. I remember when I implemented this, we had some discussion about whether to prefer failed/outdated over the annotated machines and it wasn't super clear which course was best. I ended up deleting outdated machines first to be consistent with the DockerMachinePool implementation, and to ensure that we don't get in a situation where we are unable to meet the ready replica count b/c a failed or outdated machine can't get deleted.

If I understand correctly, does your use case benefit from giving priority to annotated machines instead?

[nawazkh]

/retest

[mweibel]

If I understand correctly, does your use case benefit from giving priority to annotated machines instead?

the code as is in this PR would give priority to those, yes.

I'm not certain about this either. I guess there are three ways to handle it:

• delete annotated machines first, then annotated (next reconcile)
• delete failing/deleting machines first, then annotated (next reconcile)
• merge those three lists together (making them unique)

Do you have any preference regarding which path to take?

[mweibel]

@nawazkh @Jont828 after testing this change in production under high load (>800 windows machines), I noticed an issue which is related to the discussion/open question. When there are too many VMSS VMs in Failed state, the VMSS is marked as Unhealthy, preventing further scale up/down.

Therefore, I adjusted this PR to first delete failed/deleting machines (sorted by delete annotation) and only afterwards delete those with the delete machine annotation.

[nawazkh]

Looks good to me, thank you for putting this together.
Will wait for @Jont828 inputs.
/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 8d4532c838a358d39fbb0cc38faf598e94f3c2f7

[nojnhuh]

Looks good to me, thank you for putting this together. Will wait for @Jont828 inputs. /lgtm

/assign @Jont828

[Jont828]

@nawazkh @Jont828 after testing this change in production under high load (>800 windows machines), I noticed an issue which is related to the discussion/open question. When there are too many VMSS VMs in Failed state, the VMSS is marked as Unhealthy, preventing further scale up/down.

Therefore, I adjusted this PR to first delete failed/deleting machines (sorted by delete annotation) and only afterwards delete those with the delete machine annotation.

Thanks for your hard work! And yep that's why I originally had it to prioritize deleting failing machines over the annotation. We want to make sure we can get out of a unhealthy state before worrying about autoscaling behavior.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jont828

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Jont828]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment