update: support to wait to update function #4565

sivchari · 2023-08-24T12:39:48Z

What this PR does / why we need it:

Last patch. I support to wait to update lambda function.
If lambda function state is pending, UpdateFunctionCode is failed. So I check the lambda satate.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

khanhtc1202 · 2023-08-25T03:36:21Z

pkg/app/piped/platformprovider/lambda/client.go

 	"github.com/aws/aws-sdk-go-v2/service/lambda/types"
-	"go.uber.org/zap"
-
+	helper "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry"


Let's not use hashicorp retry but pipecd package 👀
ref: https://github.com/pipe-cd/pipecd/blob/master/pkg/backoff/backoff.go

I use retry package to get lambda function state.

codecov · 2023-08-25T09:28:21Z

Codecov Report

Patch coverage has no change and project coverage change: -0.03% ⚠️

Comparison is base (9f75446) 29.91% compared to head (e3ed289) 29.88%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4565      +/-   ##
==========================================
- Coverage   29.91%   29.88%   -0.03%     
==========================================
  Files         220      220              
  Lines       25873    25892      +19     
==========================================
- Hits         7741     7739       -2     
- Misses      17484    17506      +22     
+ Partials      648      647       -1

Files Changed	Coverage Δ
pkg/app/piped/platformprovider/lambda/client.go	`4.55% <0.00%> (-0.18%)`	⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sivchari · 2023-08-25T09:45:55Z

@khanhtc1202 @kentakozuka
I'm ready to merge this PR 🕺

khanhtc1202 · 2023-08-25T09:49:12Z

@sivchari Could you remove grpc update and go packages update from this PR 👀

khanhtc1202 · 2023-08-25T09:53:01Z

pkg/app/piped/platformprovider/lambda/client.go

+	retry := backoff.NewRetry(RequestRetryTime, backoff.NewConstant(RetryIntervalDuration))
+	input := &lambda.GetFunctionInput{
+		FunctionName: aws.String(functionName),
+	}
+	if _, err := retry.Do(ctx, func() (any, error) {
+		output, err := c.client.GetFunction(ctx, input)
+		if err != nil {
+			return nil, err
+		}
+		if output.Configuration.LastUpdateStatus != types.LastUpdateStatusSuccessful {
+			return nil, fmt.Errorf("failed to update Lambda function %s, status code %v, error reason %s",
+				functionName, output.Configuration.LastUpdateStatus, *output.Configuration.LastUpdateStatusReason)
+		}
+		return nil, nil
+	}); err != nil {
+		return err
+	}


Suggested change

retry := backoff.NewRetry(RequestRetryTime, backoff.NewConstant(RetryIntervalDuration))

input := &lambda.GetFunctionInput{

FunctionName: aws.String(functionName),

}

if _, err := retry.Do(ctx, func() (any, error) {

output, err := c.client.GetFunction(ctx, input)

if err != nil {

return nil, err

}

if output.Configuration.LastUpdateStatus != types.LastUpdateStatusSuccessful {

return nil, fmt.Errorf("failed to update Lambda function %s, status code %v, error reason %s",

functionName, output.Configuration.LastUpdateStatus, *output.Configuration.LastUpdateStatusReason)

}

return nil, nil

}); err != nil {

return err

}

input := &lambda.GetFunctionInput{

FunctionName: aws.String(functionName),

}

retry := backoff.NewRetry(RequestRetryTime, backoff.NewConstant(RetryIntervalDuration))

_, err := retry.Do(ctx, func() (interface{}, error) {

output, err := c.client.GetFunction(ctx, input)

if err != nil {

return nil, err

}

if output.Configuration.LastUpdateStatus != types.LastUpdateStatusSuccessful {

return nil, fmt.Errorf("failed to get Lambda function %s, status code %v, error reason %s",

functionName, output.Configuration.LastUpdateStatus, *output.Configuration.LastUpdateStatusReason)

}

return nil, nil

})

return err

}

How about this?

khanhtc1202 · 2023-08-25T09:55:40Z

pkg/app/piped/platformprovider/lambda/client.go

