-
Notifications
You must be signed in to change notification settings - Fork 17
Reduce package weight #107
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
Conversation
@vrothberg @cgwalters mind trying the images assuming they get posted here and make sure we're still good for bootc use cases @dustymabe wdyt ? |
LGTM |
# Install ansible for post-install configuration | ||
RUN rpm-ostree install subscription-manager gvisor-tap-vsock-gvforwarder qemu-user-static ansible-core && \ | ||
# Remove unwanted packages | ||
# Remove man pages (man binary is not present) |
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.
The remove comments would be easier for me to brain-parse above line 63. Can we move it down?
Sure, I can run some smoke tests. The changes should be fine. |
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.
This doesn't seem to work, the resulting images are larger
about 60 MB on x86_64 and 100 MB on aarch64
Seems reasonable |
@Luap99 i would like to move forward with this if you don't object. I realize there is no space savings yet. I think that can come in a secondary step. |
https://docs.fedoraproject.org/en-US/bootc/building-custom-base/ is an important reference here if you want to actually remove content from the base image. See especially https://docs.fedoraproject.org/en-US/bootc/building-custom-base/#_optimizing_container_images |
Are you specifically speaking about the rechunking? |
Well there is a difference between there is no size benefit and we are adding 50MB+ to the images with that, so how can that be? |
Because my theory is once we get the second step in, it will be smaller. but we can just live with it today and not do any of this stuff. |
And the second step is what? And why would this need to be merged before said second step is ready? Also not directly related but this is switching to using dnf and there is also not explanation why dnf is used in the commit or comments either. For proper history it is always useful to know why something was changed. |
well the commit message does say reduce the number of packages not reduce the size firstly. If they don't need to be there, then why install them? The second step is the rechunking of the images which should allow for us to see a reduction in overall image size. I'm not going to debate with you anymore. |
Reduce number of unnecessary packages in our image through the use of installing without weak dependencies for subscription-manager and qemu-user-static. Remove man pages as the man binary is not present anyways. Signed-off-by: Brent Baude <bbaude@redhat.com>
ok, with the rechunk PR merged, I think we have something viable for merge now. |
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The rechunking worked well for you? What did you think? |
We had a nice savings on the OCI image for sure. That was excellent. The savings on the disk image is somehow minimal which I don't fully understand. |
# Remove man pages (man binary is not present) | ||
RUN dnf install -y --setopt=install_weak_deps=false \ | ||
subscription-manager device-mapper qemu-user-static-aarch64 qemu-user-static-x86 && \ | ||
dnf install -y gvisor-tap-vsock-gvforwarder ansible-core && \ |
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.
Curious about the ansible-core
install, is there a motivation for that?
BTW, a pattern I recommend here (until dnf gains some declarative interface or we do https://gitlab.com/fedora/bootc/osbuild-cfg or equivalent) is the "dnf | xargs" trick like
$ cat packages.txt
# Needed to configure RHEL
subscription-manager
# Because we want to...
ansible-core
Then in your dockerfile RUN grep -vEe '^#' packages.txt | xargs dnf -y install
.
This allows one to have a super lightweight format with comments so the comments can say why the package is installed etc.
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.
Curious about the ansible-core install, is there a motivation for that?
As for chaining the format I agree we should do that do have a clear comment why each package is needed but I guess we can consider this out of scope for this change.
/lgtm |
Reduce number of unnecessary packages in our image through the use of installing without weak dependencies for subscription-manager and qemu-user-static.
Remove man pages as the man binary is not present anyways.