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

Jump to bottom

Make kative local gateway configurable #1363

Merged
merged 2 commits into from
Feb 6, 2021
Merged

Make kative local gateway configurable #1363

merged 2 commits into from
Feb 6, 2021

Conversation

yuzisun
Copy link
Member

@yuzisun yuzisun commented Feb 6, 2021

What this PR does / why we need it:

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 #1340

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Added knative local gateway to the inference service config file

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@theofpa theofpa mentioned this pull request Feb 6, 2021
Merged
theofpa
theofpa suggested changes Feb 6, 2021
View reviewed changes
Copy link
Member

@theofpa theofpa left a comment

Choose a reason for hiding this comment

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

Shouldn't we use the new localGateway convention of knative (knative-local-gateway) as the default value, and users of old versions can change that to cluster-local-gateway?

@yuzisun
Copy link
Member Author

yuzisun commented Feb 6, 2021 •
edited
Loading

Shouldn't we use the new localGateway convention of knative (knative-local-gateway) as the default value, and users of old versions can change that to cluster-local-gateway?

@theofpa Yes let’s do that in your PR, I want to first make it configurable and run the e2e rests to confirm it works with the old value against older knative version as you are upgrading knative version in e2e.

@theofpa
Copy link
Member

theofpa commented Feb 6, 2021

Shouldn't we use the new localGateway convention of knative (knative-local-gateway) as the default value, and users of old versions can change that to cluster-local-gateway?

@theofpa Yes let’s do that in your PR, I want to first make it configurable and run the e2e rests to confirm it works with the old value against older knative version as you are upgrading knative version in e2e.

ok!
/lgtm

@k8s-ci-robot k8s-ci-robot merged commit b63544f into kserve:master Feb 6, 2021
theofpa
theofpa reviewed Feb 9, 2021
View reviewed changes
pkg/apis/serving/v1beta1/configmap.go
IngressGateway string `json:"ingressGateway,omitempty"`
IngressServiceName string `json:"ingressService,omitempty"`
LocalGateway string `json:"localGateway,omitempty"`
LocalGatewayServiceName string `json:"localGatewayService,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

@yuzisun, I've missed that, but it generates a name mismatch violation in the make generate. We can either match the names or add it in the violation exceptions.

theofpa
theofpa reviewed Feb 9, 2021
View reviewed changes
config/configmap/inferenceservice.yaml
Comment on lines 151 to +152
"ingressService" : "istio-ingressgateway.istio-system.svc.cluster.local"
"localGateway" : "cluster-local-gateway"
Copy link
Member

Choose a reason for hiding this comment

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

missing commas for valid json

google-oss-robot pushed a commit that referenced this pull request Mar 12, 2021
@theofpa @yuzisun
support knative 0.19+ (#1334)
3b5b1b7 
* replace cluster-local-gateway with knative-local-gateway to support knative 0.19+

* remove istio injection

* update the new variable from #1363 to the new local gateway name

* add waiting condition for knative operator

* filter out storage-version-migration-serving-serving from knative condition

* fix json syntax on isvc cm and exclude violations

* replace cluster-local-gateway with knative-local-gateway on v0.5.0

* try removing the local svc url

* Revert "Merge remote-tracking branch 'origin/knative-0.19+' into knative-0.19+"

This reverts commit 1338b34, reversing
changes made to 40a4eff.

* replace srv.cluster.local in logger test and increase the number of pytest workers from 3 to 4

Signed-off-by: Theofilos Papapanagiotou <theofilos@gmail.com>

* update the developer guide for knative 0.19+

Signed-off-by: Theofilos Papapanagiotou <theofilos@gmail.com>

* revert example image

Signed-off-by: Theofilos Papapanagiotou <theofilos@gmail.com>

* Change the concurrency back to 3

* Update install/v0.5.0/kfserving.yaml

Co-authored-by: Dan Sun <dsun20@bloomberg.net>

* Update install/v0.5.0/kfserving.yaml

Co-authored-by: Dan Sun <dsun20@bloomberg.net>

* dev guide detail fix for knative-local-gateway

Signed-off-by: Theofilos Papapanagiotou <theofilos@gmail.com>

* set pytest concurrency to 4

Signed-off-by: Theofilos Papapanagiotou <theofilos@gmail.com>

* add namespace on knative-local-gateway

* improve e2e test for logger

* improve e2e test for logger

* rollback changes to 0.5.0 release

Co-authored-by: Dan Sun <dsun20@bloomberg.net>
@yuzisun yuzisun deleted the localGateway branch April 2, 2021 19:52
@pvaneck pvaneck mentioned this pull request Jun 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theofpa theofpa theofpa requested changes

@pugangxa pugangxa Awaiting requested review from pugangxa

@salanki salanki Awaiting requested review from salanki

Assignees

@theofpa theofpa

Labels
approved lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Istio gateways are hard coded and anti-pattern
3 participants
@yuzisun @k8s-ci-robot @theofpa