Skip to content

Update Knative to 0.14.3#1617

Merged
k8s-ci-robot merged 2 commits into
kubeflow:masterfrom
pvaneck:knative-0.14.3
Nov 11, 2020
Merged

Update Knative to 0.14.3#1617
k8s-ci-robot merged 2 commits into
kubeflow:masterfrom
pvaneck:knative-0.14.3

Conversation

@pvaneck

@pvaneck pvaneck commented Nov 7, 2020

Copy link
Copy Markdown
Member

Description of your changes:
This update the Knative installation to 0.14.3, which I believe is the latest version available that supports K8s 1.15, the minimum required Kubernetes version for KFServing. This version is also needed for Knative to work on K8s 1.18+.

This also updates knative-eventing to v0.14.2 (the closest release to v0.14.3).

Based off the following YAML:
Serving:
https://github.com/knative/serving/releases/download/v.0.14.3/serving-core.yaml
https://github.com/knative-sandbox/net-istio/releases/download/v0.14.1/release.yaml

Eventing:
https://github.com/knative/eventing/releases/download/v0.14.2/eventing.yaml (using in-memory channel and channel broker)

Tested on OpenShift 4.5 (K8s 1.18), IKS 1.17, and Minikube k8s 1.17 and I had no issues creating and using inferenceservices.

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate-changed-only
    3. make test

@pvaneck

pvaneck commented Nov 7, 2020

Copy link
Copy Markdown
Member Author

Not much was changed from the source YAML. Mainly just the config-istio config map: https://github.com/pvaneck/manifests/blob/knative-0.14.3/knative/knative-serving-install/base/config-map.yaml#L740

Another thing to note was I was having issues with the istio-injection: enabled label on the knative-serving namespace. The sidecars were causing me issues like: http: TLS handshake error from 127.0.0.1:XXXX: EOF and Internal error occurred: failed calling webhook "webhook.serving.knative.dev": Post https://webhook.knative-serving.svc:443/defaulting?timeout=30s: EOF.

Not sure if I am missing some configuration to get it working with the istio mutual TLS, but after removing the istio-injection label, all is well. Seems like in the previous version (0.11.2) all deployments in the knative-serving namespace had the sidecar.istio.io/inject: "false" annotation (e.g. #1340). So hopefully disabling istio-injection on the knative-serving namespace should be fine.

@pvaneck

pvaneck commented Nov 7, 2020

Copy link
Copy Markdown
Member Author

FYI @animeshsingh @Tomcli @yuzisun

@Jeffwan

Jeffwan commented Nov 9, 2020

Copy link
Copy Markdown
Member

@pvaneck Is this a dependency of https://github.com/kubeflow/manifests/pull/1575/files?

Without this change, does kfserving 0.4.1 work?

Does Knative 0.14.3 have a requirement on minimum Istio version?

@animeshsingh animeshsingh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yuzisun this is the last chance if we want to move to KNative 0.14.3 before next release. Quick review appreciated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this also be 0.14.3?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

0.14.2 is the actual version of the release I used since there wasn't a 0.14.3 releases for knative-eventing.

@pvaneck

pvaneck commented Nov 9, 2020

Copy link
Copy Markdown
Member Author

@Jeffwan KFserving 0.4.1 will still work without this change, but I believe only on K8s versions 1.15-1.17. For K8s 1.18+, a higher version of Knative is needed (e.g. 0.14.3).

Not sure about the minimum required Istio version. This knative release seems to be tested with istio 1.4.x, but I've been using the 1.3.1 included with the Kubeflow manifests for my testing, and it seems to work fine.

@pvaneck

pvaneck commented Nov 9, 2020

Copy link
Copy Markdown
Member Author

/retest

@pvaneck

pvaneck commented Nov 9, 2020

Copy link
Copy Markdown
Member Author

Test failure error:

 {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"deployments.apps \"autoscaler-hpa\" not found","reason":"NotFound","details
":{"name":"autoscaler-hpa","group":"apps","kind":"deployments"},"code":404}

The autoscaler-hpa deployment was a deployment that was removed in this patch. Not sure why the test is trying to check the status of it.

@yuzisun yuzisun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Replace serving.knative.dev/release: v0.11.0 with serving.knative.dev/release: v0.14.3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

v0.14.3?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this, as I didn't even notice the label issues with the tests. Seems the knative kustomization.yaml files in tests/legacy_kustomizations/ need manual updating, so I went ahead and did that to fix all the generated test labels. This should also fix the knative-eventing namespace issue in the tests.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be eventing namespace?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

v0.14.3?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

v0.14.3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

v0.14.3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

v0.14.3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

v0.14.3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

v0.14.3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

v0.14.3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Knative's recommendation is to disable sidecar in the control plane, are we disabling somewhere else now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

None of the pods in the knative-serving namespace will have sidecars as I removed the istio-injection: enabled label from the knative-serving namespace that was originally there (see change). The original Knative install YAMLs have istio-injection enabled, but this was causing issues as mentioned here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @pvaneck ! we are good then.

This also updates knative-eventing to v0.14.2 (the closest release to
v0.14.3)
@drewbutlerbb4

Copy link
Copy Markdown
Member

/lgtm

@animeshsingh
I am able to deploy and query inference services with no problems.

Autoscaling work well. We tested using section 6 of this notebook and I see the pods scaling up as intended.

Screen Shot 2020-11-10 at 3 24 34 PM

@animeshsingh

Copy link
Copy Markdown
Contributor

thanks @pvaneck and @drewbutlerbb4

/lgtm from my perspective. @yuzisun please look at the response to your comments and sign off if you are good.

@animeshsingh

Copy link
Copy Markdown
Contributor

@pvaneck we still need to get the tests passed though?

@pvaneck

pvaneck commented Nov 11, 2020

Copy link
Copy Markdown
Member Author

/retest

animeshsingh added a commit to animeshsingh/manifests that referenced this pull request Nov 11, 2020
Will be responsible for maintaining manifests for KFServing, OpenShift, IKS etc.

PRs merged: 
kubeflow#1515
kubeflow#1575
kubeflow#1580
kubeflow#1617

Reviews:
kubeflow#1582
kubeflow#1621
kubeflow#1625
kubeflow#1627
kubeflow#1628
kubeflow/kfctl#419
@animeshsingh

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@k8s-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: animeshsingh, pvaneck

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@k8s-ci-robot k8s-ci-robot merged commit e174d47 into kubeflow:master Nov 11, 2020
k8s-ci-robot pushed a commit that referenced this pull request Nov 11, 2020
* Add Paul as a reviewer

Will be responsible for maintaining manifests for KFServing, OpenShift, IKS etc.

PRs merged: 
#1515
#1575
#1580
#1617

Reviews:
#1582
#1621
#1625
#1627
#1628
kubeflow/kfctl#419

* Update OWNERS
pvaneck added a commit to pvaneck/manifests that referenced this pull request Nov 11, 2020
* Update knative-serving to v0.14.3

This also updates knative-eventing to v0.14.2 (the closest release to
v0.14.3)

* Generate tests
k8s-ci-robot pushed a commit that referenced this pull request Nov 11, 2020
* Update knative-serving to v0.14.3

This also updates knative-eventing to v0.14.2 (the closest release to
v0.14.3)

* Generate tests
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.

6 participants