Use Istio 1.0.0. by tcnghia · Pull Request #1771 · knative/serving

tcnghia · 2018-08-01T02:10:26Z

I also added some explanation of the Helm options we used for generating istio.yaml.

I dropped the sleep 20 patch (see #1370), since we can't really dictate our yaml on user's clusters --- it doesn't make sense to keep carrying the patch. We'll leave the bug open to investigate if the issue is still around (many things have changed since we added that sleep 20) and find a different solution if it still lingers. After this change this yaml is just for a convenience one-liner without asking users to go through helm installation or Istio installation docs.

I dropped the global.proxy.image=proxyv2 option because that's the default now.

Will follow up with more documentation in knative/docs to explain what we expect from Istio sidecar injection setup so that users already having Istio would know how to configure it to work with Knative.

/assign @ZhiminXiang
/assign @evankanderson
/cc @scothis
/cc @krancour

tcnghia · 2018-08-01T02:17:45Z

/test pull-knative-serving-unit-tests

mattmoor · 2018-08-01T04:46:32Z

Awesome, thanks for pulling this in so quickly.

krancour · 2018-08-01T15:26:02Z

+Our goal here is to allow sidecar injection for Pods created by Knative, and
+nothing else.  This template is used in integration tests and also released as
+an Istio-one-line-installation so that our users don't have to go through a lot
+of steps to install Istio.


It would be good to make note of whether Istio installed as configured here is a production-ready configuration or more of a "kick-the-tires" configuration. I honestly do not know, but it would be good to call it out.

This comment is non-blocking, btw.

krancour · 2018-08-01T16:21:11Z

Istio 1.0 is installing fine for me as packaged here, but I'm having a little trouble getting Knative to install on top of it:

$ cat third_party/istio-1.0.0/istio.yaml | sed 's/LoadBalancer/NodePort/' | k apply -f -
...
$ curl -L https://github.com/knative/serving/releases/download/v0.1.0/release-lite.yaml   | sed 's/LoadBalancer/NodePort/'   | kubectl apply -f -
...
metric.config.istio.io "revisionrequestcount" configured
metric.config.istio.io "revisionrequestduration" configured
metric.config.istio.io "revisionrequestsize" configured
metric.config.istio.io "revisionresponsesize" configured
prometheus.config.istio.io "revisionpromhandler" configured
rule.config.istio.io "revisionpromhttp" configured
Error from server: error when creating "STDIN": admission webhook "pilot.validation.istio.io" denied the request: configuration is invalid: server must have TLS settings for HTTPS/TLS protocols

tcnghia · 2018-08-01T16:30:34Z

@krancour can you try with the nightly release instead?
This is being fixed in #1725. Istio added validations in 1.0 that weren't in 0.8.

krancour · 2018-08-01T16:31:11Z

Maybe I need to take #1725 into account? Seems it.

I'm having trouble building release.yaml using the release.sh script.

tcnghia · 2018-08-01T16:42:11Z

@krancour The YAML released nightly should already have the change (see #1722) so you won't need to apply #1725. However, we cut our 0.1.0 before 1.0 so we'll probably include #1725 in a 0.1.1 release from the 0.1 release branch.

tcnghia · 2018-08-01T16:45:35Z

@krancour we recently changed getting started to use 0.1.0 -- you can find the old url in the PR https://github.com/knative/docs/pull/287/files . We should still document it somewhere.

krancour · 2018-08-01T17:38:14Z

@tcnghia thanks. This all checks out for me on minikube. On Azure, I encounter known issues + a new issue of the istio-galley crash looping because of failed health checks. I do not assume that to be a problem with this change though.

krancour · 2018-08-01T18:25:22Z

On Azure, I encounter known issues + a new issue of the istio-galley crash looping

The new issue was user error. So this change checks out on at least minikube and Azure.

krancour · 2018-08-01T18:26:03Z

/approve

scothis · 2018-08-02T15:41:02Z

Manually tested with the eventing e2e tests in knative/eventing#304

/lgtm

ZhiminXiang · 2018-08-02T17:06:02Z

@@ -9,7 +10,6 @@ cd istio-${ISTIO_VERSION}
 helm template --namespace=istio-system \
  --set sidecarInjectorWebhook.enabled=true \
  --set sidecarInjectorWebhook.enableNamespacesByDefault=true \
-  --set global.proxy.image=proxyv2 \


just curious, why do we not need to set "global.proxy.image=proxyv2" ?

The PR is updated with the explanation #1771 (comment)

Good to know. Thanks!

ZhiminXiang · 2018-08-02T17:21:08Z

      containers:
      - name: istio-proxy
-        # PATCH #2:  Add a prestop sleep.


I guess we may still need to keep this PATCH until we fix issue #1370

I think we can't keep that because we don't have control over how the user may install Istio. I leave the bug open to track & fix that issue, which may or may not be there anymore (since it was an issue with Istio 0.6)

(see comment in #1771 (comment))

Good to know. Thanks!

ZhiminXiang · 2018-08-02T17:29:53Z

/lgtm

tcnghia · 2018-08-02T18:01:03Z

@evankanderson can you please /approve? thanks

scothis · 2018-08-02T21:48:58Z

+apiVersion: autoscaling/v2beta1
+kind: HorizontalPodAutoscaler
+metadata:
+    name: istio-pilot


Needs namespace: istio-system

Bummer, but this is upstream https://github.com/istio/istio/blob/8daa232df6ef7cdbb2987de1187255b14e3c3989/install/kubernetes/helm/istio/charts/pilot/templates/autoscale.yaml

(compare that to https://github.com/istio/istio/blob/8daa232df6ef7cdbb2987de1187255b14e3c3989/install/kubernetes/helm/istio/charts/mixer/templates/autoscale.yaml)

I'll file a bug to Istio.

Nice catch, thanks.

What happens if we submit this without patching namespace?

Is this safe to submit, or do we need the upstream to be fixed first? Since this is an HPA, it probably just means that pilot won't scale-out if needed.

(BTW, you linked to the same file twice.)

evankanderson · 2018-08-02T22:11:01Z

I'll approve once @scothis' comment is resolved.

…

On Thu, Aug 2, 2018 at 2:49 PM Scott Andrews ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In third_party/istio-1.0.0/istio.yaml <#1771 (comment)>: > + kind: Deployment + name: istio-telemetry + metrics: + - type: Resource + resource: + name: cpu + targetAverageUtilization: 80 +--- + +--- +# Source: istio/charts/pilot/templates/autoscale.yaml + +apiVersion: autoscaling/v2beta1 +kind: HorizontalPodAutoscaler +metadata: + name: istio-pilot Needs namespace: istio-system — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1771 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlyN6uN4MMhAklAYYm0srEKCY2jfa7Gks5uM3PNgaJpZM4Vpukh> .

-- Evan Anderson <argent@google.com>

evankanderson

I'm assuming that we're planning to change our documentation to say "Install istio via their instructions."?

evankanderson · 2018-08-03T16:26:17Z

+apiVersion: autoscaling/v2beta1
+kind: HorizontalPodAutoscaler
+metadata:
+    name: istio-pilot


What happens if we submit this without patching namespace?

Is this safe to submit, or do we need the upstream to be fixed first? Since this is an HPA, it probably just means that pilot won't scale-out if needed.

(BTW, you linked to the same file twice.)

evankanderson · 2018-08-03T16:27:01Z

/approve

google-prow-robot · 2018-08-03T16:27:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, krancour, tcnghia

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:

~~OWNERS~~ [evankanderson]

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

Use Istio 1.0.0.

7fa6bc5

google-prow-robot assigned evankanderson and ZhiminXiang Aug 1, 2018

google-prow-robot requested review from krancour and scothis August 1, 2018 02:10

google-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 1, 2018

krancour reviewed Aug 1, 2018

View reviewed changes

google-prow-robot assigned scothis Aug 2, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2018

ZhiminXiang reviewed Aug 2, 2018

View reviewed changes

scothis reviewed Aug 2, 2018

View reviewed changes

evankanderson reviewed Aug 3, 2018

View reviewed changes

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2018

google-prow-robot merged commit 3b32b47 into knative:master Aug 3, 2018

Conversation

tcnghia commented Aug 1, 2018

Uh oh!

tcnghia commented Aug 1, 2018

Uh oh!

mattmoor commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krancour commented Aug 1, 2018

Uh oh!

tcnghia commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krancour commented Aug 1, 2018

Uh oh!

tcnghia commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcnghia commented Aug 1, 2018

Uh oh!

krancour commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krancour commented Aug 1, 2018

Uh oh!

krancour commented Aug 1, 2018

Uh oh!

scothis commented Aug 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhiminXiang commented Aug 2, 2018

Uh oh!

tcnghia commented Aug 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson commented Aug 2, 2018 via email

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson commented Aug 3, 2018

Uh oh!

google-prow-robot commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tcnghia commented Aug 1, 2018 •

edited

Loading

tcnghia commented Aug 1, 2018 •

edited

Loading

krancour commented Aug 1, 2018 •

edited

Loading