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

(aws-autoscaling/aws-applicationautoscaling): StepScalingPolicy supports at most 40 steps #26215

Closed
SamStephens opened this issue Jul 4, 2023 · 8 comments · Fixed by #26490
Closed
Assignees
Labels
@aws-cdk/aws-sqs Related to Amazon Simple Queue Service bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@SamStephens
Copy link
Contributor

SamStephens commented Jul 4, 2023

Describe the bug

If you create a StepScalingPolicy with more than 40 steps, the Cloudformation deployment fails with the message "There can be at most 20 step adjustments per scaling policy".

The reason the limit is 40 and not 20 is because StepScalingPolicy creates two policies, an UpperPolicy and a LowerPolicy, each containing half the steps.

Expected Behavior

The deployment succeeds, with the steps split across the required number of policies.

If I've misunderstood the code, and there's a reason to specifically have two policies, I'd expect a build time failure from CDK when the number of steps exceeds 40, rather than a build time failure.

Current Behavior

Cloudformation deployment fails with the message "There can be at most 20 step adjustments per scaling policy".

Reproduction Steps

Starting with a Fargate Service and an SQS queue (I'm trying to reproduce the tracking policy https://docs.aws.amazon.com/autoscaling/ec2/userguide/ec2-auto-scaling-target-tracking-metric-math.html describes without using metric math).

        maximum_number_of_tasks = 50
        desired_messages_per_task = 20

        scaling = service.auto_scale_task_count(
            min_capacity=1,
            max_capacity=maximum_number_of_tasks,
        )

        scaling_steps = [
            aws_applicationautoscaling.ScalingInterval(
                lower=step_number * desired_messages_per_task,
                upper=step_number * (desired_messages_per_task + 1) - 1,
                change=step_number + 1,
            )
            for step_number
            in range(0, maximum_number_of_tasks)
        ]

        scaling.scale_on_metric(
            id="Track visible messages per task",
            metric=queue.metric_approximate_number_of_messages_visible(
                statistic="Average",
                period=Duration.minutes(1),
            ),
            adjustment_type=aws_autoscaling.AdjustmentType.EXACT_CAPACITY,
            scaling_steps=scaling_steps,
            cooldown=Duration.minutes(3),
        )

Possible Solution

Allow for more than two scaling policies to be created (this may not make sense, my understanding of these policies is not great).

Additional Information/Context

No response

CDK CLI Version

2.86.0 (build 1130fab)

Framework Version

2.81.0

Node.js Version

v16.18.1

OS

Ubuntu (Windows Subsystem for Linux)

Language

Python

Language Version

3.9.7

Other information

No response

@SamStephens SamStephens added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 4, 2023
@github-actions github-actions bot added the @aws-cdk/aws-sqs Related to Amazon Simple Queue Service label Jul 4, 2023
@pahud pahud self-assigned this Jul 5, 2023
@pahud
Copy link
Contributor

pahud commented Jul 5, 2023

Yes, Agree CDK should throw when the number of steps exceeds 40, rather than a build time failure.

@pahud pahud added p2 effort/medium Medium work item – several days of effort labels Jul 5, 2023
@pahud pahud removed their assignment Jul 5, 2023
@pahud pahud removed the needs-triage This issue or PR still needs to be triaged. label Jul 5, 2023
@scanlonp scanlonp self-assigned this Jul 24, 2023
@mergify mergify bot closed this as completed in #26490 Jul 25, 2023
mergify bot pushed a commit that referenced this issue Jul 25, 2023
…ver allowable maximum (#26490)

`StepScalingPolicy` did not have a validation that the number of intervals in `scalingSteps` was in the allowable range. The [Autoscaling documentation](https://docs.aws.amazon.com/autoscaling/ec2/userguide/ec2-auto-scaling-quotas.html) states 20 is the maximum step changes in a policy, and since autoscaling creates 2 policies (an [UpperPolicy](https://github.com/aws/aws-cdk/blob/bc029fe5ac69a8b7fd2dfdbcd8834e9a2cf8e000/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts#L136-L166) and a [LowerPolicy](https://github.com/aws/aws-cdk/blob/bc029fe5ac69a8b7fd2dfdbcd8834e9a2cf8e000/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts#L105-L134)), our cap is 40.

There is an identical change in Application Autoscaling.

Closes #26215.

----

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

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@SamStephens
Copy link
Contributor Author

Thanks @scanlonp

bmoffatt pushed a commit to bmoffatt/aws-cdk that referenced this issue Jul 29, 2023
…ver allowable maximum (aws#26490)

`StepScalingPolicy` did not have a validation that the number of intervals in `scalingSteps` was in the allowable range. The [Autoscaling documentation](https://docs.aws.amazon.com/autoscaling/ec2/userguide/ec2-auto-scaling-quotas.html) states 20 is the maximum step changes in a policy, and since autoscaling creates 2 policies (an [UpperPolicy](https://github.com/aws/aws-cdk/blob/bc029fe5ac69a8b7fd2dfdbcd8834e9a2cf8e000/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts#L136-L166) and a [LowerPolicy](https://github.com/aws/aws-cdk/blob/bc029fe5ac69a8b7fd2dfdbcd8834e9a2cf8e000/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts#L105-L134)), our cap is 40.

There is an identical change in Application Autoscaling.

Closes aws#26215.

----

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

jedkass commented Aug 25, 2023

@scanlonp @SamStephens Is this the correct validation? I created a service with 22 scaling steps, which did not breach this validation. However upon deploy, I get a 400 ValidationException:

There can be at most 20 step adjustments per scaling policy (Service: AWSApplicationAutoScaling; Status Code: 400; Error Code: ValidationException, etc...

@scanlonp
Copy link
Contributor

@jedkass so this fails to deploy but not caught at synth time? I will look into it; if you have a simple reproduction stack, that would be helpful.

@jedkass
Copy link

jedkass commented Aug 25, 2023

Correct. cdk synth — no issues. cdk deploy in my CodePipeline — the error above.

I honestly don't have a suuuuper simple repro stack. But I'm essentially calling a private method like below where scalableTaskCount was created on a FargateService object:

defineAutoScaleDownPolicy(scalableTaskCount: ScalableTaskCount) {
    const minTaskCount = this.someConfig.minTaskCount;
    const maxTaskCount = this.someConfig.maxTaskCount;
    const scalingSteps = [];
    for (let lowerThreshold = 10; maxTaskCount - lowerThreshold > minTaskCount; lowerThreshold += 10) {
      scalingSteps.push({
        lower: lowerThreshold,
        change: -lowerThreshold + 10,
      });
    }

    scalableTaskCount.scaleOnMetric(`some-unique-id`, {
      metric: myMetric,
      adjustmentType: AdjustmentType.CHANGE_IN_CAPACITY,
      scalingSteps: scalingSteps,
    });
  }
}

In my case, minTaskCount is 40 and maxTaskCount is 267, which results in scalingSteps.length == 22

@scanlonp
Copy link
Contributor

@jedkass great, I will try to reproduce and edit the validation to cover this! Since its not breaking (correct?), it may take a little time to get around to, but thanks for putting this on my radar.

@jedkass
Copy link

jedkass commented Aug 25, 2023

@scanlonp I appreciate it! Yea not breaking since it's an AWS limitation anyway. Just agree with the premise of this ticket that it should be caught at build time, so wanted to offer my experience that...it doesn't seem fixed.

FYI, if I change my for loop to generate 41 scaling steps, cdk synth does fail, so your logic is def doing what it intended! I think it just currently doesn't match AWS' limits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-sqs Related to Amazon Simple Queue Service bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
4 participants