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

Create dashboard configuration to control kserve and modelmesh #1957

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

lucferbux
Copy link
Contributor

@lucferbux lucferbux commented Oct 11, 2023

Description

Closes #1939 #1975

Create dashboard configuration to control kserve and modelmesh enablement as model serving platforms

Work In Progress I need to figure out the upgrade path of existing clusters

How Has This Been Tested?

There are two main paths to test:

Upgrade path from an existing dashboardconfig

  1. Deploy and old dashboardConfig object which doesn't contain disableKServe nor disableModelMesh
  2. Start the dashboard
  3. Check that in the config response, the dashboard is returning the following: disableKServe: true and disableModelMesh: false

Upgrade in a fresh cluster

  1. Deploy the latest CRD
  2. Either delete the dashboardConfig CR object or just add whatever configuration you want with disableKServe and disableModelMesh
  3. Check that in the config response, the dashboard is returning the following disableKServe: false and disableModelMesh: true

Refresh an actual configuration

  1. Set up disableKserve: false and disableModelMesh: false
  2. Check that it's returning the correct value

Test Impact

We don't have anything in place for the backend, we might need something in the future to cover migrations and so

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

  • 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 (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@lucferbux lucferbux added the do-not-merge/work-in-progress This PR is in WIP state label Oct 11, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Oct 11, 2023
@lucferbux
Copy link
Contributor Author

I still need to figure out the proper way to the migration path from existing clusters, this covers the newly created clusters, might need to add some migration in the backend, we can do that as a follow up.

@andrewballantyne andrewballantyne changed the title Create dashboard configuration to control kserve and modelmesh [WIP] Create dashboard configuration to control kserve and modelmesh Oct 11, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Oct 11, 2023
@lucferbux lucferbux changed the title [WIP] Create dashboard configuration to control kserve and modelmesh Create dashboard configuration to control kserve and modelmesh Oct 17, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Oct 17, 2023
@lucferbux
Copy link
Contributor Author

I will continue the work and testuite for the migration here: #1975

@lucferbux
Copy link
Contributor Author

@uidoyen If you can take a look at this too it would be amazing, thanks in advance!

@lucferbux lucferbux linked an issue Oct 18, 2023 that may be closed by this pull request
@lucferbux
Copy link
Contributor Author

Ok, got the migration path, we need to have test coverage for that in the future, I'm gonna start a talk about that to see what the team thinks.

@andrewballantyne andrewballantyne added the pr/no-tests-allowed Added by an official approver - this PR is allowed no tests. Omitted, a test must accompany the PR label Oct 18, 2023
@andrewballantyne
Copy link
Member

Testing for this will be covered in other work. We need to verify testing infra is properly added to the backend... which may be in incubation at this point (I think it's part of the byon work if I recall correctly)

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

@lucferbux
Copy link
Contributor Author

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DaoDaoNoCode, lucferbux

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-ci openshift-ci bot merged commit 3eaf078 into opendatahub-io:f/model-serving Oct 18, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm pr/no-tests-allowed Added by an official approver - this PR is allowed no tests. Omitted, a test must accompany the PR
Projects
None yet
3 participants