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

Show KServe defaultDeploymentMode in DSC status #1465

Merged
merged 1 commit into from
Jan 6, 2025
Merged

Show KServe defaultDeploymentMode in DSC status #1465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions apis/components/v1alpha1/kserve_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ type KserveSpec struct {

// KserveCommonStatus defines the shared observed state of Kserve
type KserveCommonStatus struct {
// DefaultDeploymentMode is the value of the defaultDeploymentMode field
// as read from the "deploy" JSON in the inferenceservice-config ConfigMap
DefaultDeploymentMode string `json:"defaultDeploymentMode,omitempty"`
}

// KserveStatus defines the observed state of Kserve
Expand Down
5 changes: 5 additions & 0 deletions bundle/manifests/components.platform.opendatahub.io_kserves.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,11 @@ spec:
x-kubernetes-list-map-keys:
- type
x-kubernetes-list-type: map
defaultDeploymentMode:
description: |-
DefaultDeploymentMode is the value of the defaultDeploymentMode field
as read from the "deploy" JSON in the inferenceservice-config ConfigMap
type: string
observedGeneration:
format: int64
type: integer
Expand Down
5 changes: 5 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 @@ -718,6 +718,11 @@ spec:
kserve:
description: Kserve component status.
properties:
defaultDeploymentMode:
description: |-
DefaultDeploymentMode is the value of the defaultDeploymentMode field
as read from the "deploy" JSON in the inferenceservice-config ConfigMap
type: string
managementState:
description: |-
Set to one of the following values:
Expand Down
2 changes: 1 addition & 1 deletion bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ metadata:
categories: AI/Machine Learning, Big Data
certified: "False"
containerImage: quay.io/opendatahub/opendatahub-operator:v2.21.0
createdAt: "2024-12-16T13:35:31Z"
createdAt: "2025-01-02T20:06:57Z"
olm.skipRange: '>=1.0.0 <2.21.0'
operators.operatorframework.io/builder: operator-sdk-v1.31.0
operators.operatorframework.io/internal-objects: '["featuretrackers.features.opendatahub.io",
Expand Down
5 changes: 5 additions & 0 deletions config/crd/bases/components.platform.opendatahub.io_kserves.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,11 @@ spec:
x-kubernetes-list-map-keys:
- type
x-kubernetes-list-type: map
defaultDeploymentMode:
description: |-
DefaultDeploymentMode is the value of the defaultDeploymentMode field
as read from the "deploy" JSON in the inferenceservice-config ConfigMap
type: string
observedGeneration:
format: int64
type: integer
Expand Down
5 changes: 5 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 @@ -718,6 +718,11 @@ spec:
kserve:
description: Kserve component status.
properties:
defaultDeploymentMode:
description: |-
DefaultDeploymentMode is the value of the defaultDeploymentMode field
as read from the "deploy" JSON in the inferenceservice-config ConfigMap
type: string
managementState:
description: |-
Set to one of the following values:
Expand Down
26 changes: 26 additions & 0 deletions controllers/components/kserve/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package kserve

import (
"encoding/json"
"fmt"

corev1 "k8s.io/api/core/v1"
)

// ConfigMap Keys.
const (
DeployConfigName = "deploy"
IngressConfigKeyName = "ingress"
)

type DeployConfig struct {
DefaultDeploymentMode string `json:"defaultDeploymentMode,omitempty"`
}

func getDeployConfig(cm *corev1.ConfigMap) (*DeployConfig, error) {
deployConfig := DeployConfig{}
if err := json.Unmarshal([]byte(cm.Data[DeployConfigName]), &deployConfig); err != nil {
return nil, fmt.Errorf("error retrieving value for key '%s' from ConfigMap %s. %w", DeployConfigName, cm.Name, err)
}
return &deployConfig, nil
}
1 change: 1 addition & 0 deletions controllers/components/kserve/kserve_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
WithAction(deploy.NewAction(
deploy.WithCache(),
)).
WithAction(setStatusFields).
Copy link
Member

Choose a reason for hiding this comment

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

can the update function be done in UpdateDSCStatus() https://github.com/opendatahub-io/opendatahub-operator/blob/b0a5efa5db3c0d9379720fdfc52f14580f91003c/controllers/components/kserve/kserve.go#L87C31-L87C49 or have to create a new setStatusFields() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure 🤔 it looks like that will get called only during the reconciliation in the DSC controller, right? I think we should also have this set in the Kserve CR status for consistency, and to do that I think it needs to be updated here. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

hmmmmmm, why we do not add the "set status" in customizeKserveConfigMap() directly?
e.g afterinferenceServiceConfigMap.Data[DeployConfigName] = string(deployDataBytes)
then we can have a default value "Serverless" for defaultDeploymentMode , only if deployData.DefaultDeploymentMode != string(defaultmode) { it sets a different value.

--
tbh, i am not fond of setting component's status in DSC.
if possible, it should only be in the kserve CR 's .status. or we are duplicating all things in both places.
e.g modelreg namespace is only in the DSC .spec and ModelReg CR .spec , but not DSC .status.
it would be good for dashboard to check component CR to get each's .status.
but this is out of the scope for this PR, if the requirment is to get it done in DSC .status.

Copy link
Member Author

Choose a reason for hiding this comment

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

why we do not add the "set status" in customizeKserveConfigMap() directly?

The main reason that I chose to keep this separate, and after the deploy action, is that we want to read the ConfigMap from the cluster rather than relying on it from the resources list, because the user may decide to manage it themselves, and change the value from what the operator would have set it to (the value that would be in the resources list). Doing it after the deploy action should mean that the ConfigMap at least always exists on the cluster at this point. If there wasn't that edge case of unmanaged configmap, then it would make sense to do it in customizeKserveConfigMap().

it would be good for dashboard to check component CR to get each's .status.

Yeah maybe, I'm not sure what's best there. 🤔 @lburgazzoli do you have any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

let me get back from PTO :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@lburgazzoli ah apologies 🙈 in any case I think this is not a blocking conversation 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

don't worry 😉

WithAction(updatestatus.NewAction()).
// must be the final action
WithAction(gc.NewAction()).
Expand Down
15 changes: 15 additions & 0 deletions controllers/components/kserve/kserve_controller_actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,18 @@ func customizeKserveConfigMap(ctx context.Context, rr *odhtypes.ReconciliationRe

return nil
}

func setStatusFields(ctx context.Context, rr *odhtypes.ReconciliationRequest) error {
k, ok := rr.Instance.(*componentApi.Kserve)
if !ok {
return fmt.Errorf("resource instance %v is not a componentApi.Kserve)", rr.Instance)
}

ddm, err := getDefaultDeploymentMode(ctx, rr.Client, &rr.DSCI.Spec)
if err != nil {
return err
}

k.Status.DefaultDeploymentMode = ddm
return nil
}
35 changes: 24 additions & 11 deletions controllers/components/kserve/kserve_support.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,24 +148,37 @@ func removeServerlessFeatures(ctx context.Context, cli client.Client, k *compone
return serverlessFeatures.Delete(ctx, cli)
}

func getDefaultDeploymentMode(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) (string, error) {
kserveConfigMap := corev1.ConfigMap{}
if err := cli.Get(ctx, client.ObjectKey{Name: kserveConfigMapName, Namespace: dscispec.ApplicationsNamespace}, &kserveConfigMap); err != nil {
return "", err
}

deployConfig, err := getDeployConfig(&kserveConfigMap)
if err != nil {
return "", err
}

return deployConfig.DefaultDeploymentMode, nil
}

func setDefaultDeploymentMode(inferenceServiceConfigMap *corev1.ConfigMap, defaultmode componentApi.DefaultDeploymentMode) error {
// set data.deploy.defaultDeploymentMode to the model specified in the Kserve spec
var deployData map[string]interface{}
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)
deployData, err := getDeployConfig(inferenceServiceConfigMap)
if err != nil {
return err
}
modeFound := deployData["defaultDeploymentMode"]
if modeFound != string(defaultmode) {
deployData["defaultDeploymentMode"] = defaultmode

if deployData.DefaultDeploymentMode != string(defaultmode) {
deployData.DefaultDeploymentMode = string(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)
inferenceServiceConfigMap.Data[DeployConfigName] = 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 err = json.Unmarshal([]byte(inferenceServiceConfigMap.Data[IngressConfigKeyName]), &ingressData); err != nil {
return fmt.Errorf("error retrieving value for key '%s' from configmap %s. %w", IngressConfigKeyName, kserveConfigMapName, err)
}
if defaultmode == componentApi.RawDeployment {
ingressData["disableIngressCreation"] = true
Expand All @@ -176,7 +189,7 @@ func setDefaultDeploymentMode(inferenceServiceConfigMap *corev1.ConfigMap, defau
if err != nil {
return fmt.Errorf("could not set values in configmap %s. %w", kserveConfigMapName, err)
}
inferenceServiceConfigMap.Data["ingress"] = string(ingressDataBytes)
inferenceServiceConfigMap.Data[IngressConfigKeyName] = string(ingressDataBytes)
}

return nil
Expand Down
4 changes: 4 additions & 0 deletions docs/api-overview.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,9 @@ _Appears in:_
- [DSCKserveStatus](#dsckservestatus)
- [KserveStatus](#kservestatus)

| Field | Description | Default | Validation |
| --- | --- | --- | --- |
| `defaultDeploymentMode` _string_ | DefaultDeploymentMode is the value of the defaultDeploymentMode field<br />as read from the "deploy" JSON in the inferenceservice-config ConfigMap | | |


#### KserveList
Expand Down Expand Up @@ -862,6 +865,7 @@ _Appears in:_
| `phase` _string_ | | | |
| `observedGeneration` _integer_ | | | |
| `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.25/#condition-v1-meta) array_ | | | |
| `defaultDeploymentMode` _string_ | DefaultDeploymentMode is the value of the defaultDeploymentMode field<br />as read from the "deploy" JSON in the inferenceservice-config ConfigMap | | |


#### Kueue
Expand Down
1 change: 1 addition & 0 deletions tests/e2e/helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ func setupDSCInstance(name string) *dscv1.DataScienceCluster {
ManagementState: operatorv1.Removed,
},
KserveCommonSpec: componentApi.KserveCommonSpec{
DefaultDeploymentMode: componentApi.Serverless,
Serving: infrav1.ServingSpec{
ManagementState: operatorv1.Managed,
Name: "knative-serving",
Expand Down
2 changes: 2 additions & 0 deletions tests/e2e/kserve_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ func (k *KserveTestCtx) validateKserveInstance(t *testing.T) {
k.testDsc.Spec.Components.Kserve.Serving.IngressGateway.Certificate.Type),

jq.Match(`.status.phase == "%s"`, readyStatus),
jq.Match(`.status.defaultDeploymentMode == "%s"`, k.testDsc.Spec.Components.Kserve.DefaultDeploymentMode),
)),
))

Expand All @@ -86,6 +87,7 @@ func (k *KserveTestCtx) validateKserveInstance(t *testing.T) {
jq.Match(`.status.conditions[] | select(.type == "%s") | .status == "%s"`, modelcontroller.ReadyConditionType, metav1.ConditionTrue),
jq.Match(`.status.installedComponents."%s" == true`, kserve.LegacyComponentName),
jq.Match(`.status.components.%s.managementState == "%s"`, componentApi.KserveComponentName, operatorv1.Managed),
jq.Match(`.status.components.%s.defaultDeploymentMode == "%s"`, componentApi.KserveComponentName, k.testDsc.Spec.Components.Kserve.DefaultDeploymentMode),
)),
))

Expand Down
Loading