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.

Sign up for GitHub

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

Jump to bottom

Refactor Custom Serving Runtime enablement #1407

Merged
merged 9 commits into from
Jun 29, 2023
Merged

Refactor Custom Serving Runtime enablement #1407

merged 9 commits into from
Jun 29, 2023

Conversation

lucferbux
Copy link
Contributor

@lucferbux lucferbux commented Jun 16, 2023 •
edited

Description

Closes: #1205

How Has This Been Tested?

Custom Serving Runtime Panel Admin

  1. Create several Custom Serving Runtimes
  2. Start to enable/disable
  3. You should see the serving runtime names added into the OdhDashboardConfig attribute templateDisablement
Screenshot 2023-06-16 at 18 18 14

Custom Serving Runtimes available in DS Projects

  1. Enable/Disable Custom Serving Runtimes
  2. Go to DS Projects
  3. Check that users only list the enables Serving Runtimes
  4. Go back to Custom Serving Runtime
  5. Check that the state of enablement is correct for every Serving Runtime
Screenshot 2023-06-16 at 18 18 26

Migration with no OdhDashboardConfig

  1. Add the annotation opendatahub.io/template-enabled: 'false' to existing templates
  2. Delete the existing OdhDashboardConfig object
  3. Restart the backend/restart the pods
  4. A new OdhDashboardConfig object should be created
  5. This object should contain a list in templateDisablement with the templates with the opendatahub.io/template-enabled: 'false' annotation
  6. This object should have all the attributes
  7. Inspecting api/config should contain status.dependencyOperators field

Migration with OdhDashboardConfig

  1. Add the annotation opendatahub.io/template-enabled: 'false' to existing templates
  2. Ensure OdhDashboardConfig does not contain templateDisablement
  3. Restart the backend/restart the pods
  4. OdhDashboardConfig should now contain a list in templateDisablement with the templates with the opendatahub.io/template-enabled: 'false' annotation
  5. Inspecting api/config should contain status.dependencyOperators field if disablePipelines is set to false

Migration with OdhDashboardConfig and templateDisablement

  1. Add the annotation opendatahub.io/template-enabled: 'false' to existing templates
  2. Ensure OdhDashboardConfig has the attribute `templateDisablement``
  3. Restart the backend/restart the pods
  4. OdhDashboardConfig should maintain the same templateDisablement list
  5. Inspecting api/config should contain status.dependencyOperators field if disablePipelines is set to false

Test Impact

I'm going to add this testsuite alongside the testing efforts for #1198

Request review criteria:

  • The commits have meaningful messages (squashes happen on merge by the bot).
  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.
  • 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
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

@lucferbux lucferbux mentioned this pull request Jun 21, 2023
Merged
6 tasks
@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 22, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 22, 2023
@lucferbux
Copy link
Contributor Author

@DaoDaoNoCode @alexcreasy I've just updated the testing efforts in this PR, since it's refactoring a part of the resource utils

@lucferbux
Copy link
Contributor Author

It should be now ready to go.

DaoDaoNoCode
DaoDaoNoCode approved these changes Jun 23, 2023
View reviewed changes
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode left a comment

Choose a reason for hiding this comment

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

Tested, the migration and the following disablement feature work as expected, lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 23, 2023
@lucferbux
Copy link
Contributor Author

@alexcreasy @Gkrumbach07 can you guys take a look at this?

Gkrumbach07
Gkrumbach07 reviewed Jun 26, 2023
View reviewed changes
Copy link
Member

@Gkrumbach07 Gkrumbach07 left a comment

Choose a reason for hiding this comment

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

I dont see any issues with the code itself. changes in resourceUtils look good. I dont have a cluster up so i have not been able to check it live yet

@lucferbux
Copy link
Contributor Author

I dont see any issues with the code itself. changes in resourceUtils look good. I dont have a cluster up so i have not been able to check it live yet

@Gkrumbach07 if you need a cluster let me know, I can share you mine!

@Gkrumbach07
Copy link
Member

  • When adding a new custom serving runtime and setting it to disabled, it is still visible in the to be selected in ds-projects after refresh too. it shows disabled in the config.
  • deleting a custom serving runtime that is disabled, does not remove it from the config

@lucferbux
Copy link
Contributor Author

  • When adding a new custom serving runtime and setting it to disabled, it is still visible in the to be selected in ds-projects after refresh too. it shows disabled in the config.
  • deleting a custom serving runtime that is disabled, does not remove it from the config

@Gkrumbach07 that's a great catch, I've already updated it to remove lingering templates that might not be present.

btw, the disabled runtimes should not appear, I've tested it again and it's working fine for me, can we take a look at it later?

@openshift-ci openshift-ci bot removed the lgtm label Jun 29, 2023
@Gkrumbach07
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 29, 2023
@Gkrumbach07
Copy link
Member

@lucferbux can this be merged now or do we need another review

@lucferbux
Copy link
Contributor Author

@Gkrumbach07 yeah, i did some changes it needs to be re-reviewed.

@DaoDaoNoCode can you take a look please?

DaoDaoNoCode
DaoDaoNoCode approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode left a comment

Choose a reason for hiding this comment

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

lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: DaoDaoNoCode

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 9da8645 into opendatahub-io:main Jun 29, 2023
7 checks passed
Gkrumbach07 pushed a commit to Gkrumbach07/odh-dashboard that referenced this pull request Jun 30, 2023
@lucferbux @Gkrumbach07
Refactor Custom Serving Runtime enablement (opendatahub-io#1407)
bdc6d9d 
* Test serving runtime

* Refactor custom serving runtimes enablement

* Remove tensorflow from ootb

* Add startup function to migrate disablement status for Custom Serving Runtime

* Fix feedback

* Delete unused annotations

* Fix feedback

* Remove templates from disablement list when deleted

* Fix issue filtering serving runtimes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gkrumbach07 Gkrumbach07 Gkrumbach07 left review comments

@DaoDaoNoCode DaoDaoNoCode DaoDaoNoCode approved these changes

@alexcreasy alexcreasy Awaiting requested review from alexcreasy

Assignees

@Gkrumbach07 Gkrumbach07

@DaoDaoNoCode DaoDaoNoCode

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Change the way we store Enabled/Disabled State of Custom Runtimes
4 participants
@lucferbux @Gkrumbach07 @DaoDaoNoCode @openshift-merge-robot