Reimplement Lambda deployment QUICK_SYNC strategy #1443
Conversation
|
Code coverage for golang is
|
|
Code coverage for golang is
|
khanhtc1202
changed the title
|
Code coverage for golang is
|
| // Wait before ready to commit change. | ||
| in.LogPersister.Info("Waiting to update lambda function in progress...") | ||
| time.Sleep(3 * time.Minute) |
There was a problem hiding this comment.
fyi, the Lambda's CreateFunction/UpdateFunction API call returns right after we make a request to update the Lambda function code/config, but the update process still running internally in Lambda servers. If we make PublishFunction request immediately, the error ResourceConflictException: The operation cannot be performed at this time. will be raised.
There was a problem hiding this comment.
Really? 😢 I think fixed 3 minutes is not safe for all cases, but we don't know how much is good.
So I think in addition to waiting for a fixed amount of time (e.g. 1m) we should also do PublishFunction with a constant backoff.
https://github.com/pipe-cd/pipe/tree/master/pkg/backoff
There was a problem hiding this comment.
I'm working on it, but shall we make another PR to fix this separately from this PR 👀
There was a problem hiding this comment.
Yes, that is ok. Let's add a TODO and do it in another PR.
|
|
||
| if err := client.Apply(ctx, fm); err != nil { | ||
| in.LogPersister.Errorf("Failed to apply the lambda function manifest (%v)", err) | ||
| ok, err := client.AvailableFunctionName(ctx, fm.Spec.Name) |
There was a problem hiding this comment.
Maybe we should rename this function AvailableFunctionName.
How about "IsFunctionExisting" or "FunctionExists"?
There was a problem hiding this comment.
thanks, I will go for IsFunctionExist 👍
| return true | ||
| } | ||
| if err != nil { | ||
| in.LogPersister.Errorf("Failed to prapare traffic routing for Lambda function %s: %v", fm.Spec.Name, err) |
| PublishFunction(ctx context.Context, fm FunctionManifest) (version string, err error) | ||
| GetTrafficConfig(ctx context.Context, fm FunctionManifest) (routingTrafficCfg []VersionTraffic, err error) | ||
| CreateRoutingTraffic(ctx context.Context, fm FunctionManifest, version string) error | ||
| UpdateRoutingTraffic(ctx context.Context, fm FunctionManifest, routingTraffic []VersionTraffic) error |
There was a problem hiding this comment.
nit: How about CreateTrafficConfig and UpdateTrafficConfig?
| time.Sleep(3 * time.Minute) | ||
|
|
||
| // Commit version for applied Lambda function. | ||
| version, err := client.PublishFunction(ctx, fm) |
There was a problem hiding this comment.
I'm concerned that when users clicked the SYNC button to sync the application without any changes.
In that case, no changes were added on AWS Lambda side and it is ok to publish the function?
I think we may receive a "NoChange" error or something like that.
There was a problem hiding this comment.
AWS Lambda doesn't publish a version if the function's configuration and code haven't changed since the last version. Use UpdateFunctionCode or UpdateFunctionConfiguration to update the function before publishing a version.
Yes, you guess right, due to the docs of PublishVersion, we have to make some changes on source/configuration of the function to be able to publish a new version of it. So the error returned here for that case is predictable. 👍
There was a problem hiding this comment.
Added a note about this lack on the AWS documentation.
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Code coverage for golang is
|
| return fmt.Errorf("unknown error given: %w", err) | ||
| } | ||
|
|
||
| // TODO more configurable fields from here: |
There was a problem hiding this comment.
you know what, actually Kapetanios's TODO plugin uses only TODO: or FIXME: as a keyword by default. So you'd better use TODO: if you need to open a new issue.
https://kapetanios.dev/docs/plugins/todo/
| err = fmt.Errorf("resource already existed or in progress: %w", err) | ||
| } | ||
| } | ||
| err = fmt.Errorf("unknown error given: %w", err) |
There was a problem hiding this comment.
This overrides all previous errors.
There was a problem hiding this comment.
nit: Return fast to avoid deep nesting.
aerr, ok := err.(awserr.Error)
if !ok {
return ...
}
switch aeer.Code() {
}There was a problem hiding this comment.
Nice catch, let's me fix this part 🙏
| err = ErrNotFound | ||
| } | ||
| } | ||
| err = fmt.Errorf("unknown error given: %w", err) |
| return | ||
| } | ||
|
|
||
| if cfg.RoutingConfig != nil { |
There was a problem hiding this comment.
if cfg.RoutingConfig == nil {
return ...
}
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Support more configurable fields using Lambda.UpdateFunctionConfiguration.This was created by todo plugin since "TODO:" was found in 9d52465 when #1443 was merged. cc: @khanhtc1202.2. Fix Lambda.AliasConfiguration.RoutingConfig nil value.This was created by todo plugin since "TODO:" was found in 9d52465 when #1443 was merged. cc: @khanhtc1202.3. Using backoff instead of time sleep waiting for a specific duration of time.This was created by todo plugin since "TODO:" was found in 9d52465 when #1443 was merged. cc: @khanhtc1202. |
|
The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by You can check the build log from here. |
|
@nghialv @nakabonne thanks for your comments, PTAL 🙏 |
|
/golinter trigger |
|
Code coverage for golang is
|
|
Nice. |
|
👍 |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: