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

add ability to choose ingress creation for Kserve RawDeployment #1230

Open
wants to merge 2 commits into
base: incubation
Choose a base branch
from
+54 −22
Open

add ability to choose ingress creation for Kserve RawDeployment #1230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ed325b
add ability to choose ingress creation for Kserve RawDeployment and s…
VedantMahabaleshwarkar Sep 12, 2024
79a0105
pr feedback
VedantMahabaleshwarkar Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,17 @@ spec:
- Removed
pattern: ^(Managed|Unmanaged|Force|Removed)$
type: string
rawRouteCreation:
description: |-
Configures the global setting for route creation for Kserve Raw InferenceServices.
Managed: Route creation is enabled by default, InferenceServices will have to choose to opt out.
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.
enum:
- Managed
- Removed
pattern: ^(Managed|Unmanaged|Force|Removed)$
type: string
serving:
description: |-
Serving configures the KNative-Serving stack used for model serving. A Service
Expand Down
6 changes: 6 additions & 0 deletions components/kserve/kserve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ 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"`
// Configures the global setting for route creation for Kserve Raw InferenceServices.
// Managed: Route creation is enabled by default, InferenceServices will have to choose to opt out.
// 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"`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The suggestion was made here,

disclaimer: I don't have any strong opinion.

Copy link
Member

Choose a reason for hiding this comment

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

IngressCreationState ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

}

func (k *Kserve) OverrideManifests(ctx context.Context, _ cluster.Platform) error {
Expand Down
47 changes: 25 additions & 22 deletions components/kserve/kserve_config_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if defaultDeploymentMode == "" {
defaultDeploymentMode = Serverless
}
VedantMahabaleshwarkar marked this conversation as resolved.
Show resolved Hide resolved
disableIngressCreation := true
if k.RawRouteCreation == operatorv1.Managed {
Copy link
Member

Choose a reason for hiding this comment

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

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

disableIngressCreation = false
}
VedantMahabaleshwarkar marked this conversation as resolved.
Show resolved Hide resolved
VedantMahabaleshwarkar marked this conversation as resolved.
Show resolved Hide resolved
switch k.Serving.ManagementState {
case operatorv1.Managed, operatorv1.Unmanaged:
if k.DefaultDeploymentMode == "" {
// if the default mode is empty in the DSC, assume mode is "Serverless" since k.Serving is Managed
if err := k.setDefaultDeploymentMode(ctx, cli, dscispec, Serverless); err != nil {
return err
}
} else {
// if the default mode is explicitly specified, respect that
if err := k.setDefaultDeploymentMode(ctx, cli, dscispec, k.DefaultDeploymentMode); err != nil {
return err
}
if err := k.setKserveRawConfig(ctx, cli, dscispec, defaultDeploymentMode, disableIngressCreation); err != nil {
return err
}
case operatorv1.Removed:
if k.DefaultDeploymentMode == Serverless {
Expand All @@ -45,14 +45,16 @@ func (k *Kserve) setupKserveConfig(ctx context.Context, cli client.Client, logge
if k.DefaultDeploymentMode == "" {
logger.Info("Serving is removed, Kserve will default to rawdeployment")
}
if err := k.setDefaultDeploymentMode(ctx, cli, dscispec, RawDeployment); err != nil {
if err := k.setKserveRawConfig(ctx, cli, dscispec, RawDeployment, disableIngressCreation); err != nil {
return err
}
}
return nil
}

func (k *Kserve) setDefaultDeploymentMode(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec, defaultmode DefaultDeploymentMode) error {
func (k *Kserve) setKserveRawConfig(
ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec,
defaultmode DefaultDeploymentMode, disableIngressCreation bool) error {
inferenceServiceConfigMap := &corev1.ConfigMap{}
err := cli.Get(ctx, client.ObjectKey{
Namespace: dscispec.ApplicationsNamespace,
Expand All @@ -67,24 +69,25 @@ func (k *Kserve) setDefaultDeploymentMode(ctx context.Context, cli client.Client
if err = json.Unmarshal([]byte(inferenceServiceConfigMap.Data["deploy"]), &deployData); err != nil {
return fmt.Errorf("error retrieving value for key 'deploy' from configmap %s. %w", KserveConfigMapName, err)
}
var ingressData map[string]interface{}
if err = json.Unmarshal([]byte(inferenceServiceConfigMap.Data["ingress"]), &ingressData); err != nil {
return fmt.Errorf("error retrieving value for key 'ingress' from configmap %s. %w", KserveConfigMapName, err)
}
modeFound := deployData["defaultDeploymentMode"]
if modeFound != string(defaultmode) {
ingressCreationValueFound := ingressData["disableIngressCreation"]
if (modeFound != string(defaultmode)) || ingressCreationValueFound != disableIngressCreation {
deployData["defaultDeploymentMode"] = defaultmode
deployDataBytes, err := json.MarshalIndent(deployData, "", " ")
if err != nil {
return fmt.Errorf("could not set values in configmap %s. %w", KserveConfigMapName, err)
}
inferenceServiceConfigMap.Data["deploy"] = string(deployDataBytes)

var ingressData map[string]interface{}
if err = json.Unmarshal([]byte(inferenceServiceConfigMap.Data["ingress"]), &ingressData); err != nil {
return fmt.Errorf("error retrieving value for key 'ingress' from configmap %s. %w", KserveConfigMapName, err)
}
if defaultmode == RawDeployment {
ingressData["disableIngressCreation"] = true
} else {
ingressData["disableIngressCreation"] = false
clusterDomain, err := cluster.GetDomain(ctx, cli)
if err != nil {
return fmt.Errorf("error retrieving cluster domain %s. %w", KserveConfigMapName, err)
}
ingressData["ingressDomain"] = clusterDomain
Copy link
Contributor

Choose a reason for hiding this comment

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

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

ingressData["disableIngressCreation"] = disableIngressCreation
ingressDataBytes, err := json.MarshalIndent(ingressData, "", " ")
if err != nil {
return fmt.Errorf("could not set values in configmap %s. %w", KserveConfigMapName, err)
Expand Down
11 changes: 11 additions & 0 deletions config/crd/bases/datasciencecluster.opendatahub.io_datascienceclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,17 @@ spec:
- Removed
pattern: ^(Managed|Unmanaged|Force|Removed)$
type: string
rawRouteCreation:
description: |-
Configures the global setting for route creation for Kserve Raw InferenceServices.
Managed: Route creation is enabled by default, InferenceServices will have to choose to opt out.
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.
enum:
- Managed
- Removed
pattern: ^(Managed|Unmanaged|Force|Removed)$
type: string
serving:
description: |-
Serving configures the KNative-Serving stack used for model serving. A Service
Expand Down
1 change: 1 addition & 0 deletions docs/api-overview.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ _Appears in:_
| `Component` _[Component](#component)_ | | | |
| `serving` _[ServingSpec](#servingspec)_ | Serving configures the KNative-Serving stack used for model serving. A Service<br />Mesh (Istio) is prerequisite, since it is used as networking layer. | | |
| `defaultDeploymentMode` _[DefaultDeploymentMode](#defaultdeploymentmode)_ | Configures the default deployment mode for Kserve. This can be set to 'Serverless' or 'RawDeployment'.<br />The value specified in this field will be used to set the default deployment mode in the 'inferenceservice-config' configmap for Kserve.<br />This field is optional. If no default deployment mode is specified, Kserve will use Serverless mode. | | Enum: [Serverless RawDeployment] <br />Pattern: `^(Serverless\|RawDeployment)$` <br /> |
| `rawRouteCreation` _[ManagementState](#managementstate)_ | Configures the global setting for route creation for Kserve Raw InferenceServices.<br />Managed: Route creation is enabled by default, InferenceServices will have to choose to opt out.<br />Removed: Route creation is disabled globally. In this configuration it is not possible for an individual<br />InferenceService to opt in to having a Route created automatically. | | Enum: [Managed Removed] <br /> |



Expand Down