Istio updates #1314
Istio updates #1314
Conversation
samodell
added
the
do-not-merge/work-in-progress
knative-prow-robot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: samodell 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 |
knative-prow-robot
added
size/L
samodell
left a comment
samodell
left a comment
There was a problem hiding this comment.
Questions for @ZhiminXiang and @RichieEscarez
docs/install/installing-istio.md
Outdated
| Wait a few seconds for the CRDs to be committed in the Kubernetes API-server, | ||
| then continue with these instructions. | ||
|
|
||
| 1. Enter the following command to install Istio: |
There was a problem hiding this comment.
Is this the basic Istio installation we should recommend?
Bonus points -- is there a way to do our "basic" installation without using helm?
There was a problem hiding this comment.
- The default command is exactly the same as the command in the section
Installing Istio with sidecar injection. We should probably keep only one of them. - We probably want to recommend users to use the instruction
installing Istio with no sidecar injectionas the default instructions. @tcnghia @mattmoor WDYT?
There was a problem hiding this comment.
Updated to recommend no sidecar injection.
There was a problem hiding this comment.
And updated to go back to recommending automatic sidecar injection as the default, since I found info supporting that in the Custom Install guide, which I believe was reviewed by Matt and Nghia.
knative-prow-robot
added
size/XL
|
Opened #1319 to track additional work needed to update a few of the Install files which were doing unique things during the Istio install. Otherwise, I think this is pretty good to go. |
docs/install/installing-istio.md
Outdated
| If you are just getting started with Knative, you should choose automatic | ||
| sidecar injection and enable the Istio service mesh. | ||
|
|
||
| Due to current dependencies, some installable Knative options require the Istio |
There was a problem hiding this comment.
Are lines 51-53 still true? If they are, I would lean towards installing Istio with automatic sidecar injection as the default, since the majority of the install instructions right now tell people to install all the components as well as the observability plug-ins. I don't want to recommend a default that doesn't work for a lot of use cases...
There was a problem hiding this comment.
There was a problem hiding this comment.
@grantr
I guess in eventing side we are working on getting rid of mesh. I think Grant probably knows better about this part.
There was a problem hiding this comment.
Eventing 0.6 no longer requires Istio. 🎉 @akashrv worked on this, so he knows the most about it.
There was a problem hiding this comment.
That is correct. Eventing doesn't require Istio, but would continue to work with Istio mesh.
|
Okay, @ZhiminXiang I switched the recommendation back to installing without sidecar injection. Please let me know what you think and LGTM if you're happy with it! |
docs/install/installing-istio.md
Outdated
|
|
||
| When you install Istio, there are a few options depending on your goals. For a | ||
| basic Istio installation suitable for most Knative use cases, follow the | ||
| [Installing Istio with sidecar injection](#installing-istio-with-sidecar-injection) |
There was a problem hiding this comment.
nit: I think here it should be Installing Istio without sidecar injection
|
Thanks @samodell . Generally it looks good to me. Just one nit. |
|
/lgtm |
7 participants
Fixes #1244
Proposed Changes