-
Notifications
You must be signed in to change notification settings - Fork 439
Add apt clean
to reduce layer size
#1298
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.
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
Open
DavidS-ovm
wants to merge
1
commit into
devcontainers:main
Choose a base branch
from
DavidS-ovm:patch-1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this not make sense as it only removes the installation stuff in the previous layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't quite understand why this script cleans at the start and then again at the end 🤷
As a start I see an existing clean up, I add my change. If someone with a better overview of the project points me in a different direction to solve this at a higher level so that the apt cache is never stored in the image (e.g. using
RUN --mount=type=cache,target=/var/cache/apt --mount=type=cache,target=/var/lib/apt/lists
everywhere), I'd be happy to also do that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the removal of
/var/lib/apt/lists/*
is done because subsequent installations may fail if the cache remains in the lower layers.The size of the image is irrelevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on "The size of the image is irrelevant"? My team is paying for image storage by the byte and devcontainer download time is a regular annoyance, so image size is very much of concern for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the context #210.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm trying to say is that I'm wondering if it's worth adding this at the beginning of the script.
(Because it doesn't affect the layer size)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @DavidS-ovm ,
Thank you for the PR. I was trying to test docker-in-docker feature with this PR change. In fact I see that with
apt clean
right at the beginning of theinstall.sh
, package installation failing. It doesn't remove the directory structure or the lock file, so later on the following condition inapt_get_update()
function body doesn't satisfy &apt-get update -y
doesn't execute before installing the additional packages for docker-in-docker feature.It would be best to have the clean-up done once & done right at the end of the
install.sh
execution & that should serve the purpose of reducing the layer size in the resultant image. Kindly let me know in case of further concern.With Regards,
Kaniska
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the analysis! This is indeed more complex than I initially expected. Meanwhile I've done some local optimizations for other containers in my work and I'm now using
any time apt is getting called. This ensures that
Poking around in the devcontainer CLI, this seems to be the appropriate location to add that:
https://github.com/devcontainers/cli/blob/5c9623c78d17ee4f2d75899c1d9c59a9b266611e/src/spec-configuration/containerFeaturesConfiguration.ts#L292-L357
additionally there's
getContainerFeaturesBaseDockerFile
in the same file that looks like a good place toapt update
once for the entire build.Does that sound sensible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devcontainers/cli#989 as an experiment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @DavidS-ovm ,
Thank you for the contribution & the detailed analysis on this issue. I will check this further & update you.