Skip to content

Commit

Permalink
fix(autoscaling): step scaling without adjustment type fails (#29158)
Browse files Browse the repository at this point in the history
### Reason for this change

Step Scaling without `adjustmentType` fails with CFn error `You must specify an AdjustmentType for policy type: StepScaling`.

- Reproduction code
```ts
const asg = new autoscaling.AutoScalingGroup(stack, 'ASG', {
  vpc,
  instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.MICRO),
  machineImage: new ec2.AmazonLinuxImage({ generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2 }),
});

asg.scaleOnMetric('StepScaling', {
  metric: new cloudwatch.Metric({ namespace: 'AWS/EC2', metricName: 'CPUUtilization', dimensionsMap: { AutoScalingGroupName: asg.autoScalingGroupName } }),
  scalingSteps: [
    { upper: 10, change: -1 },
    { lower: 50, change: +1 },
    { lower: 90, change: +2 },
  ],
  evaluationPeriods: 10,
  datapointsToAlarm: 5,
  metricAggregationType: autoscaling.MetricAggregationType.MAXIMUM,

  // adjustmentType: autoscaling.AdjustmentType.CHANGE_IN_CAPACITY,
});
```

### Description of changes

According to [the CDK code](https://github.com/aws/aws-cdk/blob/v2.122.0/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts#L105-L115), `CHANGE_IN_CAPACITY` will be used if `adjustmentType` is not specified in the prop. But the variable is not passed into `StepScalingAction` construct (instead `props.adjustmentType` is passed as is).

[The documentation](https://github.com/aws/aws-cdk/blob/v2.122.0/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts#L26) also says that the default value is `CHANGE_IN_CAPACITY`.

So we should use `adjustmentType` instead of `props.adjustmentType`.

```ts
    const adjustmentType = props.adjustmentType || AdjustmentType.CHANGE_IN_CAPACITY;
    const changesAreAbsolute = adjustmentType === AdjustmentType.EXACT_CAPACITY;
    // ...
    // ...

      this.lowerAction = new StepScalingAction(this, 'LowerPolicy', {
        // adjustmentType: props.adjustmentType,
        adjustmentType,
```

**The fact that the error occurs if `props.adjustmentType` is not specified means that the user's successful existing stack always specifies `props.adjustmentType`. In other words, no change to the existing resource will occur due to this change, so there is no need for a feature flag.**

### Description of how you validated changes

~~Unit tests without integ tests, because this PR could cover this bug by unit tests.~~
Both unit and integ tests.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
go-to-k committed Feb 28, 2024
1 parent 7e8239b commit a7de7fe
Show file tree
Hide file tree
Showing 7 changed files with 330 additions and 5 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,99 @@
"Statistic": "Average",
"Threshold": 50
}
},
"ASGStepScalingWithDefaultAdjustmentTypeLowerPolicyA6C1EA33": {
"Type": "AWS::AutoScaling::ScalingPolicy",
"Properties": {
"AdjustmentType": "ChangeInCapacity",
"AutoScalingGroupName": {
"Ref": "ASG46ED3070"
},
"MetricAggregationType": "Maximum",
"PolicyType": "StepScaling",
"StepAdjustments": [
{
"MetricIntervalUpperBound": 0,
"ScalingAdjustment": -1
}
]
}
},
"ASGStepScalingWithDefaultAdjustmentTypeLowerAlarmF9F52487": {
"Type": "AWS::CloudWatch::Alarm",
"Properties": {
"AlarmActions": [
{
"Ref": "ASGStepScalingWithDefaultAdjustmentTypeLowerPolicyA6C1EA33"
}
],
"AlarmDescription": "Lower threshold scaling alarm",
"ComparisonOperator": "LessThanOrEqualToThreshold",
"DatapointsToAlarm": 5,
"Dimensions": [
{
"Name": "AutoScalingGroupName",
"Value": {
"Ref": "ASG46ED3070"
}
}
],
"EvaluationPeriods": 10,
"MetricName": "DiskWriteOps",
"Namespace": "AWS/EC2",
"Period": 300,
"Statistic": "Average",
"Threshold": 100
}
},
"ASGStepScalingWithDefaultAdjustmentTypeUpperPolicy08CC2D99": {
"Type": "AWS::AutoScaling::ScalingPolicy",
"Properties": {
"AdjustmentType": "ChangeInCapacity",
"AutoScalingGroupName": {
"Ref": "ASG46ED3070"
},
"MetricAggregationType": "Maximum",
"PolicyType": "StepScaling",
"StepAdjustments": [
{
"MetricIntervalLowerBound": 0,
"MetricIntervalUpperBound": 200,
"ScalingAdjustment": 1
},
{
"MetricIntervalLowerBound": 200,
"ScalingAdjustment": 2
}
]
}
},
"ASGStepScalingWithDefaultAdjustmentTypeUpperAlarm2379E17B": {
"Type": "AWS::CloudWatch::Alarm",
"Properties": {
"AlarmActions": [
{
"Ref": "ASGStepScalingWithDefaultAdjustmentTypeUpperPolicy08CC2D99"
}
],
"AlarmDescription": "Upper threshold scaling alarm",
"ComparisonOperator": "GreaterThanOrEqualToThreshold",
"DatapointsToAlarm": 5,
"Dimensions": [
{
"Name": "AutoScalingGroupName",
"Value": {
"Ref": "ASG46ED3070"
}
}
],
"EvaluationPeriods": 10,
"MetricName": "DiskWriteOps",
"Namespace": "AWS/EC2",
"Period": 300,
"Statistic": "Average",
"Threshold": 300
}
}
},
"Parameters": {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,17 @@ asg.scaleOnMetric('StepScaling', {
datapointsToAlarm: 5,
metricAggregationType: autoscaling.MetricAggregationType.MAXIMUM,
});
asg.scaleOnMetric('StepScalingWithDefaultAdjustmentType', {
metric: new cloudwatch.Metric({ namespace: 'AWS/EC2', metricName: 'DiskWriteOps', dimensionsMap: { AutoScalingGroupName: asg.autoScalingGroupName } }),
scalingSteps: [
{ upper: 100, change: -1 },
{ lower: 300, change: +1 },
{ lower: 500, change: +2 },
],
evaluationPeriods: 10,
datapointsToAlarm: 5,
metricAggregationType: autoscaling.MetricAggregationType.MAXIMUM,
});

new integ.IntegTest(app, 'autoscaling-step-scaling-integ', {
testCases: [stack],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ export class StepScalingPolicy extends Construct {
const threshold = intervals[alarms.lowerAlarmIntervalIndex].upper;

this.lowerAction = new StepScalingAction(this, 'LowerPolicy', {
adjustmentType: props.adjustmentType,
adjustmentType,
cooldown: props.cooldown,
estimatedInstanceWarmup: props.estimatedInstanceWarmup,
metricAggregationType: props.metricAggregationType ?? aggregationTypeFromMetric(props.metric),
Expand Down Expand Up @@ -179,7 +179,7 @@ export class StepScalingPolicy extends Construct {
const threshold = intervals[alarms.upperAlarmIntervalIndex].lower;

this.upperAction = new StepScalingAction(this, 'UpperPolicy', {
adjustmentType: props.adjustmentType,
adjustmentType,
cooldown: props.cooldown,
estimatedInstanceWarmup: props.estimatedInstanceWarmup,
metricAggregationType: props.metricAggregationType ?? aggregationTypeFromMetric(props.metric),
Expand Down
Loading

0 comments on commit a7de7fe

Please sign in to comment.