Update kustomization to use params env and yml on notebook-controller and odh-notebook-controller #364
Conversation
|
I'm looking at a diff between your branch and red-hat-data-services/kubeflow:main, and I see lots of noise in the diff. And that's despite your PR branch is in sync with opendatahub-io/notebooks:v1.7-branch and individual files that show differences in the diff view are actually much more similar in reality
atheo89/kubeflow@work-on-kustomize...red-hat-data-services:kubeflow:master#diff-b02147f941d8f6f01343f0167180165c33c9469e108f7f66d063bb4a4aa6b540 (Files Changed tab) but looking at the actual file in both repos, it only differs in a few blank lines I'm still trying to understand if I am using GitHub wrong. I may have to checkout both locally and run diff on my machine. |
|
Ok, local diff works much better; here are the relevant differences. I wrote down how I got the diff on https://stackoverflow.com/questions/1968512/getting-the-difference-between-two-repositories/78734836#78734836 First differences,
downstream has the operator image parameterized, and references the ose-oauth-proxy by hash Second differencesthe manager-openshift-patch.yaml file is different
|
|
My understanding of this task is that manifests between opendatahub-io and red-hat-data-services repos should be made identical, with the only difference of image hashes in the .env files. If you think the scope is smaller than that, I can accept it, just that we are on the same page about this. |
|
Thank for the review!
You mean that we have to apply this sync in all manifest files to be identical with downstream? if so, i guess i can do that. I had the impression according to the issue description that we have to do it only on the https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/notebook-controller/config/overlays/openshift/kustomization.yaml however i expanded it also to odh-notebook-controller |
|
Description on the Jira first says
but then the Dev section below is more selective, as you say. |
|
Give me some time to discuss and groom this further with Harshad and i will check it again |
atheo89
force-pushed
the
work-on-kustomize
branch
2 times, most recently
from
0ffe5df
to
6724edb
Compare
atheo89
force-pushed
the
work-on-kustomize
branch
2 times, most recently
from
73f5b34
to
08616ca
Compare
There was a problem hiding this comment.
This changes are good, however it is missing the referencing of the image.
as the image substitution is changed from kustomize image, to config-param method, we need to invoke the variable in deployment, for the substitution to happen.
for notebook-controller:
- Following change is needed: https://github.com/red-hat-data-services/kubeflow/blob/750bf133f7b51f7d319d35389de06eadf9bb3eed/components/notebook-controller/config/overlays/openshift/manager_openshift_patch.yaml#L35
For odh-notebook-controller:
- https://github.com/red-hat-data-services/kubeflow/blob/750bf133f7b51f7d319d35389de06eadf9bb3eed/components/odh-notebook-controller/config/manager/manager.yaml#L24
- https://github.com/red-hat-data-services/kubeflow/blob/master/components/odh-notebook-controller/config/manager/params.yaml
Please take a look.
|
And then, with the changes Harshad described, you'd probably have to do also this, for ci to keep working red-hat-data-services#62 (it's also suggested in the QA section of the Jira issue) |
|
Thank you @harshad16 and @jiridanek for your review 🙂 I have addressed your comments, and the PR is now ready for another round of review. |
|
@jiridanek After that change on the CI, it make me think that the ci doesn't check the images of the controller generated of this PR. We should consider opening an issue to ensure these images are being properly checked. |
can you elaborate? If the problem you see that in the odh- case the integration test is not using the image that was just built, then that's a regression compared to
|
|
Okkk, so the |
.github/workflows/odh_notebook_controller_integration_test.yaml
Outdated
Show resolved
Hide resolved
.github/workflows/odh_notebook_controller_integration_test.yaml
Outdated
Show resolved
Hide resolved
| @@ -8,17 +8,27 @@ commonLabels: | |||
| app.kubernetes.io/part-of: odh-notebook-controller | |||
There was a problem hiding this comment.
downstream does not have namespace: on line 6, is that necessary to have it hardcoded here, or should the operator be responsible for deciding the namespace depending on user configuration?
There was a problem hiding this comment.
Hmm not sure, probably there is some reason @harshad16 wdyt?
Upstream: https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/notebook-controller/config/overlays/openshift/kustomization.yaml#L6
Downstream: https://github.com/red-hat-data-services/kubeflow/blob/master/components/notebook-controller/config/overlays/openshift/kustomization.yaml
There was a problem hiding this comment.
i m not aware, let ask this to operator team.
for now, lets keep this as it is.
.github/workflows/odh_notebook_controller_integration_test.yaml
Outdated
Show resolved
Hide resolved
atheo89
force-pushed
the
work-on-kustomize
branch
2 times, most recently
from
c885cc2
to
52e49f5
Compare
components/odh-notebook-controller/config/manager/kustomization.yaml
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1 @@ | |||
| odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:1.7-35b81f5 | |||
There was a problem hiding this comment.
So when we previously referenced the image as
image: quay.io/opendatahub/odh-notebook-controller:latest
that meant that whatever version of odh user installed, they would get the latest controller image we built?
does the odh operator uses some mechanism to set operator image hashes? the rhoai operator certainly does something like that
There was a problem hiding this comment.
image: quay.io/opendatahub/odh-notebook-controller:latest
I didn't check the code thoroughly, but you mean, we referenced also an image from a different repository before?
edit: I think that we used same repo, your concern is just the latest tag, I suppose
There was a problem hiding this comment.
This is just the https://issues.redhat.com/browse/RHOAIENG-9234 problem, nothing new.
edit: yes, the latest tag, and I actually haven't figured out how precisely could Andrej get
- name: quay.io/opendatahub/odh-dashboard:main
- name: quay.io/opendatahub/odh-notebook-controller:1.7-35b81f5
in odh-nightly.
There was a problem hiding this comment.
that meant that whatever version of odh user installed, they would get the latest controller image we built?
Latest was the fall back, it like if kustomize fails to patch the image, it would just stay latest.
in previous version, kustomize image was been used.
does the odh operator uses some mechanism to set operator image hashes? the rhoai operator certainly does something like that
rhoai operator switch image based on the sha give from builds.
perhaps worth check with operator team.
.github/workflows/odh_notebook_controller_integration_test.yaml
Outdated
Show resolved
Hide resolved
atheo89
force-pushed
the
work-on-kustomize
branch
from
52e49f5
to
acff2c3
Compare
|
/LGTM from me, we have some unresolved questions and followup issues in Jira, but that's just that, followup |
|
Hey @harshad16 could you please take a look? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harshad16 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 |
|
/cherrypick stable |
|
@jiridanek: once the present PR merges, I will cherry-pick it on top of stable in a new PR and assign it to you. In response to this:
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. |
|
/override "Code static analysis / govulncheck (components/notebook-controller)" |
|
@jiridanek: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In response to this:
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. |
|
/override "govulncheck (components/notebook-controller)" |
|
@jiridanek: Overrode contexts on behalf of jiridanek: govulncheck (components/notebook-controller) In response to this:
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. |
|
/override "govulncheck (components/odh-notebook-controller)" Vulnerabille go deps are already present on base branch |
|
@jiridanek: Overrode contexts on behalf of jiridanek: govulncheck (components/odh-notebook-controller) In response to this:
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. |
openshift-merge-bot
bot
merged commit 301bfce
into
opendatahub-io:v1.7-branch
|
@jiridanek: #364 failed to apply on top of branch "stable": In response to this:
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. |
Related to: https://issues.redhat.com/browse/RHOAIENG-9008
Description
Sync manifests among upstream
v1.7-branchand downstreammasterbranchesNote: For reference, the parametrization of the manifests incorporated downstream via this PR is as follows:: red-hat-data-services#29
How has been tested
Evaluate notebook-controller deployment by running the following:
$ cd components/notebook-controller$ kustomize build config/overlays/openshiftCheck on the image on the
notebook-controller-deploymentdeployment that matched with theimage: quay.io/opendatahub/kubeflow-notebook-controller:1.7-35b81f5Evaluate odh-notebook-controller deployment by running the following:
$ cd components/odh-notebook-controller$ kustomize build config/baseCheck on the image on the deployment of
odh-notebook-controller-managerthat matched with theimage: quay.io/opendatahub/kubeflow-notebook-controller:1.7-35b81f5Merge criteria: