Inhibit scale-down by autoscaler during roll-outs.

[hardikdr]

What this PR does / why we need it: With this PR, during roll out of the machine-deployment, all the machines under it are annotated with cluster-autoscaler.kubernetes.io/scale-down-disabled: "True".

This will help us keeping the cluster-autoscaler running even during the cluster-rollouts. CA will not scale-down a specific machine-deployment which is being rolled, but will be able to scale-down the rest of them. Also, CA will be able to scale-up all the machine-deployments during roll-outs.

There are mainly 2 parts to the PR:

1. Adding the annotation during roll-out.

   • To determine if the roll-out if on-going we check if there are old-machine-sets that are not scaled-to-zero completely. We check the up-to-date, and ready replicas as well here.

2. Removing the annotation after the roll-out.

   • Keeping the controller nature in mind, we need a way to differentiate between 2 possibilities:
     • Cluster rollout just got over, MachineDeployment is complete[all machine-replicas ready] and we should remove the annotation.
     • There have been no roll-outs recently, MachineDeployment is complete and we shouldn't remove the annotation. Looking at older machine-set doesn't help as I see them retained after roll-outs.
   • If we don't consider the possibilities above, we may end up removing the same ca-annotation which might have been added by the user via NodeTemplate.
   • I have mitigated the situation, by introducing the by-mcm annotation, which is added on the node along with the original annotation. This place-holder-annotation helps in deciding if the original annotation is added by mcm or by the user. This helps in avoiding potential race-condition with nodeTemplate logic.
   • Open for further suggestions or alternatives.

Unfortunately, we cant use the nodeTemplate feature for adding/removing the annotation:

1. If we set the annotation on the MachineSet's nodeTemplate, machine-deployment overwrites it back.
   • If we ignore the specific annotation in code, user-provided CA-annotation might also be overwritten.
2. If we set the annotation on the MachineDeployment's nodeTemplate, worker-controller will eventually overwrite it to the original state.

Which issue(s) this PR fixes:
Fixes https://github.com/gardener/autoscaler/issues/48
FIxes #472

Special notes for your reviewer:
We need to consider the possibility that users might also be using the same annotation for their use cases. We should make sure, autoscaler annotations from users are maintained.

Release note:

All nodes under machine deployments being rolled-out are annotated with `cluster-autoscaler.kubernetes.io/scale-down-disabled: "True"` during the period of rolling-update.

A new command line flag `autoscaler-scaldown-annotation-during-rollout` is introduced to disable annotating the nodes with cluster-autoscaler annotation `cluster-autoscaler.kubernetes.io/scale-down-disabled` during rollout.

[vlerenc]

I have mitigated the situation, by introducing the by-mcm annotation, which is added on the node along with the original annotation. This place-holder-annotation helps in deciding if the original annotation is added by mcm or by the user. This helps in avoiding potential race-condition with nodeTemplate logic.
Open for further suggestions or alternatives.

Not really, nothing more elegant. That would have been also my approach/was also what I was thinking when I heard/read the above.

[amshuman-kr]

@hardikdr Thanks for the PR. Just a couple of questions/comments. Otherwise looks good, apart from tests.

[hardikdr]

Thanks for the reviews.
I'll rebase and add unit tests soon.

[amshuman-kr]

One more question. The PR covers only the RollingUpdate strategy. Wouldn't the feature be complete if we cover the Recreate strategy too?

[hardikdr]

The PR covers only the RollingUpdate strategy. Wouldn't the feature be complete if we cover the Recreate strategy too?

Recreate strategy simply deletes all the existing machines and creates the new ones altogether. That is already super disruptive, not sure how autoscaler could harm it any further.

[amshuman-kr]

Recreate strategy simply deletes all the existing machines and creates the new ones altogether. That is already super disruptive, not sure how autoscaler could harm it any further.

What if CA scales down/up while the new machines are still joining? Isn't it better to cover all bases?

[hardikdr]

What if CA scales down/up while the new machines are still joining? Isn't it better to cover all bases?

I spent a bit more time and experimented. Basically, during Recreate the old machine-set is completely brought down and made sure no old machines are running. Once there are no old machines running, the new machine set is brought up to the full. A couple of points to ponder upon:

1. It is hard to differentiate between the following cases:
   • Scale-up of machineset due to usual-scale-up of machine-deployment
   • Scale-up of machineset due to recreate-update flow.

• It's certainly possible to make it work, just that it'll demand more changes in the code as it seems.

2. CA scales down a node only if the node is registered and under-utilized for a certain period of time. So if few nodes have joined the cluster and not taking any Pending workload for a certain timeout period during recreate-update, I believe CA will anyways remove it immediately after roll-out.

3. Scale-up is anyways allowed in both strategies.

Overall, Recreate doesn't seem to be a production-friendly feature. If an application can survive literally no-machines in the cluster/worker-group, we should probably not invest too much to avoid minor scale down in between.

• Just to add, this is not being exposed/used by the gardener, also not planned afaik.

In summary, I felt it definitely sounds sensible to cover all the strategies for the feature from bird's eye view, but there are more details. But if we see the value, I am more than happy to somehow make it work. :)

[prashanth26]

Minor typo.
/lgtm otherwise

Also ditto comment on squashing commits.

[hardikdr]

@amshuman-kr @ggaurav10 Can you please take a final look and approve if possible?

[prashanth26]

/ping

[gardener-robot]

@prashanth26

Message

/ping

[ggaurav10]

lgtm 👍