+	return nil
+}
+
+func (c *client) waitFunctionUpdated(ctx context.Context, functionName string, timeout time.Duration) error {


this timeout is not being used

sivchari · 2023-08-25T10:30:41Z

@khanhtc1202
Thanks, I fixed these comments.

khanhtc1202 · 2023-08-25T10:39:23Z

btw, @sivchari due to the comment here
Should we rearrange this function as well (to be called before the UpdateFunctionCode)
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/platformprovider/lambda/client.go#L252-L255

Signed-off-by: sivchari <[email protected]>

sivchari · 2023-08-25T10:51:55Z

@khanhtc1202
This answer is very strong Yes ! I rearranged it.

khanhtc1202 · 2023-08-25T10:57:40Z

pkg/app/piped/platformprovider/lambda/client.go

+	// Update function configuration.
+	if err := c.updateFunctionConfiguration(ctx, fm); err != nil {
+		return err
+	}
+


Probably should be at the top of this function (before L230 reading zip) to avoid unnecessary disk read if the update function configuration returns error.

You're right. Thnaks.

khanhtc1202 · 2023-08-25T11:03:16Z

pkg/app/piped/platformprovider/lambda/client.go

+	return nil
+}
+
+func (c *client) waitFunctionUpdated(ctx context.Context, functionName string) error {


How about moving this function body to L298? Looks like this function is specified to be used only by updateFunctionConfiguration, so I don't think we have enough reason to separate it as an independent function 🤔 Also, the current function name is quite common, waitFunctionUpdated while we have many UpdateFunctionXXX, which could cause misleading. So removing this function and moving all of its body to be part of updateFunctionConfiguration could be a better choice if you only want to wait after updating function configuration. wdyt?

Okay.

Also, the current function name is quite common, waitFunctionUpdated while we have many UpdateFunctionXXX, which could cause misleading.

I thinks so too.

khanhtc1202

Look neat, just left nits comments 😀

pkg/app/piped/platformprovider/lambda/client.go

khanhtc1202 · 2023-08-25T14:40:25Z

pkg/app/piped/platformprovider/lambda/client.go

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


return err

nits

khanhtc1202 · 2023-08-26T03:40:46Z

pkg/app/piped/platformprovider/lambda/client.go

+	input := &lambda.GetFunctionInput{
+		FunctionName: aws.String(fm.Spec.Name),
+	}
+	_, err = retry.Do(ctx, func() (any, error) {


Sorry, I just noticed that we reused the previous retry object here. I think we should recreate a new one instead since the same object may contain previously updated backoff internal, thus the number of retry times is not as expected.

Suggested change

_, err = retry.Do(ctx, func() (any, error) {

retry = backoff.NewRetry(RequestRetryTime, backoff.NewConstant(RetryIntervalDuration))

_, err = retry.Do(ctx, func() (any, error) {

@khanhtc1202
I fixed it.

khanhtc1202 · 2023-08-26T03:41:53Z

@sivchari After this got merge, I will release v0.45.3-rc1 then you can test it before we make release v0.45.3 😄

Signed-off-by: sivchari <[email protected]>

khanhtc1202

LGTM, thanks 🍏

* update: support to wait to update function Signed-off-by: sivchari <[email protected]> * rearrange: updatefunctionfromsource Signed-off-by: sivchari <[email protected]> --------- Signed-off-by: sivchari <[email protected]>

* update: support to wait to update function (#4565) * update: support to wait to update function Signed-off-by: sivchari <[email protected]> * rearrange: updatefunctionfromsource Signed-off-by: sivchari <[email protected]> --------- Signed-off-by: sivchari <[email protected]> * Wait ECS taskset stable (#4573) * Wait ECS taskset stable Signed-off-by: khanhtc1202 <[email protected]> * Change number of retry time Signed-off-by: khanhtc1202 <[email protected]> * No sleep required Signed-off-by: khanhtc1202 <[email protected]> --------- Signed-off-by: khanhtc1202 <[email protected]> --------- Signed-off-by: sivchari <[email protected]> Signed-off-by: khanhtc1202 <[email protected]> Co-authored-by: sivchari <[email protected]>

* update: support to wait to update function Signed-off-by: sivchari <[email protected]> * rearrange: updatefunctionfromsource Signed-off-by: sivchari <[email protected]> --------- Signed-off-by: sivchari <[email protected]> Signed-off-by: moko-poi <[email protected]>

* update: support to wait to update function Signed-off-by: sivchari <[email protected]> * rearrange: updatefunctionfromsource Signed-off-by: sivchari <[email protected]> --------- Signed-off-by: sivchari <[email protected]>

sivchari requested review from kentakozuka, khanhtc1202 and nghialv as code owners August 24, 2023 12:39

github-actions bot added the area/go label Aug 24, 2023

sivchari force-pushed the support-wait-function-update branch 2 times, most recently from c9502f9 to 4380779 Compare August 24, 2023 12:50

khanhtc1202 reviewed Aug 25, 2023

View reviewed changes

sivchari force-pushed the support-wait-function-update branch 3 times, most recently from a82e690 to f20ea30 Compare August 25, 2023 09:23

sivchari force-pushed the support-wait-function-update branch from f20ea30 to def49fe Compare August 25, 2023 09:36

khanhtc1202 reviewed Aug 25, 2023

View reviewed changes

sivchari force-pushed the support-wait-function-update branch from 9b0afdb to f645600 Compare August 25, 2023 10:30

update: support to wait to update function

91f0475

Signed-off-by: sivchari <[email protected]>

sivchari force-pushed the support-wait-function-update branch from f645600 to 91f0475 Compare August 25, 2023 10:50

khanhtc1202 reviewed Aug 25, 2023

View reviewed changes

sivchari force-pushed the support-wait-function-update branch from 6a6e441 to 5f278a9 Compare August 25, 2023 10:59

khanhtc1202 reviewed Aug 25, 2023

View reviewed changes

sivchari force-pushed the support-wait-function-update branch from 5f278a9 to c577c6e Compare August 25, 2023 11:07

khanhtc1202 reviewed Aug 25, 2023

View reviewed changes

sivchari force-pushed the support-wait-function-update branch from c577c6e to 32f955b Compare August 25, 2023 17:45

khanhtc1202 reviewed Aug 26, 2023

View reviewed changes

rearrange: updatefunctionfromsource

e3ed289

Signed-off-by: sivchari <[email protected]>

sivchari force-pushed the support-wait-function-update branch from 32f955b to e3ed289 Compare August 28, 2023 03:25

khanhtc1202 approved these changes Aug 28, 2023

View reviewed changes

khanhtc1202 merged commit 6e9660d into pipe-cd:master Aug 28, 2023

khanhtc1202 mentioned this pull request Aug 28, 2023

Cherry-pick #4565 #4573 #4576

Merged

github-actions bot mentioned this pull request Dec 1, 2023

Release v0.45.4 #4697

Merged

github-actions bot mentioned this pull request Feb 6, 2024

Release v0.46.0 #4775

Merged

	_, err = retry.Do(ctx, func() (any, error) {
	retry = backoff.NewRetry(RequestRetryTime, backoff.NewConstant(RetryIntervalDuration))
	_, err = retry.Do(ctx, func() (any, error) {

update: support to wait to update function #4565

update: support to wait to update function #4565

Uh oh!

Conversation

sivchari commented Aug 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sivchari commented Aug 25, 2023

Uh oh!

khanhtc1202 commented Aug 25, 2023

Uh oh!

khanhtc1202 Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivchari commented Aug 25, 2023

Uh oh!

khanhtc1202 commented Aug 25, 2023

Uh oh!

sivchari commented Aug 25, 2023

Uh oh!

khanhtc1202 Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

khanhtc1202 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

khanhtc1202 commented Aug 26, 2023

Uh oh!

khanhtc1202 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Aug 25, 2023 •

edited

Loading

khanhtc1202 Aug 25, 2023 •

edited

Loading

khanhtc1202 Aug 25, 2023 •

edited

Loading