Adding ProvisionedConcurrency Support for Alias #97

Vandita2020 · 2023-07-12T16:26:19Z

Issue #, if available:

Summary
Adding support to configure ProvisionedConcurrency for alias in ACK.

Description
This PR adds support to configure ProvisionedConcurrency for Lambda function's alias using ACK. To implement this functionality I added ProvisionedConcurrencyConfig as an inline property to alias resource. Thus the user can directly set configurations for Provisioned Concurrency while creating alias. The user can edit the following code to set Provisioned Concurrency along with alias.

apiVersion: lambda.services.k8s.aws/v1alpha1
kind: Alias
metadata:
  name: $ALIAS_NAME
  annotations:
    services.k8s.aws/region: $AWS_REGION
spec:
  name: $ALIAS_NAME
  functionName: $FUNCTION_NAME
  functionVersion: $FUNCTION_VERSION
  description: alias created by ACK lambda-controller e2e tests
  provisionedConcurrencyConfig:
    provisionedConcurrentExecutions: $PROVISIONED_CONCURRENT_EXECUTIONS

This PR includes both implementation code and e2e tests.

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

ack-prow · 2023-07-12T16:26:22Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Vandita2020 · 2023-07-13T20:23:51Z

/test all

a-hilaly

Very good! Thank you @Vandita2020 !
I left few comments inline

a-hilaly · 2023-07-15T12:49:13Z

pkg/resource/alias/hooks.go

+	if desired.ko.Spec.ProvisionedConcurrencyConfig != nil {
+		if desired.ko.Spec.ProvisionedConcurrencyConfig.ProvisionedConcurrentExecutions != nil {
+			input.ProvisionedConcurrentExecutions = aws.Int64(*desired.ko.Spec.ProvisionedConcurrencyConfig.ProvisionedConcurrentExecutions)
+		} else {
+			input.ProvisionedConcurrentExecutions = aws.Int64(0)
+		}
+	}


What if the user sets spec.provisionedConcurrencyConfig to nil? we want to input.ProvisionedConcurrentExecutions = aws.Int64(0)

I have added the else condition to assign ProvisionedConcurrency value to 0 if user doesn't pass any value.

a-hilaly · 2023-07-15T12:49:48Z

pkg/resource/alias/hooks.go

+	if err != nil {
+		ko.Spec.ProvisionedConcurrencyConfig = nil
+	} else {


We should return an error instead of ignore it

Same thing for L137 below

This error is ignored intentionally, the error is returned by the API as there is no existing provisionedConcurrency set for the alias, and when it checks the current value in the Console, it doesn't find the configuration, as they are yet not created. So we wanted to ignore that error and instead assign the variable to nil on ACK side before we create it. To make the delta function know that there is a difference in state.

Same reason for FunctionEventInvokeConfig

In this case, can we ignore specifically NotFound errors? i'm thinking about cases where API returns something different than resource doesn't exist

Can you please share what is the exact error returned by the API?

a-hilaly · 2023-07-15T12:52:21Z

test/e2e/tests/test_alias.py

@@ -44,7 +44,7 @@ def lambda_function():
        replacements["BUCKET_NAME"] = resources.FunctionsBucket.name
        replacements["LAMBDA_ROLE"] = resources.EICRole.arn
        replacements["LAMBDA_FILE_NAME"] = LAMBDA_FUNCTION_FILE_ZIP
-        replacements["RESERVED_CONCURRENT_EXECUTIONS"] = "0"
+        replacements["RESERVED_CONCURRENT_EXECUTIONS"] = "3"


Why is this needed?

If we allocate both reserved concurrency and provisioned concurrency for the same function. Then, the amount of provisioned concurrency cannot exceed the amount of reserved concurrency.

a-hilaly · 2023-07-15T12:54:41Z

test/e2e/tests/test_alias.py

+        assert deleted
+
+        time.sleep(DELETE_WAIT_AFTER_SECONDS)
+


It would be nice to add another update to try to delete the provisionedConcurency field and check if the controller sets it to 0

The min value of provisionedConcurrentExecution needs to be 1, as mentioned here in the provisionedConcurrencyConfigInput type

a-hilaly · 2023-07-15T12:59:16Z

pkg/resource/alias/hooks.go

@@ -78,6 +109,21 @@ func (rm *resourceManager) setResourceAdditionalFields(
 	exit := rlog.Trace("rm.setResourceAdditionalFields")


setResourceAdditionalFields is getting huge, can you make shorter by putting some of it's logic into different functions? for example

func (rm *resourceManager) setResourceAdditionalFields(...) error { // ... ko.Spec.ProvisionnedConcurencyConfig, err = rm.getProvisionedConcurencyConfig(...) if err ! = nil { return nil } ko.Spec.FunctionEventInvokeConfig, err = rm.getFunctionInvokeConfig(...) if err ! = nil { return nil } // etc.... }

a-hilaly · 2023-07-15T12:59:53Z

apis/v1alpha1/alias.go

@@ -63,7 +63,8 @@ type AliasSpec struct {
 	FunctionVersion *string `json:"functionVersion"`
 	// The name of the alias.
 	// +kubebuilder:validation:Required
-	Name *string `json:"name"`
+	Name                         *string                               `json:"name"`
+	ProvisionedConcurrencyConfig *PutProvisionedConcurrencyConfigInput `json:"provisionedConcurrencyConfig,omitempty"`


Can we inject some documentation here?

a-hilaly · 2023-07-17T23:21:59Z

pkg/resource/alias/hooks.go

+	if err != nil {
+		return nil
+	}
+
 	return nil


a-hilaly · 2023-07-24T18:02:54Z

templates/hooks/alias/sdk_update_pre_build_request.go.tpl

+if !delta.DifferentExcept("Spec.ProvisionedConcurrencyConfig"){
    return desired, nil
 }


I think this should be come if !delta.DifferentExcept("Spec.ProvisionedConcurrencyConfig", "Spec.FunctionEventInvokeConfig") and L7-9 should be removed. Thoughts?

Writing them together makes sense. We just need an if condition to check the changes if any before executing the rest of the Update function.

a-hilaly

Great work on this @Vandita2020 ! 🚀
/lgtm

ack-prow · 2023-07-24T18:23:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, Vandita2020

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~~ [Vandita2020,a-hilaly]

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

Issue #, if available: Related PR: #97 **Summary** Fixing deletion part for `ProvisionedConcurrency` field. **Description:** This PR is related to bug fixes in [previous PR](#97) related to `ProvisionedConcurrency` implementation. When the user deleted the set configurations for Alias resource, the configurations were not deleted from the Console side, instead it used to throw an error. Fixed the bug by introducing a new condition to check if the desired configurations are nil or not, if yes then call the delete API. Example of changes for `ProvisionedConcurrency` in Alias resource is given below: ``` if desired.ko.Spec.ProvisionedConcurrencyConfig == nil { input_delete := &svcsdk.DeleteProvisionedConcurrencyConfigInput{ FunctionName: aws.String(*desired.ko.Spec.FunctionName), Qualifier: aws.String(*desired.ko.Spec.Name), } _, err = rm.sdkapi.DeleteProvisionedConcurrencyConfigWithContext(ctx, input_delete) rm.metrics.RecordAPICall("DELETE", "DeleteProvisionedConcurrency", err) if err != nil { return err } return nil } ``` This PR contains implementation code and E2E test to validate the changes. **Acknowledgment** By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Issue #, if available: Related PR: #92 #93 **Summary** Fixing deletion part for `EventInvokeConfig` field. **Description:** This PR is related to bug fixes in [previous PR](#97) related to `EventInvokeConfig` implementation. When the user deleted the set configurations for Function/Alias resource, the configurations were not deleted from the Console side, instead it used to throw an error. Fixed the bug by introducing a new condition to check if the desired configurations are nil or not, if yes then call the delete API. Example of changes for `EventInvokeConfig` in Alias resource is given below: ``` if r.ko.Spec.FunctionEventInvokeConfig == nil { input_delete := &svcsdk.DeleteFunctionEventInvokeConfigInput{ FunctionName: aws.String(*r.ko.Spec.FunctionName), Qualifier: aws.String(*r.ko.Spec.Name), } _, err = rm.sdkapi.DeleteFunctionEventInvokeConfigWithContext(ctx, input_delete) rm.metrics.RecordAPICall("DELETE", "DeleteFunctionEventInvokeConfig", err) if err != nil { return nil, err } return r, nil } ``` This PR contains implementation code and E2E test to validate the changes. **Acknowledgment** By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2023

ack-prow bot requested review from jljaco and RedbackThomson July 12, 2023 16:26

Vandita2020 force-pushed the provisioned_concurrecny_config_alias branch 18 times, most recently from c596c26 to 02f3de0 Compare July 13, 2023 20:23

Vandita2020 marked this pull request as ready for review July 13, 2023 20:59

ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 13, 2023

ack-prow bot requested a review from a-hilaly July 13, 2023 20:59

a-hilaly reviewed Jul 15, 2023

View reviewed changes

Vandita2020 force-pushed the provisioned_concurrecny_config_alias branch from 02f3de0 to 1814ecf Compare July 17, 2023 23:02

a-hilaly reviewed Jul 17, 2023

View reviewed changes

pkg/resource/alias/hooks.go

if err != nil {

return nil

}

return nil

Copy link

Member

a-hilaly Jul 17, 2023

Choose a reason for hiding this comment

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

👍

Vandita2020 force-pushed the provisioned_concurrecny_config_alias branch from 305ba44 to e751f64 Compare July 24, 2023 17:17

ack-prow bot added the approved label Jul 24, 2023

a-hilaly reviewed Jul 24, 2023

View reviewed changes

Adding ProvisionedConcurrency Support for Alias

d090b9e

Vandita2020 force-pushed the provisioned_concurrecny_config_alias branch from e751f64 to d090b9e Compare July 24, 2023 18:21

a-hilaly approved these changes Jul 24, 2023

View reviewed changes

ack-prow bot assigned a-hilaly Jul 24, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2023

ack-prow bot merged commit 00df311 into aws-controllers-k8s:main Jul 24, 2023

This was referenced Aug 1, 2023

Bug fix for ProvisionedConcurrency #106

Merged

Bug fix for EventInvokeConfig #107

Merged

		@@ -78,6 +109,21 @@ func (rm *resourceManager) setResourceAdditionalFields(
		exit := rlog.Trace("rm.setResourceAdditionalFields")

Adding ProvisionedConcurrency Support for Alias #97

Adding ProvisionedConcurrency Support for Alias #97

Uh oh!

Conversation

Vandita2020 commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-prow bot commented Jul 12, 2023

Uh oh!

Vandita2020 commented Jul 13, 2023

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

ack-prow bot commented Jul 24, 2023

Uh oh!

Uh oh!

Vandita2020 commented Jul 12, 2023 •

edited

Loading