Add KServe manifests to odh-manifests #838

ReToCode · 2023-06-05T06:53:24Z

Description

This PR adds the initial manifests for KServe and necessary patches to make it run in ODH. It fetches the manifests from opendatahub-io/kserve and adds some final patches. So updates should be fairly simple in the future.

How Has This Been Tested?

It was tested using https://github.com/ReToCode/knative-kserve#installation-of-knative-with-service-mesh.

Use then use:

And test an InferenceService

cat <<-EOF | oc apply -f -
apiVersion: "serving.kserve.io/v1beta1"
kind: "InferenceService"
metadata:
  name: "sklearn-iris"
  namespace: kserve-demo
  annotations:
    sidecar.istio.io/inject: "true"
    sidecar.istio.io/rewriteAppHTTPProbers: "true"
    serving.knative.openshift.io/enablePassthrough: "true"
spec:
  predictor:
    model:
      runtime: kserve-sklearnserver
      modelFormat:
        name: sklearn
      storageUri: "gs://kfserving-examples/models/sklearn/1.0/model"
EOF

oc get isvc -n kserve-demo
NAME           URL                                                                                          READY   PREV   LATEST   PREVROLLEDOUTREVISION   LATESTREADYREVISION            AGE
sklearn-iris   http://sklearn-iris-kserve-demo.apps.rlehmann-ocp-4-12.serverless.devcluster.openshift.com   True           100                              sklearn-iris-predictor-00001   2m14s

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

openshift-ci · 2023-06-05T06:53:38Z

Hi @ReToCode. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

skonto · 2023-06-06T08:21:07Z

@ReToCode nice! I think the next step would be to have some basic testing (smoke tests) in https://github.com/opendatahub-io/odh-manifests/tree/master/tests/basictests? I am not aware of the CI work for ODH and KServe but it would be nice to be able to test the manifests and future work against Kserve examples etc.

Jooho · 2023-06-09T20:03:24Z

@ReToCode Opendatahub operator is using kfctl and it is using the base folder as an entry point. I can help to change the structure a little bit to be worked by kfctl .
However, there is another issue. kfctl is not updated for a long time while kustomize keeps updating. So some parameters such as replacements in the kserve/default/kustomization.yaml does not seem that it is possible to be parsed by kfcfl. So even though we change the directory structure, it should not work at the moment.

@VaishnaviHire do we have the plan to support kustomize for the odh-manifests?

kserve/odh-overlays/controller/inferenceservice-config-patch.yaml

kserve/README.md

kserve/odh-overlays/runtimes/kustomization.yaml

kserve/hack/update-kserve-manifests.sh

ReToCode · 2023-06-12T06:13:55Z

Opendatahub operator is using kfctl and it is using the base folder as an entry point. I can help to change the structure a little bit to be worked by kfctl .

@Jooho I updated the folder structure according to this, then we could have the following:

  - kustomizeConfig:
      repoRef:
        name: odh-manifests
        path: kserve/controller
    name: kserve-controller
  - kustomizeConfig:
      repoRef:
        name: odh-manifests
        path: kserve/runtimes
    name: kserve-runtimes

Would that work?

danielezonca · 2023-06-12T07:22:17Z

FYI ticket to discuss the inclusion of KServe into ODH

Single Model Deployment (KServe) opendatahub-community#54

Jooho · 2023-06-12T17:17:09Z

@ReToCode Technically it should be ok but I don't know why we need 2 entry points.

kserve/controller/odh-overlays/kserve-controller-manager-patch.yaml

kserve/controller/base/kustomization.yaml

ReToCode · 2023-06-13T05:57:19Z

@ReToCode Technically it should be ok but I don't know why we need 2 entry points.

Would have been for the runtimes (the namespace stuff). But if we omit it here, we can drop them.

ReToCode · 2023-06-14T10:53:00Z

Updated the PR according to Add KServe manifests to odh-manifests #838 (comment)
Added new files to fix an issue with webhook-certificates
Resolved discussions that are now no longer relevant
Updated the description of the PR with an updated installation/testing manual
The two entrypoints are no longer necessary, as runtimes come from the template. For Kfdef is would be something like now:

  - kustomizeConfig:
      repoRef:
        name: odh-manifests
        path: kserve
    name: kserve

Still assuming that Serverless + Service Mesh are installed prior to installing ODH.

kserve/odh-overlays/kustomization.yaml

danielezonca · 2023-06-19T20:17:23Z

Issue for the PR (just for tracking) https://github.com/opendatahub-io/odh-manifests/issues/848

kserve/README.md

