Update the Handler when watch on SetContainerImageFromRegistry functi… #336

atheo89 · 2024-05-28T12:28:31Z

…on to check also for update

Related to: https://issues.redhat.com/browse/RHOAIENG-3411

Description

How Has This Been Tested?

Replace odh-nbc-controller image with: quay.io/opendatahub/odh-notebook-controller:pr-336

Dashboard changes for the !Deleted label are on quay.io/opendatahub/odh-dashboard:pr-2867

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

components/odh-notebook-controller/controllers/notebook_webhook.go

jiridanek · 2024-05-28T13:33:46Z

components/odh-notebook-controller/controllers/notebook_webhook.go

@@ -456,100 +459,95 @@ func InjectCertConfig(notebook *nbv1.Notebook, configMapName string) error {
 // Otherwise, it checks the last-image-selection annotation to find the image stream and fetches the image from status.dockerImageReference,
 // assigning it to the container.image value.
 func SetContainerImageFromRegistry(ctx context.Context, config *rest.Config, notebook *nbv1.Notebook, log logr.Logger) error {
-    // Create a dynamic client


I did not notice this did not pass through go fmt before.

harshad16

Great intiative, provide some suggestion:

components/odh-notebook-controller/controllers/notebook_webhook.go

jstourac · 2024-05-29T08:46:38Z

Thank you for this, Andriana, I'm gonna test this out today. Apart from what Jiri proposed, I have no more objections.

Just some nitpicks I commented on the previous PR in case you feel make sense and could be implemented:

atheo89 · 2024-05-30T09:25:08Z

Thank you for this, Andriana, I'm gonna test this out today. Apart from what Jiri proposed, I have no more objections.

Just some nitpicks I commented on the previous PR in case you feel make sense and could be implemented:

https://github.com/opendatahub-io/kubeflow/pull/329/files#r1617192294

https://github.com/opendatahub-io/kubeflow/pull/329/files#r1617191061

Thanks, Jan. I have updated the logic based on your feedback in this comment. The code now lists the containers and performs the updates when container.name matches notebook.name. Regarding this comment, I have applied logs where applicable.

atheo89 · 2024-05-30T09:46:16Z

/test odh-notebook-controller-unit

components/odh-notebook-controller/controllers/notebook_webhook.go

jstourac · 2024-05-30T12:58:56Z

components/odh-notebook-controller/controllers/notebook_webhook.go

+						log.Info("Internal registry found. Will pickup the default value from image field.")
+						return nil
+					} else {
+						log.Info("No Internal registry found, pick up imageHash from status.tag.dockerImageReference")


Also, I see this message twice in my attempts now. At the beginning and at the end of the notebook spin up 🤔

{"level":"info","ts":"2024-05-30T12:54:28Z","logger":"controllers.Notebook","msg":"Injecting trusted-ca volume and environment variables","notebook":"older-tag","namespace":"test"} {"level":"info","ts":"2024-05-30T12:54:28Z","logger":"controllers.Notebook","msg":"No Internal registry found, pick up imageHash from status.tag.dockerImageReference","notebook":"older-tag","namespace":"test"} {"level":"info","ts":"2024-05-30T12:54:28Z","logger":"controllers.Notebook","msg":"Creating Network Policy","name":"older-tag-ctrl-np"} {"level":"info","ts":"2024-05-30T12:54:28Z","logger":"controllers.Notebook","msg":"Creating Network Policy","name":"older-tag-oauth-np"} {"level":"info","ts":"2024-05-30T12:54:28Z","logger":"controllers.Notebook","msg":"Creating Service Account","notebook":"older-tag","namespace":"test"} {"level":"info","ts":"2024-05-30T12:54:28Z","logger":"controllers.Notebook","msg":"Creating OAuth Service","notebook":"older-tag","namespace":"test"} {"level":"info","ts":"2024-05-30T12:54:28Z","logger":"controllers.Notebook","msg":"Creating OAuth Secret","notebook":"older-tag","namespace":"test"} {"level":"info","ts":"2024-05-30T12:54:28Z","logger":"controllers.Notebook","msg":"Creating Route","notebook":"older-tag","namespace":"test"} {"level":"info","ts":"2024-05-30T12:54:28Z","logger":"controllers.Notebook","msg":"Removing reconciliation lock","notebook":"older-tag","namespace":"test"} {"level":"info","ts":"2024-05-30T12:54:34Z","logger":"controllers.Notebook","msg":"No Internal registry found, pick up imageHash from status.tag.dockerImageReference","notebook":"older-tag","namespace":"test"}

This seems that is generated by the admissionv1 (L258) one when it creates the notebook and one when the notebook is been updated.
To understand this a bit added some debug prints fmt.Println("2. ", container.Image) and i saw the following:

{"level":"info","ts":"2024-05-30T16:49:09Z","msg":"Starting workers","controller":"notebook","controllerGroup":"kubeflow.org","controllerKind":"Notebook","worker count":1} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"odh-trusted-ca-bundle ConfigMap is not present, not starting mounting process.","notebook":"q7","namespace":"test"} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"Entering SetContainerImageFromRegistry","notebook":"q7","namespace":"test"} 2. :2024.1 {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"No internal registry found, let's pick up image reference from relevant ImageStream 'status.tags[].tag.dockerImageReference'","notebook":"q7","namespace":"test"} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"Exiting SetContainerImageFromRegistry","notebook":"q7","namespace":"test"} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"Creating Network Policy","name":"q7-ctrl-np"} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"Creating Network Policy","name":"q7-oauth-np"} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"Creating Service Account","notebook":"q7","namespace":"test"} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"Creating OAuth Service","notebook":"q7","namespace":"test"} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"Creating OAuth Secret","notebook":"q7","namespace":"test"} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"Creating Route","notebook":"q7","namespace":"test"} {"level":"info","ts":"2024-05-30T16:49:27Z","logger":"controllers.Notebook","msg":"Removing reconciliation lock","notebook":"q7","namespace":"test"} {"level":"info","ts":"2024-05-30T16:49:28Z","logger":"controllers.Notebook","msg":"Entering SetContainerImageFromRegistry","notebook":"q7","namespace":"test"} 2. quay.io/opendatahub/workbench-images@sha256:61fa79b91af6dc2276d7d8d799f8af36edc8e256db23a808362d9c8c4c45bb51 {"level":"info","ts":"2024-05-30T16:49:28Z","logger":"controllers.Notebook","msg":"Exiting SetContainerImageFromRegistry","notebook":"q7","namespace":"test"}

However, when you EDIT the notebook to for example ds:2023.2 the received debug message always 2. :2023.1 which means that when on UPDATE mode it get the default container.Image value that comes from the dashboard.

So, maybe the idea that i had to use something like this to display or not the log info maybe can not be applied if we use the admission update functionality

// If the image value is invalid means that we don't have internal registry picked up by default from dashboard invalidImage := regexp.MustCompile(`^:\d+\.\d+$`) if invalidImage.MatchString(container.Image) { log.Info("No internal registry found, let's pick up image reference from relevant ImageStream 'status.tags[].tag.dockerImageReference'") }

I don't have a good solution as for now Jan for this, I suggest to track it on a jira issue to implemented later on as enhancement. I really afraid to touch it right now 🤣

components/odh-notebook-controller/controllers/notebook_webhook.go

…on to check also for update Add test for checking the update of the notebook

harshad16

/lgtm
/approve

Based on everyone review and discussion
this seems apt for the merge.
Thanks for all the work 💯

openshift-ci · 2024-05-31T15:10:27Z

[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:

~~OWNERS~~ [harshad16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

harshad16 · 2024-05-31T15:58:39Z

/cherrypick stable

openshift-cherrypick-robot · 2024-05-31T15:59:25Z

@harshad16: #336 failed to apply on top of branch "stable":

Applying: Update the Handler when watch on SetContainerImageFromRegistry function to check also for update
Using index info to reconstruct a base tree...
M	components/odh-notebook-controller/controllers/notebook_controller_test.go
M	components/odh-notebook-controller/controllers/notebook_webhook.go
M	components/odh-notebook-controller/e2e/notebook_controller_setup_test.go
Falling back to patching base and 3-way merge...
Auto-merging components/odh-notebook-controller/e2e/notebook_controller_setup_test.go
Auto-merging components/odh-notebook-controller/controllers/notebook_webhook.go
Auto-merging components/odh-notebook-controller/controllers/notebook_controller_test.go
CONFLICT (content): Merge conflict in components/odh-notebook-controller/controllers/notebook_controller_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Update the Handler when watch on SetContainerImageFromRegistry function to check also for update
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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.

atheo89 requested a review from harshad16 May 28, 2024 12:28

openshift-ci bot requested a review from VaishnaviHire May 28, 2024 12:28

jiridanek reviewed May 28, 2024

View reviewed changes

components/odh-notebook-controller/controllers/notebook_webhook.go Show resolved Hide resolved

jiridanek reviewed May 28, 2024

View reviewed changes

harshad16 requested changes May 29, 2024

View reviewed changes

components/odh-notebook-controller/controllers/notebook_webhook.go Show resolved Hide resolved

openshift-ci bot assigned harshad16 May 29, 2024

atheo89 force-pushed the fix-edit branch from 369a85d to 28d763e Compare May 30, 2024 09:16

atheo89 force-pushed the fix-edit branch from 28d763e to ffe6d60 Compare May 30, 2024 10:44

jstourac reviewed May 30, 2024

View reviewed changes

components/odh-notebook-controller/controllers/notebook_webhook.go Outdated Show resolved Hide resolved

jstourac reviewed May 30, 2024

View reviewed changes

components/odh-notebook-controller/controllers/notebook_webhook.go Outdated Show resolved Hide resolved

jstourac reviewed May 30, 2024

View reviewed changes

components/odh-notebook-controller/controllers/notebook_webhook.go Outdated Show resolved Hide resolved

atheo89 force-pushed the fix-edit branch 2 times, most recently from 6109c51 to 76ff751 Compare May 31, 2024 10:06

This comment was marked as duplicate.

Sign in to view

Update the Handler when watch on SetContainerImageFromRegistry functi…

5dbf075

…on to check also for update Add test for checking the update of the notebook

atheo89 force-pushed the fix-edit branch from 76ff751 to 5dbf075 Compare May 31, 2024 12:01

harshad16 approved these changes May 31, 2024

View reviewed changes

openshift-ci bot added the lgtm label May 31, 2024

openshift-ci bot added the approved label May 31, 2024

openshift-merge-bot bot merged commit 92e8404 into opendatahub-io:v1.7-branch May 31, 2024
7 checks passed

harshad16 mentioned this pull request May 31, 2024

[stable]Update the Handler when watch on SetContainerImageFromRegistry func #337

Merged

This was referenced Jun 4, 2024

Add image trigger annotation for filling in image-field-value of container. Use imagestream name and tag for JUPYTER_IMAGE opendatahub-io/odh-dashboard#800

Closed

Image change trigger plugin exclude image field from reconciliation #133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the Handler when watch on SetContainerImageFromRegistry functi… #336

Update the Handler when watch on SetContainerImageFromRegistry functi… #336

atheo89 commented May 28, 2024 •

edited

Loading

jiridanek May 28, 2024

jiridanek May 28, 2024

harshad16 left a comment

jstourac commented May 29, 2024

atheo89 commented May 30, 2024

atheo89 commented May 30, 2024

jstourac May 30, 2024

atheo89 May 30, 2024

atheo89 May 31, 2024

This comment was marked as duplicate.

harshad16 left a comment

openshift-ci bot commented May 31, 2024

harshad16 commented May 31, 2024

openshift-cherrypick-robot commented May 31, 2024

Update the Handler when watch on SetContainerImageFromRegistry functi… #336

Update the Handler when watch on SetContainerImageFromRegistry functi… #336

Conversation

atheo89 commented May 28, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

jiridanek May 28, 2024

Choose a reason for hiding this comment

jiridanek May 28, 2024

Choose a reason for hiding this comment

harshad16 left a comment

Choose a reason for hiding this comment

jstourac commented May 29, 2024

atheo89 commented May 30, 2024

atheo89 commented May 30, 2024

jstourac May 30, 2024

Choose a reason for hiding this comment

atheo89 May 30, 2024

Choose a reason for hiding this comment

atheo89 May 31, 2024

Choose a reason for hiding this comment

This comment was marked as duplicate.

harshad16 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 31, 2024

harshad16 commented May 31, 2024

openshift-cherrypick-robot commented May 31, 2024

atheo89 commented May 28, 2024 •

edited

Loading