-
Notifications
You must be signed in to change notification settings - Fork 20
Prow-scripts #1062
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?
Prow-scripts #1062
Conversation
@paigerube14 could you please review this PR? |
fa3fb2c
to
2c5ec3a
Compare
/lgtm |
prow/Dockerfile
Outdated
# Update and use not only best candidate packages (avoiding failures) | ||
RUN dnf update -y --nobest | ||
|
||
# Install development tools and necessary dependencies | ||
RUN dnf group install -y "Development Tools" \ | ||
&& dnf install -y podman jq | ||
|
||
# Prerequisite for Python installation | ||
ARG python_full_version=3.12.3 | ||
RUN dnf install -y openssl-devel bzip2-devel wget libffi-devel | ||
|
||
# Install Python | ||
RUN wget https://www.python.org/ftp/python/${python_full_version}/Python-${python_full_version}.tgz \ | ||
&& tar -xzf Python-${python_full_version}.tgz \ | ||
&& cd Python-${python_full_version} \ | ||
&& ./configure --enable-optimizations \ | ||
&& make altinstall \ | ||
&& echo alias python=python3.12 >> ~/.bashrc \ | ||
&& rm -rf Python-${python_full_version}.tgz | ||
|
||
# install & run benchmark-runner (--no-cache-dir for take always the latest) | ||
RUN python3.12 -m pip install --upgrade pip && python3.12 -m pip install --upgrade benchmark-runner | ||
|
||
# Passed dynamically | ||
ARG OCP_CLIENT_VERSION | ||
RUN curl -L "https://mirror.openshift.com/pub/openshift-v4/clients/ocp/${OCP_CLIENT_VERSION}/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz" -o "/tmp/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz" \ | ||
&& tar -xzvf /tmp/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz -C /tmp/ \ | ||
&& mv /tmp/kubectl /usr/local/bin/kubectl \ | ||
&& mv /tmp/oc /usr/local/bin/oc \ | ||
&& rm -rf /tmp/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz /tmp/kubectl /tmp/oc | ||
|
||
# Passed dynamically | ||
ARG VIRTCTL_VERSION | ||
RUN curl -L "https://github.com/kubevirt/kubevirt/releases/download/v${VIRTCTL_VERSION}/virtctl-v${VIRTCTL_VERSION}-linux-amd64" -o /usr/local/bin/virtctl \ | ||
&& chmod +x /usr/local/bin/virtctl | ||
|
||
# Activate root alias | ||
RUN source ~/.bashrc | ||
|
||
# Create necessary directories with the correct permissions | ||
RUN mkdir -p ~/.kube ~/.ssh /tmp/run_artifacts | ||
|
||
# download benchmark-operator to /tmp default path | ||
RUN git clone -b v1.0.4 https://github.com/cloud-bulldozer/benchmark-operator /tmp/benchmark-operator | ||
|
||
# download clusterbuster to /tmp default path && install cluster-buster dependency | ||
RUN git clone -b v1.2.2-kata-ci https://github.com/RobertKrawitz/OpenShift4-tools /tmp/OpenShift4-tools \ | ||
&& dnf install -y hostname bc procps-ng | ||
|
||
# Cleanup to reduce image size | ||
RUN dnf clean all && rm -rf /var/cache/dnf |
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.
Combine all of these RUN statements into 1 by means of
RUN dnf update -y --nobest && \
dnf group install -y "Development Tools" \
&& dnf install -y podman jq
and so forth. That will result in a much smaller image because each RUN creates its own layer in the image. In particular, running dnf clean all
after dnf install ...
is self-defeating, because the dnf install
will add a lot of things that the dnf clean all
will remove, so there will be two separate deltas.
You might also be able to use microdnf rather than dnf, which will further shrink the image.
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 prefer not to combine them into one command, because if it fails at
dnf group install -y "Development Tools" && dnf install -y podman jq,
there's no need to rerun the update step, which is resource-intensive
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 is only during the build. It's to make the image smaller. You can do something like this:
# Prerequisite for Python installation
ARG python_full_version=3.12.3
ARG OCP_CLIENT_VERSION
ARG VIRTCTL_VERSION
RUN dnf update -y --nobest \
&& dnf group install -y "Development Tools" \
&& dnf install -y podman jq \
&& dnf install -y openssl-devel bzip2-devel wget libffi-devel \
&& wget https://www.python.org/ftp/python/${python_full_version}/Python-${python_full_version}.tgz \
&& tar -xzf Python-${python_full_version}.tgz \
&& cd Python-${python_full_version} \
&& ./configure --enable-optimizations \
&& make altinstall \
&& echo alias python=python3.12 >> ~/.bashrc \
&& rm -rf Python-${python_full_version}.tgz \
&& python3.12 -m pip install --upgrade pip \
&& python3.12 -m pip install --upgrade benchmark-runner \
&& curl -L "https://mirror.openshift.com/pub/openshift-v4/clients/ocp/${OCP_CLIENT_VERSION}/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz" -o "/tmp/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz" \
&& tar -xzvf /tmp/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz -C /tmp/ \
&& mv /tmp/kubectl /usr/local/bin/kubectl \
&& mv /tmp/oc /usr/local/bin/oc \
&& rm -rf /tmp/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz /tmp/kubectl /tmp/oc \
&& curl -L "https://github.com/kubevirt/kubevirt/releases/download/v${VIRTCTL_VERSION}/virtctl-v${VIRTCTL_VERSION}-linux-amd64" -o /usr/local/bin/virtctl \
&& chmod +x /usr/local/bin/virtctl \
&& . ~/.bashrc \
&& mkdir -p ~/.kube ~/.ssh /tmp/run_artifacts \
&& git clone -b v1.0.4 https://github.com/cloud-bulldozer/benchmark-operator /tmp/benchmark-operator \
&& git clone -b v1.2.2-kata-ci https://github.com/RobertKrawitz/OpenShift4-tools /tmp/OpenShift4-tools \
&& dnf install -y hostname bc procps-ng \
&& dnf clean all \
&& rm -rf /var/cache/dnf
If any of the steps fail, it will then fail correctly, without ever running the update step. You want to absolutely minimize the number of RUN statements, because each one will create a separate layer in the image, bloating it.
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.
Actually, another way you might want to group it to minimize rebuild time is something like this:
# Prerequisite for Python installation
ARG python_full_version=3.12.3
ARG OCP_CLIENT_VERSION
ARG VIRTCTL_VERSION
RUN dnf update -y --nobest \
&& dnf group install -y "Development Tools" \
&& dnf install -y podman jq \
&& dnf install -y openssl-devel bzip2-devel wget libffi-devel \
&& dnf install -y hostname bc procps-ng \
&& dnf clean all \
&& rm -rf /var/cache/dnf
RUN wget https://www.python.org/ftp/python/${python_full_version}/Python-${python_full_version}.tgz \
&& tar -xzf Python-${python_full_version}.tgz \
&& cd Python-${python_full_version} \
&& ./configure --enable-optimizations \
&& make altinstall \
&& echo alias python=python3.12 >> ~/.bashrc \
&& rm -rf Python-${python_full_version}.tgz \
&& python3.12 -m pip install --upgrade pip
RUN python3.12 -m pip install --upgrade benchmark-runner \
&& curl -L "https://mirror.openshift.com/pub/openshift-v4/clients/ocp/${OCP_CLIENT_VERSION}/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz" -o "/tmp/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz" \
&& tar -xzvf /tmp/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz -C /tmp/ \
&& mv /tmp/kubectl /usr/local/bin/kubectl \
&& mv /tmp/oc /usr/local/bin/oc \
&& rm -rf /tmp/openshift-client-linux-${OCP_CLIENT_VERSION}.tar.gz /tmp/kubectl /tmp/oc \
&& curl -L "https://github.com/kubevirt/kubevirt/releases/download/v${VIRTCTL_VERSION}/virtctl-v${VIRTCTL_VERSION}-linux-amd64" -o /usr/local/bin/virtctl \
&& chmod +x /usr/local/bin/virtctl \
&& . ~/.bashrc \
&& mkdir -p ~/.kube ~/.ssh /tmp/run_artifacts \
&& git clone -b v1.0.4 https://github.com/cloud-bulldozer/benchmark-operator /tmp/benchmark-operator \
&& git clone -b v1.2.2-kata-ci https://github.com/RobertKrawitz/OpenShift4-tools /tmp/OpenShift4-tools \
Then you do all the dnf changes (which should be pretty stable) in one layer, the Python install (why do you need to do this, isn't python 3.12 available in the image?), and then the benchmark-runner specific changes in a third layer. So when you update benchmark-runner, you don't need to update the others.
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 Dockerfile was already reviewed in the past.
I know that combining steps can reduce the image size by creating fewer layers, but I prefer to keep them separate so each step runs independently — for easier debugging and more reliable builds.
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.
At least group all of the dnf commands together and do the dnf clean and rm -rf /var/cache/dnf with the dnf commands. If you need to break them up, put the clean commands after each dnf command so that the temporary files don't make it into any 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.
@RobertKrawitz Agree.
I have aggregated the steps according to subjects.
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.
Combine all of the dnf things (including cleanup) into one RUN statement, and fix the missing trailing newlines, please.
prow/OWNERS
Outdated
reviewers: | ||
- RobertKrawitz | ||
- jeniferh | ||
- gqlo |
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.
Need a trailing newline here.
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.
Done
12687f4
to
61a7b84
Compare
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.
Combine all of the dnf-related commands into one, and fix the missing trailing newlines, and I will approve it.
.ci-operator.yaml
Outdated
build_root_image: | ||
namespace: openshift | ||
name: release | ||
tag: golang-1.18 |
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.
Needs trailing newline.
prow/Dockerfile
Outdated
&& dnf install -y podman jq | ||
|
||
# Python installation | ||
RUN dnf install -y openssl-devel bzip2-devel wget libffi-devel \ |
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.
Combine this dnf install
and the dnf install -y hostname bc procps-ng && dnf clean all && rm -rf /var/cache/dnf
with the above DNF commands and I will approve it. The point is to do all of the dnf commands in one RUN statement.
eee073b
to
06e25b6
Compare
Type of change
Note: Fill x in []
Description
Add Dockerfile for Prow Ci
For security reasons, all pull requests need to be approved first before running any automated CI