Initialize Internal APIs for components #1304

VaishnaviHire · 2024-10-14T15:08:08Z

Description

This PR includes following changes. Some of the items are yet to be completed:

Define Internal API structs for components
Implement one component controller (Dashboard)
Move away from ComponentInterface
Update and add Tests
Update an add statuses
Update the DataScienceCluster controller

How Has This Been Tested?

CatalogSource: quay.io/vhire/opendatahub-operator-catalog:v2.19.4

Install operator by deploying above catalogsource
Create DSCI
Create DSC (only enable Dashboard)
Verify Dashboard CR is created
Verify Dashboard resources are deployed

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

controllers/components/dashboard_controller.go

zdtsw · 2024-10-14T16:50:45Z

controllers/components/dashboard_controller.go

+	DeleteFunc: func(e event.DeleteEvent) bool {
+		labelList := e.Object.GetLabels()
+		if value, exist := labelList[labels.ODH.Component(ComponentNameUpstream)]; exist && value == "true" {
+			return true
+		}
+		return false
+	},


do we want apply label on Dashboard CR as well just like all the other resources we are doing now?

controllers/components/base_reconciler_type.go

controllers/components/dashboard_controller.go

PROJECT

apis/components/component.go

(cherry picked from commit 086fe25)

Co-authored-by: Wen Zhou <wenzhou@redhat.com>

codecov · 2024-10-17T22:28:40Z

Codecov Report

Attention: Patch coverage is 51.20482% with 81 lines in your changes missing coverage. Please review.

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

Files with missing lines	Patch %	Lines
...atasciencecluster/datasciencecluster_controller.go	0.00%	50 Missing ⚠️
pkg/controller/actions/action_update_status.go	78.46%	11 Missing and 3 partials ⚠️
pkg/controller/actions/action_delete_resources.go	75.55%	9 Missing and 2 partials ⚠️
pkg/plugins/addLabelsplugin.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##             feature-operator-refactor    #1304   +/-   ##
============================================================
  Coverage                             ?   19.98%           
============================================================
  Files                                ?       32           
  Lines                                ?     3438           
  Branches                             ?        0           
============================================================
  Hits                                 ?      687           
  Misses                               ?     2684           
  Partials                             ?       67

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

lburgazzoli · 2024-10-18T05:38:23Z

LGTM

ykaliuta · 2024-10-18T07:53:33Z

/hold

The option is deprecated by opendatahub-io#1304 Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

ykaliuta · 2024-10-18T10:51:57Z

controllers/components/dashboard/dashboard_controller.go

+		}
+	}
+
+	if rr.DSCI.Spec.DevFlags != nil {


Remove that, deprecated by coming #1307

ykaliuta · 2024-10-18T11:23:09Z

components/dashboard/dashboard.go

-	components.Component `json:""`
-}
-
-func (d *Dashboard) Init(ctx context.Context, platform cluster.Platform) error {


This is should not be reverted, it is sort of regression.

ykaliuta · 2024-10-18T11:33:30Z

controllers/components/dashboard/dashboard_controller.go

+	return nil
+}
+
+func CreateDashboardInstance(dsc *dscv1.DataScienceCluster) *componentsv1.Dashboard {


It just allocates a structure with some predefined fields. Can it be named less dramatic :) ? NewDashboard() or something?

…ogging setup (cherry picked from commit 3a8f265)

grdryn

I haven't gotten all of the way through this yet, but leaving some minor comments from this pass.

Again, I'm very rusty when it comes to Go and operators currently, so please excuse any comments that don't make sense! 🙂

Makefile

grdryn · 2024-10-18T16:11:04Z

PROJECT

+  controller: true
+  domain: opendatahub.io
+  group: components
+  kind: Kserve


Suggested change

kind: Kserve

kind: KServe

I guess this might be slightly preferred? I don't know what the implications of one over the other are though.

I am not sure either. I defined the crd Kserve to be consistent with current component spec https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/apis/datasciencecluster/v1/datasciencecluster_types.go#L68

i dont see it is a big problem, we are using Kserve in most of the places from Operator, even the current DSC.components.

pkg/upgrade/upgrade.go

zdtsw

some small bits as comments

apis/components/v1/dashboard_types.go

apis/components/component.go

components/dashboard/dashboard.go

config/crd/kustomization.yaml

controllers/components/dashboard/dashboard_controller.go

zdtsw · 2024-10-21T10:01:30Z

controllers/components/dashboard/dashboard_controller.go

+		// Include status changes as we need to determine the component
+		// readiness by observing the status of the deployments
+		Watches(&appsv1.Deployment{}, componentEventHandler, builder.WithPredicates(componentLabelPredicate)).
+		// Ignore status changes


not sure i understand this comment on Route?

The route status can change, but that should not trigger a reconcile loop

controllers/components/dashboard/dashboard_controller.go

controllers/datasciencecluster/kubebuilder_rbac.go

pkg/metadata/annotations/annotations.go

feat(dashboard): configure watched resources event filter (cherry picked from commit c9ef475) Update predicate for create event Fix: Update manifests map in initialization Update controllers/components/dashboard/dashboard_controller.go Co-authored-by: Wen Zhou <wenzhou@redhat.com> Update controllers/components/dashboard/dashboard_controller.go Co-authored-by: Wen Zhou <wenzhou@redhat.com> Update pkg/metadata/annotations/annotations.go Co-authored-by: Wen Zhou <wenzhou@redhat.com> Update controllers/datasciencecluster/kubebuilder_rbac.go Co-authored-by: Wen Zhou <wenzhou@redhat.com> Update apis/components/component.go Co-authored-by: Wen Zhou <wenzhou@redhat.com> Update controllers/components/dashboard/dashboard_controller.go Co-authored-by: Wen Zhou <wenzhou@redhat.com> Update bundle

ykaliuta · 2024-10-21T19:58:13Z

/unhold

Co-authored-by: Wen Zhou <wenzhou@redhat.com>

zdtsw · 2024-10-22T06:18:39Z

lgtm

openshift-ci · 2024-10-22T12:16:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lburgazzoli

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:

~~OWNERS~~ [lburgazzoli]

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

VaishnaviHire added 3 commits October 14, 2024 05:33

Initialize Component APIs

a717823

Add spec for Dashboard API

157e0f3

Add Dashboard Controller

590777d

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

VaishnaviHire requested a review from zdtsw October 14, 2024 15:08

openshift-ci bot requested review from lburgazzoli and MarianMacik October 14, 2024 15:08

VaishnaviHire removed the request for review from MarianMacik October 14, 2024 15:08

zdtsw reviewed Oct 14, 2024

View reviewed changes

Add status sub resource to component's APIs

c88a428

VaishnaviHire requested review from ykaliuta, csams and lphiri October 15, 2024 04:07

grdryn reviewed Oct 16, 2024

View reviewed changes

apis/components/component.go Show resolved Hide resolved

VaishnaviHire and others added 16 commits October 17, 2024 17:15

Add component CRDs as internal objects

cfb2297

shared reconciler

487dbe8

(cherry picked from commit 086fe25)

Update DataScienceCLuster controller to only deploy Dashboard

a812a26

Update implementation of base reconciler

b438f3a

feat: create odh client

a54e58b

feat: rename BaseReconciler to ComponentReconciler

dc67181

feat: add common status update action

ba56369

feat: improve common conditions

aa0ea69

feat: add basic test for delete resource action

a364358

feat: add basic status management

0f2ad33

feat: add basic test for update status action

645ccf6

feat: move common code to pkg/controller to avoid circular dependecies

102881e

feat: copy DSCDashboard from DataScienceCluster

23da2f9

feat: add components.opendatahub.io/name label

0eedf2c

Add TODO and refactor component reconciler

2c5901c

Co-authored-by: Wen Zhou <wenzhou@redhat.com>

Fix linters

c00dcce

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

openshift-ci bot added the do-not-merge/hold label Oct 18, 2024

ykaliuta mentioned this pull request Oct 18, 2024

RFC: centralize manifest overriding #1227

Closed

5 tasks

ykaliuta added a commit to ykaliuta/opendatahub-operator that referenced this pull request Oct 18, 2024

dashboard: remove own handling of DSCI.Spec.DevFlags

73e5638

The option is deprecated by opendatahub-io#1304 Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

ykaliuta reviewed Oct 18, 2024

View reviewed changes

VaishnaviHire and others added 3 commits October 18, 2024 11:25

Delete component CRs on Removed

04e1ccd

Add managementState annotation

8baa3e5

component(dashboard): initialize images at startup, remove devflags l…

5016936

…ogging setup (cherry picked from commit 3a8f265)

VaishnaviHire force-pushed the initialize_api branch from 4b40c43 to af2c1b3 Compare October 18, 2024 17:51

grdryn reviewed Oct 18, 2024

View reviewed changes

zdtsw reviewed Oct 21, 2024

View reviewed changes

VaishnaviHire force-pushed the initialize_api branch from 3e2e580 to 72d34b2 Compare October 21, 2024 17:17

openshift-ci bot removed the do-not-merge/hold label Oct 21, 2024

VaishnaviHire and others added 3 commits October 21, 2024 16:09

Move rbac to dashboard_controller

a3a1273

fix linters

d81e733

Update apis/components/v1/dashboard_types.go

c005b6e

Co-authored-by: Wen Zhou <wenzhou@redhat.com>

lburgazzoli approved these changes Oct 22, 2024

View reviewed changes

openshift-ci bot assigned lburgazzoli Oct 22, 2024

openshift-ci bot added the lgtm label Oct 22, 2024

openshift-ci bot added the approved label Oct 22, 2024

openshift-merge-bot bot merged commit 6db3f70 into opendatahub-io:feature-operator-refactor Oct 22, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize Internal APIs for components #1304

Initialize Internal APIs for components #1304

VaishnaviHire commented Oct 14, 2024 •

edited

Loading

zdtsw Oct 14, 2024

codecov bot commented Oct 17, 2024 •

edited

Loading

lburgazzoli commented Oct 18, 2024

ykaliuta commented Oct 18, 2024

ykaliuta Oct 18, 2024

ykaliuta Oct 18, 2024

VaishnaviHire Oct 21, 2024

ykaliuta Oct 18, 2024

grdryn left a comment

grdryn Oct 18, 2024

VaishnaviHire Oct 21, 2024

zdtsw Oct 21, 2024

zdtsw left a comment

zdtsw Oct 21, 2024

lburgazzoli Oct 21, 2024

ykaliuta commented Oct 21, 2024

zdtsw commented Oct 22, 2024

openshift-ci bot commented Oct 22, 2024

Initialize Internal APIs for components #1304

Initialize Internal APIs for components #1304

Conversation

VaishnaviHire commented Oct 14, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

lburgazzoli commented Oct 18, 2024

ykaliuta commented Oct 18, 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

grdryn left a 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

zdtsw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykaliuta commented Oct 21, 2024

zdtsw commented Oct 22, 2024

openshift-ci bot commented Oct 22, 2024

VaishnaviHire commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading