Change Lambdas to create and update in parallel #3976
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
…mbda code archive When creating lambda functions and directly uploading the code, then the whole archive is being logged as a base64 encoded string as part of the HTTP request logger. In order to do so, multiple copies of the body are created in memory, which leads to memory bloating. This change fixes that by redacting the body in the logs for the Create/Update Lambda calls.
| + return &wrappedRequestResponseLogger{wrapped: wrapped} | ||
| +} | ||
| + | ||
| +//go:linkname decomposeHTTPResponse github.com/hashicorp/aws-sdk-go-base/v2.decomposeHTTPResponse |
There was a problem hiding this comment.
I wanna highlight this. If we don't do this, we need to fork aws-sdk-go-base or not log the response at all.
In case upstream changes the symbol, we catch it during compile time
|
This could be a nice win for our users, but it might be worth proposing an upstream PR before making a patch. |
I'm already working on preparing an upstream PR, but I'd propose that we still go forward with the patch for the time being because this change will touch two different repos upstream (hashicorp/aws-sdk-go-base & hashicorp/terraform-provider-aws). |
| + // lambda code. Logging the lambda code leads to memory bloating because it allocates a lot of copies of the | ||
| + // body | ||
| + o.APIOptions = append(o.APIOptions, func(stack *middleware.Stack) error { | ||
| + loggingMiddleware, err := stack.Deserialize.Remove("TF_AWS_RequestResponseLogger") |
There was a problem hiding this comment.
If the name of the logging middleware ever changes it'll fail during the integration tests
| } | ||
|
|
||
| if v, ok := d.GetOk("filename"); ok { | ||
| - // Grab an exclusive lock so that we're only reading one function into memory at a time. |
| +//go:linkname decomposeHTTPResponse github.com/hashicorp/aws-sdk-go-base/v2.decomposeHTTPResponse | ||
| +func decomposeHTTPResponse(ctx context.Context, resp *http.Response, elapsed time.Duration) (map[string]any, error) | ||
| + | ||
| +func (r *wrappedRequestResponseLogger) HandleDeserialize(ctx context.Context, in middleware.DeserializeInput, next middleware.DeserializeHandler, |
There was a problem hiding this comment.
Guessing this was inlined and edited from upstream ? I think that's great and worth doing, just would be helpful to indicate that in the comment, and highlight the edits, just in case. Since we forked it the upstream changes to this handler will not propagate anymore but that sounds ok.
|
It fixes #2206 right or there's some work remaining? |
It fixes it! The added test would fail without the patches |
flostadler
changed the title
The root cause behind why the serialization of Lambda creation/update was introduced upstream is excessive memory usage (see hashicorp/terraform#9364).
After investigation we found that this is caused by the HTTP request logging middleware. It logs the lambda archive as a base64 encoded string. In order to do so, multiple copies of the body are created in memory, which leads to memory bloating.
This change fixes that by redacting the body in the logs for the Create/Update Lambda calls.
The PR introduces two patches. One removes the Lambda serialization and the other fixes the HTTP request logging middleware for the Lambda
CreateFunctionandUpdateFunctionCodeoperations.After this, Lambdas are created/updated in parallel and don't suffer from excessive memory usage. Users can still limit the parallelism with the CLI flag
--parallelif they wish so.Relates to #2206