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

Automatically inject expected ODH annotations to InferenceGraph and InferenceServices #339

Conversation

israel-hdez
Copy link
Contributor

@israel-hdez israel-hdez commented Dec 16, 2024

NOTE: Requires PR #324 before merging (fulfilled)

Description

On creation of InferenceGraph or InferenceService resources, the following default annotations will be added:

  • serving.knative.openshift.io/enablePassthrough: true
  • sidecar.istio.io/inject: true
  • sidecar.istio.io/rewriteAppHTTPProbers: true

The annotations are added only for Serverless mode, and only if they are missing.

Fixes https://issues.redhat.com/browse/RHOAIENG-15337

How Has This Been Tested?

  • Deploy odh-model-controller from this PR, either by creating a custom build of the operator, or by using dev-flags
  • Follow the InferenceGraph example at https://kserve.github.io/website/latest/modelserving/inference_graph/image_pipeline/#deploy-the-inferenceservices (you will need to configure pytorch runtime)
  • When you create the InferenceGraph resource, confirm that the three default annotations (mentioned up) are automatically injected to the created InferenceGraph resource.
  • Delete the InferenceGraph resource, and re-create it in Raw mode. Confirm that the default annotations are not injected.
  • Delete the InferenceGraph resource, and re-create it in Serverless mode, while manually specifying one (or some) of the annotations with a non-default value (e.g. serving.knative.openshift.io/enablePassthrough: false). Confirm that the manually specified annotations are honored, while the others are injected to the default values.

Similar tests can be performed on InferenceService resources.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • 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
Contributor

openshift-ci bot commented Dec 16, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@israel-hdez israel-hdez force-pushed the j15337-ig-inject-annotations branch from 2786c97 to 2412463 Compare December 16, 2024 18:56
@israel-hdez israel-hdez changed the title J15337 ig inject annotations Automatically inject expected ODH annotations to InferenceGraph and InferenceServices Dec 16, 2024
@israel-hdez israel-hdez marked this pull request as ready for review December 16, 2024 22:31
@openshift-ci openshift-ci bot requested review from mwaykole and spolti December 16, 2024 22:31
@israel-hdez israel-hdez force-pushed the j15337-ig-inject-annotations branch from 2412463 to 35a5bea Compare December 18, 2024 23:28
@israel-hdez
Copy link
Contributor Author

@spolti This PR is now fully ready, although we need first to merge the migration #324

On creation of InferenceGraph or InferenceService resources, the following default annotations will be added:
* `serving.knative.openshift.io/enablePassthrough: true`
* `sidecar.istio.io/inject: true`
* `sidecar.istio.io/rewriteAppHTTPProbers: true`

The annotations are added only for Serverless mode, and only if they are missing.

Signed-off-by: Edgar Hernández <[email protected]>
Dockerfile Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
Extract "ENABLE_WEBHOOKS" string to constant

Signed-off-by: Edgar Hernández <[email protected]>
Copy link
Contributor

openshift-ci bot commented Jan 10, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: israel-hdez, spolti

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

@spolti
Copy link
Member

