GSC image size optimizations #141
Conversation
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @amathew3)
a discussion (no related file):
gsc-centos_8_1 latest a7e864abfd9d 29 seconds ago 682MB gsc-centos_8_1 latest 881329d83db1 3 hours ago 559MB
Looks like there is not much benefit for CentOS? Why is that so?
-- commits line 2 at r1:
I would rename to Reduce size of the final GSC image
templates/centos/Dockerfile.build.template line 21 at r1 (raw file):
&& /usr/bin/python3 -B -m pip install click jinja2 protobuf \ 'tomli>=1.1.0' 'tomli-w>=0.4.0' \ && dnf repolist \
Why is this dnf repolist required?
templates/centos/Dockerfile.build.template line 23 at r1 (raw file):
&& dnf repolist \ # For compatibility with Gramine v1.3 or lower && /usr/bin/python3 -B -m pip install 'toml>=0.10' \
Why do you "mix" different logical steps into one Dockerfile step? Could we separate them into four:
- The previous
dnf update - The previous
pip install toml(for compatibility) - The previous
dnf install python3-pyelftools(hm, ok, this could be installed together with the first step, as it is logically the same as step 1) - All those removals starting from
rpm -e --nodeps ...
I'm asking for this clear separation because on the one hand, it is more readable, and on the other hand, it doesn't "worsen" the final Docker image size. Correct me if I'm wrong here.
templates/debian/Dockerfile.build.template line 24 at r1 (raw file):
&& locale-gen en_US.UTF-8 \ && apt remove -y python3-pip locales locales-all && apt autoremove -y \ && rm -rf /var/lib/apt/lists/*
ditto (split into several independent steps)
templates/debian/Dockerfile.build.template line 37 at r1 (raw file):
ENV LC_ALL en_US.UTF-8 ENV LANG en_US.UTF-8 ENV LANGUAGE en_US.UTF-8
These LC_ALL, LANG and LANGUAGE are automatically set by locale-gen en_US.UTF-8? Why can we remove them?
amathew3
force-pushed
the
image_opmization
branch
from
3c87737 to
8266169
Compare
There was a problem hiding this comment.
Introduced a new flag --remove_gramine_deps during the image signing stage, default value is false . if set to true, this will remove the packages needed for building . This will reduce the final gsc image size.
Reviewable status: 1 of 7 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
gsc-centos_8_1 latest a7e864abfd9d 29 seconds ago 682MB gsc-centos_8_1 latest 881329d83db1 3 hours ago 559MBLooks like there is not much benefit for CentOS? Why is that so?
In CentOS GSC build we update the repo mirror list and perform dnf update, this in an expensive operation and consume close to 300MB. This reflect in the final image also.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would rename to
Reduce size of the final GSC image
Done
templates/centos/Dockerfile.build.template line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is this
dnf repolistrequired?
As we install epel-release, it is recommended to do 'dnf repolist' to refresh the repo list.
followed recommendation in this forum https://www.cyberciti.biz/faq/how-to-enable-and-install-epel-repo-on-centos-8-x/
templates/centos/Dockerfile.build.template line 23 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you "mix" different logical steps into one Dockerfile step? Could we separate them into four:
- The previous
dnf update- The previous
pip install toml(for compatibility)- The previous
dnf install python3-pyelftools(hm, ok, this could be installed together with the first step, as it is logically the same as step 1)- All those removals starting from
rpm -e --nodeps ...I'm asking for this clear separation because on the one hand, it is more readable, and on the other hand, it doesn't "worsen" the final Docker image size. Correct me if I'm wrong here.
Each RUN command in the dockerfile creates a new layer in the final images, which adds to the total storage requirement of the image. Final images size is the sum of each layer size.
Here is the output from separate RUN commands in dockerfile
docker history gsc-ubuntu_20.04_2
IMAGE CREATED CREATED BY SIZE COMMENT
d6577f9b1c39 3 hours ago /bin/sh -c #(nop) COPY file:a3898d15a5eb9473… 1.15kB
01d88ccac714 3 hours ago /bin/sh -c #(nop) COPY file:76db0b5b6d16fd07… 1.81kB
544f823d9434 3 hours ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B
5d2e08a4f8ca 3 hours ago |0 /bin/sh -c chmod u+x /gramine/app_files/a… 1.73kB
bc794a36f395 3 hours ago /bin/sh -c #(nop) ENV PATH=/gramine/meson_b… 0B
a09ab8469b07 3 hours ago |0 /bin/sh -c cd /gramine/app_files/ && … 9B
ecaee6b7e1e9 3 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:5f59… 824B
e01c58d7e421 3 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:9508… 802B
9f4fc9d86135 3 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:44ed… 6.79kB
5a1a984705d9 3 hours ago /bin/sh -c #(nop) COPY --chown=rootdir:8bbfc… 26.7MB
b519c7b14216 3 hours ago |0 /bin/sh -c rm -rf $HOME/.cache 0B
e4146ba9e68f 3 hours ago /bin/sh -c #(nop) USER root 0B
6654c2fd4602 3 hours ago |0 /bin/sh -c rm -rf $HOME/.cache 0B
8bf768980175 3 hours ago |0 /bin/sh -c chown root /gramine/app_files/ 0B
819f0ee0ad43 3 hours ago |0 /bin/sh -c mkdir -p /gramine/app_files 0B
c0362b89c9e2 3 hours ago /bin/sh -c #(nop) ENV LANGUAGE=en_US.UTF-8 0B
5c398297fbe2 3 hours ago /bin/sh -c #(nop) ENV LANG=en_US.UTF-8 0B
a48d6d898504 3 hours ago /bin/sh -c #(nop) ENV LC_ALL=en_US.UTF-8 0B
2d3e3a958e9c 3 hours ago |0 /bin/sh -c rm -rf /var/lib/apt/lists/* 0B
f7b0787f1745 3 hours ago |0 /bin/sh -c apt remove -y python3-pip loca… 1.57MB
410cb90cfbf1 3 hours ago |0 /bin/sh -c locale-gen en_US.UTF-8 3.05MB
5341a1e9007b 3 hours ago |0 /bin/sh -c /usr/bin/python3 -B -m pip ins… 116kB
f0ebb57a9bd7 3 hours ago |0 /bin/sh -c apt-get update && env DEBI… 626MB
dd512acb29e5 3 days ago /bin/sh -c #(nop) USER root 0B
d68ca430d4d2 2 weeks ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B
e89e2699a8c8 2 weeks ago /bin/sh -c #(nop) COPY file:4f97079988b6df57… 49B
e40cf56b4be3 2 months ago /bin/sh -c #(nop) CMD ["/bin/bash"] 0B
<missing> 2 months ago /bin/sh -c #(nop) ADD file:8b180a9b4497de0c6… 72.8MB
<missing> 2 months ago /bin/sh -c #(nop) LABEL org.opencontainers.… 0B
<missing> 2 months ago /bin/sh -c #(nop) LABEL org.opencontainers.… 0B
<missing> 2 months ago /bin/sh -c #(nop) ARG LAUNCHPAD_BUILD_ARCH 0B
<missing> 2 months ago /bin/sh -c #(nop) ARG RELEASE 0B
The docker image layer size when the RUN commands are combined.
docker history gsc-ubuntu_20.04_2
IMAGE CREATED CREATED BY SIZE COMMENT
1c222c0ae278 16 seconds ago /bin/sh -c #(nop) COPY file:a3898d15a5eb9473… 1.15kB
dfb98aa3e7e8 17 seconds ago /bin/sh -c #(nop) COPY file:76db0b5b6d16fd07… 1.81kB
9edfe3b5adf8 5 hours ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B
b08dba7c0f73 5 hours ago |0 /bin/sh -c chmod u+x /gramine/app_files/a… 1.73kB
9955d8919954 5 hours ago /bin/sh -c #(nop) ENV PATH=/gramine/meson_b… 0B
d9ba7bfc1848 5 hours ago |0 /bin/sh -c cd /gramine/app_files/ && … 9B
617e72effd7e 5 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:5f59… 824B
2c4ddd2fa5f7 5 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:9508… 802B
2b1cf2ebdebc 5 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:44ed… 6.79kB
58a4161dc7df 5 hours ago /bin/sh -c #(nop) COPY --chown=rootdir:8bbfc… 26.7MB
3b9de5480060 5 hours ago |0 /bin/sh -c rm -rf $HOME/.cache 0B
e44a1a008ede 5 hours ago /bin/sh -c #(nop) USER root 0B
5932f76657d0 5 hours ago |0 /bin/sh -c rm -rf $HOME/.cache 0B
4a2a0ee00536 5 hours ago |0 /bin/sh -c chown root /gramine/app_files/ 0B
045968a4d4d3 5 hours ago |0 /bin/sh -c mkdir -p /gramine/app_files 0B
1555b610017c 5 hours ago |0 /bin/sh -c apt-get update && env DEBI… 84.1MB
dd512acb29e5 3 days ago /bin/sh -c #(nop) USER root 0B
d68ca430d4d2 2 weeks ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B
e89e2699a8c8 2 weeks ago /bin/sh -c #(nop) COPY file:4f97079988b6df57… 49B
e40cf56b4be3 2 months ago /bin/sh -c #(nop) CMD ["/bin/bash"] 0B
<missing> 2 months ago /bin/sh -c #(nop) ADD file:8b180a9b4497de0c6… 72.8MB
<missing> 2 months ago /bin/sh -c #(nop) LABEL org.opencontainers.… 0B
<missing> 2 months ago /bin/sh -c #(nop) LABEL org.opencontainers.… 0B
<missing> 2 months ago /bin/sh -c #(nop) ARG LAUNCHPAD_BUILD_ARCH 0B
<missing> 2 months ago /bin/sh -c #(nop) ARG RELEASE 0B
templates/debian/Dockerfile.build.template line 24 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (split into several independent steps)
Each RUN command in the dockerfile creates a new layer in the final images, which adds to the total storage requirement of the image. Final image size is the sum of each layer size.
templates/debian/Dockerfile.build.template line 37 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
These
LC_ALL,LANGandLANGUAGEare automatically set bylocale-gen en_US.UTF-8? Why can we remove them?
The idea was to completely remove the locales locales-all packages and defaulting the locale to C(POSIX)
Since this may create some impact, removing the locales-all package and retaining the locales package which is needed to set the locale to en_US.UTF-8. Brought back setting LC_ALL, LANG and LANGUAGE environment variables.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
gsc.py line 369 at r2 (raw file):
# builds, to not leave the signing key lingering in any Docker containers build_docker_image(docker_socket.api, tmp_build_path, signed_image_name, 'Dockerfile.sign', forcerm=True, buildargs={"passphrase": args.passphrase,"remove_depends":str(args.remove_gramine_deps)})
Why do you rename the variable here? I'd keep it the same (i.e. remove_gramine_deps).
gsc.py line 505 at r2 (raw file):
sub_sign.add_argument('key', help='Key to sign the Intel SGX enclaves inside the Docker image.') sub_sign.add_argument('-p', '--passphrase', help='Passphrase for the signing key.') sub_sign.add_argument('--remove_gramine_deps', action='store_true', help='Removes Gramine dependencies that are not needed at runtime')
Typically argument names use - instead of _. So please change to --remove-gramine-deps.
(I see that --config_file is there -- this looks like a legacy mistake... We could fix that argument too, in a separate PR.)
gsc.py line 505 at r2 (raw file):
sub_sign.add_argument('key', help='Key to sign the Intel SGX enclaves inside the Docker image.') sub_sign.add_argument('-p', '--passphrase', help='Passphrase for the signing key.') sub_sign.add_argument('--remove_gramine_deps', action='store_true', help='Removes Gramine dependencies that are not needed at runtime')
Please re-wrap to 100 chars per line. This means that help=... will be on the new line (see other arguments for examples).
gsc.py line 505 at r2 (raw file):
sub_sign.add_argument('key', help='Key to sign the Intel SGX enclaves inside the Docker image.') sub_sign.add_argument('-p', '--passphrase', help='Passphrase for the signing key.') sub_sign.add_argument('--remove_gramine_deps', action='store_true', help='Removes Gramine dependencies that are not needed at runtime')
Removes -> Remove
Documentation/index.rst line 152 at r2 (raw file):
Provide passphrase for the enclave signing key (if applicable) .. option:: --remove_gramine_deps
ditto (- instead of _)
Documentation/index.rst line 154 at r2 (raw file):
.. option:: --remove_gramine_deps Removes Gramine dependencies that are not needed at runtime.
Removes -> Remove. Please also add that this decreases the final Docker-image size, but may have a negative side effect of removing even those dependencies that are actually needed by the original application.
Documentation/index.rst line 154 at r2 (raw file):
.. option:: --remove_gramine_deps Removes Gramine dependencies that are not needed at runtime.
Well, in reality aren't you only removing python3-pip package (and some cached files) from the final Docker image layers? So it's a bit wrong what you say here.
templates/Dockerfile.common.sign.template line 17 at r2 (raw file):
# and passphrase) FROM {{image}}
Please retain empty lines!
templates/Dockerfile.common.sign.template line 17 at r2 (raw file):
# and passphrase) FROM {{image}} ARG remove_depends
Hm, why do you declare this variable here? I think it's much more readable to declare this variable in the uninstall blocks, right-before its actual usage.
templates/Dockerfile.common.sign.template line 20 at r2 (raw file):
COPY --from=unsigned_image /gramine/app_files/*.sig /gramine/app_files/ COPY --from=unsigned_image /gramine/app_files/*.sgx /gramine/app_files/ {% block uninstall %}{% endblock %}
Please add an empty line before this line
templates/Dockerfile.common.sign.template line 20 at r2 (raw file):
COPY --from=unsigned_image /gramine/app_files/*.sig /gramine/app_files/ COPY --from=unsigned_image /gramine/app_files/*.sgx /gramine/app_files/ {% block uninstall %}{% endblock %}
Actually, wait, what is the point of this uninstall step? You told me that each Dockerfile step only adds space to the final Docker image. So this uninstall step is useless for reducing the size, right?
templates/centos/Dockerfile.build.template line 23 at r1 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
Each RUN command in the dockerfile creates a new layer in the final images, which adds to the total storage requirement of the image. Final images size is the sum of each layer size.
Here is the output from separate RUN commands in dockerfiledocker history gsc-ubuntu_20.04_2 IMAGE CREATED CREATED BY SIZE COMMENT d6577f9b1c39 3 hours ago /bin/sh -c #(nop) COPY file:a3898d15a5eb9473… 1.15kB 01d88ccac714 3 hours ago /bin/sh -c #(nop) COPY file:76db0b5b6d16fd07… 1.81kB 544f823d9434 3 hours ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B 5d2e08a4f8ca 3 hours ago |0 /bin/sh -c chmod u+x /gramine/app_files/a… 1.73kB bc794a36f395 3 hours ago /bin/sh -c #(nop) ENV PATH=/gramine/meson_b… 0B a09ab8469b07 3 hours ago |0 /bin/sh -c cd /gramine/app_files/ && … 9B ecaee6b7e1e9 3 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:5f59… 824B e01c58d7e421 3 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:9508… 802B 9f4fc9d86135 3 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:44ed… 6.79kB 5a1a984705d9 3 hours ago /bin/sh -c #(nop) COPY --chown=rootdir:8bbfc… 26.7MB b519c7b14216 3 hours ago |0 /bin/sh -c rm -rf $HOME/.cache 0B e4146ba9e68f 3 hours ago /bin/sh -c #(nop) USER root 0B 6654c2fd4602 3 hours ago |0 /bin/sh -c rm -rf $HOME/.cache 0B 8bf768980175 3 hours ago |0 /bin/sh -c chown root /gramine/app_files/ 0B 819f0ee0ad43 3 hours ago |0 /bin/sh -c mkdir -p /gramine/app_files 0B c0362b89c9e2 3 hours ago /bin/sh -c #(nop) ENV LANGUAGE=en_US.UTF-8 0B 5c398297fbe2 3 hours ago /bin/sh -c #(nop) ENV LANG=en_US.UTF-8 0B a48d6d898504 3 hours ago /bin/sh -c #(nop) ENV LC_ALL=en_US.UTF-8 0B 2d3e3a958e9c 3 hours ago |0 /bin/sh -c rm -rf /var/lib/apt/lists/* 0B f7b0787f1745 3 hours ago |0 /bin/sh -c apt remove -y python3-pip loca… 1.57MB 410cb90cfbf1 3 hours ago |0 /bin/sh -c locale-gen en_US.UTF-8 3.05MB 5341a1e9007b 3 hours ago |0 /bin/sh -c /usr/bin/python3 -B -m pip ins… 116kB f0ebb57a9bd7 3 hours ago |0 /bin/sh -c apt-get update && env DEBI… 626MB dd512acb29e5 3 days ago /bin/sh -c #(nop) USER root 0B d68ca430d4d2 2 weeks ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B e89e2699a8c8 2 weeks ago /bin/sh -c #(nop) COPY file:4f97079988b6df57… 49B e40cf56b4be3 2 months ago /bin/sh -c #(nop) CMD ["/bin/bash"] 0B <missing> 2 months ago /bin/sh -c #(nop) ADD file:8b180a9b4497de0c6… 72.8MB <missing> 2 months ago /bin/sh -c #(nop) LABEL org.opencontainers.… 0B <missing> 2 months ago /bin/sh -c #(nop) LABEL org.opencontainers.… 0B <missing> 2 months ago /bin/sh -c #(nop) ARG LAUNCHPAD_BUILD_ARCH 0B <missing> 2 months ago /bin/sh -c #(nop) ARG RELEASE 0BThe docker image layer size when the RUN commands are combined.
docker history gsc-ubuntu_20.04_2 IMAGE CREATED CREATED BY SIZE COMMENT 1c222c0ae278 16 seconds ago /bin/sh -c #(nop) COPY file:a3898d15a5eb9473… 1.15kB dfb98aa3e7e8 17 seconds ago /bin/sh -c #(nop) COPY file:76db0b5b6d16fd07… 1.81kB 9edfe3b5adf8 5 hours ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B b08dba7c0f73 5 hours ago |0 /bin/sh -c chmod u+x /gramine/app_files/a… 1.73kB 9955d8919954 5 hours ago /bin/sh -c #(nop) ENV PATH=/gramine/meson_b… 0B d9ba7bfc1848 5 hours ago |0 /bin/sh -c cd /gramine/app_files/ && … 9B 617e72effd7e 5 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:5f59… 824B 2c4ddd2fa5f7 5 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:9508… 802B 2b1cf2ebdebc 5 hours ago /bin/sh -c #(nop) COPY --chown=rootfile:44ed… 6.79kB 58a4161dc7df 5 hours ago /bin/sh -c #(nop) COPY --chown=rootdir:8bbfc… 26.7MB 3b9de5480060 5 hours ago |0 /bin/sh -c rm -rf $HOME/.cache 0B e44a1a008ede 5 hours ago /bin/sh -c #(nop) USER root 0B 5932f76657d0 5 hours ago |0 /bin/sh -c rm -rf $HOME/.cache 0B 4a2a0ee00536 5 hours ago |0 /bin/sh -c chown root /gramine/app_files/ 0B 045968a4d4d3 5 hours ago |0 /bin/sh -c mkdir -p /gramine/app_files 0B 1555b610017c 5 hours ago |0 /bin/sh -c apt-get update && env DEBI… 84.1MB dd512acb29e5 3 days ago /bin/sh -c #(nop) USER root 0B d68ca430d4d2 2 weeks ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B e89e2699a8c8 2 weeks ago /bin/sh -c #(nop) COPY file:4f97079988b6df57… 49B e40cf56b4be3 2 months ago /bin/sh -c #(nop) CMD ["/bin/bash"] 0B <missing> 2 months ago /bin/sh -c #(nop) ADD file:8b180a9b4497de0c6… 72.8MB <missing> 2 months ago /bin/sh -c #(nop) LABEL org.opencontainers.… 0B <missing> 2 months ago /bin/sh -c #(nop) LABEL org.opencontainers.… 0B <missing> 2 months ago /bin/sh -c #(nop) ARG LAUNCHPAD_BUILD_ARCH 0B <missing> 2 months ago /bin/sh -c #(nop) ARG RELEASE 0B
Indeed, now I understand. This article also helped me: https://towardsdatascience.com/using-multi-stage-builds-to-make-your-docker-image-almost-10x-smaller-239068cb6fb0
templates/centos/Dockerfile.build.template line 27 at r2 (raw file):
&& rpm -e --nodeps python3-pip && dnf -y clean all \ && rm -rf /var/cache/dnf/* \ && rm -rf /var/lib/dnf/system-upgrade/*
This is not hidden under the new command line argument?
templates/centos/Dockerfile.build.template line 27 at r2 (raw file):
&& rpm -e --nodeps python3-pip && dnf -y clean all \ && rm -rf /var/cache/dnf/* \ && rm -rf /var/lib/dnf/system-upgrade/*
In any case, this "lumping together" of commands needs to be explained by a comment
templates/centos/Dockerfile.sign.template line 4 at r2 (raw file):
{% block uninstall %} RUN if [ "$remove_depends" = "True" ];then dnf update -y && dnf install -y python3-pip && pip3 uninstall -y tomli jinja2 click && rpm -e --nodeps python3-pip binutils epel-release expect python3-cryptography && dnf -y clean all && rm -rf /var/cache/dnf/*&& rm -rf /var/lib/dnf/system-upgrade/*;fi {% endblock %}
Is there any point in this, given that this is a separate step in the build, so it will not reduce the final image size?
templates/debian/Dockerfile.build.template line 24 at r1 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
Each RUN command in the dockerfile creates a new layer in the final images, which adds to the total storage requirement of the image. Final image size is the sum of each layer size.
This is not hidden under the new command line argument?
templates/debian/Dockerfile.build.template line 33 at r2 (raw file):
vim {% endif %}
Please retain the empty line, otherwise it's hard visually to read this file
templates/debian/Dockerfile.sign.template line 1 at r2 (raw file):
{% extends "Dockerfile.common.sign.template" %}
Is there any point in this, given that this is a separate step in the build, so it will not reduce the final image size?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3 and @dimakuv)
templates/Dockerfile.common.sign.template line 20 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, wait, what is the point of this uninstall step? You told me that each Dockerfile step only adds space to the final Docker image. So this uninstall step is useless for reducing the size, right?
Taking unwanted packages out has also reduced the overall CVEs flagged against GSC images. So that's another reason for adding this step (although it does add a bit of size, but not too much).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3 and @aneessahib)
templates/Dockerfile.common.sign.template line 20 at r2 (raw file):
So that's another reason for adding this step (although it does add a bit of size, but not too much).
But then this is the only reason for adding this step? I mean, the first reason (lower the final image size) is contradicted by this step, as it actually increases the final size.
Anyway, I don't mind removing unwanted packages, but this should be in a separate PR, with a correct commit title.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3 and @dimakuv)
templates/Dockerfile.common.sign.template line 20 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
So that's another reason for adding this step (although it does add a bit of size, but not too much).
But then this is the only reason for adding this step? I mean, the first reason (lower the final image size) is contradicted by this step, as it actually increases the final size.
Anyway, I don't mind removing unwanted packages, but this should be in a separate PR, with a correct commit title.
How about we change current PR commit to "GSC Image optimization". And call out the two objectives clearly
- Reduce size
- Minimize CVE report against a GSC image
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3 and @aneessahib)
templates/Dockerfile.common.sign.template line 20 at r2 (raw file):
Previously, aneessahib (Anees Sahib) wrote…
How about we change current PR commit to "GSC Image optimization". And call out the two objectives clearly
- Reduce size
- Minimize CVE report against a GSC image
Sorry, but why so complicated? It sounds much easier to just create two new PRs, with one commit each, and have very clear descriptions in each.
The idea with "two separate issues in one commit" definitely sounds wrong. And given that the changes are pretty small, I see no problem in splitting in correct and clean commits/PRs.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
templates/Dockerfile.common.sign.template line 20 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Sorry, but why so complicated? It sounds much easier to just create two new PRs, with one commit each, and have very clear descriptions in each.
The idea with "two separate issues in one commit" definitely sounds wrong. And given that the changes are pretty small, I see no problem in splitting in correct and clean commits/PRs.
Yes we can create two PRs. Thats not a problem. @amathew3 is working on it. I feel a single PR with two commits would have sufficed though, if we club it under one package cleanup PR.
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
templates/centos/Dockerfile.build.template line 27 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is not hidden under the new command line argument?
python3-pip we can remove safely, because we only use it for installing pip packages.
Once the pip packages are installed in the system and the image is finalized we don't need python3-pip
templates/centos/Dockerfile.build.template line 27 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
In any case, this "lumping together" of commands needs to be explained by a comment
Done
templates/debian/Dockerfile.build.template line 24 at r1 (raw file):
python3-pip we can remove safely, because we only use it for installing pip packages.
Once the pip packages are there in the system and the image is finalized we don't need python3-pip
templates/debian/Dockerfile.build.template line 33 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please retain the empty line, otherwise it's hard visually to read this file
Done
gsc.py line 369 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you rename the variable here? I'd keep it the same (i.e.
remove_gramine_deps).
Taken out these changes based on the discussion. This will be submitted as a separate PR
gsc.py line 505 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Typically argument names use
-instead of_. So please change to--remove-gramine-deps.(I see that
--config_fileis there -- this looks like a legacy mistake... We could fix that argument too, in a separate PR.)
Taken out this changes, will be addressed in separate PR
gsc.py line 505 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please re-wrap to 100 chars per line. This means that
help=...will be on the new line (see other arguments for examples).
Taken out this changes, will be addressed in separate PR
gsc.py line 505 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Removes->Remove
Taken out this changes, will be addressed in separate PR
Documentation/index.rst line 152 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (
-instead of_)
Taken out this changes, will be addressed in separate PR
Documentation/index.rst line 154 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Removes->Remove. Please also add that this decreases the final Docker-image size, but may have a negative side effect of removing even those dependencies that are actually needed by the original application.
Taken out this changes, will be addressed in separate PR
Documentation/index.rst line 154 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Well, in reality aren't you only removing
python3-pippackage (and some cached files) from the final Docker image layers? So it's a bit wrong what you say here.
Taken out this changes, will be addressed in separate PR
templates/Dockerfile.common.sign.template line 17 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please retain empty lines!
Taken out this changes, will be addressed in separate PR
templates/Dockerfile.common.sign.template line 17 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hm, why do you declare this variable here? I think it's much more readable to declare this variable in the
uninstallblocks, right-before its actual usage.
Taken out this changes, will be addressed in separate PR
templates/Dockerfile.common.sign.template line 20 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add an empty line before this line
Taken out this changes, will be addressed in separate PR
templates/centos/Dockerfile.sign.template line 4 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is there any point in this, given that this is a separate step in the build, so it will not reduce the final image size?
Removing CVE's will be addressed in separate PR
templates/debian/Dockerfile.sign.template line 1 at r2 (raw file):
Removing CVE's will be addressed in separate PR
There was a problem hiding this comment.
Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
templates/centos/Dockerfile.build.template line 19 at r3 (raw file):
python3-pip \ python3-protobuf \ # Combining all the installation and removal steps in single RUN command to reduce the layer size.
This comment should be moved before the RUN command line.
templates/centos/Dockerfile.build.template line 19 at r3 (raw file):
python3-pip \ python3-protobuf \ # Combining all the installation and removal steps in single RUN command to reduce the layer size.
I would expand this comment since this is a non-trivial quirk of Docker:
# Combine all installation and removal steps in a single RUN command to reduce the final image size.
# This is because each Dockerfile command creates a new layer which necessarily adds size to the
# final image. This trick allows to decrease the image size by hundreds of MBs.
templates/centos/Dockerfile.build.template line 27 at r3 (raw file):
# Install pyelftools after the installation of epel-release as it is provided by the EPEL repo && dnf install -y python3-pyelftools \ && rpm -e --nodeps python3-pip && dnf -y clean all \
Can you please add a line here that now we can uninstall python3-pip safely?
templates/centos/Dockerfile.build.template line 29 at r3 (raw file):
&& rpm -e --nodeps python3-pip && dnf -y clean all \ && rm -rf /var/cache/dnf/* \ && rm -rf /var/lib/dnf/system-upgrade/*
Actually, why do you need these two explicit removals? Doesn't dnf clean all do this already?
There was a problem hiding this comment.
Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
templates/centos/Dockerfile.build.template line 19 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This comment should be moved before the
RUNcommand line.
Done
templates/centos/Dockerfile.build.template line 19 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would expand this comment since this is a non-trivial quirk of Docker:
# Combine all installation and removal steps in a single RUN command to reduce the final image size. # This is because each Dockerfile command creates a new layer which necessarily adds size to the # final image. This trick allows to decrease the image size by hundreds of MBs.
Done
templates/centos/Dockerfile.build.template line 27 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you please add a line here that now we can uninstall
python3-pipsafely?
Done
templates/centos/Dockerfile.build.template line 29 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, why do you need these two explicit removals? Doesn't
dnf clean alldo this already?
yes, dnf clean all removes all the caches, but I saw some forums talks about doing rm -rf /var/cache/dnf after dnf clean all.
But in testing don't see any difference in image size with those two rm -rf commands, hence removing those two explicit removals.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
templates/centos/Dockerfile.build.template line 29 at r4 (raw file):
# Install pyelftools after the installation of epel-release as it is provided by the EPEL repo && dnf install -y python3-pyelftools \ # Since all needed pip packages are installed, we can uninstall python3-pip safely
Please remove trailing space on this line
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
&& dnf install -y python3-pyelftools \ # Since all needed pip packages are installed, we can uninstall python3-pip safely && rpm -e --nodeps python3-pip && dnf -y clean all
You have double-space between --nodeps and python3-pip. Please leave only one space.
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
&& dnf install -y python3-pyelftools \ # Since all needed pip packages are installed, we can uninstall python3-pip safely && rpm -e --nodeps python3-pip && dnf -y clean all
Wait, what happens here? Why do you have two tools here (rpm and dnf)? Aren't you supposed to use only one tool always -- in this case dnf?
templates/debian/Dockerfile.build.template line 26 at r4 (raw file):
# Since all needed pip packages are installed, we can uninstall python3-pip safely && apt remove -y python3-pip && apt autoremove -y \ && rm -rf /var/lib/apt/lists/*
Why do you need this explicit rm?
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
templates/centos/Dockerfile.build.template line 29 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove trailing space on this line
Done
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wait, what happens here? Why do you have two tools here (
rpmanddnf)? Aren't you supposed to use only one tool always -- in this casednf?
Ideally we should use one tool, but here dnf remove, removes all its dependencies. For eg: dnf remove -y python3-pip remove python package also, which we are not targeting.
rpm -e --deps removes only the specific package name given, so i think this is more safe to do.
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You have double-space between
--nodepsandpython3-pip. Please leave only one space.
Done
templates/debian/Dockerfile.build.template line 26 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need this explicit
rm?
Thought of removing list of packages downloaded as part of apt update. The rm -rf /var/lib/apt/lists/* is also recommended in https://docs.docker.com/develop/develop-images/dockerfile_best-practices/
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @woju)
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
Ideally we should use one tool, but here dnf remove, removes all its dependencies. For eg:
dnf remove -y python3-pipremove python package also, which we are not targeting.
rpm -e --depsremoves only the specific package name given, so i think this is more safe to do.
@woju Is this a legit thing to do (to use both dnf and rpm)?
templates/debian/Dockerfile.build.template line 26 at r4 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
Thought of removing list of packages downloaded as part of
apt update. The rm -rf /var/lib/apt/lists/* is also recommended inhttps://docs.docker.com/develop/develop-images/dockerfile_best-practices/
Ok, indeed, thanks for the Docker link.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
Depending on where it's being done, it's most likely wrong from both 1) distro pkg management POV and also 2) from features POV.
So 1) it appears to work because rpm -e doesn't honor dependencies, but the package is liable to be reinstalled during next dnf install. So not really working.
For eg:
dnf remove -y python3-pipremove python package also, which we are not targeting.
Yes, because in RHEL 8 there's dependency from python3 to python3-pip:
# rpm -q --requires python36-3.6.8-38.module_el8.5.0+2569+5c5719bc.x86_64
/bin/sh
/bin/sh
/usr/libexec/platform-python
alternatives >= 1.19.1-1
alternatives >= 1.19.1-1
alternatives >= 1.19.1-1
python3-pip [← there it is --woju]
python3-setuptools
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1
In RHEL 9 and derivatives instead there's dependency against python3-pip-wheel (and also python3-pip-setuptools instead of python3-setuptools above), hidden behind python3-libs intermediate dependency:
# rpm -q --requires python3
libc.so.6()(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.34)(64bit)
libpython3.9.so.1.0()(64bit)
python3-libs(x86-64) = 3.9.14-1.el9_1.2
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsZstd) <= 5.4.18-1
rtld(GNU_HASH)
[root@2503bc221629 /]# rpm -q --requires python3-libs
[...]
python3-pip-wheel >= 21.2.3-4
python3-setuptools-wheel >= 53.0.0-7
[...]
Those dependencies are there for a reason, which brings us to 2) those packages are needed for python3 -m venv module. If someone uses venvs inside this image, oops they're now broken. But only on RHEL 8 and derivatives, because this PR's author chose to omit removal of python3-pip-wheel.
@dimakuv That's also why the comment you've suggested that "pip can be removed safely" is incorrect without a more general explanation (for example, that this is "our" image and not our user's app). For user's app, you don't know that unless you know that there's no venv involved anywhere down this path (for example, aren't there any plugins to be installed in runtime). Ultimately this would be a stupid thing to do in an app (it would be something close to RCE for anyone that could "cause the issuance", because pip mostly doesn't verify downloads, it relies only on HTTPS), but we don't know if it's being used. We just don't know.
I'm not blocking because I don't have time to check it, plz block at your discretion. But if you see rpm -e (or dpkg -r or -P) without thorough explanation about the thing being removed that would demonstrate profound understanding of all dependencies involved, why distro maintainers put that dependency in the first place and why we can afford to dismiss their solution, you can bet something is off.
Also, even if this would be technically correct, I'd ask exactly how much this removal gains us. If not too much, then it's probably not worth it to introduce a precedent of using rpm -e || dpkg -[rP].
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3 and @woju)
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Depending on where it's being done, it's most likely wrong from both 1) distro pkg management POV and also 2) from features POV.
So 1) it appears to work because
rpm -edoesn't honor dependencies, but the package is liable to be reinstalled during nextdnf install. So not really working.For eg:
dnf remove -y python3-pipremove python package also, which we are not targeting.Yes, because in RHEL 8 there's dependency from
python3topython3-pip:# rpm -q --requires python36-3.6.8-38.module_el8.5.0+2569+5c5719bc.x86_64 /bin/sh /bin/sh /usr/libexec/platform-python alternatives >= 1.19.1-1 alternatives >= 1.19.1-1 alternatives >= 1.19.1-1 python3-pip [← there it is --woju] python3-setuptools rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rpmlib(PayloadIsXz) <= 5.2-1In RHEL 9 and derivatives instead there's dependency against
python3-pip-wheel(and alsopython3-pip-setuptoolsinstead ofpython3-setuptoolsabove), hidden behindpython3-libsintermediate dependency:# rpm -q --requires python3 libc.so.6()(64bit) libc.so.6(GLIBC_2.2.5)(64bit) libc.so.6(GLIBC_2.34)(64bit) libpython3.9.so.1.0()(64bit) python3-libs(x86-64) = 3.9.14-1.el9_1.2 rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rpmlib(PayloadIsZstd) <= 5.4.18-1 rtld(GNU_HASH) [root@2503bc221629 /]# rpm -q --requires python3-libs [...] python3-pip-wheel >= 21.2.3-4 python3-setuptools-wheel >= 53.0.0-7 [...]Those dependencies are there for a reason, which brings us to 2) those packages are needed for
python3 -m venvmodule. If someone uses venvs inside this image, oops they're now broken. But only on RHEL 8 and derivatives, because this PR's author chose to omit removal ofpython3-pip-wheel.@dimakuv That's also why the comment you've suggested that "pip can be removed safely" is incorrect without a more general explanation (for example, that this is "our" image and not our user's app). For user's app, you don't know that unless you know that there's no venv involved anywhere down this path (for example, aren't there any plugins to be installed in runtime). Ultimately this would be a stupid thing to do in an app (it would be something close to RCE for anyone that could "cause the issuance", because pip mostly doesn't verify downloads, it relies only on HTTPS), but we don't know if it's being used. We just don't know.
I'm not blocking because I don't have time to check it, plz block at your discretion. But if you see
rpm -e(ordpkg -ror-P) without thorough explanation about the thing being removed that would demonstrate profound understanding of all dependencies involved, why distro maintainers put that dependency in the first place and why we can afford to dismiss their solution, you can bet something is off.Also, even if this would be technically correct, I'd ask exactly how much this removal gains us. If not too much, then it's probably not worth it to introduce a precedent of using
rpm -e || dpkg -[rP].
So given what @woju explained, it looks like we must not use rpm tool (to delete a single package, potentially breaking dependency tracking of dnf).
Given that this will give different results on RHEL8 and RHEL9, this is doubly-bad.
So based on the two factors:
- Woju explains that using
rpmto remove a single package is wrong. - RHEL/CentOS images do not benefit from this removal much anyway (according to statistics in the PR description).
I am blocking on this change, and I want this rpm command to be removed.
templates/debian/Dockerfile.build.template line 26 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, indeed, thanks for the Docker link.
Actually, @woju , could you also comment on this rm -rf /var/lib/apt/lists/*?
Docker "best practices" suggests to have this command, but maybe Docker documentation is not a good source of truth?
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
So given what @woju explained, it looks like we must not use
rpmtool (to delete a single package, potentially breaking dependency tracking ofdnf).Given that this will give different results on RHEL8 and RHEL9, this is doubly-bad.
So based on the two factors:
- Woju explains that using
rpmto remove a single package is wrong.- RHEL/CentOS images do not benefit from this removal much anyway (according to statistics in the PR description).
I am blocking on this change, and I want this
rpmcommand to be removed.
rpm command is removed. Now we have only the size saving with combining the installation commands in to single RUN command.
Also i just had check on python3 -m venv broken case after removing python3-pip.
My test results shows pyhton3 -pip is not necessary to be there in base image for venv to work.
[root@47eb997df0d1 /]# rpm -q --requires python36-3.6.8-38.module+el8.5.0+12207+5c5719bc.x86_64
/bin/sh
/bin/sh
/usr/libexec/platform-python
alternatives >= 1.19.1-1
alternatives >= 1.19.1-1
alternatives >= 1.19.1-1
python3-pip
python3-setuptools
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1
[root@47eb997df0d1 /]# rpm -e --nodeps python3-pip
[root@47eb997df0d1 /]# pip3
bash: pip3: command not found
[root@47eb997df0d1 /]# mkdir /tmp/test
[root@47eb997df0d1 /]# python3 -m venv /tmp/test/
[root@47eb997df0d1 /]# source /tmp/test/bin/activate
(test) [root@47eb997df0d1 /]# pip3 install click
Collecting click
Downloading https://files.pythonhosted.org/packages/4a/a8/0b2ced25639fb20cc1c9784de90a8c25f9504a7f18cd8b5397bd61696d7d/click-8.0.4-py3-none-any.whl (97kB)
100% |████████████████████████████████| 102kB 1.1MB/s
Collecting importlib-metadata; python_version < "3.8" (from click)
Downloading https://files.pythonhosted.org/packages/a0/a1/b153a0a4caf7a7e3f15c2cd56c7702e2cf3d89b1b359d1f1c5e59d68f4ce/importlib_metadata-4.8.3-py3-none-any.whl
Collecting typing-extensions>=3.6.4; python_version < "3.8" (from importlib-metadata; python_version < "3.8"->click)
Downloading https://files.pythonhosted.org/packages/45/6b/44f7f8f1e110027cf88956b59f2fad776cca7e1704396d043f89effd3a0e/typing_extensions-4.1.1-py3-none-any.whl
Collecting zipp>=0.5 (from importlib-metadata; python_version < "3.8"->click)
Downloading https://files.pythonhosted.org/packages/bd/df/d4a4974a3e3957fd1c1fa3082366d7fff6e428ddb55f074bf64876f8e8ad/zipp-3.6.0-py3-none-any.whl
Installing collected packages: typing-extensions, zipp, importlib-metadata, click
Successfully installed click-8.0.4 importlib-metadata-4.8.3 typing-extensions-4.1.1 zipp-3.6.0
You are using pip version 9.0.3, however version 23.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
(test) [root@47eb997df0d1 /]#
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
rpmcommand is removed. Now we have only the size saving with combining the installation commands in to single RUN command.Also i just had check on
python3 -m venvbroken case after removing python3-pip.
My test results showspyhton3 -pipis not necessary to be there in base image forvenvto work.[root@47eb997df0d1 /]# rpm -q --requires python36-3.6.8-38.module+el8.5.0+12207+5c5719bc.x86_64 /bin/sh /bin/sh /usr/libexec/platform-python alternatives >= 1.19.1-1 alternatives >= 1.19.1-1 alternatives >= 1.19.1-1 python3-pip python3-setuptools rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rpmlib(PayloadIsXz) <= 5.2-1 [root@47eb997df0d1 /]# rpm -e --nodeps python3-pip [root@47eb997df0d1 /]# pip3 bash: pip3: command not found [root@47eb997df0d1 /]# mkdir /tmp/test [root@47eb997df0d1 /]# python3 -m venv /tmp/test/ [root@47eb997df0d1 /]# source /tmp/test/bin/activate (test) [root@47eb997df0d1 /]# pip3 install click Collecting click Downloading https://files.pythonhosted.org/packages/4a/a8/0b2ced25639fb20cc1c9784de90a8c25f9504a7f18cd8b5397bd61696d7d/click-8.0.4-py3-none-any.whl (97kB) 100% |████████████████████████████████| 102kB 1.1MB/s Collecting importlib-metadata; python_version < "3.8" (from click) Downloading https://files.pythonhosted.org/packages/a0/a1/b153a0a4caf7a7e3f15c2cd56c7702e2cf3d89b1b359d1f1c5e59d68f4ce/importlib_metadata-4.8.3-py3-none-any.whl Collecting typing-extensions>=3.6.4; python_version < "3.8" (from importlib-metadata; python_version < "3.8"->click) Downloading https://files.pythonhosted.org/packages/45/6b/44f7f8f1e110027cf88956b59f2fad776cca7e1704396d043f89effd3a0e/typing_extensions-4.1.1-py3-none-any.whl Collecting zipp>=0.5 (from importlib-metadata; python_version < "3.8"->click) Downloading https://files.pythonhosted.org/packages/bd/df/d4a4974a3e3957fd1c1fa3082366d7fff6e428ddb55f074bf64876f8e8ad/zipp-3.6.0-py3-none-any.whl Installing collected packages: typing-extensions, zipp, importlib-metadata, click Successfully installed click-8.0.4 importlib-metadata-4.8.3 typing-extensions-4.1.1 zipp-3.6.0 You are using pip version 9.0.3, however version 23.1 is available. You should consider upgrading via the 'pip install --upgrade pip' command. (test) [root@47eb997df0d1 /]#
Thanks for checking @amathew3 . @woju @dimakuv Maybe we can revisit this solution later if anyone complaints about CentOS GSC images. For now, the issue is raised only on Debian and derivatives.
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3 and @dimakuv)
templates/debian/Dockerfile.build.template line 25 at r4 (raw file):
&& /usr/bin/python3 -B -m pip install 'toml>=0.10' \ # Since all needed pip packages are installed, we can uninstall python3-pip safely && apt remove -y python3-pip && apt autoremove -y \
No two commands on a single line please, unless you've something to hide.
templates/debian/Dockerfile.build.template line 26 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, @woju , could you also comment on this
rm -rf /var/lib/apt/lists/*?Docker "best practices" suggests to have this command, but maybe Docker documentation is not a good source of truth?
This is either legit or at least harmless. It removes local apt cache, which contains repository metadata (what is downloaded during apt-get update). Inside this cache is list of available packages from the repositories you've configured (incl. metadata like names, versions, checksums, architectures, dependencies, and list of files to be installed, but not including actual .deb files) and normally is used for dependency resolution during package installation and removal, and package search (apt-cache search, apt-cache show etc.). Without this metadata you can't install new package, but when you're building "base image" (for things like VM golden images, or docker image intended to be downloaded and FROM'd by other images) this is harmless, because you know that people need to run apt-get update anyway, because between image creation some time might have passed and the metadata is probably useless anyway. Even if you don't care about latest versions, if you have stale metadata pointing to older version of packages, those packages might have already been removed from repos and you'll get 404s during apt-get install's package download.
For non-"base" images this is slightly more complicated, because you'll need to make sure you reap the benefits of the smaller image, because local docker cache stores intermediate steps, so you'll have that step before RUN rm -rf stored on disk anyway. But it's entirely harmless if you don't touch apt after this step.
The gain, if it works out, looks substantial: on one of my Gramine development VMs with Debian 11 and backports repo enabled, it's 171 MB (163 MiB):
% sudo du -sc --si * | sort -hr
171M total
53M deb.debian.org_debian_dists_bullseye_main_Contents-all.lz4
46M deb.debian.org_debian_dists_bullseye_main_binary-amd64_Packages
31M deb.debian.org_debian_dists_bullseye_main_i18n_Translation-en
18M deb.debian.org_debian_dists_bullseye_main_Contents-amd64.lz4
9.3M deb.debian.org_debian_dists_bullseye-backports_main_Contents-all.lz4
3.7M deb.debian.org_debian_dists_bullseye-backports_main_Contents-amd64.lz4
2.6M deb.debian.org_debian_dists_bullseye-backports_main_binary-amd64_Packages
2.1M deb.debian.org_debian_dists_bullseye-backports_main_i18n_Translation-en
1.7M deb.debian.org_debian_dists_bullseye_non-free_Contents-all.lz4
1.6M deb.debian.org_debian-security_dists_bullseye-security_main_binary-amd64_Packages
1.2M deb.debian.org_debian-security_dists_bullseye-security_main_i18n_Translation-en
... [the rest is below 1MB --woju]
I didn't look into this exact image, but I would accept without looking. Just check if there aren't any apt-get or apt-cache commands below this line (probably there aren't, they'd break anyway).
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
templates/centos/Dockerfile.build.template line 30 at r4 (raw file):
Previously, aneessahib (Anees Sahib) wrote…
Thanks for checking @amathew3 . @woju @dimakuv Maybe we can revisit this solution later if anyone complaints about CentOS GSC images. For now, the issue is raised only on Debian and derivatives.
I'm ok with this code now.
If somebody will complain about CentOS image sizes in the future, we may revisit again -- what to do with python3-pip dependency and how to "yank" it out in a correct way.
templates/centos/Dockerfile.build.template line 40 at r6 (raw file):
strace \ vim {% endif %}
Did you try this PR with ./gsc build --debug? I guess Debian/Ubuntu dockerfile definitely fails, but maybe this CentOS dockerfile survives this test. Please check.
templates/debian/Dockerfile.build.template line 26 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
This is either legit or at least harmless. It removes local
aptcache, which contains repository metadata (what is downloaded duringapt-get update). Inside this cache is list of available packages from the repositories you've configured (incl. metadata like names, versions, checksums, architectures, dependencies, and list of files to be installed, but not including actual.debfiles) and normally is used for dependency resolution during package installation and removal, and package search (apt-cache search,apt-cache showetc.). Without this metadata you can't install new package, but when you're building "base image" (for things like VM golden images, or docker image intended to be downloaded andFROM'd by other images) this is harmless, because you know that people need to runapt-get updateanyway, because between image creation some time might have passed and the metadata is probably useless anyway. Even if you don't care about latest versions, if you have stale metadata pointing to older version of packages, those packages might have already been removed from repos and you'll get 404s duringapt-get install's package download.For non-"base" images this is slightly more complicated, because you'll need to make sure you reap the benefits of the smaller image, because local docker cache stores intermediate steps, so you'll have that step before
RUN rm -rfstored on disk anyway. But it's entirely harmless if you don't touchaptafter this step.The gain, if it works out, looks substantial: on one of my Gramine development VMs with Debian 11 and backports repo enabled, it's 171 MB (163 MiB):
% sudo du -sc --si * | sort -hr 171M total 53M deb.debian.org_debian_dists_bullseye_main_Contents-all.lz4 46M deb.debian.org_debian_dists_bullseye_main_binary-amd64_Packages 31M deb.debian.org_debian_dists_bullseye_main_i18n_Translation-en 18M deb.debian.org_debian_dists_bullseye_main_Contents-amd64.lz4 9.3M deb.debian.org_debian_dists_bullseye-backports_main_Contents-all.lz4 3.7M deb.debian.org_debian_dists_bullseye-backports_main_Contents-amd64.lz4 2.6M deb.debian.org_debian_dists_bullseye-backports_main_binary-amd64_Packages 2.1M deb.debian.org_debian_dists_bullseye-backports_main_i18n_Translation-en 1.7M deb.debian.org_debian_dists_bullseye_non-free_Contents-all.lz4 1.6M deb.debian.org_debian-security_dists_bullseye-security_main_binary-amd64_Packages 1.2M deb.debian.org_debian-security_dists_bullseye-security_main_i18n_Translation-en ... [the rest is below 1MB --woju]I didn't look into this exact image, but I would accept without looking. Just check if there aren't any
apt-getorapt-cachecommands below this line (probably there aren't, they'd break anyway).
Ok, thanks Woju for such a detailed answer.
templates/debian/Dockerfile.build.template line 37 at r6 (raw file):
strace \ vim {% endif %}
Did you try this PR with ./gsc build --debug? Looks like it will fail on this line, because you removed the APT cache and now you're trying to install some more packages.
I think it will be fine to just add apt-get update here, because we will be under the debug mode of GSC -- in this mode, we don't care about image size.
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
templates/debian/Dockerfile.build.template line 25 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
No two commands on a single line please, unless you've something to hide.
Corrected.
templates/debian/Dockerfile.build.template line 26 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, thanks Woju for such a detailed answer.
Thanks
templates/debian/Dockerfile.build.template line 37 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Did you try this PR with
./gsc build --debug? Looks like it will fail on this line, because you removed the APT cache and now you're trying to install some more packages.I think it will be fine to just add
apt-get updatehere, because we will be under thedebugmode of GSC -- in this mode, we don't care about image size.
Yes, I missed this, added apt update just before the installation of additional plackages.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3 and @woju)
templates/centos/Dockerfile.build.template line 40 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Did you try this PR with
./gsc build --debug? I guess Debian/Ubuntu dockerfile definitely fails, but maybe this CentOS dockerfile survives this test. Please check.
What about this one? You corrected for Ubuntu but not here (for CentOS).
templates/debian/Dockerfile.build.template line 25 at r7 (raw file):
&& /usr/bin/python3 -B -m pip install 'toml>=0.10' \ # Since all needed pip packages are installed, we can uninstall python3-pip safely && apt remove -y python3-pip \
Please remove a trailing space
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
templates/centos/Dockerfile.build.template line 40 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What about this one? You corrected for Ubuntu but not here (for CentOS).
In CentOS dnf update is not needed, the debug build proceed with out any issue. Looks like dnf clean all is not removing all metadata, so next package installations is proceeding without any issue.
templates/debian/Dockerfile.build.template line 25 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove a trailing space
Done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @woju)
templates/centos/Dockerfile.build.template line 40 at r6 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
In CentOS
dnf updateis not needed, the debug build proceed with out any issue. Looks likednf clean allis not removing all metadata, so next package installations is proceeding without any issue.
Thanks for the reply! Makes sense, unblocking.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @woju)
a discussion (no related file):
I did a quick test on Ubuntu 18.04 and Helloworld Bash example (that we have in the GSC repo under tests/).
Here is the result:
With PR 141:
REPOSITORY TAG IMAGE ID CREATED SIZE
gsc-ubuntu18.04-bash latest c98280c0cbc0 About a minute ago 235MB
gsc-ubuntu18.04-bash-unsigned latest 500bc9428eba 6 minutes ago 234MB
ubuntu18.04-bash latest 6524f8f34511 13 minutes ago 108MB
ubuntu 18.04 3941d3b032a8 6 weeks ago 63.1MB
Without PR (master branch):
REPOSITORY TAG IMAGE ID CREATED SIZE
gsc-ubuntu18.04-bash latest 18973402aa74 56 seconds ago 688MB
gsc-ubuntu18.04-bash-unsigned latest 807fad2e1add 2 minutes ago 684MB
ubuntu18.04-bash latest 4415b37fcb92 11 minutes ago 108MB
ubuntu 18.04 3941d3b032a8 6 weeks ago 63.1MB
So indeed, the current PR significantly optimizes the Docker image size, from 688MB to 235MB.
There was a problem hiding this comment.
Reviewed 5 of 7 files at r3, 1 of 1 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 5 of 7 files at r3, 1 of 1 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
Signed-off-by: abin <[email protected]>
dimakuv
force-pushed
the
image_opmization
branch
from
62a5fab to
c1413c2
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I did a quick test on Ubuntu 18.04 and Helloworld Bash example (that we have in the GSC repo under
tests/).Here is the result:
With PR 141: REPOSITORY TAG IMAGE ID CREATED SIZE gsc-ubuntu18.04-bash latest c98280c0cbc0 About a minute ago 235MB gsc-ubuntu18.04-bash-unsigned latest 500bc9428eba 6 minutes ago 234MB ubuntu18.04-bash latest 6524f8f34511 13 minutes ago 108MB ubuntu 18.04 3941d3b032a8 6 weeks ago 63.1MB Without PR (master branch): REPOSITORY TAG IMAGE ID CREATED SIZE gsc-ubuntu18.04-bash latest 18973402aa74 56 seconds ago 688MB gsc-ubuntu18.04-bash-unsigned latest 807fad2e1add 2 minutes ago 684MB ubuntu18.04-bash latest 4415b37fcb92 11 minutes ago 108MB ubuntu 18.04 3941d3b032a8 6 weeks ago 63.1MBSo indeed, the current PR significantly optimizes the Docker image size, from 688MB to 235MB.
Did a final rebase and tested again. Same results, looks good to me.
templates/debian/Dockerfile.build.template line 26 at r8 (raw file):
# Since all needed pip packages are installed, we can uninstall python3-pip safely && apt remove -y python3-pip \ && apt autoremove -y \
FYI: I changed apt to apt-get since apt command is not recommended for use in scripts and Dockerfiles.
dimakuv
force-pushed
the
image_opmization
branch
from
c1413c2 to
8aae6e3
Compare
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
Changes for the GSC image size optimization. Removed package manager caches and packages which is needed in the intermediate layer.
Debian GSC image size before and after this PR
gsc-ubuntu_20.04_1 latest 6fd76caa14e6 6 seconds ago 721MB
gsc-ubuntu_20.04_1 latest 15815de4c481 3 hours ago 184MB
CentOS Image before and after this PR.
gsc-centos_8_1 latest a7e864abfd9d 29 seconds ago 682MB
gsc-centos_8_1 latest 881329d83db1 3 hours ago 559MB
Fixes #140
How to test this PR?
Create a GSC image debian:11 and observe the size.
Pick the PR and create the GSC image and observe the image size.
GSC image should run without any issues.
This change is