feat: add support for modelmeshserving as component #1338

zdtsw · 2024-11-04T10:42:37Z

Description

add modelmeshserving into component
introduce a non-DSC spec visible modelcontroller component.

How Has This Been Tested?

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
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

openshift-ci · 2024-11-04T10:42:48Z

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

OWNERS

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

zdtsw · 2024-11-04T12:53:18Z

/test opendatahub-operator-e2e

grdryn · 2024-11-12T00:14:03Z

apis/components/v1/kserve_types.go

@@ -21,6 +21,10 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

+const (
+	ModelMeshCtrlerComponentName = "odh-model-controller" // shared by kserve and mm


Suggested change

ModelMeshCtrlerComponentName = "odh-model-controller" // shared by kserve and mm

ModelControllerComponentName = "odh-model-controller" // shared by kserve and mm

I assume that it shouldn't have ModelMesh in the name if it's not modelmesh specific?

controllers/components/modelmeshserving/modelmesh_support.go

lburgazzoli · 2024-11-20T10:38:09Z

controllers/components/modelcontroller/modelcontroller_actions.go

+	}
+	// Implement devflags support logic
+	// If dev flags are set, update default manifests path
+	if len(mc.Spec.DevFlags.Manifests) != 0 {


A common pattern is to try to align the happy path on the left, so I would do something like:

if len(mc.Spec.DevFlags.Manifests) == 0 { return nil }

lburgazzoli · 2024-11-20T10:38:48Z

controllers/components/modelmeshserving/modelmeshserving_actions.go

+	}
+	// Implement devflags support logic
+	// If dev flags are set, update default manifests path
+	if len(mm.Spec.DevFlags.Manifests) != 0 {


A common pattern is to try to align the happy path on the left, so I would do something like:

if len(mc.Spec.DevFlags.Manifests) == 0 { return nil }

lburgazzoli · 2024-11-20T10:39:37Z

controllers/components/modelmeshserving/modelmeshserving_actions.go

+	return nil
+}
+
+func updatePodSecurityRolebinding(ctx context.Context, rr *odhtypes.ReconciliationRequest) error {


we have a shared action for this

lburgazzoli · 2024-11-20T10:39:46Z

controllers/components/modelcontroller/modelcontroller_actions.go

+	return nil
+}
+
+func updatePodSecurityRolebinding(ctx context.Context, rr *odhtypes.ReconciliationRequest) error {


we have a shared action for this

yep this is not updated since dashbaord's change, let me remove it

lburgazzoli · 2024-11-20T10:43:37Z

controllers/datasciencecluster/datasciencecluster_controller.go

+	}
+
+	// Deploy ModelController
+	if instance, err = r.ReconcileComponent(ctx, instance, componentsv1.ModelControllerComponentName, func() (error, bool) {


Guess this should be synced with #1347

correct. i can comment out the placeholder for kserve first

lburgazzoli · 2024-11-20T10:46:09Z

controllers/components/modelcontroller/modelcontroller_controller.go

+		Owns(&templatev1.Template{}).
+		Owns(&appsv1.Deployment{}, builder.WithPredicates(resources.NewDeploymentPredicate())).
+		Owns(&componentsv1.ModelMeshServing{}).     // watch ModelMeshServing
+		Owns(&componentsv1.Kserve{}).               // watch Kserve


is Own correct here ? I think the ModelMeshServing and Kserve are owned by the DSC

you are right, should be watch...... my code not match my comments

the watch should also probably have some custom handler since the default one uses the part-of label to find out the parent, like a resources.ToNamed(componentsv1.ModelControllerInstanceName)

lburgazzoli · 2024-11-20T11:05:54Z

controllers/components/modelmeshserving/modelmeshserving_actions.go

+	}
+	rr.Manifests = []odhtypes.ManifestInfo{}
+	// setup Manifets[0] for modelmeshserving
+	rr.Manifests = append(rr.Manifests, odhtypes.ManifestInfo{


you can probably combine line 36 & 38

lburgazzoli · 2024-11-20T11:07:09Z

main.go

+	if err := mmctrl.Init(p); err != nil {
+		return err
+	}
+	if err := mmctrl.Init(p); err != nil {


seems a duplication, probably you meant mcctrl

yes, came after rebase , fixed now

codecov · 2024-11-22T11:47:48Z

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature-operator-refactor@d748e02). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...atasciencecluster/datasciencecluster_controller.go	0.00%	7 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##             feature-operator-refactor    #1338   +/-   ##
============================================================
  Coverage                             ?   27.19%           
============================================================
  Files                                ?       60           
  Lines                                ?     4722           
  Branches                             ?        0           
============================================================
  Hits                                 ?     1284           
  Misses                               ?     3289           
  Partials                             ?      149

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

- add new modelcontroller component: do not show in DSC.components - e2e on modelcontroller wont run in parallel but after others are done - if modelmesh is enabled, or kserve (later) is enabled, modelcontroller should be managed - update: devFlag for modelcontroller + rolebinding function + watches Signed-off-by: Wen Zhou <wenzhou@redhat.com>

… not set modelcontroller in installatedcomponents - do not leave component which is in Unknonw in the cluster, should be treated as Removed - remove old "init" status updates for "enabled/disable" message - modelcontroller is hidden component, no need to show in installedcomponents list - steps: 1. DSC create components CR 2. DSc update status if failed to create any non-modelcontroller CR 3.1 if component is enabled and component CR is created, update status to successful 3.2 if component is disabled, remove from status if previous exist - placeholder to update status if component CR status NotReady Signed-off-by: Wen Zhou <wenzhou@redhat.com>

…oncile - change in DSC to enabled/disable kserve and modelmesh is enough to trigger reconcile on modelcontroller by DSC CR - it directly reflect into modelcontroller .spec.kserve / .spec.modelmeshserving fileds - keep DSC CR ad the controller for modelcontroller CR, but add modelmesh and kserve as ownerreference for now - no need extra watches from ModelController directly to Kserve or ModelMeshServing Signed-off-by: Wen Zhou <wenzhou@redhat.com>

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

openshift-ci · 2024-11-26T11:45:34Z

@zdtsw: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/opendatahub-operator-e2e	`1cbbe66`	link	true	`/test opendatahub-operator-e2e`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress label Nov 4, 2024

openshift-ci bot requested review from grdryn and mattmahoneyrh November 4, 2024 10:42

zdtsw removed request for grdryn and mattmahoneyrh November 4, 2024 10:48

zdtsw force-pushed the jira/13177 branch 2 times, most recently from 6694329 to d862434 Compare November 4, 2024 10:56

zdtsw force-pushed the jira/13177 branch 6 times, most recently from cb90c62 to a833dd5 Compare November 8, 2024 11:34

lburgazzoli added the feature-operator-refactor label Nov 11, 2024

zdtsw force-pushed the jira/13177 branch from a833dd5 to e4d6b90 Compare November 11, 2024 17:12

grdryn reviewed Nov 12, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Nov 18, 2024

zdtsw force-pushed the jira/13177 branch from e4d6b90 to f1e161a Compare November 18, 2024 17:05

openshift-merge-robot removed the needs-rebase label Nov 18, 2024

zdtsw force-pushed the jira/13177 branch 2 times, most recently from f2ae263 to 206adab Compare November 19, 2024 15:55

openshift-merge-robot added the needs-rebase label Nov 19, 2024

zdtsw force-pushed the jira/13177 branch from 206adab to 6bd2161 Compare November 19, 2024 17:02

openshift-merge-robot removed the needs-rebase label Nov 19, 2024

zdtsw force-pushed the jira/13177 branch 2 times, most recently from 2fbf65d to f84aa91 Compare November 19, 2024 17:38

zdtsw changed the title ~~[WIP] feat: add support for modelmeshserving as component~~ feat: add support for modelmeshserving as component Nov 19, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 19, 2024

zdtsw force-pushed the jira/13177 branch 2 times, most recently from e363f25 to b828d2a Compare November 19, 2024 17:48

lburgazzoli reviewed Nov 20, 2024

View reviewed changes

zdtsw force-pushed the jira/13177 branch from b828d2a to ee8ac4e Compare November 20, 2024 10:47

lburgazzoli reviewed Nov 20, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Nov 20, 2024

zdtsw force-pushed the jira/13177 branch 2 times, most recently from 18b90a8 to 70e59a7 Compare November 20, 2024 14:11

openshift-merge-robot removed the needs-rebase label Nov 20, 2024

zdtsw force-pushed the jira/13177 branch from 70e59a7 to 8e4c953 Compare November 20, 2024 14:22

lburgazzoli mentioned this pull request Nov 20, 2024

feat: initial move of kserve to new structure #1347

Open

5 tasks

zdtsw force-pushed the jira/13177 branch 3 times, most recently from a5bc347 to d675123 Compare November 22, 2024 10:26

zdtsw force-pushed the jira/13177 branch from d675123 to 100084c Compare November 22, 2024 12:14

zdtsw added 3 commits November 23, 2024 12:53

zdtsw force-pushed the jira/13177 branch from 100084c to 9e72341 Compare November 25, 2024 17:28

zdtsw mentioned this pull request Nov 26, 2024

update: DSC status update and remove component CR's case #1390

Open

5 tasks

fix: wrong dups which cause predicate not work

1cbbe66

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for modelmeshserving as component #1338

feat: add support for modelmeshserving as component #1338

zdtsw commented Nov 4, 2024 •

edited

Loading

openshift-ci bot commented Nov 4, 2024

zdtsw commented Nov 4, 2024

grdryn Nov 12, 2024

lburgazzoli Nov 20, 2024

lburgazzoli Nov 20, 2024

lburgazzoli Nov 20, 2024

lburgazzoli Nov 20, 2024

zdtsw Nov 20, 2024

lburgazzoli Nov 20, 2024

zdtsw Nov 20, 2024

lburgazzoli Nov 20, 2024

zdtsw Nov 20, 2024

lburgazzoli Nov 20, 2024 •

edited

Loading

lburgazzoli Nov 20, 2024

lburgazzoli Nov 20, 2024

zdtsw Nov 20, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading

openshift-ci bot commented Nov 26, 2024

	ModelMeshCtrlerComponentName = "odh-model-controller" // shared by kserve and mm
	ModelControllerComponentName = "odh-model-controller" // shared by kserve and mm

feat: add support for modelmeshserving as component #1338

Are you sure you want to change the base?

feat: add support for modelmeshserving as component #1338

Conversation

zdtsw commented Nov 4, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

openshift-ci bot commented Nov 4, 2024

zdtsw commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lburgazzoli Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

openshift-ci bot commented Nov 26, 2024

zdtsw commented Nov 4, 2024 •

edited

Loading

lburgazzoli Nov 20, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading