Use annotations to set labels and taints for clusterapi nodegroups

[cnmcavoy]

Which component this PR applies to?

cluster-autoscaler

What type of PR is this?

/kind feature

What this PR does / why we need it:

Updates the clusterapi provider to use label or taint capacity annotations on MachineDeployments to supply information about the nodegroup shape when the nodegroup replicas are scaled to zero.

Currently, the clusterapi provider does not support nodegroup shapes with labels or taints and scaling up from zero replicas, which can cause nodegroups to be skipped in evaluation and fail to scale up. This scenario occurs when a pod is created that tolerates the nodegroup's taints and has a node selector that can only be satisfied by those nodegroups. The cluster-autoscaler needs the taint and label information about a nodegroup to correctly match such a pod to the nodegroup.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added support for supplying labels and taints when scaling to and from zero nodes for the cluster autoscaler's Cluster API provider. Enabling this feature will require changes by the user, for instruction please see the Cluster API (clusterapi) provider README file in the autoscaler repository.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Welcome @cnmcavoy!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[elmiko]

thanks @cnmcavoy , i think this is a novel and simple solution to the issue. i'd like to get a wider consensus from the cluster-api community about adding these annotations. assuming the community agrees, i don't see any reason why we can't use these.

related to kubernetes-sigs/cluster-api#7685

i'm putting a hold here just to ensure we get wider discussion before merging
/hold for wider cluster api community review

[elmiko]

code generally looks good to me, thanks for adding the unit test =)

[sbueringer]

The idea looks good to me as well (didn't review the implementation).

As noted in kubernetes-sigs/cluster-api#7685 (comment) with an upcoming ClusterAPI release there should be a way to get the labels from MD/MS, but those might be incomplete (as the bootstrap provider can add more labels) and that doesn't cover taints.

Even if we solve this in Cluster API eventually for labels and taints and including bootstrap providers I think it's fine to support this with autoscaler-specific annotations today. We can still deprecate and remove them eventually once a solution in Cluster API has been implemented.

Also the annotation approach will be compatible with all Cluster API versions already.

[elmiko]

Even if we solve this in Cluster API eventually for labels and taints and including bootstrap providers I think it's fine to support this with autoscaler-specific annotations today. We can still deprecate and remove them eventually once a solution in Cluster API has been implemented.

Also the annotation approach will be compatible with all Cluster API versions already.

+1, given that we already have the annotations as a way to override the values from the capi objects, i think having these label/taint annotations is a great addition.

@cnmcavoy i am going to bring this PR up as a topic at tomorrow's cluster api meeting, if there are no objections to the approach here i will remove the hold.

[fabriziopandini]

I'm wondering if we can avoid asking the users to specify the list of labels by inferring if from the MachineDeployment itself.

According to what is defined in https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20220927-label-sync-between-machine-and-nodes.md and in https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20221003-In-place-propagation-of-Kubernetes-objects-only-changes.md

• Labels that will be propagated to nodes derive from labels defined in MachineDeployment.spec.template.metadata.labels
• Among those labels only the one node-role.kubernetes.io/* label, node-restriction.kubernetes.io/ domain, node.cluster.x-k8s.io domain are going to be propagated

(NOTE: this will apply to CAPI >=1.4 if we manage to complete the design proposal implementation during the current release cycle)
(NOTE: this is only the subset of labels that CAPI will apply, it doesn't include labels Kubelet adds automatically)

[elmiko]

I'm wondering if we can avoid asking the users to specify the list of labels by inferring if from the MachineDeployment itself.

i think we definitely want to do this once that implementation is released, but i think we could still keep these annotations as a continuation of the overrides we currently have (eg for cpu, memory, etc).

so, ultimately, the cluster autoscaler would inspect the labels from MachineDeployment.spec.template.metadata.labels, and use some of those labels (based on the rules in the enhancement), and then inspect the annotations on the machinedeployment to see if any labels should be overridden.

[elmiko]

@fabriziopandini just a follow up here, is there an objection to adding these override annotations in addition to whatever we do with reading the labels from the MachineDeployment?

[elmiko]

@cnmcavoy just as a followup, we discussed this PR at the meeting today (recording), there were no major objections, but some questions about interactions with cluster api controllers were brought up. i'd like to give folks a few days to discuss here, and then if there are no objections we could move forward with this PR.

as a followup, i would like to make sure that we update the autoscaling from zero enhancement to include the changes here. i am happy to help update that document.

[x13n]

I'm looking at unassigned PRs, looks like this is one is already taken care of:

/assign @elmiko

[elmiko]

i'm removing the hold here, i think the capi community has discussed this and there were no objections to moving forward on this.

/hold cancel

[elmiko]

i've created kubernetes-sigs/cluster-api#8036 to cover the docs update in cluster-api

[elmiko]

@cnmcavoy thanks for the update, just waiting to make sure there are no objections before merging this. i don't think there will be but i want to make sure.

[cnmcavoy]

Any updates on this @elmiko? We've been using this in our clusters since January and would love to get off our fork.

[elmiko]

@cnmcavoy apologies for the delay, i'll poke @fabriziopandini to see if he had any further requests here

[elmiko]

discussed in the meeting today, no objections to merging. thanks again @cnmcavoy !

/approve
/lgtm