Skip to content

Bug fix for ProvisionedConcurrency #106

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 21, 2023
Merged

Bug fix for ProvisionedConcurrency #106

merged 1 commit into from
Aug 21, 2023

Conversation

Vandita2020
Copy link
Member

@Vandita2020 Vandita2020 commented Jul 31, 2023 •
edited
Loading

Issue #, if available:
Related PR: #97

Summary
Fixing deletion part for ProvisionedConcurrency field.

Description:
This PR is related to bug fixes in previous PR 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.

@ack-prow ack-prow bot requested review from RedbackThomson and vijtrip2 July 31, 2023 18:59
@ack-prow ack-prow bot added the approved label Jul 31, 2023
@Vandita2020 Vandita2020 force-pushed the provisioned_concurrency_correction branch 5 times, most recently from aa2a5e7 to c4c0fc4 Compare August 2, 2023 22:57
@Vandita2020 Vandita2020 changed the title Bug fix for ProvisionedConcurrency and FunctionEventInvokeConfig Bug fix for ProvisionedConcurrency Aug 2, 2023
valerena
valerena reviewed Aug 15, 2023
View reviewed changes
test/e2e/tests/test_function.py Outdated
@@ -395,7 +395,7 @@ def test_function_package_type_image_with_signing_config(self, lambda_client):

time.sleep(UPDATE_WAIT_AFTER_SECONDS)

cr = k8s.wait_resource_consumed_by_controller(ref)
# cr = k8s.wait_resource_consumed_by_controller(ref)
Copy link
Member

Choose a reason for hiding this comment

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

Is this okay to be commented out?

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, this line of code waits for resource to be consumed after update and store the new status in "cr". We are doing the same thing in Line 407 too. The idea is when the first update is made, we wait and let the controller consume the resource, save the new status in cr and use it for second update. Either of the place for this line of code would work fine. I just chose to do it right before moving for the second update. Keeping both increases the time of execution and lead to timeout while running the tests.

Copy link
Member

Choose a reason for hiding this comment

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

I see. In that case, we should delete one instead of commenting it

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

pkg/resource/alias/hooks.go Outdated
Comment on lines 217 to 214
// To set Asynchronous Invocations for the function's alias
err = rm.getFunctionEventInvokeConfig(ctx, ko)
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

I guess this change doesn't belong into this PR, because it's related to event invoke config. Either we put everything together in one PR, or we actually split in two (both the changes to this file are added in both PRs)

Copy link
Member Author

@Vandita2020 Vandita2020 Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This change was just regarding changing the name of eic_err and pc_err field to err, that's why I made changes for both. I can leave it the same for EventInvokeConfig in this PR and change only for ProvisionedConcurrency.

pkg/resource/alias/hooks.go Outdated
@@ -183,6 +204,8 @@ func (rm *resourceManager) getFunctionEventInvokeConfig(
return nil
}

// setResourceAdditionalFields will describe the fields that are not return by
Copy link
Member

Choose a reason for hiding this comment

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

I think the comment would be more accurate if it said ".. the fields that are not returned by the getFunctionConfiguration API call" (Considering that the info here is still returned by API calls)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that makes more sense.

@Vandita2020 Vandita2020 force-pushed the provisioned_concurrency_correction branch 4 times, most recently from 7481094 to 9a6243c Compare August 16, 2023 22:26
@Vandita2020 Vandita2020 force-pushed the provisioned_concurrency_correction branch 2 times, most recently from e76aa73 to 0402545 Compare August 17, 2023 22:10
a-hilaly
a-hilaly reviewed Aug 21, 2023
View reviewed changes
test/e2e/tests/test_function.py Outdated
@@ -395,7 +395,6 @@ def test_function_package_type_image_with_signing_config(self, lambda_client):

time.sleep(UPDATE_WAIT_AFTER_SECONDS)

cr = k8s.wait_resource_consumed_by_controller(ref)
Copy link
Member

Choose a reason for hiding this comment

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

nit: we don't need to remove this. Maybe it's safer to keep it?

pkg/resource/alias/hooks.go Outdated
}
} else {
input.ProvisionedConcurrentExecutions = aws.Int64(0)
if dspec.ProvisionedConcurrencyConfig.ProvisionedConcurrentExecutions != nil {
Copy link
Member

Choose a reason for hiding this comment

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

What if the user sets dspec.ProvisionedConcurrencyConfig.ProvisionedConcurrentExecutions to nil ? is that equivalent to setting the value to 0 or 1, or maybe delete it fully ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the user sets ProvisionedConcurrency to 1, they will get the error as ProvisionedConcurrency must be greater than 1. For nil, I added the condition along with nil checking for ProvisionedConcurrency in the beginning of the updateProvisionedConcurrency function.

Copy link
Member

Choose a reason for hiding this comment

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

Since you added if desired.ko.Spec.ProvisionedConcurrencyConfig == nil || desired.ko.Spec.ProvisionedConcurrencyConfig.ProvisionedConcurrentExecutions == nil { can we remove this if statement now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

pkg/resource/alias/hooks.go
func (rm *resourceManager) getProvisionedConcurrencyConfig(
// setProvisionedConcurrencyConfig sets the Provisioned Concurrency
// for the Function's Alias
func (rm *resourceManager) setProvisionedConcurrencyConfig(
Copy link
Member

Choose a reason for hiding this comment

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

Good function rename

@Vandita2020 Vandita2020 force-pushed the provisioned_concurrency_correction branch 2 times, most recently from 68c25de to d7d9d0a Compare August 21, 2023 22:38
@Vandita2020 Vandita2020 force-pushed the provisioned_concurrency_correction branch from d7d9d0a to cc53cbb Compare August 21, 2023 22:42
a-hilaly
a-hilaly approved these changes Aug 21, 2023
View reviewed changes
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Vandita2020! & @valerena !
/lgtm
/approve

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

ack-prow bot commented Aug 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, valerena, 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,valerena]

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

@a-hilaly a-hilaly mentioned this pull request Aug 21, 2023
Merged
@ack-prow ack-prow bot merged commit c4e68a1 into aws-controllers-k8s:main Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-hilaly a-hilaly a-hilaly approved these changes

@valerena valerena valerena approved these changes

@RedbackThomson RedbackThomson Awaiting requested review from RedbackThomson

@vijtrip2 vijtrip2 Awaiting requested review from vijtrip2

Assignees

@a-hilaly a-hilaly

Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vandita2020 @a-hilaly @valerena