allow setting default deployment mode for Kserve in DSC

[VedantMahabaleshwarkar]

Description

This change is to support Raw Deployments for Kserve. JIRA: https://issues.redhat.com/browse/RHOAIENG-3132
This PR adds a field DefaultDeploymentMode to the Kserve spec in the DSC. The default value is Serverless, which introduces no changes in behavior. However, if the DefaultDeploymentMode is set to RawDeployment, the operator updates the configmap inferenceservice-config in the applications NS. The updates are as follows :

• deploy["defaultDeploymentMode"] is set to match the value passed to the DSC since they both use the same enum
• ingress["disableIngressCreation "] is set to true if the deployment mode is RawDeployments, otherwise it is false

How Has This Been Tested?

Changes tested against dev cluster by building and deploying quay.io/vedantm/opendatahub-operator:rawdeployment
Testing instructions :

• clone the operator repo and run make deploy -e IMG=quay.io/vedantm/opendatahub-operator:latest -e OPERATOR_NAMESPACE=opendatahub-operator-system
• Create a DSCInitialization as follows

spec:
  applicationsNamespace: opendatahub
  monitoring:
    managementState: Managed
    namespace: opendatahub
  serviceMesh:
    controlPlane:
      metricsCollection: Istio
      name: data-science-smcp
      namespace: istio-system
    managementState: Unmanaged

• Create a DSC as follows

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

• Once the DSC reconcile is successful, verify values in the inferenceservice-config ConfigMap in the applications NS
  
  
• Change the DSC spec such that default deployment is now Serverless, wait for DSC to reconcile and verify the corresponding changes in the inferenceservice-config object

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

[bartoszmajsak]

I would like to offer some suggestions for further improvement but also have a few questions that can help me understand better its intended use and scope.

I would also very much appreciate implementing some tests, as it would improve maintenance and reliability long term.

Hint: running default make target invokes linter so you can see exact errors GH Action complains about right now.

[bartoszmajsak]

@VedantMahabaleshwarkar if you could rebase your PR (didn't want to do it on your behalf through GH UI) we might see if GH Linter Action will comment on the issues now.

[zdtsw]

@ykaliuta bought up a good point in this PR: why not we do patch on manifests instead of changing CM in the cluster.
wont this cause reconcile as manifests drift from CM?

[bartoszmajsak]

@ykaliuta bought up a good point in this PR: why not we do patch on manifests instead of changing CM in the cluster.
wont this cause reconcile as manifests drift from CM?

Very valid point, thanks @ykaliuta (though I do not see your comment here). IIRC, in KfDef version (operator-v1) if the resource already existed it was not reconciled to the "desired" state, it was simply skipped, leaving it customizable to the user. It seems to be different now with server-side apply and forced ownership:

opendatahub-operator/pkg/deploy/deploy.go

Lines 301 to 307 in 45638fd

	// Perform server-side apply
	data, err := json.Marshal(obj)
	if err != nil {
	return err
	}
	return cli.Patch(ctx, found, client.RawPatch(types.ApplyPatchType, data), client.ForceOwnership, client.FieldOwner(owner.GetName()))

This makes all resources brought as part of kustomize manifests being reconciled and effectively static. Not sure this is what we want for the configuration resources in particular. What is the thought here @danielezonca?

[VedantMahabaleshwarkar]

@bartoszmajsak @ykaliuta

@ykaliuta bought up a good point in this PR: why not we do patch on manifests instead of changing CM in the cluster.
wont this cause reconcile as manifests drift from CM?
Very valid point, thanks @ykaliuta (though I do not see your comment here). IIRC, in KfDef version (operator-v1) if the resource already existed it was not reconciled to the "desired" state, it was simply skipped, leaving it customizable to the user. It seems to be different now with server-side apply and forced ownership:

Right now there's only a couple of changes that we are supporting to the inferenceservice-config configmap. However if you look at the total number of options presented in the configmap, there's a lot more. IMO relying on manifest patches to generate the correct configmap spec is doable while we are exposing a minimal number of configurable options, but in the future if we decide to expose more options in the DSC it will be difficult to maintain a bunch of manifest patches to support all possible configurations.
Also, I don't know how we'd be able to support changes made to the DSC after install time using this method. (for e.g: an admin changing the default deployment mode ~2 weeks after install)

In my testing of this PR, I did not observe repeated reconciliation of the kserve component even though we are diverging from the manifests. I could be wrong here but since we're editing the configmap in the component reconcile loop, once the loop succeeds the operator reports the component as successfully reconciled. The operator would not try to reconcile the configmap back to the original spec according to the manifests here until something else triggers reconciliation again (in which case the configmap will again be set according to the DSC spec)

[ykaliuta]

Should it include e2e test as well?

[bartoszmajsak]

The operator would not try to reconcile the configmap back to the original spec according to the manifests here until something else triggers reconciliation again (in which case the configmap will again be set according to the DSC spec)

This is because the code in question first calls deploy.DeployManifestsFromPath and then setDefaultDeploymentMode within the same loop. My remark was more about the overall approach, as any other config map that does not have such a special treatment will be eventually reverted to the state defined in the provided manifests.

[VaishnaviHire]

Given this is an API change, tagging @etirelli @danielezonca as well for reviews

[israel-hdez]

This PR seems to be going the right direction, but my thinking tells me the operator can be smarter.

• It looks good to expose the new defaultDeploymentMode.
• It is OK to automatically set the ingress class name to openshift-default, but I think this field should also be exposed in the DSC. We cannot make the hard assumption that this will always be OK.
• Since the DSCI and the DSC already expose settings for OSSM and Serverless, these new configs can automatically inferred:
  • If in the DSC, the spec.components.kserve.serving.managementState field is set to Removed, the operator should automatically apply RawDeployment configurations rather than requiring the user to manually specify it (so, we should allow the new field(s) to be empty) -- IMHO, it is redundant to require the user to manually specify the deployment mode, if the operator already has the knowledge that Serverless is going to be absent.
  • Although, if the deployment mode (and, possibly the ingress class) is manually set, then honor whatever the user set.

Additionally, I agree with the comments from others about that patching the ConfigMap like this can lead to a reconciliation loop.

[zdtsw]

maybe i am being stupid, but:
why we do not just keep the logic as-was, instead of adding a new manifests yaml in kserve's repo?
e.g

• rename inferenceservice-config-patch.yaml to inferenceservice-config-istio-patch.yaml
• create a new one inferenceservice-config-raw-patch.yaml with openshift-default for ingressClassName

if DeploymentMode==default,
we use serverless + istio, call inferenceservice-config-istio-patch.yaml to create CM "inferenceservice-config" => same as what we are doing without changes from this PR

if DeploymentMode==raw,

1. DSC spec.components.kserve.serving.managementState to Remove
2. ignore value of DSCI spec.serviceMesh.managementState
3. call inferenceservice-config-istio-patch.yaml to create CM "inferenceservice-config"

if flip back from raw to default for DeploymentMode, the checks on serverless and istio is the same: need 2 depend operatos pre-installed + need CRDs in place. => same as what we are doing without changes from this PR

then, we do not need to manipulate reconcile part.

but if you can change patch by templating ingressClassName in inferenceservice-config-patch.yaml + move the logic from odh operator to kserve, that will be even better: no need above "rename + create" part.

[israel-hdez]

why we do not just keep the logic as-was, instead of adding a new manifests yaml in kserve's repo?

I think it is important to note that this is just a default deployment mode.

So, if serverless is Removed we want to help the user by defaulting to raw deployment mode without requiring the user to explicitly specify that. If serverless is Managed or Unmanaged, we want to use Serverless mode by default (to be backwards compatible), however the user may still want to use Raw by default even if a Serverless setup is present.

In consequence, (looking in the other direction) if the user sets Raw as the preferred mode, we should not switch Serverless to Removed as we don't know if the user will still want to use Serverless mode for some InferenceServices. Although, if the user sets Serverless as the default deployment mode, we definitely know that we need a Serverless setup, and we should help the user by validating it, or doing the setup for him.

As I said in another comment, once the new fields/configs are exposed, my thinking is that we cannot make hard assumptions. However, we should have guards to check invalid configurations/combinations.

[VedantMahabaleshwarkar]

In the latest commit, I added a file kserve_config_handler.go. I moved the existing functions for dependency setup to this file. I also added logic for a sanitycheck where if DefaultDeploymentMode = Serverless but Serving.ManagementState = Removed, the operator will throw an error saying that this is incompatible. If Serving.ManagementState = Removed, it is assumed that DefaultDeploymentMode = RawDeployment

[israel-hdez]

IMO, the default should be empty to:

• be able to have a minimal DSC (i.e. reduce the verbosity);
• and, if empty, this would let the operator choose the best value, given the managementState of Serverless (because managementState fields don't allow empties).

[VedantMahabaleshwarkar]

@danielezonca @zdtsw @bartoszmajsak wdyt about this approach?

[danielezonca]

I agree to avoid "polluting" to much the API so fine for me to have an empty default with the meaning of "Serverless"

[bartoszmajsak]

We can also make it a pointer so it's not visible in "defaulted" CR if not defined.

Considering problems with kubebuilder defaults when struct is shared between different API "owning structs" (that was a while ago though, not sure if still present - need to check), but also rules which I assume are coming into play, maybe it would be good to plan some enhacement in the validating webhook to using CustomDefaulter. Of course that is based on the assumption we have requirements identified and they're good enough.

WDYT @VaishnaviHire @zdtsw @ykaliuta

[VaishnaviHire]

So the reason we are not using the pointers in general for fields in DSCI/DSC is there was a previous requirement that if a field is not set, it should still be visible for users with a default value, so that they know readily what configurations are available. Ref

However, this is a field within a component spec, so I think it should be okay to set it as pointer.

[VedantMahabaleshwarkar]

Removed the // +kubebuilder:default=Serverless line to make the default empty and added logic to infer Serverless as the default deployment mode if a value is not explicitly provided
cc @israel-hdez

[zdtsw]

should we add some description into README for this new feature (even it wont break nightly)?

[israel-hdez]

should we add some description into README for this new feature (even it wont break nightly)?

We, probably, should have a small conversation about where ODH community docs should live. Right now, I'm writing something for KServe authorization and I'm planning to put it on opendatahub-io/kserve repo. I'd cover how to configure the feature (the operator part) and also how to use the feature. Clearly, the how to configure may also belong to this repo. But, then, docs would be scattered.

I'm 👍 on having community docs, but I'm 👎 on not having a single place to host them. At the very least, we should have a single entry point and, then, links (and cross-links) between the relevant repos.

[israel-hdez]

This
/lgtm

I'll let the operator team to do their review and give the approval to merge.

[VedantMahabaleshwarkar]

@zdtsw @VaishnaviHire can we merge this? We have an lgtm from the serving team

[zdtsw]

/lgtm

[openshift-ci]

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

/lgtm