Cluster-api-provider-aws: AWSMachinePool does not work in combination with cluster-autoscaler

[dthorsen]

/kind bug

What steps did you take and what happened:

• Create a workload cluster with the experimental EKS Control Plane
• Create a MachinePool without specifying replicas and create associated AWSMachinePool resources.
• Install cluster-autoscaler and configure it to autoscale the autoscaling group managed by the AWSMachinePool
• Create a deployment that will trigger cluster-autoscaler to scale up the autoscaling group.

This caused the AWSMachinePool controller and the cluster-autoscaler to both attempt to manage DesiredInstances. The result being the DesiredInstances value on the ASG was alternating between the value cluster-autoscaler computed as needed and the value of replicas: 1 coming from the defaulted value on the MachinePool object.

What did you expect to happen:
cluster-autoscaler should manage the DesiredInstances of the autoscaling group and the AWSMachinePool controller should ignore changes to the replicas and DesiredInstances fields.

Anything else you would like to add:
Even though replicas is an optional field on the MachinePool resource, it defaults to 1, so leaving the value unspecified is insufficient to prevent the AWSMachinePool controller from attempting to managed the DesiredInstance count.

Environment:

• Cluster-api-provider-aws version: Commit: 3338cd4
• Kubernetes version: (use kubectl version): v.1.17.9
• OS (e.g. from /etc/os-release): Amazon Linux 2

[randomvariable]

I'm not sure this is a bug actually, there's going to be a clash of behaviour here.

I think you'll want to use the Cluster API native mode of Cluster Autoscaler? @benmoss is more knowledgeable in this area.

There's probably outstanding tasks we need to have in terms of reaching functional parity. In the meantime, at worst, possibly, and I'm loathe to do it, add an immediately and a deliberately deprecated field that is "disableReplicaCountReconciliation"

[dthorsen]

@randomvariable There is indeed a clash of behavior. I listed it as a bug because it failed to operate in the manner we originally designed due to the fact that the MachinePool replicas can't really be nil. The original thought being, if you don't set replicas at all, then CAPA won't manage replicas/DesiredInstances. If you want CAPA to manage them, then you set replicas. That seemed like a sensible interface.

At the moment there are no Machine or AWSMachine objects created from a MachinePool. Only Node objects. For the time being for our use cases, we planned to use CAPA to create the ASG's and let cluster-autoscaler manage scaling. It enables us to take advantage of cluster-autoscaler's full feature set today including things like scale to and from 0. I think a field in the spec would be a good idea to make this behavior configurable for the user. I am not certain it should be a deprecated field. It is hard to imagine all the possible use cases around autoscaling groups and where the replica count may be best controlled. I have been told also that cluster-autoscaler is frequently forked and customized for special use cases. I think flexibility in the interface could be valuable for the foreseeable future.

[randomvariable]

Interesting data point for @elmiko too here.

[elmiko]

thanks for the ping @randomvariable , an interesting issue for sure.

i agree with @randomvariable , there is a clash happening between the MachinePool and the way the cluster-autoscaler is attempting to scale. i don't know the MachinePool mechanics very well, but from the cluster-autoscaler side the capi provider attempts to manipulate the MachineSet and MachineDeployment replicas when it wants to scale. if these replica counts are disjointed from the MachinePool then i could envision some unexpected behavior.

edit:

if the cluster-autoscaler is being configured to use the aws provider, then there might be some issues between what the capi machinery is doing and how the cluster-autoscaler is attempting to scale. if this is the case, then i would expect the issue to be more on the capi/capa side of things.

[dthorsen]

That is correct @elmiko, this behavior is observed when operating cluster-autoscaler with the AWS provider, not the CAPI provider. In our local installation we have worked around this for now by simply telling CAPA not to react to replicas as long as replicas is set to 1. This allows cluster-autoscaler to work, so if we had a field in the AWSMachinePool spec to more properly ignore replicas, that would allow cluster-autoscaler with AWS provider to just work as it would with any other ASG.

[elmiko]

that makes sense @dthorsen, seems like this is really a feature request to add something that will ignore the replicas in this type of situation.

[benmoss]

I don't really understand this, why does the AWSMachinePool controller keep changing DesiredInstances? It's not clear this is an autoscaler bug, seems more appropriate to file it against CAPA.

[dthorsen]

@benmoss This bug is filed against CAPA, not cluster-autoscaler.

@elmiko I am OK with changing it to a CAPA feature request instead of a CAPA bug if that is everyone's preference based on the context provided. I only listed it as bug because it did not behave as we initially designed it.

[dthorsen]

/kind feature

[dthorsen]

/remove-kind bug

[benmoss]

Hah, my mistake 😸

[dthorsen]

/assign

[dthorsen]

I propose that we add an optional field to the AWSMachinePool spec called ignoreReplicas which defaults to false. The reconciliation logic will use this field to ignore the MachinePool replicas field and to always omit DesiredInstances field in any requests to the AWS AutoScalingGroup API.

@MarcusNoble @randomvariable Does this sound like an acceptable solution?

[MarcusNoble]

Sounds good to me. Just not 100% sure on the naming. replicas makes me think about kubernetes resources rather than ASG instances. How about something like ignoreDesiredSize ?

[dthorsen]

@MarcusNoble Hmm, technically we are ignoring both when this is set. From cluster-api user's perspective, if they update replicas on the MachinePool Kubernetes resource, the controller will do nothing if this field is set.

[MarcusNoble]

That is a good point. The name makes sense then.

The alternative is to handle an unset replicas value as meaning ignore, rather than setting a new property.

Edit: looking at the code that might actually be what's currently happening if I'm following correctly. If no replicas is set on the MachinePool then it looks like it'll default to MinSize (as that's what AWS does) and never get updated by CAPA.

https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/e526925b25ff006774885d129a16de6da504f196/pkg/cloud/services/autoscaling/autoscalinggroup.go#L163-L165

https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/e526925b25ff006774885d129a16de6da504f196/pkg/cloud/services/autoscaling/autoscalinggroup.go#L208-L210

https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/e526925b25ff006774885d129a16de6da504f196/pkg/cloud/services/autoscaling/autoscalinggroup.go#L280-L282

[dthorsen]

That was the original intent, but the code doesn't work as intended because replicas will never be nil (also incidentally why I originally opened this issue as a bug). It will be defaulted to a value of 1 if omitted. It also occurred to me that using replicas to indicate "don't reconcile this value" is a pretty fragile indicator. If cluster-autoscaler has scaled up the pool to say, 100 instances, and someone comes back and misunderstands how it works and sets replicas to 120, AWSMachinePool will suddenly begin reconciling replicas and scale up to 120 instances. Then cluster-autoscaler will try to scale down, and we are back into a thrashing scenario. I think based on this feedback I am going to start on a PR to implement a proper ignoreReplicas spec field.