add ability to choose ingress creation for Kserve RawDeployment #1230

VedantMahabaleshwarkar · 2024-09-12T19:46:15Z

Description

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

Remove the logic to set the disableIngressCreation flag in inferenceservice-config
- This is now done in the Kserve Manifests itself, the flag will always be true
Added a field in the kserve spec for disableIngressCreation. Default is true, if user has set a value then that value will be used.
Set the ingressDomain in inferenceservice-config to the clusterDomain

Merge after opendatahub-io/odh-model-controller#274 and opendatahub-io/kserve#419

How Has This Been Tested?

Deploy custom operator+kserve+odh-model-controller:
Using custom fork+branch to be able to apply the CRD changes along with the custom operator image
-- git clone git@github.com:VedantMahabaleshwarkar/opendatahub-operator.git
-- cd opendatahub-operator
-- git checkout origin/j-10291
-- make deploy -e IMG=quay.io/vedantm/opendatahub-operator:latest -e OPERATOR_NAMESPACE=opendatahub-operator-system
To test Inference with auth:
-- Create any kserve isvc+SR
-- isvc should have annotation : "serving.kserve.io/deploymentMode" : RawDeployment
-- isvc should have label: "networking.kserve.io/odh-auth": "true"
To test Inference without auth:
-- remove isvc label "networking.kserve.io/odh-auth": "true"
To test disabling route for a particular ISVC
-- Create any kserve isvc+SR
-- isvc should have annotation : "serving.kserve.io/deploymentMode" : RawDeployment
-- isvc should have label : "networking.kserve.io/visibility": "cluster-local"

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 · 2024-09-12T19:46:19Z

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

openshift-ci · 2024-09-13T06:55:14Z

[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:

~~OWNERS~~ [zdtsw]

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

zdtsw · 2024-09-13T07:01:09Z

i put this on hold because of the dependent PRs are not merged yet.

openshift-ci · 2024-10-09T19:34:39Z

New changes are detected. LGTM label has been removed.

codecov-commenter · 2024-10-09T21:48:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (incubation@294cc1e). Learn more about missing BASE report.

Additional details and impacted files

@@              Coverage Diff              @@
##             incubation    #1230   +/-   ##
=============================================
  Coverage              ?   18.91%           
=============================================
  Files                 ?       30           
  Lines                 ?     2670           
  Branches              ?        0           
=============================================
  Hits                  ?      505           
  Misses                ?     2103           
  Partials              ?       62

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

terrytangyuan

Minor comment. Otherwise LGTM.

bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml

components/kserve/kserve_config_handler.go

ykaliuta · 2024-10-11T06:56:59Z

components/kserve/kserve_config_handler.go

@@ -25,18 +25,22 @@ const (
 func (k *Kserve) setupKserveConfig(ctx context.Context, cli client.Client, logger logr.Logger, dscispec *dsciv1.DSCInitializationSpec) error {
 	// as long as Kserve.Serving is not 'Removed', we will setup the dependencies

+	var defaultDeploymentMode DefaultDeploymentMode
+	var disableIngressCreation bool


The name is misleading since it states disable but can mean both yes and no. It should be isDisable or something which indicates it's a flag (only variable name in the code).

I agree that the name is confusing but since this variable is setting the field ingress["disableIngressCreation"] that is why I named it to match.

Then it should be something like disableIngressCreationValue

ykaliuta · 2024-10-11T07:02:57Z

components/kserve/kserve_config_handler.go

@@ -25,18 +25,22 @@ const (
 func (k *Kserve) setupKserveConfig(ctx context.Context, cli client.Client, logger logr.Logger, dscispec *dsciv1.DSCInitializationSpec) error {
 	// as long as Kserve.Serving is not 'Removed', we will setup the dependencies

+	var defaultDeploymentMode DefaultDeploymentMode


Since it selects mode from default to Serverless, the name may be misleading. Probably, it is developmentMode now? I do not remember details where the default mode can be changed, so ignore if I'm wrong.

the field is called deploymentMode in Kserve so we are keeping the terminology consistent. The options for deploymentMode are RawDeployment or Serverless

So, you confirmed that variable defaultDeploymentMode should be called deploymentMode.

lburgazzoli · 2024-10-11T07:10:39Z

components/kserve/kserve.go

@@ -54,6 +54,8 @@ type Kserve struct {
 	// This field is optional. If no default deployment mode is specified, Kserve will use Serverless mode.
 	// +kubebuilder:validation:Enum=Serverless;RawDeployment
 	DefaultDeploymentMode DefaultDeploymentMode `json:"defaultDeploymentMode,omitempty"`
+	// +kubebuilder:validation:Type:bool
+	DisableIngressCreation bool `json:"disableIngressCreation,omitempty"`


As per the Open Shift API guidelines, boolean should be avoided if not strictly necessary.

To follow other conventions, eventually this option could be defined as an enum, like ingress: Managed|Unmanaged with its default value set of Managed, which to me seems to be more clear and avoid using negative names

Changed it to be a enum

…et cluster domain Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

components/kserve/kserve_config_handler.go

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

israel-hdez · 2024-10-12T19:48:22Z

components/kserve/kserve.go

+	// Removed: Route creation is disabled globally. In this configuration it is not possible for an individual
+	// InferenceService to opt in to having a Route created automatically.
+	// +kubebuilder:validation:Enum=Managed;Removed
+	RawRouteCreation operatorv1.ManagementState `json:"rawRouteCreation,omitempty"`


Maybe, expose the configuration with the same name as in our ConfigMap? I'm not seeing beneficial renaming it.

The suggestion was made here,

disclaimer: I don't have any strong opinion.

IngressCreationState ?

Well... we already started by using the same name with defaultDeploymentMode one. So, I'd simply lean towards using the same names as in the ConfigMap.

Additionally, sometimes we recommend manually modifying our ConfigMap. So, having a different name than in the ConfigMap will be confusing.

But yes, I think it is quite unfortunate that KServe project is using booleans, while OpenShift recommendation is to not use them.

Additionally, sometimes we recommend manually modifying our ConfigMap. So, having a different name than in the ConfigMap will be confusing.

What would be the expected behavior in this case ? I think that if we make such option visible through the platform APIs, then the config map should not be touched by the end user otherwise it would become very confusing (IMHO)

Typically, the ConfigMap is going to be rolled back by the operator. So, in the default state, it is true that the user cannot touch the ConfigMap.

If the users needs to do manual changes, a special annotation is required: opendatahub.io/managed. With the annotation, the operator will no longer touch the ConfigMap and these fields in the DSC will be useless. Even upgrades won't receive the updated manifests. Currently, docs do not mention the annotation, so IMO this is only for special cases. I do agree that having two ways of configuration is confusing.

I do not know what are the future plans around the annotation, TBH. I simply know that if we ever document it, we will need to maintain in our docs what's our supported ConfigMap, and what we support users from modifying.

@danielezonca @terrytangyuan any thoughts here? my initial preference was to match the upstream configmap but then we'd be violating openshift API guidelines. I don't have a strong preference either way right now

israel-hdez · 2024-10-12T20:20:18Z

components/kserve/kserve_config_handler.go

 		}
+		ingressData["ingressDomain"] = clusterDomain


I'd think that we should always ensure that the domain is correctly configured. So, probably this should be outside the if.

israel-hdez · 2024-10-12T20:26:39Z

components/kserve/kserve_config_handler.go

@@ -25,18 +25,18 @@ const (
 func (k *Kserve) setupKserveConfig(ctx context.Context, cli client.Client, logger logr.Logger, dscispec *dsciv1.DSCInitializationSpec) error {
 	// as long as Kserve.Serving is not 'Removed', we will setup the dependencies

+	defaultDeploymentMode := k.DefaultDeploymentMode


In reality, this is changing the outcome and doesn't look right.
Previously

When Knative is available:

If the user does not specify a default deployment mode:

We configure Serverless as the default mode.

We enable creation of the ingress.

If the user specifies a default mode:

We respect the user specified default deployment mode.

We disable creation of the ingress if the user specified Raw. Otherwise, we enable it.

When Knative is NOT available:

If the user does not specify a default deployment mode:

We configure Raw as the default mode.

We disable creation of the ingress.

If the user specifies a default mode:

If the user specified Serverless as default mode, we stop reconciliation with an error.

Because of the previous, the user can only specify Raw. So, we use Raw.

We disable creation of the ingress.

Now

When Knative is available:

If the user does not specify a default deployment mode:

We configure Serverless as the default mode.

We disable the ingress if the user didn't specify a config. Otherwise, we use the user config.

The default is different from the previous state, as there is no conditional.

If the user specifies a default mode:

We respect the user specified default deployment mode.

We disable the ingress if the user didn't specify a config. Otherwise, we use the user config.

The default is different from the previous state, as there is no conditional.

When Knative is NOT available:

If the user does not specify a default deployment mode:

We take Serverless as the default mode. Then, we return an error with our own default.

This is quite wrong, IMO.

If the user specifies a default mode:

If the user specified Serverless as default mode, we stop reconciliation with an error.

Because of the previous, the user can only specify Raw. So, we use Raw.

We disable the ingress if the user didn't specify a config. Otherwise, we use the user config.

This one looks OK.

Generally speaking, any upgrade of a previous install will receive an updated config which, IMO, shouldn't happen. We should be backwards compatible here.

zdtsw · 2024-10-14T08:12:27Z

components/kserve/kserve_config_handler.go

+		defaultDeploymentMode = Serverless
+	}
+	disableIngressCreation := true
+	if k.RawRouteCreation == operatorv1.Managed {


what would happen(assume the new field is called "rawRouteCreation" for now) , when user set

kserve: managementState: Managed defaultDeploymentMode: RawDeployment rawRouteCreation: Removed serving: ingressGateway: certificate: type: OpenshiftDefaultIngress managementState: Managed name: knative-serving

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

VedantMahabaleshwarkar marked this pull request as ready for review September 12, 2024 19:46

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

openshift-ci bot requested review from lburgazzoli and mattmahoneyrh September 12, 2024 19:46

VedantMahabaleshwarkar mentioned this pull request Sep 12, 2024

enable ingress creation for RawDeployments opendatahub-io/kserve#406

Open

7 tasks

zdtsw approved these changes Sep 13, 2024

View reviewed changes

openshift-ci bot assigned zdtsw Sep 13, 2024

openshift-ci bot added the lgtm label Sep 13, 2024

openshift-ci bot added the approved label Sep 13, 2024

zdtsw added the do-not-merge/hold label Sep 13, 2024

VedantMahabaleshwarkar force-pushed the j-10291 branch from 540e23d to 0f1a36c Compare October 9, 2024 19:34

openshift-ci bot removed the lgtm label Oct 9, 2024

VedantMahabaleshwarkar changed the title ~~set clusterDomain as ingressDomain for Kserve RawDeployment Routes~~ add ability to choose ingress creation for Kserve RawDeployment Oct 9, 2024

VedantMahabaleshwarkar mentioned this pull request Oct 9, 2024

add oauth-proxy to rawdeployments if odh auth label is present opendatahub-io/kserve#419

Open

7 tasks

VedantMahabaleshwarkar force-pushed the j-10291 branch 2 times, most recently from 49da6af to 37122a5 Compare October 10, 2024 15:22

terrytangyuan reviewed Oct 10, 2024

View reviewed changes

bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml Outdated Show resolved Hide resolved

ykaliuta reviewed Oct 11, 2024

View reviewed changes

lburgazzoli reviewed Oct 11, 2024

View reviewed changes

add ability to choose ingress creation for Kserve RawDeployment and s…

9ed325b

…et cluster domain Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

VedantMahabaleshwarkar force-pushed the j-10291 branch from 37122a5 to 9ed325b Compare October 11, 2024 17:40

ykaliuta reviewed Oct 11, 2024

View reviewed changes

components/kserve/kserve_config_handler.go Show resolved Hide resolved

pr feedback

79a0105

Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

israel-hdez requested changes Oct 12, 2024

View reviewed changes

openshift-ci bot assigned israel-hdez Oct 12, 2024

zdtsw reviewed Oct 14, 2024

View reviewed changes

zdtsw removed the approved label Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ability to choose ingress creation for Kserve RawDeployment #1230

add ability to choose ingress creation for Kserve RawDeployment #1230

VedantMahabaleshwarkar commented Sep 12, 2024 •

edited

Loading

openshift-ci bot commented Sep 12, 2024

openshift-ci bot commented Sep 13, 2024

zdtsw commented Sep 13, 2024

openshift-ci bot commented Oct 9, 2024

codecov-commenter commented Oct 9, 2024 •

edited

Loading

terrytangyuan left a comment

ykaliuta Oct 11, 2024

VedantMahabaleshwarkar Oct 11, 2024

ykaliuta Oct 14, 2024

ykaliuta Oct 11, 2024

VedantMahabaleshwarkar Oct 11, 2024

ykaliuta Oct 11, 2024

lburgazzoli Oct 11, 2024 •

edited

Loading

VedantMahabaleshwarkar Oct 11, 2024

israel-hdez Oct 12, 2024

lburgazzoli Oct 14, 2024

zdtsw Oct 14, 2024

israel-hdez Oct 14, 2024

lburgazzoli Oct 14, 2024

israel-hdez Oct 14, 2024

VedantMahabaleshwarkar Oct 15, 2024

israel-hdez Oct 12, 2024

israel-hdez Oct 12, 2024

zdtsw Oct 14, 2024

add ability to choose ingress creation for Kserve RawDeployment #1230

Are you sure you want to change the base?

add ability to choose ingress creation for Kserve RawDeployment #1230

Conversation

VedantMahabaleshwarkar commented Sep 12, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

openshift-ci bot commented Sep 12, 2024

openshift-ci bot commented Sep 13, 2024

zdtsw commented Sep 13, 2024

openshift-ci bot commented Oct 9, 2024

codecov-commenter commented Oct 9, 2024 • edited Loading

Codecov Report

terrytangyuan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lburgazzoli Oct 11, 2024 • edited Loading

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

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

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

VedantMahabaleshwarkar commented Sep 12, 2024 •

edited

Loading

codecov-commenter commented Oct 9, 2024 •

edited

Loading

lburgazzoli Oct 11, 2024 •

edited

Loading