Skip to content

feat: remove the created intermediate containers while building the lambda functions of Image Type#3922

Merged
moelasmar merged 7 commits intoaws:developfrom
moelasmar:develop_remove_intermediate_containers
Jun 6, 2022
Merged

feat: remove the created intermediate containers while building the lambda functions of Image Type#3922
moelasmar merged 7 commits intoaws:developfrom
moelasmar:develop_remove_intermediate_containers

Conversation

@moelasmar
Copy link
Contributor

@moelasmar moelasmar commented May 30, 2022

Which issue(s) does this change fix?

The default behaviour of docker image create API is to keep the intermediate containers created. So, when customer execute sam build for an Image type Lambda Function, the customer will find some created containers exist after sam build command finish. This PR is to clean these intermediate containers, and let the docker API delete these containers after the image built process finish.

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

@github-actions github-actions bot added pr/internal area/build sam build command labels May 30, 2022
@jfuss
Copy link
Contributor

jfuss commented Jun 1, 2022

Is this the right behavior we want? I ask because from https://docs.docker.com/engine/reference/commandline/images/: "Docker images have intermediate layers that increase reusability, decrease disk usage, and speed up docker build by allowing each step to be cached. These intermediate layers are not shown by default." This suggests that (at least currently) some customers may actually want this as the behavior because it saves the layers on the machine to be re-used. And I assume this leads to faster builds, if you build twice.

@moelasmar
Copy link
Contributor Author

my understanding is this change will only remove the intermediate containers, but will keep the images. This is the same behaviour we implement in sam build --use-container, we cleaned the created containers after build is done.

By the way, this also the same behaviour of docker build command.

@torresxb1
Copy link
Contributor

torresxb1 commented Jun 2, 2022

I also found this in the documentation "If you wish to keep the intermediate containers after the build is complete, you must use --rm=false. This does not affect the build cache.". So it seems this option doesn't affect the caching. https://docs.docker.com/engine/reference/commandline/build/#build-with-path

There's another --no-cached option which is the one that handles caching I think: https://stackoverflow.com/a/69014549

Copy link
Contributor

@torresxb1 torresxb1 left a comment

Choose a reason for hiding this comment

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

couple of questions

"decode": True,
"platform": get_docker_platform(architecture),
"rm": True,
"forcerm": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that here we have rm True but not forcerm: https://github.com/aws/aws-sam-cli/blob/develop/samcli/local/docker/lambda_image.py#L266
Should we keep them consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually I was hesitated to add forcerm, I will remove it.

Comment on lines 49 to 53
@skipIf(
# Hits public ECR pull limitation, move it to canary tests
((not RUN_BY_CANARY) or (IS_WINDOWS and RUNNING_ON_CI) and not CI_OVERRIDE),
"Skip build tests on windows when running in CI unless overridden",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't want to skip anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it is my bad .. I removed them to run testing in my local machine, and forget to return them back :(

@moelasmar moelasmar merged commit 6fa0213 into aws:develop Jun 6, 2022
@moelasmar moelasmar deleted the develop_remove_intermediate_containers branch June 6, 2022 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/build sam build command pr/internal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants