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
10 changes: 5 additions & 5 deletions apis/v1alpha1/ack-generate-metadata.yaml
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
ack_generate_info:
build_date: "2023-05-15T22:45:51Z"
build_hash: 8f3ba427974fd6e769926778d54834eaee3b81a3
go_version: go1.19
version: v0.26.1
build_date: "2023-08-07T18:42:42Z"
build_hash: 7081d1c2e797bb7c8b1e4e6a298d1d85f25a07bb
go_version: go1.20.1
version: v0.26.1-8-g7081d1c
api_directory_checksum: 81d152c4602b014d435a9ba3d716ed5112273013
api_version: v1alpha1
aws_sdk_go_version: v1.44.93
generator_config_info:
file_checksum: b2ac33cc79c75dfc65066ea3fca66ad781d4ae18
file_checksum: dd979bc999bfaa10546ce09b0bedecd039ea85c7
original_file_name: generator.yaml
last_modification:
reason: API generation
6 changes: 6 additions & 0 deletions apis/v1alpha1/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ operations:
resource_name: ScalableTarget
DescribeScalableTargets:
custom_check_required_fields_missing_method: customCheckRequiredFieldsMissing
DescribeScalingPolicies:
custom_check_required_fields_missing_method: customCheckRequiredFieldsMissing
resources:
ScalableTarget:
hooks:
Expand Down Expand Up @@ -50,6 +52,10 @@ resources:
code: rm.customSetLastModifiedTimeToCreationTime(ko)
sdk_update_post_set_output:
code: rm.customSetLastModifiedTimeToCurrentTime(ko)
sdk_read_many_post_build_request:
code: rm.customSetDescribeScalingPoliciesInput(ctx, r, input)
post_set_resource_identifiers:
template_path: scaling_policy/post_set_resource_identifiers.go.tpl
fields:
ResourceID:
is_primary_key: true
Expand Down
6 changes: 6 additions & 0 deletions generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ operations:
resource_name: ScalableTarget
DescribeScalableTargets:
custom_check_required_fields_missing_method: customCheckRequiredFieldsMissing
DescribeScalingPolicies:
custom_check_required_fields_missing_method: customCheckRequiredFieldsMissing
resources:
ScalableTarget:
hooks:
Expand Down Expand Up @@ -50,6 +52,10 @@ resources:
code: rm.customSetLastModifiedTimeToCreationTime(ko)
sdk_update_post_set_output:
code: rm.customSetLastModifiedTimeToCurrentTime(ko)
sdk_read_many_post_build_request:
code: rm.customSetDescribeScalingPoliciesInput(ctx, r, input)
post_set_resource_identifiers:
template_path: scaling_policy/post_set_resource_identifiers.go.tpl
fields:
ResourceID:
is_primary_key: true
Expand Down
15 changes: 14 additions & 1 deletion helm/templates/deployment.yaml
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

Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ spec:
app.kubernetes.io/instance: {{ .Release.Name }}
template:
metadata:
{{- if .Values.deployment.annotations }}
annotations:
{{- range $key, $value := .Values.deployment.annotations }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
labels:
app.kubernetes.io/name: {{ include "app.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
Expand Down Expand Up @@ -104,11 +106,19 @@ spec:
value: {{ include "aws.credentials.path" . }}
- name: AWS_PROFILE
value: {{ .Values.aws.credentials.profile }}
{{- end }}
{{- if .Values.deployment.extraEnvVars -}}
{{ toYaml .Values.deployment.extraEnvVars | nindent 8 }}
{{- end }}
volumeMounts:
{{- if .Values.aws.credentials.secretName }}
- name: {{ .Values.aws.credentials.secretName }}
mountPath: {{ include "aws.credentials.secret_mount_path" . }}
readOnly: true
{{- end }}
{{- if .Values.deployment.extraVolumeMounts -}}
{{ toYaml .Values.deployment.extraVolumeMounts | nindent 12 }}
{{- end }}
securityContext:
allowPrivilegeEscalation: false
privileged: false
Expand All @@ -133,9 +143,12 @@ spec:
hostIPC: false
hostNetwork: false
hostPID: false
{{ if .Values.aws.credentials.secretName -}}
volumes:
{{- if .Values.aws.credentials.secretName -}}
- name: {{ .Values.aws.credentials.secretName }}
secret:
secretName: {{ .Values.aws.credentials.secretName }}
{{ end -}}
{{- if .Values.deployment.extraVolumes }}
{{ toYaml .Values.deployment.extraVolumes | indent 8}}
{{- end }}
9 changes: 9 additions & 0 deletions helm/values.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@
},
"priorityClassName": {
"type": "string"
},
"extraVolumeMounts": {
"type": "array"
},
"extraVolumes": {
"type": "array"
},
"extraEnvVars": {
"type": "array"
}
},
"required": [
Expand Down
22 changes: 21 additions & 1 deletion helm/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,26 @@ deployment:
# Which priorityClassName to set?
# See: https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#pod-priority
priorityClassName: ""
extraVolumes: []
extraVolumeMounts: []

# Additional server container environment variables
#
# You specify this manually like you would a raw deployment manifest.
# This means you can bind in environment variables from secrets.
#
# e.g. static environment variable:
# - name: DEMO_GREETING
# value: "Hello from the environment"
#
# e.g. secret environment variable:
# - name: USERNAME
# valueFrom:
# secretKeyRef:
# name: mysecret
# key: username
extraEnvVars: []


# If "installScope: cluster" then these labels will be applied to ClusterRole
role:
Expand Down Expand Up @@ -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

defaultResyncPeriod: 36000 # 10 Hours
# An object representing the reconcile resync configuration for each specific resource.
resourceResyncPeriods: {}

Expand Down
32 changes: 31 additions & 1 deletion pkg/resource/scaling_policy/custom_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@
package scaling_policy

import (
"context"
"time"

svcapitypes "github.com/aws-controllers-k8s/applicationautoscaling-controller/apis/v1alpha1"
svcsdk "github.com/aws/aws-sdk-go/service/applicationautoscaling"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"time"
)

// customSetLastModifiedTimeToCreationTime sets the LastModifiedTime field to the creationTime
Expand All @@ -31,3 +34,30 @@ func (rm *resourceManager) customSetLastModifiedTimeToCurrentTime(ko *svcapitype
currentTime := metav1.Time{Time: time.Now().UTC()}
ko.Status.LastModifiedTime = &currentTime
}

// customSetDescribeScalingPoliciesInput sets the policy name in DescribeScalingPoliciesInput
func (rm *resourceManager) customSetDescribeScalingPoliciesInput(
ctx context.Context,
latest *resource,
input *svcsdk.DescribeScalingPoliciesInput,
) {
spec := latest.ko.Spec

var policyNames []*string
if spec.PolicyName != nil {
policyNames = append(policyNames, spec.PolicyName)
input.SetPolicyNames(policyNames)
}
}

// customCheckRequiredFieldsMissing returns true if there are any fields
// for the ReadOne Input shape that are required but not present in the
// resource's Spec or Status
func (rm *resourceManager) customCheckRequiredFieldsMissing(
r *resource,
) bool {
if r.ko.Spec.ResourceID == nil || r.ko.Spec.ScalableDimension == nil || r.ko.Spec.ServiceNamespace == nil || r.ko.Spec.PolicyName == nil {
return true
}
return false
}
4 changes: 4 additions & 0 deletions pkg/resource/scaling_policy/resource.go

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

3 changes: 2 additions & 1 deletion pkg/resource/scaling_policy/sdk.go

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

2 changes: 1 addition & 1 deletion pkg/resource/scaling_policy/testdata/test_suite.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ tests:
message: "resource not found"
invoke: ReadOne
expect:
error: ": resource not found\n\tstatus code: 0, request id: "
error: "resource not found"
- name: "ReadOne=NotFound"
description: "Testing ReadOne when Describe fails to find the resource on SageMaker"
given:
Expand Down
4 changes: 4 additions & 0 deletions templates/scaling_policy/post_set_resource_identifiers.go.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
f6, f6ok := identifier.AdditionalKeys["policyName"]
if f6ok {
r.ko.Spec.PolicyName = &f6
}
20 changes: 9 additions & 11 deletions test/e2e/bootstrap_resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,29 +16,27 @@
"""

from dataclasses import dataclass
from acktest.resources import read_bootstrap_config
from acktest.bootstrapping import Resources
from acktest.bootstrapping.iam import Role
from acktest.bootstrapping.s3 import Bucket
from e2e import bootstrap_directory

SAGEMAKER_SOURCE_DATA_BUCKET = "source-data-bucket-592697580195-us-west-2"


@dataclass
class TestBootstrapResources:
SageMakerDataBucketName: str
SageMakerExecutionRoleARN: str
ScalableDynamoTableName: str # To be used for testing RegisterScalableTarget
RegisteredDynamoTableName: str # Already registered, for testing scaling policies
class TestBootstrapResources(Resources):
DataBucket: Bucket
SageMakerExecutionRole: Role


_bootstrap_resources = None


def get_bootstrap_resources(bootstrap_file_name: str = "bootstrap.yaml"):
def get_bootstrap_resources(bootstrap_file_name: str = "bootstrap.pkl"):
global _bootstrap_resources
if _bootstrap_resources is None:
_bootstrap_resources = TestBootstrapResources(
**read_bootstrap_config(
bootstrap_directory, bootstrap_file_name=bootstrap_file_name
),
_bootstrap_resources = TestBootstrapResources.deserialize(
bootstrap_directory, bootstrap_file_name=bootstrap_file_name
)
return _bootstrap_resources
4 changes: 2 additions & 2 deletions test/e2e/common/sagemaker_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,12 @@ def sagemaker_make_model(model_name):
return model_input, model_response


def sagemaker_make_endpoint_config(model_name, endpoint_config_name):
def sagemaker_make_endpoint_config(model_name, variant_name, endpoint_config_name):
endpoint_config_input = {
"EndpointConfigName": endpoint_config_name,
"ProductionVariants": [
{
"VariantName": "variant-1",
"VariantName": variant_name,
"ModelName": model_name,
"InitialInstanceCount": 1,
"InstanceType": REPLACEMENT_VALUES["ENDPOINT_INSTANCE_TYPE"],
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/replacement_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@
}

REPLACEMENT_VALUES = {
"SAGEMAKER_DATA_BUCKET": get_bootstrap_resources().SageMakerDataBucketName,
"SAGEMAKER_EXECUTION_ROLE_ARN": get_bootstrap_resources().SageMakerExecutionRoleARN,
"SAGEMAKER_DATA_BUCKET": get_bootstrap_resources().DataBucket.name,
"SAGEMAKER_EXECUTION_ROLE_ARN": get_bootstrap_resources().SageMakerExecutionRole.arn,
"SAGEMAKER_XGBOOST_IMAGE_URI": f"{SAGEMAKER_XGBOOST_IMAGE_URIS[get_region()]}/sagemaker-xgboost:1.0-1-cpu-py3",
"ENDPOINT_INSTANCE_TYPE": ENDPOINT_INSTANCE_TYPES.get(get_region(), 'ml.c5.large'),
}
2 changes: 1 addition & 1 deletion test/e2e/requirements.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
acktest @ git+https://github.com/aws-controllers-k8s/test-infra.git@a14cabd5be76c8367792e27357ff4e9e203d880c
acktest @ git+https://github.com/aws-controllers-k8s/test-infra.git@96f7481047da87c4ebd97540b051b7b1dd664498
10 changes: 0 additions & 10 deletions test/e2e/resources/dynamodb_scalabletarget.yaml

This file was deleted.

16 changes: 0 additions & 16 deletions test/e2e/resources/dynamodb_scalingpolicy.yaml

This file was deleted.

2 changes: 2 additions & 0 deletions test/e2e/resources/sagemaker_endpoint_adopted_policy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ spec:
nameOrID: $RESOURCE_ID
additionalKeys:
serviceNamespace: sagemaker
scalableDimension: sagemaker:variant:DesiredInstanceCount
policyName: $POLICY_NAME
kubernetes:
group: applicationautoscaling.services.k8s.aws
kind: ScalingPolicy
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
apiVersion: applicationautoscaling.services.k8s.aws/v1alpha1
kind: ScalingPolicy
metadata:
name: $SCALINGPOLICY_NAME
spec:
policyName: $SCALINGPOLICY_NAME
policyType: TargetTrackingScaling
resourceID: $RESOURCE_ID
scalableDimension: "sagemaker:variant:DesiredInstanceCount"
serviceNamespace: sagemaker
targetTrackingScalingPolicyConfiguration:
targetValue: 60
scaleInCooldown: 700
scaleOutCooldown: 300
customizedMetricSpecification:
metricName: CPUUtilization
namespace: /aws/sagemaker/Endpoints
dimensions:
- name: EndpointName
value: $ENDPOINT_NAME
- name: VariantName
value: $VARIANT_NAME
statistic: Average
unit: Percent
Loading