Skip to content
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

feat(autoscaling): instance maintenance policy for AutoScalingGroup #28092

Merged
merged 20 commits into from
Dec 22, 2023

Conversation

go-to-k
Copy link
Contributor

@go-to-k go-to-k commented Nov 21, 2023

@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Nov 21, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team November 21, 2023 11:17
@github-actions github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Nov 21, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation dismissed their stale review November 21, 2023 12:22

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@go-to-k go-to-k marked this pull request as ready for review November 21, 2023 15:42
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 21, 2023
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍
I added some suggestions for improvements, feel free to comment.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 21, 2023
fix by the review

add new validation to clear previously set values

add new unit tests
@go-to-k go-to-k force-pushed the feat/autoscaling-instance-maintenance-policy branch from 8e4e844 to b4ef9f7 Compare November 22, 2023 05:10
@go-to-k
Copy link
Contributor Author

go-to-k commented Nov 22, 2023

@lpizzinidev

Thanks for your review. I fixed them!

@go-to-k go-to-k force-pushed the feat/autoscaling-instance-maintenance-policy branch from 9f14ea4 to 5eb173e Compare November 25, 2023 11:39
@go-to-k
Copy link
Contributor Author

go-to-k commented Nov 25, 2023

@lpizzinidev

I changed the properties to flat and optional! Please check them.

@go-to-k go-to-k changed the title feat(autoscaling): add InstanceMaintenancePolicy property to AutoScalingGroup feat(autoscaling): support instance maintenance policy to AutoScalingGroup Nov 25, 2023
@go-to-k go-to-k changed the title feat(autoscaling): support instance maintenance policy to AutoScalingGroup feat(autoscaling): support instance maintenance policy for AutoScalingGroup Nov 25, 2023
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍
Just some minor adjustments and it will be good to go for me.

@go-to-k
Copy link
Contributor Author

go-to-k commented Nov 27, 2023

@lpizzinidev

I changed. Please look at this comment.

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 27, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Thanks @go-to-k. Some minor comments, but great start!

@@ -288,6 +288,38 @@ autoScalingGroup.scaleOnSchedule('AllowDownscalingAtNight', {
});
```

### Instance Maintenance Policy

The instance maintenance policy allows you to configure an instance maintenance policy for
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The instance maintenance policy allows you to configure an instance maintenance policy for
You can configure an instance maintenance policy for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed.

instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.MICRO),
machineImage: ec2.MachineImage.latestAmazonLinux2(),
maintenancePolicyMaxHealthPercentage: 200,
maintenancePolicyMinHealthPercentage: 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not necessarily against this, but what was the reasoning against just minHealthPercentage and maxHealthPercentage? I think i prefer that unless it would be confusing if minimum health could point to other things besides maintenance policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was not sure at first, but I think I will do that.

Copy link
Contributor Author

@go-to-k go-to-k Dec 21, 2023

Choose a reason for hiding this comment

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

Changed to minHealthyPercentage and maxHealthyPercentage to match CloudFormation (with Health'y').

*
* @see https://docs.aws.amazon.com/autoscaling/ec2/userguide/ec2-auto-scaling-instance-maintenance-policy.html
*
* @default - No instance maintenance policy.
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, I think we should just set the default to -1. If I am reading the docs correctly, this is equivalent to undefined in CloudFormation. The only issue I see is if CloudFormation somehow fails if you set the value to -1 and there wasn't any previous value to clear. If that happens, then lets go down this path. If not, we can treat -1 and undefined equivalently in CDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tried to run CloudFormation with -1 from the beginning, I got the following error in CFn. In other words, it seems that -1 and undefined must be distinguished.

MinHealthyPercentage must be between 0 and 100 (inclusive).

Copy link
Contributor

Choose a reason for hiding this comment

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

:(

Comment on lines 1892 to 1900
if (maxHealthPercentage === -1 && minHealthPercentage === -1) {
return {
maxHealthyPercentage: maxHealthPercentage,
minHealthyPercentage: minHealthPercentage,
};
}
if (minHealthPercentage === -1 || maxHealthPercentage === -1) {
throw new Error(`Both maintenancePolicyMinHealthPercentage and maintenancePolicyMaxHealthPercentage must be -1 to clear the previously set value, got maintenancePolicyMinHealthPercentage: ${minHealthPercentage} and maintenancePolicyMaxHealthPercentage: ${maxHealthPercentage}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (maxHealthPercentage === -1 && minHealthPercentage === -1) {
return {
maxHealthyPercentage: maxHealthPercentage,
minHealthyPercentage: minHealthPercentage,
};
}
if (minHealthPercentage === -1 || maxHealthPercentage === -1) {
throw new Error(`Both maintenancePolicyMinHealthPercentage and maintenancePolicyMaxHealthPercentage must be -1 to clear the previously set value, got maintenancePolicyMinHealthPercentage: ${minHealthPercentage} and maintenancePolicyMaxHealthPercentage: ${maxHealthPercentage}`);
}
if (minHealthPercentage === -1 || maxHealthPercentage === -1 && minHealthPercentage !== maxHealthPercentage) {
throw new Error(`Both maintenancePolicyMinHealthPercentage and maintenancePolicyMaxHealthPercentage must be -1 to clear the previously set value, got maintenancePolicyMinHealthPercentage: ${minHealthPercentage} and maintenancePolicyMaxHealthPercentage: ${maxHealthPercentage}`);
}

This would be equivalent and remove an additional if condition, but isn't a huge deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, in the case for the code and both maxHealthPercentage and minHealthPercentage is -1, the next branch causes an error.

    if (maxHealthPercentage < 100 || maxHealthPercentage > 200) {
      throw new Error(`maxHealthPercentage must be between 100 and 200, or -1 to clear the previously set value, got ${maxHealthPercentage}`);
    }
    if (minHealthPercentage < 0 || minHealthPercentage > 100) {
      throw new Error(`minHealthPercentage must be between 0 and 100, or -1 to clear the previously set value, got ${minHealthPercentage}`);
    }

So, if both are -1, we need to return them as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, the following code would work. I will go with this!

    if ((minHealthPercentage === -1 || maxHealthPercentage === -1) && minHealthPercentage !== maxHealthPercentage) {
      throw new Error(`Both minHealthPercentage and maxHealthPercentage must be -1 to clear the previously set value, got minHealthPercentage: ${minHealthPercentage} and maxHealthPercentage: ${maxHealthPercentage}`);
    }
    if (maxHealthPercentage !== -1 && (maxHealthPercentage < 100 || maxHealthPercentage > 200)) {
      throw new Error(`maxHealthPercentage must be between 100 and 200, or -1 to clear the previously set value, got ${maxHealthPercentage}`);
    }
    if (minHealthPercentage !== -1 && (minHealthPercentage < 0 || minHealthPercentage > 100)) {
      throw new Error(`minHealthPercentage must be between 0 and 100, or -1 to clear the previously set value, got ${minHealthPercentage}`);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed.

}
if (maxHealthPercentage - minHealthPercentage > 100) {
throw new Error(`The difference between maintenancePolicyMinHealthPercentage and maintenancePolicyMaxHealthPercentage cannot be greater than 100, got ${maxHealthPercentage - minHealthPercentage}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can store all errors we get in an array and throw them all at the same time, so the user will know all the issues present.

Copy link
Contributor Author

@go-to-k go-to-k Dec 21, 2023

Choose a reason for hiding this comment

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

Hmmm, is that necessary?

I think the information given to the user would be more complex, and type inference would not work and branching would become more complicated.

(Because of the eslint error, in numerical comparisons, minHealthyPercentage !== undefined and maxHealthyPercentage !== undefined must be added to the numerical comparison.)

    const errors: string[] = [];

    if (minHealthyPercentage === undefined && maxHealthyPercentage === undefined) return;
    if (minHealthyPercentage === undefined || maxHealthyPercentage === undefined) {
      errors.push(`Both or neither of minHealthyPercentage and maxHealthyPercentage must be specified, got minHealthyPercentage: ${minHealthyPercentage} and maxHealthyPercentage: ${maxHealthyPercentage}`);
    }
    if ((minHealthyPercentage === -1 || maxHealthyPercentage === -1) && minHealthyPercentage !== maxHealthyPercentage) {
      errors.push(`Both minHealthyPercentage and maxHealthyPercentage must be -1 to clear the previously set value, got minHealthyPercentage: ${minHealthyPercentage} and maxHealthyPercentage: ${maxHealthyPercentage}`);
    }
    if (minHealthyPercentage !== -1 && minHealthyPercentage !== undefined && (minHealthyPercentage < 0 || minHealthyPercentage > 100)) {
      errors.push(`minHealthyPercentage must be between 0 and 100, or -1 to clear the previously set value, got ${minHealthyPercentage}`);
    }
    if (maxHealthyPercentage !== -1 && maxHealthyPercentage !== undefined && (maxHealthyPercentage < 0 || maxHealthyPercentage > 100)) {
      errors.push(`maxHealthyPercentage must be between 100 and 200, or -1 to clear the previously set value, got ${maxHealthyPercentage}`);
    }
    if (maxHealthyPercentage !== undefined && minHealthyPercentage !== undefined && maxHealthyPercentage - minHealthyPercentage > 100) {
      errors.push(`The difference between minHealthyPercentage and maxHealthyPercentage cannot be greater than 100, got ${maxHealthyPercentage - minHealthyPercentage}`);
    }

    if (errors.length > 0) {
      throw new Error(errors.join('\n'));
    }

    return {
      minHealthyPercentage,
      maxHealthyPercentage,
    };

@kaizencc kaizencc changed the title feat(autoscaling): support instance maintenance policy for AutoScalingGroup feat(autoscaling): instance maintenance policy for AutoScalingGroup Dec 20, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 20, 2023
@mergify mergify bot dismissed kaizencc’s stale review December 21, 2023 05:00

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 21, 2023
@go-to-k
Copy link
Contributor Author

go-to-k commented Dec 21, 2023

@kaizencc

Thanks for your review!

I fixed and commented back. I would especially appreciate it if you would read the following comment.

#28092 (comment)

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

I think the -1 thing is kind of dumb but I can't think of a reasonable way of doing it in cdk-land besides just passing on that user-experience to the user. Oh well. Everything else looks good -- my comments on the errors were just nits anyway. Thanks again.

Copy link
Contributor

mergify bot commented Dec 22, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 22, 2023
Copy link
Contributor

mergify bot commented Dec 22, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 225e5f1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 5eb2c26 into aws:main Dec 22, 2023
9 checks passed
Copy link
Contributor

mergify bot commented Dec 22, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-cdk-lib.aws-autoscaling: Add support for configuring AutoScalingGroup's instance maintenance policy
4 participants