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

Support multiple scaling policies per scalable dimension #106

Merged

Conversation

surajkota
Copy link
Member

@surajkota surajkota commented Aug 7, 2023

Description of changes:

Customer facing issue in using multiple scaling policies for a scalable dimension because originally the code assumed it is 1:1 mapping. To uniquely identify a scaling policy resource, we need policy name, resource-id, service namespace and scalable dimension. This PR adds ability to create multiple scaling policies per scalable dimension and also updates the following things:

  • Pass PolicyName for sdkFind, it is a required field in CRD
  • Added logic to error on missing required fields per description above for DescribeScalingPolicyInput
  • Updated tests to cover multiple scaling policies scenario per dimension case
  • Updated test logic to use correct dependency of fixtures and order of execution
  • Updates the tests to use object oriented style bootstrapping. Hit Upgrade ack-test PyYAML dependency test-infra#368 issue while trying to fix the issue and cannot use latest ack-test without updating the bootstrap logic
  • Removed duplicate test with dynamodb. SageMaker and dynamodb tests are testing same thing while SM tests have more coverage. No reason to maintain both set of tests, dynamodb tests add additional bootstrapping as well

⚠️ Pending before merge
Hit another issue described in aws-controllers-k8s/community#1871. Fix aws-controllers-k8s/code-generator#459 has been merged

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ack-prow bot requested review from a-hilaly and RedbackThomson August 7, 2023 00:35
@ack-prow ack-prow bot added the approved label Aug 7, 2023
@surajkota surajkota changed the title support multiple scaling policies per scalable dimension Support multiple scaling policies per scalable dimension Aug 7, 2023
@surajkota
Copy link
Member Author

/retest

"ack-sagemaker-execution-role",
"sagemaker.amazonaws.com",
managed_policies=[
"arn:aws:iam::aws:policy/AmazonSageMakerFullAccess",
Copy link
Member

Choose a reason for hiding this comment

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

Is sagemaker and s3 fullaccess sufficient for autoscaling? Looks like that was what was there previously, but double-checking.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. SageMakerFullAccess has all the permissions reqd for autoscaling

@@ -90,7 +110,7 @@ deletionPolicy: delete
# controller reconciliation configurations
reconcile:
# The default duration, in seconds, to wait before resyncing desired state of custom resources.
defaultResyncPeriod: 0
Copy link
Member

Choose a reason for hiding this comment

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

Reasoning for this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed from latest code generator

Copy link
Member

Choose a reason for hiding this comment

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

Are these changes part of new code-gen or added personally?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed from latest code generator

Copy link
Contributor

@RedbackThomson RedbackThomson left a comment

Choose a reason for hiding this comment

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

A lot to follow, given there are code gen changes, updates to the testing framework and pending changes. I would have preferred we split these out, so that we can review each separately (and make it easier to revert if need be in the future).

Changes do seem to be pretty self explanatory, though

@ryansteakley
Copy link
Member

/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2023
@ack-prow
Copy link

ack-prow bot commented Aug 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RedbackThomson, ryansteakley, surajkota

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [RedbackThomson,ryansteakley,surajkota]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow bot merged commit fbb24e9 into aws-controllers-k8s:main Aug 7, 2023
ack-prow bot pushed a commit that referenced this pull request Aug 7, 2023
Description of changes:
Release artifacts for `v1.0.6`. Fixes #106 

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants