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.

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

Add the grpc route creation option #35

Closed
wants to merge 7 commits into from

Conversation

heyselbi
Copy link
Contributor

@heyselbi heyselbi commented May 8, 2023

Description

Addressing RHODS-6401 : https://issues.redhat.com/browse/RHODS-6401

How Has This Been Tested?

The port creation has been tested and is created as expected.
To test it, below are commands to follow in your terminal.

First, an image needs to be build. A personal quay repo can be used if desired -- selbi will need to change to yours. Alternatively, one can use a ready image that was already build (quay.io/selbi/odh-model-controller:grpc-v3)
To re-create the image:

git clone https://github.com/heyselbi/odh-model-controller.git
cd odh-model-controller
git checkout rhods-6401-grpc
podman build -t quay.io/selbi/odh-model-controller:grpc-v4 .
podman push quay.io/selbi/odh-model-controller:grpc-v4
cd ../

Now deploy the new image with modelmesh-serving. First, login to your cluster (oc login...). **If you skipped image building part and decided to use the ready image, change the image tag from :grpc-v4 to :grpc-v3.

git clone [email protected]:opendatahub-io/modelmesh-serving.git
git checkout release-v0.11.0-alpha
cd modelmesh-serving/opendatahub/odh-manifests/model-mesh
sed -i 's/opendatahub\/odh-model-controller:v0.11.0-alpha/selbi\/odh-model-controller:grpc-v4/' base/params.env
oc new-project opendatahub
kustomize build ./base  | oc apply -f -

Go to your cluster, and make sure that all pods are Running in opendatahub namespace. Or you can check with cli:

[selbi@fedora modelmesh-serving]$ oc get pods -n opendatahub
NAME                                    READY   STATUS    RESTARTS   AGE
etcd-5bc65bd9cf-9bshv                   1/1     Running   0          3m26s
modelmesh-controller-d49b698fc-8jppp    1/1     Running   0          3m26s
modelmesh-controller-d49b698fc-qjrld    1/1     Running   0          3m26s
modelmesh-controller-d49b698fc-wg92c    1/1     Running   0          3m26s
odh-model-controller-697d49b9bb-g2ppw   1/1     Running   0          26s
odh-model-controller-697d49b9bb-wn56p   1/1     Running   0          36s
odh-model-controller-697d49b9bb-xn5s4   1/1     Running   0          16s

Check that the image running in odh-model-controller is the one built earlier:

[selbi@fedora modelmesh-serving]$ oc get deployment/odh-model-controller -o wide
NAME                   READY   UP-TO-DATE   AVAILABLE   AGE     CONTAINERS   IMAGES                                       SELECTOR
odh-model-controller   3/3     3            3           5m58s   manager      quay.io/selbi/odh-model-controller:grpc-v4   app.kubernetes.io/managed-by=odh-model-controller,app.kubernetes.io/part-of=model-mesh,control-plane=odh-model-controller

Now we deploy a sample servingRuntime with sample inferenceService.

oc new-project testing
oc label namespace testing "modelmesh-enabled=true" --overwrite=true || echo "Failed to apply modelmesh-enabled label."
oc apply -f ../../../quickstart/openvino-serving-runtime.yaml
oc apply -f ../../../quickstart/openvino-inference-service.yaml

Check that two routes are created:

[selbi@fedora model-mesh]$ oc get routes
NAME                      HOST/PORT                                                               PATH                            SERVICES            PORT   TERMINATION            WILDCARD
example-onnx-mnist        example-onnx-mnist-testing.apps.selbishka.dev.datahub.redhat.com        /v2/models/example-onnx-mnist   modelmesh-serving   8443   reencrypt/Redirect     None
example-onnx-mnist-grpc   example-onnx-mnist-grpc-testing.apps.selbishka.dev.datahub.redhat.com                                   modelmesh-serving   8033   passthrough/Redirect   None

To clean up:

oc delete -f ../../../quickstart/openvino-inference-service.yaml
oc delete -f ../../../quickstart/openvino-serving-runtime.yaml
oc delete project testing
oc project opendatahub
kustomize build ./base  | oc delete -f -
oc delete project opendatahub

One might ask a great question, does this gRPC port do what a gRPC port is supposed to do (aka does it function as a gRPC port). One can find that answer in another PR: opendatahub-io/modelmesh-serving#106 :)

PS. Ignore the failing openshift-ci. It is not working properly in odh-model-controller overall yet.

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
Copy link
Contributor

openshift-ci bot commented May 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: heyselbi

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:

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

@openshift-ci openshift-ci bot added the approved label May 8, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 13, 2023

@heyselbi: 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/fvt 403b9c7 link true /test fvt

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.

@heyselbi heyselbi requested review from Jooho and israel-hdez June 14, 2023 04:19
}
}

return nil
}

// ReconcileRoute will manage the creation, update and deletion of the
// TLS route when the predictor is reconciled
// TLS route wheoptionsList[v]n the predictor is reconciled
Copy link
Contributor

Choose a reason for hiding this comment

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

copy&paste mistake?

Copy link
Contributor

@israel-hdez israel-hdez left a comment

Choose a reason for hiding this comment

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

Minimally, fix the copy&paste mistake and I can approve.

if !createRoute {
log.Info("Serving runtime does not have 'enable-route' annotation set to 'True'. Skipping route creation")
return nil
for _, v := range optionsList {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer extracting the code inside this for to a function, so that you can call inline in the if block that is up these lines avoiding the need for this for loop.

@heyselbi heyselbi linked an issue Jul 11, 2023 that may be closed by this pull request
@spolti
Copy link
Member

spolti commented Mar 15, 2024

Do we still need this PR?

@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

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.

@heyselbi heyselbi closed this Apr 2, 2024
spolti pushed a commit to spolti/odh-model-controller that referenced this pull request Sep 13, 2024
[RHOAIENG-6877] - odh-model-controller breaks Knative if KServe-Serve…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable gRPC inferencing by exposing route
4 participants