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.

Sign up for GitHub

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

Jump to bottom

[autoscaling] StepScalingPolicy ignores estimatedInstanceWarmup property #10514

Closed
ddneilson opened this issue Sep 24, 2020 · 4 comments · Fixed by #20936
Closed

[autoscaling] StepScalingPolicy ignores estimatedInstanceWarmup property #10514

ddneilson opened this issue Sep 24, 2020 · 4 comments · Fixed by #20936
Labels
@aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort p1

Comments

@ddneilson
Copy link
Contributor

The implementation of StepScalingPolicy ignores the estimatedInstanceWarmup property of the construct.

The property should be passed on to the StepScalingAction objects (lowerAction & upperAction) that the StepScalingPolicy creates, but it is not.

Reproduction Steps

Create a StepScalingPolicy with an estimatedInstanceWarmup property value.

What did you expect to happen?

The value of estimatedInstanceWarmup is translated into a value for the EstimatedInstanceWarmup property of the generated AWS::AutoScaling::ScalingPolicy.

What actually happened?

There is no EstimatedInstanceWarmup value in the generated AWS::AutoScaling::ScalingPolicy resource.

Environment

  • CLI Version : 1.63.0+
  • Framework Version: 1.63.0+
  • Node.js Version: any
  • OS : any
  • Language (Version): all

This is 🐛 Bug Report
@ddneilson ddneilson added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 24, 2020
@github-actions github-actions bot added the @aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling label Sep 24, 2020
@NetaNir NetaNir added needs-triage This issue or PR still needs to be triaged. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 24, 2020
@SomayaB SomayaB added the bug This issue is a bug. label Oct 19, 2020
@BDeus
Copy link
Contributor

BDeus commented Feb 24, 2021

Furthermore, the cooldown property should not be proposed on BasicStepScalingPolicyProps since it used only for SimpleScalingPolicy AWS Doc

@NetaNir NetaNir added p1 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 24, 2021
@NetaNir NetaNir removed their assignment Jun 21, 2021
@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 21, 2022
@BDeus BDeus mentioned this issue Jun 30, 2022
Merged
3 tasks
@BDeus
Copy link
Contributor

BDeus commented Jun 30, 2022 •
edited
Loading

Furthermore, the cooldown property should not be proposed on BasicStepScalingPolicyProps since it used only for SimpleScalingPolicy AWS Doc

it appears that it used as a default for instance warmup since cloudformation doesn't support defaultInstanceWarmup for now
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-as-group.html#cfn-as-group-defaultinstancewarmup

mergify bot pushed a commit that referenced this issue Jul 1, 2022
@BDeus
feat(autoscaling): step scaling policy supports estimatedInstanceWarm…
e4c7b97 
…up property (#20936)

Fix #10514

----

### All Submissions:

* [ x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
daschaa pushed a commit to daschaa/aws-cdk that referenced this issue Jul 9, 2022
@BDeus @daschaa
feat(autoscaling): step scaling policy supports estimatedInstanceWarm…
eb5fc48 
…up property (aws#20936)

Fix aws#10514

----

### All Submissions:

* [ x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@rgoltz
Copy link

rgoltz commented Nov 8, 2022

it appears that it used as a default for instance warmup since cloudformation doesn't support defaultInstanceWarmup for now https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-as-group.html#cfn-as-group-defaultinstancewarmup

Hi @BDeus Hi @ddneilson - It seems the CFN documentation changed for DefaultInstanceWarmup within the last days - It seems that is now available via CFN (hence at minimum as L1 in CDK).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
@aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort p1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(autoscaling): step scaling policy supports estimatedInstanceWarmup property BDeus/aws-cdk
5 participants
@BDeus @NetaNir @rgoltz @SomayaB @ddneilson