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

RHOAIENG-72: Update Dockerfile for {odh-}notebook-controller component #340

Merged

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Jun 4, 2024

https://issues.redhat.com/browse/RHOAIENG-72, this does not fix the issue, but takes the first step towards fixing it in a subsequent PR.

The new Dockerfile in this PR closely corresponds to what is used in OpenShift AI downstream build. This way we will have closer match between what we release upstream and downstream.

Description

This commit modifies the Dockerfile for the odh-notebook-controller component in the Kubeflow project. The changes include updating the base image, specifying the Golang version to 1.20, and configuring a non-root user for execution. More robust and secure build processes are now used including different steps for package installation, code copying, and building the notebook-controller.

How Has This Been Tested?

docker build -f notebook-controller/Dockerfile . --progress plain
docker build -f odh-notebook-controller/Dockerfile . --progress plain

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

This commit modifies the Dockerfile for the {odh-}notebook-controller component in the Kubeflow project. The changes include updating the base image, specifying the Golang version to 1.20, and configuring a non-root user for execution. More robust and secure build processes are now used including different steps for package installation, code copying, and building the notebook-controller.

The Dockerfile closely corresponds to what is used in OpenShift AI build. This way we will have closer match between what we release upstream and downstream.
@openshift-ci openshift-ci bot requested review from atheo89 and dibryant June 4, 2024 16:18
@jiridanek jiridanek requested a review from harshad16 June 4, 2024 18:26
@atheo89
Copy link
Member

atheo89 commented Jun 7, 2024

/test all

@atheo89
Copy link
Member

atheo89 commented Jun 7, 2024

Thanks Jiri,

/lgtm
/approve

Copy link

openshift-ci bot commented Jun 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atheo89

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

@openshift-ci openshift-ci bot added the approved label Jun 7, 2024
RUN useradd --uid 1001 --create-home --user-group --system rhods

## Set workdir directory to user home
WORKDIR /home/rhods
Copy link
Member

Choose a reason for hiding this comment

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

Do we like this branding in an upstream?

Copy link
Member Author

@jiridanek jiridanek Jun 7, 2024

Choose a reason for hiding this comment

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

I mentioned this myself in #340 (comment). @VaishnaviHire mentioned on slack that opendatahub-io is "midstream", so I think we can afford this.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's like we depend on the oauth proxy image that is only available with redhat subscription, not on unregistered okd, and odh needs operators in the redhat catalog that aren't in the community catalog

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... this is all insane... I wonder who installs ODH since they need to have access to OpenShift and Red Hat catalogue anyway... why not to use properly supported RHOAI... only reason I could possibly see are the new features there that haven't been ported to RHOAI yet... which shall change...

Anyway, I don't have strict opinion... but we use different naming in ODH and RHOAI in some places - e.g. dashboard pods are named as odh-dahsboard in ODH and rhods-dashboard in RHOAI. On the other hand here in IDE scrum, we keep the same name of notebook manager with odh- prefix both in RHOAI and ODH... this change just backports naming from RHOAI back to ODH... so we are doing a completely disgusting mix...

Copy link
Member

Choose a reason for hiding this comment

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

Just saying... I also like things to be same for ease and convenience... it's just that it's a total mess and chaos here. Feel free to ignore this thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aren't we planning on abandoning the @red-hat-data-science repos in the future (cc @adelton). How are we going to be doing this kind of "branding" then? Changing the username in dockerfile seems kind of silly and pointless waste of effort to me, best to have some neutral name that can be used everywhere the same.

@openshift-merge-bot openshift-merge-bot bot merged commit 87f4b2d into opendatahub-io:v1.7-branch Jun 7, 2024
9 checks passed
@jiridanek jiridanek deleted the jd_dockerfile branch June 7, 2024 08:33
@harshad16
Copy link
Member

/cherrypick stable

@openshift-cherrypick-robot

@harshad16: new pull request created: #350

In response to this:

/cherrypick stable

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants