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

Initialize Internal APIs for components #1304

Open
wants to merge 35 commits into
base: feature-operator-refactor
Choose a base branch
from

Conversation

VaishnaviHire
Copy link
Member

@VaishnaviHire VaishnaviHire commented Oct 14, 2024

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

  1. Install operator by deploying above catalogsource
  2. Create DSCI
  3. Create DSC (only enable Dashboard)
  4. Verify Dashboard CR is created
  5. 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

Copy link

openshift-ci bot commented Oct 14, 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 vaishnavihire. 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

@VaishnaviHire VaishnaviHire removed the request for review from MarianMacik October 14, 2024 15:08
controllers/components/dashboard_controller.go Outdated Show resolved Hide resolved
controllers/components/dashboard_controller.go Outdated Show resolved Hide resolved
}), builder.WithPredicates(dashboardPredicates)).
Watches(&appsv1.ReplicaSet{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request {
return r.watchDashboardResources(ctx, a)
}), builder.WithPredicates(dashboardPredicates)).
Copy link
Member

Choose a reason for hiding this comment

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

this probably not needed

Comment on lines 216 to 217
Watches(&corev1.PersistentVolumeClaim{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request {
return r.watchDashboardResources(ctx, a)
Copy link
Member

Choose a reason for hiding this comment

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

this probabaly only need to be added in workbench SetupWithManager()

we can refine them one component by component

controllers/components/dashboard_controller.go Outdated Show resolved Hide resolved
Comment on lines 274 to 280
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
},
Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
controllers/components/dashboard_controller.go Outdated Show resolved Hide resolved
PROJECT Show resolved Hide resolved
VaishnaviHire and others added 8 commits October 17, 2024 17:15
Make it possible to compile operator without webhook enabled (with
-tags nowebhook). Create a stub webhook.Init() function for that.

Add run-nowebhook target to run webhook locally. It requires
`make install` to be executed on the cluster beforehand.

Since it repeats `make run`, move the command to a variable.
Also use variable RUN_ARGS for operator arguments. It makes it
possible to override them from the command line.

In VSCode it is possible to debug it with the following example
launch.json configuration:

```
        {
            "name": "Debug Operator No Webhook",
            "type": "go",
            "request": "launch",
            "mode": "debug",
            "program": "main.go",
            "buildFlags": [
                "-tags", "nowebhook"
            ],
            "env": {
                "OPERATOR_NAMESPACE": "opendatahub-operator-system",
                "DEFAULT_MANIFESTS_PATH": "./opt/manifests"
            },
            "args": [
                "--log-mode=devel"
            ],
            "cwd": "${workspaceFolder}",
        }
```

Signed-off-by: Yauheni Kaliuta <[email protected]>
(cherry picked from commit 4d5874d)
@VaishnaviHire VaishnaviHire changed the title [WIP] Initialize Internal APIs for components Initialize Internal APIs for components Oct 17, 2024
Copy link

codecov bot commented Oct 17, 2024

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
Copy link
Contributor

LGTM

@ykaliuta
Copy link
Contributor

/hold

ykaliuta added a commit to ykaliuta/opendatahub-operator that referenced this pull request Oct 18, 2024
The option is deprecated by
opendatahub-io#1304

Signed-off-by: Yauheni Kaliuta <[email protected]>
}
}

if rr.DSCI.Spec.DevFlags != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove that, deprecated by coming #1307

components.Component `json:""`
}

func (d *Dashboard) Init(ctx context.Context, platform cluster.Platform) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return nil
}

func CreateDashboardInstance(dsc *dscv1.DataScienceCluster) *componentsv1.Dashboard {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@grdryn grdryn left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
controller: true
domain: opendatahub.io
group: components
kind: Kserve
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -25,13 +25,14 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/opendatahub-io/opendatahub-operator/v2/apis/components"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is also imported on L29 as componentsold, are both intentional?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

5 participants