-
Notifications
You must be signed in to change notification settings - Fork 37
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-15141] Fix failing E2E tests #473
Conversation
@@ -65,7 +65,9 @@ func (tc *testContext) testNotebookUpdate(nbContext notebookContext) error { | |||
} | |||
|
|||
// Example update: Change the Notebook image | |||
updatedNotebook.Spec.Template.Spec.Containers[0].Image = "new-image:latest" | |||
// newImage := "new-image:latest" quay.io/thoth-station/s2i-minimal-notebook:v0.2.2 | |||
newImage := "quay.io/opendatahub/workbench-images:jupyter-minimal-ubi9-python-3.11-20241119-3ceb400" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question here: I deliberately used our own workbench repository for this. But for the original first workbench, there is used that quay.io/thoth-station/s2i-minimal-notebook
... soo... let me know if we want that instead for some reason 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put toth into the github actions when I was touching these things. I guess it does not really matter, so let's go with your choice here, lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the images must be consistent with whatever the readiness / and-the-other-one probes that are defined on notebook CR, guess that's pretty much it and anything else would work; maybe that the entrypoint runs indefinitely, that may be also important, that the image does not exit on its own (or does not exit quickly)
The linter GHA failure is valid, but we use this method on multiple places there and I just moved it's call to a different place - so actually no change in regards to this. We shall think about get rid of these deprecated calls in the future though. |
I would've fixed it here, but I guess it's just me. I can approve with linter error present. |
🎉 |
360bb8e
to
0185ce6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just remove some of the old commented out code, the few lines about new-image:latest, and I think its good
let's see if anyone else wants to review in the pre-us-holidays rush
0185ce6
to
37cf97f
Compare
done |
There were two E2E tests in failure: --- FAIL: TestE2ENotebookController/update/thoth-minimal-oauth-notebook/Notebook_Statefulset_Validation_After_Update (60.28s) --- FAIL: TestE2ENotebookController/update/thoth-minimal-oauth-notebook/Verify_Notebook_Traffic_After_Update (10.75s) The reason was that after the culling test, the culling configuration is reverted, but the Notebook CR isn't updated and the annotation that was added by the notebook controller: `kubeflow-resource-stopped` stays in that CR and as such, the workbench instance isn't started back. This change deletes this annotation at the end of the culler test and the workbench is up and running and ready for the followup tests. Another issue there was that the update test updated the workbench to a non-existent image link, which resulted in a ImagePullBackOff Error in the end. This change updates this link to some existing image.
37cf97f
to
d977fbd
Compare
/lgtm |
I also rebased this now - so we can see how this works on the latest changes from main branch. |
/lgtm |
Thank you for all your reviews, guys. Let's move this in now! /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jstourac 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 |
/override golangci-lint (components/odh-notebook-controller) |
@jstourac: /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 "golangci-lint (components/odh-notebook-controller)" |
@jstourac: Overrode contexts on behalf of jstourac: golangci-lint (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. |
fc5ffd4
into
opendatahub-io:main
There were two E2E tests in failure:
--- FAIL: TestE2ENotebookController/update/thoth-minimal-oauth-notebook/Notebook_Statefulset_Validation_After_Update (60.28s)
--- FAIL: TestE2ENotebookController/update/thoth-minimal-oauth-notebook/Verify_Notebook_Traffic_After_Update (10.75s)
The reason was that after the culling test, the culling configuration is reverted, but the Notebook CR isn't updated and the annotation that was added by the notebook controller:
kubeflow-resource-stopped
stays in that CR and as such, the workbench instance isn't started back. This change deletes this annotation at the end of the culler test and the workbench is up and running and ready for the followup tests.Another issue there was that the update test updated the workbench to a non-existent image link, which resulted in a ImagePullBackOff Error in the end. This change updates this link to some existing image.
https://issues.redhat.com/browse/RHOAIENG-15141
How Has This Been Tested?
You can try to run locally e.g. by this:
Merge criteria: