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

fix: nbc not use correct image for downstream + cleanup downstream path #1226

Merged

Conversation

zdtsw
Copy link
Member

@zdtsw zdtsw commented Sep 9, 2024

Description

  • fix wrong caller on notebookControllerPath
    this is the problem when we have reconcile twice on odh-notebook-controller see https://issues.redhat.com/browse/RHOAIENG-12732
    old: DeployManifestsFromPath on notebookControllerPath, then update params.env file.
    new: update params.env file then do call DeployManifestsFromPath on notebookControllerPath
  • remove downstream speicific path for notebook: notebookImagesPathSupported is removed (along with defaultKustomizePathSupported) , see downstream PR
  • move default workbench namespace into const
  • removed duplicated comments, probably from rebase long time ago

a similar PR in downstream red-hat-data-services#343, since it also need to change the path so i cannot just have sync to it.

with these two PRs, the code in ODH and downstream should be 100% the same for workbenches.

https://issues.redhat.com/browse/RHOAIENG-12732

How Has This Been Tested?

local build: quay.io/wenzhou/opendatahub-operator-catalog:v2.14.910-0

  • install build
  • create DSC with workbench enabled
  • CM notebook is created
  • CM notebooks-parameters is created
  • 13 imagestreams CR created
  • kf-notebook controller quay.io/opendatahub/kubeflow-notebook-controller:1.7-5d51c2b is running
  • odh-notebook controller pods quay.io/opendatahub/odh-notebook-controller:1.7-5d51c2b is running

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • 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

@zdtsw zdtsw requested review from harshad16 and VaishnaviHire and removed request for gzaronikas and jackdelahunt September 9, 2024 15:53
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

Till i understand the code-base, this is reference to old path , that was maintained in the odh-manifests.
i believe it was correct: https://github.com/red-hat-data-services/odh-manifests/blob/master/jupyterhub/notebook-images/overlays/additional/kustomization.yaml
there is no folder: /jupyterhub/notebooks/base

is that a correct understanding or am i missing something ?

@zdtsw
Copy link
Member Author

zdtsw commented Sep 9, 2024

Till i understand the code-base, this is reference to old path , that was maintained in the odh-manifests. i believe it was correct: https://github.com/red-hat-data-services/odh-manifests/blob/master/jupyterhub/notebook-images/overlays/additional/kustomization.yaml there is no folder: /jupyterhub/notebooks/base

is that a correct understanding or am i missing something ?

it is a little bit tricky
i think back to we just started v2, there was a difference of manifests structure in ODH and RHOAI.
so we make a different way
in ODH https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/get_all_manifests.sh#L16
in RHOAI: https://github.com/red-hat-data-services/rhods-operator/blob/main/get_all_manifests.sh#L17

this basically leads to for the RHOAI path it is point to jupyterhub/notebooks

one thing might be easier to narrow these gaps: is there any difference in manifests structure still between ODH and RHOAI for workbenches (both notebook and nbc)
if the answer is "no" i can do more updates on workbenches on top of this PR

- remove downstream speicific path which is not needed any more
- remove duplicated call with deploy manifests for notebooks
- move default wb NS in to const
- fix comments

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@harshad16
Copy link
Member

it is a little bit tricky i think back to we just started v2, there was a difference of manifests structure in ODH and RHOAI. so we make a different way in ODH https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/get_all_manifests.sh#L16 in RHOAI: https://github.com/red-hat-data-services/rhods-operator/blob/main/get_all_manifests.sh#L17

this basically leads to for the RHOAI path it is point to jupyterhub/notebooks

one thing might be easier to narrow these gaps: is there any difference in manifests structure still between ODH and RHOAI for workbenches (both notebook and nbc) if the answer is "no" i can do more updates on workbenches on top of this PR

Ack, on the explanation.
However, I'm still confused about the path: jupyterhub/notebooks

Currently the upstream and downstream. manifests are synced for both Notebook and NBC.
The only difference is the param file with different content in them.
example:
Notebook:

@zdtsw
Copy link
Member Author

zdtsw commented Sep 10, 2024

it is a little bit tricky i think back to we just started v2, there was a difference of manifests structure in ODH and RHOAI. so we make a different way in ODH https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/get_all_manifests.sh#L16 in RHOAI: https://github.com/red-hat-data-services/rhods-operator/blob/main/get_all_manifests.sh#L17
this basically leads to for the RHOAI path it is point to jupyterhub/notebooks
one thing might be easier to narrow these gaps: is there any difference in manifests structure still between ODH and RHOAI for workbenches (both notebook and nbc) if the answer is "no" i can do more updates on workbenches on top of this PR

Ack, on the explanation. However, I'm still confused about the path: jupyterhub/notebooks

Currently the upstream and downstream. manifests are synced for both Notebook and NBC. The only difference is the param file with different content in them. example: Notebook:

I did some new changes this morning,
i think this will be easier to review: in short, the "jupyter" is removed. so we only have one path for notebook image for both ODH and RHOAI

@zdtsw zdtsw requested a review from jstourac September 10, 2024 10:01
@jstourac
Copy link
Member

I can't really talk about the path changes. The code logic having the manifest deployment after it is patched makes sense though.

I also tested IIB on my cluster and tried to delete the operator pod as described in https://issues.redhat.com/browse/RHOAIENG-12409 and correct image for both notebook related deployments was used as expected, which is fine.

Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

lgtm
thanks for the explanation

@zdtsw zdtsw changed the title fix: wrong path which is used in downstream fix: nbc not use correct image for downstream + cleanup downstream path Sep 11, 2024
@zdtsw zdtsw added the odh-2.18 label Sep 11, 2024
Copy link
Member

@VaishnaviHire VaishnaviHire left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Sep 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VaishnaviHire

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-merge-bot openshift-merge-bot bot merged commit a8525cd into opendatahub-io:incubation Sep 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants