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

RHOAIENG-14784: tests(odh-notebook-controller): add upgrade test that compares controller does not modify deployed pods during upgrade #430

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Oct 23, 2024

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

Description

It would be nice if we could test upgrade directly in the repository, without having to build container images, bundle, iib, and then deploying the entire rhoai.

Otherwise it would be hard to ensure e.g. no workbench pods restart on upgrade, which is (probably) a requirement we have.

Here's that test presented for your consideration.

The idea is to load YAMLs from an older version where notebook is deployed and controller has done its thing, and then start a new version of the controller over these yamls, and check what did and did not change in the CRs.

How Has This Been Tested?

  • I need a deps structure change to access the sibling notebook-controller from odh-notebook-controller's tests. This is essentially a subset of the RHOAI-xxx changes that @rkpattnaik780 (of fond memories) tried to make early this year and it failed CPaaS builds. I know how to check for it ahead of time, so if we decide to go with it, I can test CPaaS build pre-merge and I will figure out how to do the same for Konflux.

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

Copy link

openshift-ci bot commented Oct 23, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jiridanek. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@@ -78,3 +79,5 @@ require (
)

replace google.golang.org/grpc => google.golang.org/grpc v1.56.3

replace github.com/kubeflow/kubeflow/components/notebook-controller => ../notebook-controller
Copy link
Member Author

Choose a reason for hiding this comment

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

if we don't want to do this, I can put the test directly under components/ as a separate component; but I'd still like to do it this way, so if there's no opposition to the general direction, I'd like to go test it out in cpaas

@jiridanek
Copy link
Member Author

ready now to be looked over
/cc @harshad16

@openshift-ci openshift-ci bot requested a review from harshad16 October 24, 2024 07:48
Copy link
Member Author

@jiridanek jiridanek Oct 24, 2024

Choose a reason for hiding this comment

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

yeah, we probably want to filter the stored CRs, this csv is not used for anything, so it should be kept out of git

Comment on lines +84 to +87
//_ = Step("", func() string {
// return "baf"
//})

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
//_ = Step("", func() string {
// return "baf"
//})

Copy link
Member Author

Choose a reason for hiding this comment

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

/test odh-notebook-controller-e2e

@atheo89
Copy link
Member

atheo89 commented Oct 30, 2024

Wow this is huge PR, we need to set a meeting to review that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants