-
Notifications
You must be signed in to change notification settings - Fork 90
RHOAIENG-10350 feat(Dockerfiles): switch from s2i python images to plain ubi/c9s bases #641
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
base: main
Are you sure you want to change the base?
Conversation
base/ubi8-python-3.8/Dockerfile
Outdated
@@ -1,4 +1,21 @@ | |||
FROM registry.access.redhat.com/ubi8/python-38:latest | |||
FROM registry.access.redhat.com/ubi8/ubi:latest |
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 we use ubi-minimal
instead?
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.
That would either mean living with microdnf, or installing regular dnf first thing, at which point the benefits or the ubi-minimal base are pretty much lost, me thinks.
In any case, I'd want to do such large changes in multiple steps, across multiple PRs.
So far, I don't even have the modest change in this PR groomed. It's necessary to move slowly and cautiously, and to build consensus.
The disk savings are small, 300mb across three base images.
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.
/retest
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.
/hold
@jiridanek can you share there is JIRA or github issue linked for this work.
This PR is great, definitely will be good to have however
This work has significant impact on the overall product, without changing the architecture , this change would disrupt some flow i believe.
the s2i image bring working dir: /opt/app-root/src
i don't think ubi:latest, has that working dir,
this might work only because, the PVC gets mounted at the path,
As mount would create the path.
however we should thoroughly check these changes.
Great! In that case I'll go forward creating a Jira so this can be groomed. https://issues.redhat.com/browse/RHOAIENG-10350 |
our images explicitly set this in the
sure, that's why I created a jira for doing all the work in organized manner; the disk space savings are not all that significant, but I like how this removes much of the s2i magic, so that's what makes this seem worth doing to me |
BTW, I was wondering what are the most important changes, so I prepared some basic script for the image comparison (I have a plan to improve it further and put it into our CI/GHA for our convenience). I tried to compare one of the base images you change here with what we have (note that your build is 2 weeks old, I took the quay image from today):
|
+1 for this update, I like this. 🙂 As Harshad mentioned, we should thoroughly test everything to ensure there are no issues with the PVCs. Otherwise, it /lgtm |
yeah; was just thinking whether nginx gets properly installed after this change; I did not think of it before https://issues.redhat.com/browse/RHOAIENG-10350 is opened to properly schedule the work |
e2f0598
to
aa02b11
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The main benefit is size and cve exposure, as the python images come with packages we don't use; python and pip is enough for us. Additionally, using plain ubi makes things more explicit.
db683dd
to
1e8dd31
Compare
…less podman machine ``` base_image_test.py:146: in test_oc_command_runs_fake_fips assert ecode == 0, output.decode() E AssertionError: assertion failed [!result.is_error]: Unable to open /proc/sys/vm/mmap_min_addr E (VMAllocationTracker.cpp:317 init) E E assert 137 == 0 ``` ``` lima cat /proc/sys/vm/mmap_min_addr 65536 ``` ``` podman machine ssh cat /proc/sys/vm/mmap_min_addr 65536 ``` ``` podman run --entrypoint /bin/bash --rm -it ghcr.io/jiridanek/notebooks/workbench-images:base-ubi9-python-3.11-jd_ubi_base_1e8dd3140d980ff573d56d3ae746959f31825d8a WARNING: image platform (linux/amd64) does not match the expected platform (linux/arm64) bash-5.1$ cat /proc/sys/vm/mmap_min_addr 65536 ```
@jiridanek: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
https://issues.redhat.com/browse/RHOAIENG-10350
Description
The main benefit is size and cve exposure, as the python images come with packages we don't use; python and pip is enough for us.
Every image is made approx 300MB smaller by doing this!
Before:
After:
Additionally, using plain ubi makes things more explicit.
How Has This Been Tested?
I'll test this when making this change is planned into a sprint.
Merge criteria: