Skip to content
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

OKD-40: Unify OKD and OCP Dockerfiles #1058

Merged
merged 1 commit into from
May 14, 2024

Conversation

jupierce
Copy link
Contributor

@jupierce jupierce commented May 10, 2024

In order to support build OKD images on SCOS, the OKD team needs either (a) the same dockerfile to build successfully on CentOS and RHEL or (b) a separate file that builds on CentoS with the same number of layers as used by the OCP Dockerfile built by ART.
This approach unifies the NTO Dockerfile.rhel9 to support model (a).

The Dockerfile.rhel9 OKD image size is ~495MiB vs a 453MB Dockerfile build.

@jmencak
Copy link
Contributor

jmencak commented May 10, 2024

Thank you for the PR, Justin. I probably shouldn't be doing reviews Friday evening, so only a few initial thoughts.

  • I believe this approach is better than the original PR mentioned above.
  • This PR introduces code duplication (potential config drift in the future) between dockerfile_install_support.sh and Dockerfile. Can we also adjust Dockerfile to use dockerfile_install_support.sh ?
  • The dockerfile_install_support.sh should probably go to the /hack directory.

I'll try to do a more thorough review next week. PTO Monday. Thank you.

@Prashanth684
Copy link
Contributor

/test okd-scos-images

Copy link
Contributor

@jmencak jmencak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes. podman build --squash-all helps a lot with reducing the resulting images sizes.

Only a couple of nits/questions left from my side. There is quite a lot of duplication in the if/else clauses in dockerfile_install_support.sh, but we/I can clean this up after the fact. Thank you for the PR.
/approve
Almost looks good to me.

.gitignore Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile.rhel9 Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2024
@jmencak
Copy link
Contributor

jmencak commented May 14, 2024

/lgtm cancel

@jmencak
Copy link
Contributor

jmencak commented May 14, 2024

/approve

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels May 14, 2024
Copy link
Contributor

openshift-ci bot commented May 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmencak, jupierce

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

set -o xtrace

source /etc/os-release
if [[ "${ID}" == "centos" ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be centos OR fedora, in case we bring back OKD/FCOS builds

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, nevermind, this wouldn't work, as OKD/FCOS payload images would still be using the centos base image for components.

Maybe this should instead make use of the ARG TAGS=ocp, which would be overriden in the OKD builds with TAGS=scos for OKD/SCOS and TAGS=fcos for OKD/FCOS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, not a blocker.

@Prashanth684 Prashanth684 changed the title Unify OKD and OCP Dockerfiles OKD-40: Unify OKD and OCP Dockerfiles May 14, 2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 14, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented May 14, 2024

@jupierce: This pull request references OKD-40 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

In order to support build OKD images on SCOS, the OKD team needs either (a) the same dockerfile to build successfully on CentOS and RHEL or (b) a separate file that builds on CentoS with the same number of layers as used by the OCP Dockerfile built by ART.
This approach unifies the NTO Dockerfile.rhel9 to support model (a).

The Dockerfile.rhel9 OKD image size is ~495MiB vs a 453MB Dockerfile build.

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 openshift-eng/jira-lifecycle-plugin repository.

@jupierce
Copy link
Contributor Author

As this is one of the last images we need to get OKD building again and the changes should be a no-op to OCP.
/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label May 14, 2024
@jmencak
Copy link
Contributor

jmencak commented May 14, 2024

Thank you for the changes, Justin. I thought we could get rid of the second "RUN" completely in both Dockerfiles, i.e. moving:

dnf clean all
rm -rf /var/cache/yum ~/patches /root/rpms
useradd -r -u 499 cluster-node-tuning-operator

too. But I don't think this is a blocker but we can clean this up later on.

So
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2024
Copy link
Contributor

openshift-ci bot commented May 14, 2024

@jupierce: all tests passed!

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.

@openshift-merge-bot openshift-merge-bot bot merged commit f7fcf8c into openshift:master May 14, 2024
16 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build cluster-node-tuning-operator-container-v4.17.0-202405151441.p0.gf7fcf8c.assembly.stream.el9 for distgit cluster-node-tuning-operator.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants