Don't restrict the nbc image update to downstream only

jstourac commented 1 month ago

This should address the [1]. With current code it wasn't possible to override the images used for the notebook controllers via env property, now it should be possible.

[1] https://issues.redhat.com/browse/RHOAIENG-11134

How Has This Been Tested?

I installed the latest OpenDataHub operator (v2.16) and amended the CSV by:

pointing the deployment containter to the image build for this PR
added the desired env variables RELATED_IMAGE_ODH_KF_NOTEBOOK_CONTROLLER_IMAGE and RELATED_IMAGE_ODH_NOTEBOOK_CONTROLLER_IMAGE
then create the DSCI and DSC instances and see what images were used for the both notebook controller pods

CSV

...
                containers:
                    env:
                      - name: DISABLE_DSC_CONFIG
                        value: 'true'
                      - name: OPERATOR_NAMESPACE
                        valueFrom:
                          fieldRef:
                            fieldPath: metadata.namespace
                      - name: DEFAULT_MANIFESTS_PATH
                        value: /opt/manifests
                      - name: RELATED_IMAGE_ODH_DASHBOARD_IMAGE
                        value: 'registry.redhat.io/rhoai/odh-dashboard-rhel8@sha256:f11a6e12fdba4547578f909cea56bc1de54e66d3cf4e3808494cb16dba4091a1'
                      - name: RELATED_IMAGE_ODH_KF_NOTEBOOK_CONTROLLER_IMAGE
                        value: 'registry.redhat.io/rhoai/odh-kf-notebook-controller-rhel8@sha256:f3a9f12560560739c8d0f44dd654a5fa834e5f4337f4aea13fc2e929f299e222'
                      - name: RELATED_IMAGE_ODH_NOTEBOOK_CONTROLLER_IMAGE
                        value: 'registry.redhat.io/rhoai/odh-notebook-controller-rhel8@sha256:1e1bddd06ed0d4234bcc9890bfafec29d06ad0e35a6c663c9d7174a8b2eaf3ab'
                    image: 'quay.io/opendatahub/opendatahub-operator@sha256:120687732c88e817cd176805d37866b475b837fee73e9fb8ed442cf2fae555f1'
...

Resulting in:

$ oc describe deployment -n opendatahub notebook-controller-deployment | grep Image
    Image:      registry.redhat.io/rhoai/odh-kf-notebook-controller-rhel8@sha256:f3a9f12560560739c8d0f44dd654a5fa834e5f4337f4aea13fc2e929f299e222

$ oc describe deployment -n opendatahub odh-notebook-controller-manager | grep Image
    Image:       registry.redhat.io/rhoai/odh-notebook-controller-rhel8@sha256:1e1bddd06ed0d4234bcc9890bfafec29d06ad0e35a6c663c9d7174a8b2eaf3ab

So now the changes are applied not only for the RHOAI build, but also for the ODH - which will be handy for the ODH-nightly builds, but also for the devel purpose and custom changes testing.

I also checked that using this new image without specifying the RELATED_IMAGE_ODH_KF_NOTEBOOK_CONTROLLER_IMAGE and RELATED_IMAGE_ODH_NOTEBOOK_CONTROLLER_IMAGE env variables doesn't cause any harm and the default images for these components are used, see:

$ oc describe deployment -n opendatahub notebook-controller-deployment | grep Image    
    Image:      quay.io/opendatahub/kubeflow-notebook-controller:1.7-8a9b65a

$ oc describe deployment -n opendatahub odh-notebook-controller-manager | grep Image
    Image:       quay.io/opendatahub/odh-notebook-controller:1.7-8a9b65a

Merge criteria

[x] You have read the contributors guide.
[x] Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
[x] Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
[x] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
[x] The developer has manually tested the changes and verified that the changes work

openshift-ci[bot] commented 1 month ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

jiridanek commented 1 month ago

/test all

jiridanek commented 2 weeks ago

/lgtm

jiridanek commented 2 weeks ago

/test all

jstourac commented 2 weeks ago

I'm not sure why there was an e2e test failure, seemed like unrelated to changes in this PR. I tried to rebase this and let's see the results again.

ykaliuta commented 2 weeks ago

/test opendatahub-operator-e2e

jstourac commented 2 weeks ago

FTR - looking on the https://github.com/opendatahub-io/opendatahub-operator/pull/1191/files#diff-6d5312e9d3f4f1a257318e13d950ef49b99608320bb6c5893fee2a1699a16094 - if that gets merged, it contains changes here already and IIUC my testing approach I used for this won't work in the future - would have to actually restart the operator pod to trigger the image init.

ykaliuta commented 2 weeks ago

This can go first, it's a small change comparing to.

zdtsw commented 2 weeks ago

i agree

jstourac commented 2 weeks ago

Thanks guys! Feel free to merge this whenever it suits you. I don't have rights to do so, obviously :slightly_smiling_face:

jiridanek commented 2 weeks ago

let's put it to the test /approve

openshift-ci[bot] commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiridanek, zdtsw

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: - ~~[OWNERS](https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/OWNERS)~~ [zdtsw] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

opendatahub-io / opendatahub-operator

Don't restrict the nbc image update to downstream only #1170

How Has This Been Tested?

Merge criteria