feat: respect externally managed annotation on unmanaged MachinePools

[mweibel]

What type of PR is this?
/kind feature

What this PR does / why we need it:
This adds support for marking MachinePool resources as externally managed. Similar to #2444 but for unmanaged clusters. It does not rely on a property in AzureMachinePool since, unlike in AzureManagedMachinePool`, this property does not exist.

Special notes for your reviewer:
Please note that this is just a Draft PR in order to get some feedback on the approach. I reused code and methodology from #2444 in the hope that it matches the existing code style.
I did not yet add tests since I just added the code and I'm trying it on a local cluster.

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

adds the ability to annotate a MachinePool with `cluster.x-k8s.io/replicas-managed-by-autoscaler` to synchronize VMSS capacity with MachinePool replicas automatically.

[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/test-infra repository.

[CecileRobertMichon]

#2444 got merged so you should be able to reuse the code from it now if you rebase

See also kubernetes-sigs/cluster-api#7107 which is relevant / in progress

[CecileRobertMichon]

/ok-to-test

[mweibel]

See also kubernetes-sigs/cluster-api#7107 which is relevant / in progress

yes I saw that, thanks. Once that PR is merged and released, we can adjust CAPZ code to use the const from CAPI.

[mweibel]

I tested using a separate empty commit and the tests ran through. I squashed them together and now there's again a conflict in the e2e test.

What can I do about this?

[CecileRobertMichon]

/retest

the failure above looks like a flake

[CecileRobertMichon]

/cc @jackfrancis

[jackfrancis]

@mweibel this is an interesting use-case. So the idea is that you might want to use cluster-autoscaler on your cluster, but not leverage the Cluster API provider, and instead use the Azure provider (which updates replicas directly via the Azure VMSS API)?

Or are there other use cases you're thinking of?

I'm wondering if there's a better, general workflow to indicate this rather than requiring the user to manually add a well-known annotation to the corresponding capi MachinePool resource?

[CecileRobertMichon]

/lifecyle active

[mweibel]

@mweibel this is an interesting use-case. So the idea is that you might want to use cluster-autoscaler on your cluster, but not leverage the Cluster API provider, and instead use the Azure provider (which updates replicas directly via the Azure VMSS API)?

yes exactly. I contributed the same to CAPA already. The use case we have currently is that the cluster-api autoscaler does not yet have enough functionality to replace the native azure one. Crucial features like taints/labels and separate cool down times etc. are not available yet and will probably not for a little while (especially the taints since that's onlyin a proposal how to manage it in CAPI itself).

I'm wondering if there's a better, general workflow to indicate this rather than requiring the user to manually add a well-known annotation to the corresponding capi MachinePool resource?

I settled for an annotation since there was discussion on CAPA about this (annotations vs. a separate field) and they preferred an annotation. It also kind of fits together with the changes in kubernetes-sigs/cluster-api#7107.

[CecileRobertMichon]

/milestone v1.7

[jackfrancis]

/assign

[jackfrancis]

/assign @dthorsen

[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-exp	46e860c	link	false	/test pull-cluster-api-provider-azure-e2e-exp

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/test-infra repository. I understand the commands that are listed here.

[jackfrancis]

@mweibel this lgtm, I think we need to rebase this PR on top of latest main to overcome the E2E flake

Thanks again for this, let's try to land this before mid-next week so it can be included in the 1.7. release!

[CecileRobertMichon]

@jackfrancis do we need a PR to get rid of this CAPZ specific annotation in favor of the CAPI one that you added?

kubernetes-sigs/cluster-api#7107

[CecileRobertMichon]

we should even be able to get rid of this function altogether and directly use the ReplicasManagedByExternalAutoscaler() helper from CAPI in ReconcileReplicas below

[jackfrancis]

For this PR, yes, replacing that func w/ the CAPI one should work. For AzureManaged, we'll need to incorporate some back-compat w/ the capz annotation (I can do that as a separate PR).

[CecileRobertMichon]

I think we need to rebase this PR on top of latest main to overcome the E2E flake

Prow does the rebase when running tests

/retest

[CecileRobertMichon]

as discussed in https://kubernetes.slack.com/archives/CEX9HENG7/p1673034488923059, let's merge this and refactor to use the CAPI helper function in a separate PR: #2993

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 [CecileRobertMichon]

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