kserve/hack/build-kserve-manifests.sh

kserve/kserve-built/kserve-built.yaml

kserve/hack/build-kserve-manifests.sh

skonto · 2023-06-23T14:15:15Z

From the base dir we include the variant:

base/kustomization.yam:

resources:
  - ../odh-overlays

Should not this be the other way around and run kustomize build kserve/odh-overlays instead of kustomize build kserve/base.
That is the common pattern I see, for example: https://github.com/kubernetes-sigs/kustomize/blob/master/examples/helloWorld/README.md#deploy.

israel-hdez

@ReToCode Can you also, please, squash the commits?

kserve/base/kustomization.yaml

Update folder structure and KServe fork url Pre-build KServe manifests to avoid issues with kfctl Change the webhook certificates Add runtime field to example InferenceService fix minor issues fix labels no cert-manager no certmanager Use images from quay.io/opendatahub version 0.10.2 Fetch manifests from opendatahub-io/kserve Revert back to assembled manifests KServe manifests using `kustomize` directly Remove istio-cni env variable in controller Simplify folder structure Use release-0.10 branch to generate built yaml Fix configuration params

ReToCode · 2023-07-12T08:13:24Z

@ReToCode Can you also, please, squash the commits?

Done and tested, see https://github.com/ReToCode/knative-kserve/tree/main and https://redhat-internal.slack.com/archives/C05742W6F7T/p1689149577193889.

ReToCode · 2023-07-12T08:48:32Z

oc apply -f ./odh-core.yaml
error: resource mapping not found for name: "odh-core" namespace: "opendatahub" from "./odh-core.yaml": no matches for kind "KfDef" in version "kfdef.apps.kubeflow.org/v1"

Pretty sure this is not related to my changes.

/test odh-manifests-e2e

israel-hdez · 2023-07-12T12:10:57Z

/retest

israel-hdez · 2023-07-12T14:58:49Z

/retest

Tests look unstable... :-/

Jooho · 2023-07-12T16:57:58Z

This PR does not have an integration test script for kserve. This is modelmesh one(https://github.com/opendatahub-io/odh-manifests/blob/kserve-prototype/tests/basictests/modelmesh.sh).

So the e2e test failure has nothing to do with this pr. I think we can add the test part with another PR after we merge this because we need these manifests as soon as possible.

israel-hdez · 2023-07-12T19:00:23Z

/retest

One more time. If it doesn't pass, I'll find who can help with it.

israel-hdez · 2023-07-12T22:55:30Z

/approve

israel-hdez · 2023-07-12T22:58:38Z

/assign vaishnavihire

israel-hdez · 2023-07-13T01:43:48Z

/retest

Jooho · 2023-07-13T03:43:36Z

kserve/base/params.env

@@ -0,0 +1,8 @@
+kserve-controller=quay.io/opendatahub/kserve-controller:v0.10.2


@israel-hdez do we need to update this to latest or v0.11-latest to avoid the anyuid issue?

Manifests are for 0.10. So, we could use v0.10-latest, which should also be patched. Doesn't it work?

Or, do we want to move to 0.11? In upstream 0.11 is still unreleased. So, I'm not sure...

I am ok any version that includes the changes regarding anyuid.

LaVLaS · 2023-07-13T03:55:54Z

/retest

LaVLaS · 2023-07-13T16:05:28Z

/retest

openshift-ci · 2023-07-13T18:58:38Z

@ReToCode: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/odh-manifests-e2e	`d6827d4`	link	true	`/test odh-manifests-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

LaVLaS · 2023-07-13T22:28:15Z

This appears to be an failure due to the odh operator deploying successfully during the CI test run. I am going to debug the live system to see what is causing the issue. Since this PR has no CI tests or modifies the existing functionality of any other components, this should merge regardless of the debug session

LaVLaS · 2023-07-14T13:26:11Z

/approve

Merging this since the ocp411 passed and we have initial confirmation that the ocp412 issues are due to a bug in one of the openshift-ci build images outside of our control.

@israel-hdez @Jooho @ReToCode If there are plans to integrate this a feature into ODH, you'll need to add CI tests when these manifests are updated

openshift-ci · 2023-07-14T13:26:18Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

openshift-ci bot requested review from anishasthana and LaVLaS June 5, 2023 06:53

openshift-ci bot added the needs-ok-to-test label Jun 5, 2023

ReToCode force-pushed the add-kserve-manifests branch 2 times, most recently from d3b1a4c to a0d8530 Compare June 5, 2023 07:02

taneem-ibrahim requested review from Jooho and israel-hdez June 8, 2023 20:30