spolti commented Jan 10, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 10, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 2a69143 into opendatahub-io:incubating Jan 10, 2025
7 checks passed
@israel-hdez israel-hdez deleted the j15337-ig-inject-annotations branch January 10, 2025 18:08
openshift-merge-bot bot pushed a commit that referenced this pull request Jan 16, 2025
* update global ca bundle logic and storage-config logic to follow up odh operator pr(1339) (#308)

Signed-off-by: jooho lee <[email protected]>

* disable dashboard and fix servingruntime display name

Signed-off-by: jooho lee <[email protected]>

* Use the main branch to build stable image tags, incubating for latest image tags (#316)

Signed-off-by: Hannah DeFazio <[email protected]>

* [RHOAIENG-13638] - Do not allow isvc creation in protected isvc (#311)

* [RHOAIENG-13638] - Do not allow isvc creation in protected namespace

chore: Fixes [RHOAIENG-13638] - Kserve model is not Ready after a kserve model is created and deleted from istio-system namespace

Signed-off-by: Spolti <[email protected]>

* review suggestions

Signed-off-by: Spolti <[email protected]>

* Update controllers/webhook/isvc_validator.go

Co-authored-by: Edgar Hernández <[email protected]>
Signed-off-by: Spolti <[email protected]>

---------

Signed-off-by: Spolti <[email protected]>
Co-authored-by: Edgar Hernández <[email protected]>

* update gitaction based on branch strategy change (#322)

Signed-off-by: jooho lee <[email protected]>

* feat: added performance metric grpahs config for nvidia nim (#320)

* feat: added performance metric grpahs config for nvidia nim

Signed-off-by: Tomer Figenblat <[email protected]>

* chore: modifyed the runtime id annotation

Co-authored-by: Edgar Hernández <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>

---------

Signed-off-by: Tomer Figenblat <[email protected]>
Co-authored-by: Edgar Hernández <[email protected]>

* Add NIM flag logic (#312)

Signed-off-by: mtrujillo <[email protected]>

* Grab the old release tag based on creation date

Signed-off-by: Hannah DeFazio <[email protected]>

* Updated the checkout code command

Signed-off-by: Mariah Holder <[email protected]>

* Updated the checkout code command (#329)

Signed-off-by: Mariah Holder <[email protected]>
Co-authored-by: Mariah Holder <[email protected]>

* Add reconciliation for Kserve Raw (#274)

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

* chore: added pagination support for nim catalog response (#332)

Signed-off-by: Tomer Figenblat <[email protected]>

* feat(mr): enable model registry inference reconcile (#326)

Signed-off-by: Alessio Pragliola <[email protected]>

* add upstream release metadata (#333)

Signed-off-by: heyselbi <[email protected]>

* Migration to kubebuilder v4 (#324)

* Migration to kubebuilder v4

Signed-off-by: Edgar Hernández <[email protected]>

* Restore MR E2Es

Signed-off-by: Edgar Hernández <[email protected]>

* Restore top-level files

Signed-off-by: Edgar Hernández <[email protected]>

* Cleaning

Signed-off-by: Edgar Hernández <[email protected]>

* Fixing Makefile and Containerfile

Signed-off-by: Edgar Hernández <[email protected]>

* Linter fixes

Signed-off-by: Edgar Hernández <[email protected]>

* Initial rework of manifests

Signed-off-by: Edgar Hernández <[email protected]>

* Fix manifests

Signed-off-by: Edgar Hernández <[email protected]>

* Fix lint issues

Signed-off-by: Edgar Hernández <[email protected]>

* Deactivate E2Es

Because setup is not automated, yet.

Signed-off-by: Edgar Hernández <[email protected]>

* Feedback: Filippe

Signed-off-by: Edgar Hernández <[email protected]>

* Feedback: Filippe

Test differences after `go mod tidy`

Signed-off-by: Edgar Hernández <[email protected]>

* Apply suggestions from code review: Filippe

Co-authored-by: Filippe Spolti <[email protected]>
Signed-off-by: Edgar Hernández <[email protected]>

* Feedback: Filippe

* Pin go-toolset base image in Containerfile.
* Add `gosec` linter

Signed-off-by: Edgar Hernández <[email protected]>

* Update config/prometheus/monitor.yaml

Co-authored-by: Filippe Spolti <[email protected]>
Signed-off-by: Edgar Hernández <[email protected]>

* Feedback: Filippe

* Small change to comments in Makefile, to make the text clearer.
* Remove (again) `gosec` linter

Signed-off-by: Edgar Hernández <[email protected]>

* Fix panic on controller startup

Signed-off-by: Edgar Hernández <[email protected]>

---------

Signed-off-by: Edgar Hernández <[email protected]>
Co-authored-by: Filippe Spolti <[email protected]>

* chore: use naming convention for resources created by nim (#340)

* chore: use naming convention for resources created by nim

Signed-off-by: Tomer Figenblat <[email protected]>

* test: added assertions for dyamic nim resources name

Signed-off-by: Tomer Figenblat <[email protected]>

---------

Signed-off-by: Tomer Figenblat <[email protected]>

* chore: set nim runtime api call page size to 1000 (#344)

Signed-off-by: Tomer Figenblat <[email protected]>

* Nim enablement change default to managed and add clean up job (#342)

* initial commit for clean up of nim and managed set as default

Signed-off-by: mtrujillo <[email protected]>

* remove space

Signed-off-by: mtrujillo <[email protected]>

* fix code length for linting

Signed-off-by: mtrujillo <[email protected]>

* fixed comments / adjusted import

Signed-off-by: mtrujillo <[email protected]>

---------

Signed-off-by: mtrujillo <[email protected]>

* chore: added new graph object for nim runtimes (#334)

* chore: added new graph object for nim runtimes

Signed-off-by: Tomer Figenblat <[email protected]>

* chore: added REQUEST_OUTCOMES nim graph

Signed-off-by: Tomer Figenblat <[email protected]>

* chore: added fixed typo in nim query object

Signed-off-by: Tomer Figenblat <[email protected]>

* chore: fixed typo in nim query object

Signed-off-by: Tomer Figenblat <[email protected]>

* chore: added initial query for nim gpu cache usage

Signed-off-by: Tomer Figenblat <[email protected]>

* chore: rewrite queries for nim new graphs

Signed-off-by: Tomer Figenblat <[email protected]>

---------

Signed-off-by: Tomer Figenblat <[email protected]>

* Update ovms to current build (#343)

Signed-off-by: Steve Grubb <[email protected]>
Co-authored-by: Steve Grubb <[email protected]>

* Automatically inject expected ODH annotations to InferenceGraph and InferenceServices (#339)

* Implementation of ODH defaulters for InferenceGraph and InferenceService

On creation of InferenceGraph or InferenceService resources, the following default annotations will be added:
* `serving.knative.openshift.io/enablePassthrough: true`
* `sidecar.istio.io/inject: true`
* `sidecar.istio.io/rewriteAppHTTPProbers: true`

The annotations are added only for Serverless mode, and only if they are missing.

Signed-off-by: Edgar Hernández <[email protected]>

* Feedback: Filippe

Extract "ENABLE_WEBHOOKS" string to constant

Signed-off-by: Edgar Hernández <[email protected]>

---------

Signed-off-by: Edgar Hernández <[email protected]>

* Authorization for InferenceGraph (Serverless) (#345)

* Authorization for InferenceGraph (Serverless)

This adds a new controller for KServe InferenceGraph resources. This new controller will have the responsibility of creating Authorino AuthConfig resources (similarly to InferenceServices case), when authorization is available in ODH platform.

InferenceGraphs can now be annotated with `security.opendatahub.io/enable-auth: "true"` to secure InferenceGraphs and only serve requests that are authorized.

Signed-off-by: Edgar Hernández <[email protected]>

* Feedback: Filippe - Event when auth is not available

Signed-off-by: Edgar Hernández <[email protected]>

---------

Signed-off-by: Edgar Hernández <[email protected]>

* [RHOAIENG-10293] add metrics resources for rawdeployment (#347)

* [RHOAIENG-10293] add metrics resources for rawdeployment

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

* [RHOAIENG-10293] address feedback

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

---------

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

* [RHOAIENG-16851] rawdeployment route bug fixes (#341)

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

* fix null pointer error (RHOAIENG-18228) (#349)

Signed-off-by: jooho lee <[email protected]>

* remove old file

Signed-off-by: jooho lee <[email protected]>

update go.mod

Signed-off-by: jooho lee <[email protected]>

---------

Signed-off-by: jooho lee <[email protected]>
Signed-off-by: Hannah DeFazio <[email protected]>
Signed-off-by: Spolti <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: mtrujillo <[email protected]>
Signed-off-by: Mariah Holder <[email protected]>
Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>
Signed-off-by: Alessio Pragliola <[email protected]>
Signed-off-by: heyselbi <[email protected]>
Signed-off-by: Edgar Hernández <[email protected]>
Signed-off-by: Steve Grubb <[email protected]>
Co-authored-by: Hannah DeFazio <[email protected]>
Co-authored-by: Filippe Spolti <[email protected]>
Co-authored-by: Edgar Hernández <[email protected]>
Co-authored-by: Tomer Figenblat <[email protected]>
Co-authored-by: Marcus Trujillo <[email protected]>
Co-authored-by: Mariah Holder <[email protected]>
Co-authored-by: Mariah Holder <[email protected]>
Co-authored-by: Vedant Mahabaleshwarkar <[email protected]>
Co-authored-by: Tomer Figenblat <[email protected]>
Co-authored-by: Alessio Pragliola <[email protected]>
Co-authored-by: Selbi Nuryyeva <[email protected]>
Co-authored-by: Steven Grubb <[email protected]>
Co-authored-by: Steve Grubb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants