Skip to content

Ignore ASG desiredCapacity when it's not set to make it compatible with the cluster-autoscaler #1797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

handlerww
Copy link

@handlerww handlerww commented Jun 30, 2023

Description of your changes

As mentioned in https://github.com/crossplane/crossplane/blob/master/design/one-pager-ignore-changes.md?plain=1#L24-L27 and #502, there is a race condition
between crossplane and cluster autoscaler. When we use asg as self-managed nodegroup with cluster-autoscaler, the reconciling of crossplane asg controller will revert changes from cluster-autoscaler. If desiredCapacity is not set, we should leave it blank, and just let the cluster-autoscaler to manage it. This PR implements a similar logic in the managed nodegroup.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I've manually test it, the test plan is here:

  1. create ASG:
    a. create a ASG without desiredCapacity, the ASG can be created.
  maxSize: 400
  minSize: 0
 maxSize: 400
 minSize: 1

b. create a ASG with desiredCapacity, the ASG configuration is as expected.

 maxSize: 400
 minSize: 0
 desiredCapacity: 1
  1. update ASG
    a. modify ASG configuration on AWS console, modify the maxSize to 401, trigger the reconciling, the desiredCapacity doesn't change.
    b. add desiredCapacity configuration on ASG CR, the ASG configuration on AWS console changes as expected.
    c. remove desiredCapacity configuration from ASG CR, and modify the configuration on AWS console, it works as expected.

@handlerww handlerww force-pushed the ignore-changes-asg branch from 5694dc7 to 736206b Compare June 30, 2023 00:19
@handlerww handlerww changed the title Ignore ASG desiredCapacity when it's not set to make it compatible with the cluster-autoscaler. Ignore ASG desiredCapacity when it's not set to make it compatible with the cluster-autoscaler Jun 30, 2023
@csuzhangxc
Copy link

@MisterMX can you take a looK?

in.DesiredCapacity = awsclients.LateInitializeInt64Ptr(in.DesiredCapacity, obs.DesiredCapacity)
// if desiredCapacity is not set, don't update the value
if in.DesiredCapacity != nil {
in.DesiredCapacity = awsclients.LateInitializeInt64Ptr(in.DesiredCapacity, obs.DesiredCapacity)
Copy link
Collaborator

@MisterMX MisterMX Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the issue this change wants to tackle I don't think it makes sense the way it is implemented: LateInitialize is supposed to set update the spec with the observed value if the former is nil.

Also, LateInitializeInt64Ptr only updates the value if in.DesiredCapacity == nil (see here). So this change would effectively cause in.DesiredCapacity to be never late initialized.

I think it makes more sense to explicitly specify the desired behaviour of the controller rather then doing it implicitly through a special prop value. It could by defining a new property (i.e. scalingMode: Managed | External) that tells the controller to not set the value in the request to AWS during Update.

@bobh66
Copy link
Contributor

bobh66 commented Jul 3, 2023

This issue will also be addressed by the Observe Only feature where you will be able to set spec.initProvider.desiredSize: X and leave spec.forProvider.desiredSize empty which will cause the field to be set on create and then ignored for all subsequent reconciles.

@handlerww
Copy link
Author

handlerww commented Jul 5, 2023

This issue will also be addressed by the Observe Only feature where you will be able to set spec.initProvider.desiredSize: X and leave spec.forProvider.desriedSize empty which will cause the field to be set on create and then ignored for all subsequent reconciles.

So, would it be addressed in Ignore Changes? It seems like a better solution.

@bobh66
Copy link
Contributor

bobh66 commented Aug 8, 2023

This issue will also be addressed by the Observe Only feature where you will be able to set spec.initProvider.desiredSize: X and leave spec.forProvider.desriedSize empty which will cause the field to be set on create and then ignored for all subsequent reconciles.

So, would it be addressed in Ignore Changes? It seems like a better solution.

Yes, my mistake. Thanks for the correction.

@handlerww handlerww closed this Aug 11, 2023
@handlerww handlerww deleted the ignore-changes-asg branch August 11, 2023 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants