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

feat: add support for Ray #1315

Merged
merged 2 commits into from
Oct 31, 2024
Merged

feat: add support for Ray #1315

merged 2 commits into from
Oct 31, 2024

Conversation

zdtsw
Copy link
Member

@zdtsw zdtsw commented Oct 24, 2024 •
edited
Loading

Description

  • move component name to api as const
  • move create+reconcile compoent CR into func
  • cleanup old component files: ray and dashboard
  • rename function to be more generic
  • e2e test for ray: similar to dashboard
  • update golangci for ireturn (if set inline get removed back and forth)

https://issues.redhat.com/browse/RHOAIENG-13182

How Has This Been Tested?

local build: quay.io/wenzhou/opendatahub-operator-catalog:v2.16.13182-3

  • install operator
  • enable dashbaord ray and kueue
   components:
    codeflare: {}
    kserve:
      serving:
        ingressGateway:
          certificate:
            type: OpenshiftDefaultIngress
        managementState: Managed
        name: knative-serving
    modelregistry:
      registriesNamespace: odh-model-registries
    trustyai: {}
    ray:
      managementState: Managed
    kueue:
      managementState: Managed
    workbenches: {}
    dashboard:
      managementState: Managed
    modelmeshserving: {}
    datasciencepipelines: {}
    trainingoperator: {}
  • DSC status:
  installedComponents:
    dashboard: true
    ray: true
  phase: Ready
  • 2 dashboard pods and 1 ray pod running in application namespace, no kueue pod
    update ray CR set to
spec: {}
status:
  conditions:
    - lastTransitionTime: '2024-10-30T14:31:17Z'
      message: 1/1 deployments ready
      observedGeneration: 1
      reason: Ready
      status: 'True'
      type: Ready
  observedGeneration: 1
  phase: Ready
  • update DSC to set Ray to Removed
  • check ray CR again, it is gone
  • check ray pod again, it is gone too
  • check DSC
 installedComponents:
    dashboard: true
    ray: false
  phase: Ready

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

@zdtsw zdtsw requested a review from VaishnaviHire October 24, 2024 10:19
@openshift-ci openshift-ci bot requested review from grdryn and MarianMacik October 24, 2024 10:19
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Oct 24, 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 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:

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

@zdtsw zdtsw requested review from lburgazzoli and removed request for grdryn and MarianMacik October 24, 2024 10:19
@codecov Codecov
Copy link

codecov bot commented Oct 24, 2024 •
edited
Loading

Codecov Report

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

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

Files with missing lines Patch % Lines
...atasciencecluster/datasciencecluster_controller.go 0.00% 22 Missing ⚠️
Additional details and impacted files 
@@                     Coverage Diff                      @@
##             feature-operator-refactor    #1315   +/-   ##
============================================================
  Coverage                             ?   24.54%           
============================================================
  Files                                ?       52           
  Lines                                ?     4273           
  Branches                             ?        0           
============================================================
  Hits                                 ?     1049           
  Misses                               ?     3095           
  Partials                             ?      129

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

@zdtsw zdtsw marked this pull request as draft October 24, 2024 14:17
@zdtsw zdtsw marked this pull request as ready for review October 30, 2024 14:33
@zdtsw
feat: add support for Ray
bc766ab 
- cleanup old component files: ray and dashboard
- rename function to be more generic
- status update on component for installedcomponent

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw
test: add e2e test for ray
b93d4c5 
- move ireturn into golangci config

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
lburgazzoli
lburgazzoli reviewed Oct 31, 2024
View reviewed changes
.golangci.yml
@@ -58,6 +58,7 @@ linters-settings:
- stdlib
# also allow generics
- generic
- EventHandler # for ToOwner
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

lburgazzoli
lburgazzoli reviewed Oct 31, 2024
View reviewed changes
controllers/datasciencecluster/datasciencecluster_controller.go
instance = r.reportError(err, instance, fmt.Sprintf("failed to reconcile %s on DataScienceCluster", componentName))
instance, _ = status.UpdateWithRetry(ctx, r.Client, instance, func(saved *dscv1.DataScienceCluster) {
status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileFailed, fmt.Sprintf("Component reconciliation failed: %v", err), corev1.ConditionFalse)
})
return instance, err
}

r.Log.Info("Dashboard component reconciled successfully")
r.Log.Info("component reconciled successfully: " + componentName)
Copy link
Contributor

Choose a reason for hiding this comment

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

not for this PR, but we can probably squeeze the 3 UpdateWithRetry calls in a single one

@lburgazzoli
Copy link
Contributor

LGTM

@zdtsw zdtsw merged commit a1f0e62 into opendatahub-io:feature-operator-refactor Oct 31, 2024
9 of 10 checks passed
This was referenced Dec 8, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lburgazzoli lburgazzoli lburgazzoli left review comments

@VaishnaviHire VaishnaviHire Awaiting requested review from VaishnaviHire

Assignees
No one assigned
Labels
feature-operator-refactor
Projects
ODH Platform Planning
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zdtsw @lburgazzoli @VaishnaviHire @openshift-merge-robot