🐛 Update ASG - do not set desired value for machinepool which have externally managed replicas

[calvix]

/kind bug

What this PR does / why we need it:
For machine pools that have externally managed replicas (for example by cluster-autoscaler) the cluster-api-provider-aws should not set the desired replicas otherwise it can cause a problem where the controller is trying to update an ASG with desired replicas that are out of bounds of min/max.

Example:
The machine pool is set to have min:1 instances and max: 3 instances and it's currently running on 2 desired replicas. The user wants to update to new values min: 5 and max: 10. The update operation fails with an AWS API error

E1122 16:24:14.026636       1 controller.go:324] "Reconciler error" err=<
	unable to update ASG: failed to update ASG "tsa05-ndp01": ValidationError: Desired capacity:2 must be between the specified min size:5 and max size:10
		status code: 400, request id: f142fa2f-6582-43c7-8296-0a88ac104adf

Updating the MachinePool.spec.replicas will not help as the field is updated based on the current amount of nodes/replicas due to externally managed replicas configuration so it will be set back to the current value of 2

Release note:

Treat the annotation `cluster.x-k8s.io/replicas-managed-by` on MachinePool CR as described in [CAPI documentation](https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/book/src/developer/architecture/controllers/machine-pool.md#externally-managed-autoscaler) and do not set desired replicas on ASG when the annotation is present.

[k8s-ci-robot]

Hi @calvix. 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.

Details

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.

[calvix]

the renaming of the variable from scope to machinePoolScope is necessary to be able to access the package scope and the function scope.ReplicasExternallyManaged

[AndiDog]

/ok-to-test

[AndiDog]

Change looks good per se, however the existing implementation of the annotation check function seems strange:

func ReplicasExternallyManaged(mp *expclusterv1.MachinePool) bool {
	val, ok := mp.Annotations[ReplicasManagedByAnnotation]
	return ok && val == ExternalAutoscalerReplicasManagedByAnnotationValue
}

CAPA requires a specific value which is defined like this:

const (
	// ReplicasManagedByAnnotation is an annotation that indicates external (non-Cluster API) management of infra scaling.
	// The practical effect of this is that the capi "replica" count is derived from the number of observed infra machines,
	// instead of being a source of truth for eventual consistency.
	//
	// N.B. this is to be replaced by a direct reference to CAPI once https://github.com/kubernetes-sigs/cluster-api/pull/7107 is meged.
	ReplicasManagedByAnnotation = "cluster.x-k8s.io/replicas-managed-by"

	// ExternalAutoscalerReplicasManagedByAnnotationValue is used with the "cluster.x-k8s.io/replicas-managed-by" annotation
	// to indicate an external autoscaler enforces replica count.
	//
	// N.B. this is to be replaced by a direct reference to CAPI once https://github.com/kubernetes-sigs/cluster-api/pull/7107 is meged.
	ExternalAutoscalerReplicasManagedByAnnotationValue = "external-autoscaler"
)

But the CAPI "contract" for this annotation says:

Cluster API treats the annotation as a "boolean", meaning that the presence of the annotation is sufficient to indicate external replica count management, with one exception: if the value is "false", then that indicates to Cluster API that replica enforcement is nominal, and managed by Cluster API.

Providers may choose to implement the cluster.x-k8s.io/replicas-managed-by annotation with different values (e.g., external-autoscaler, or karpenter) that may inform different provider-specific behaviors, but those values will have no effect upon Cluster API.

So the check should be patched as follows (untested):

func ReplicasExternallyManaged(mp *expclusterv1.MachinePool) bool {
	val, ok := mp.Annotations[ReplicasManagedByAnnotation]
	return ok && val != "false"
}

And we should delete the const ExternalAutoscalerReplicasManagedByAnnotationValue since any non-false value is okay.

This is out of scope for the PR, but I quickly wrote a test + fixed the above, so that will be pushed to this PR branch soon by my colleague @calvix.

We should have someone from another company, and with expertise with that code, to review as well, please.

[AndiDog]

@calvix Please squash into a single commit with a reasonable commit message (e.g. the PR title). Also, please edit the PR to include a release note (see PR template if you need to know the formatting).

This now contains my commits, so I shouldn't review.

[cnmcavoy]

/lgtm

[richardcase]

/test ?

[k8s-ci-robot]

@richardcase: The following commands are available to trigger required jobs:

• /test pull-cluster-api-provider-aws-build
• /test pull-cluster-api-provider-aws-test
• /test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

• /test pull-cluster-api-provider-aws-apidiff-main
• /test pull-cluster-api-provider-aws-e2e
• /test pull-cluster-api-provider-aws-e2e-blocking
• /test pull-cluster-api-provider-aws-e2e-clusterclass
• /test pull-cluster-api-provider-aws-e2e-conformance
• /test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
• /test pull-cluster-api-provider-aws-e2e-eks
• /test pull-cluster-api-provider-aws-e2e-eks-gc
• /test pull-cluster-api-provider-aws-e2e-eks-testing

Use /test all to run the following jobs that were automatically triggered:

• pull-cluster-api-provider-aws-apidiff-main
• pull-cluster-api-provider-aws-build
• pull-cluster-api-provider-aws-test
• pull-cluster-api-provider-aws-verify

Details

In response to this:

/test ?

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.

[richardcase]

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

[richardcase]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [richardcase]

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