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: initial move of kserve to new structure #1347

Merged
merged 3 commits into from
Nov 28, 2024
Merged

feat: initial move of kserve to new structure #1347

merged 3 commits into from
Nov 28, 2024

Conversation

grdryn
Copy link
Member

@grdryn grdryn commented Nov 6, 2024 •
edited
Loading

Description

This change moves responsibility of reconciling KServe to the kserve controller, which reacts to a Kserve CR.

JIRA: https://issues.redhat.com/browse/RHOAIENG-13179

How Has This Been Tested?

  • Tested locally in different configurations
  • e2e tests added (open to suggestions for additional tests on top of what's included here)

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 openshift-ci bot requested review from biswassri and zdtsw November 6, 2024 11:15
@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch from 2f4dd28 to 381f775 Compare November 6, 2024 11:26
jackdelahunt
jackdelahunt reviewed Nov 6, 2024
View reviewed changes
main.go Show resolved Hide resolved
@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch from 381f775 to bddf178 Compare November 6, 2024 13:25
@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch from bddf178 to d8bdb45 Compare November 9, 2024 01:08
grdryn
grdryn commented Nov 9, 2024
View reviewed changes
Copy link
Member Author

@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.

Self-review to highlight some new things, and questions

controllers/components/kserve/kserve_controller.go Outdated Show resolved Hide resolved
controllers/components/kserve/kserve_support.go Outdated Show resolved Hide resolved
controllers/components/kserve/kserve_controller.go Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Nov 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

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

Files with missing lines Patch % Lines
...atasciencecluster/datasciencecluster_controller.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@                     Coverage Diff                      @@
##             feature-operator-refactor    #1347   +/-   ##
============================================================
  Coverage                             ?   27.15%           
============================================================
  Files                                ?       60           
  Lines                                ?     4805           
  Branches                             ?        0           
============================================================
  Hits                                 ?     1305           
  Misses                               ?     3348           
  Partials                             ?      152

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

@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch from d8bdb45 to 516aca7 Compare November 11, 2024 23:50
@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch from 516aca7 to 217ca78 Compare November 12, 2024 00:19
@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch 2 times, most recently from 94f4211 to ac19c0e Compare November 12, 2024 22:31
@grdryn
Copy link
Member Author

grdryn commented Nov 13, 2024

/retest

@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch 6 times, most recently from df27878 to 270d2f0 Compare November 18, 2024 16:46
@grdryn grdryn changed the title WIP: initial move of kserve to new structure feat: initial move of kserve to new structure Nov 18, 2024
@grdryn grdryn requested review from zdtsw and lburgazzoli November 18, 2024 18:36
@grdryn
Copy link
Member Author

grdryn commented Nov 27, 2024

/test ci-index

2 similar comments
@grdryn
Copy link
Member Author

grdryn commented Nov 27, 2024

/test ci-index

@grdryn
Copy link
Member Author

grdryn commented Nov 27, 2024

/test ci-index

@grdryn
Copy link
Member Author

grdryn commented Nov 27, 2024

/test ci-index

@grdryn
Copy link
Member Author

grdryn commented Nov 27, 2024

/test opendatahub-operator-e2e

@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch from c01da29 to 5551df0 Compare November 27, 2024 18:04
@lburgazzoli
Copy link
Contributor

LGTM, @zdtsw ?

@lburgazzoli
Copy link
Contributor

@grdryn PR requires yet another rebase :)

zdtsw
zdtsw approved these changes Nov 28, 2024
View reviewed changes
controllers/status/status.go Show resolved Hide resolved
controllers/status/status.go Show resolved Hide resolved
controllers/components/kserve/kserve_support.go Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm label Nov 28, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zdtsw

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

@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch from 5551df0 to 460b4d3 Compare November 28, 2024 14:24
@openshift-ci openshift-ci bot removed the lgtm label Nov 28, 2024
@grdryn
feat: initial move of kserve to new structure
aed4893 
JIRA: https://issues.redhat.com/browse/RHOAIENG-13179
@grdryn
chore: templatize provider in AuthorizationPolicy
c055814 
With the old value, opendatahub-odh-auth-provider, the value would be
set, then almost immediately be reset to the value of
{{.AuthExtensionName}} (which appears to be equivalent to
<dsci.ApplicationNamespace>-auth-provider), via the separate manifest
specified in the following file, which gets applied by the same list
of resources in the feature:

z-migrations/kserve-predictor-authorizationpolicy.patch.tmpl.yaml

This causes a complication when watching the AuthorizationPolicy,
because it causes it to continuously update, flip-flopping between the
two values.

It seems to make more sense to just set the value to the expected
value from the outset, so that's what this change attempts to do.
@grdryn
kserve: add custom FeatureTracker eventHandler
6563397
@grdryn grdryn force-pushed the RHOAIENG-13179-kserve branch from 460b4d3 to 6563397 Compare November 28, 2024 15:28
@lburgazzoli
Copy link
Contributor

LGTM, @zdtsw ?

@zdtsw
Copy link
Member

zdtsw commented Nov 28, 2024

LGTM, @zdtsw ?

yep, lgtm

@lburgazzoli
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 28, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 6281fdd into opendatahub-io:feature-operator-refactor Nov 28, 2024
8 checks passed
@grdryn grdryn deleted the RHOAIENG-13179-kserve branch November 28, 2024 22:12
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

@biswassri biswassri biswassri left review comments

@jackdelahunt jackdelahunt jackdelahunt left review comments

@lburgazzoli lburgazzoli lburgazzoli left review comments

@zdtsw zdtsw zdtsw approved these changes

Assignees

@zdtsw zdtsw

@lburgazzoli lburgazzoli

Labels
approved feature-operator-refactor lgtm odh-2.22
Projects
ODH Platform Planning
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@grdryn @lburgazzoli @zdtsw @jackdelahunt @biswassri @VaishnaviHire @openshift-merge-robot