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

Allows to make Istio and VirtualServices optional #2380

Merged

Conversation

Suresh-Nakkeran
Copy link
Contributor

@Suresh-Nakkeran Suresh-Nakkeran commented Aug 11, 2022

Signed-off-by: Suresh-Nakkeran suresh.n@ideas2it.com

What this PR does / why we need it:
This PR allows followining:

  • Istio is optional for Serverless mode and other networking layer Knative supported can be integrated with KServe such as Contour.
  • KServe Serverless installation can fully run in mesh mode without knative local gateway.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1336

Type of changes
Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Feature/Issue validation/testing:

Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • When DisableIstioVirtualHost is true, KServe does not create the top level virtual service thus Istio is no longer required for serverless mode.
  • When DisableIstioVirtualHost is true, KServe works with Contour.
  • When DisableIstioVirtualHost is false, KServe works like before.

Special notes for your reviewer:

  1. The Service url in status returns the InferenceService component ksvc url instead of the url with virtual host(isvc name) as top level vs is now disabled, e.g when transformer is added to the InferenceService, the url returns the transformer ksvc url otherwise it returns the predictor ksvc url.

Checklist:

  • Have you added unit/e2e tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

Release note:

`DisableIstioVirtualHost` is introduced in the configmap to disable top level Istio virtual service, Istio is no longer required for serverless mode.

@Suresh-Nakkeran Suresh-Nakkeran force-pushed the feature/istio-optional branch 4 times, most recently from b7c0fbc to bc21ef6 Compare August 16, 2022 08:02
@Suresh-Nakkeran Suresh-Nakkeran force-pushed the feature/istio-optional branch 2 times, most recently from 6c4fc81 to a8e0915 Compare August 23, 2022 10:12
@Suresh-Nakkeran
Copy link
Contributor Author

/retest

@Suresh-Nakkeran Suresh-Nakkeran force-pushed the feature/istio-optional branch 2 times, most recently from c3e6bca to ebc183b Compare August 25, 2022 15:16
@Suresh-Nakkeran Suresh-Nakkeran force-pushed the feature/istio-optional branch 3 times, most recently from c0d3847 to b1b7465 Compare September 2, 2022 15:19
Signed-off-by: Suresh Nakkeran <suresh.n@ideas2it.com>
@yuzisun
Copy link
Member

yuzisun commented Sep 9, 2022

/lgtm
/approve

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Suresh-Nakkeran, yuzisun

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

@kserve-oss-bot kserve-oss-bot merged commit 48f45eb into kserve:master Sep 9, 2022
alexagriffith pushed a commit to alexagriffith/kserve that referenced this pull request Sep 19, 2022
Signed-off-by: Suresh Nakkeran <suresh.n@ideas2it.com>

Signed-off-by: Suresh Nakkeran <suresh.n@ideas2it.com>
Signed-off-by: alexagriffith <agriffith96@gmail.com>
@rachitchauhan43
Copy link
Contributor

rachitchauhan43 commented Mar 1, 2023

@Suresh-Nakkeran : 1qq, say we go with disabling the top level virtual service and now deploy an ISVC with both transformer and a predictor.

  1. Now when i make a call to my ISVC, I will have to use transformer’s url right ?
  2. Will Kserve still support routing request from transformer to predictor or not ?

Is it possible to add a diagram of what was there earlier with Top Level VS and what can now be achieved with disabling top level VS ?

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.

Is it possible to make top level KFServing virtual service optional?
4 participants