-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add documentation for Helm deployment #875
Conversation
@sbezverk would you mind reviewing. There are a couple spots where the documentation has NEEDS REVIEW which could use a one liner description. |
_docs/setup/kubernetes/helm.md
Outdated
@@ -0,0 +1,76 @@ | |||
--- | |||
title: Alpha Quality Helm Chart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't normally put Alpha in doc... the status should be tracked in feature stage doc to be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
_docs/setup/kubernetes/helm.md
Outdated
1. Create the Helm chart: | ||
```bash | ||
helm install install/kuberentes/istio --name istio | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo on kubernetes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, still kuberenetes
_docs/setup/kubernetes/helm.md
Outdated
## Prerequisites | ||
|
||
The following instructions require you have access to Helm **2.7.2 or newer** in your Kubernetes environment or | ||
alternately the ability to modify RBAC rules required to install Helm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us refer to k8s version in the setup doc too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
_docs/setup/kubernetes/helm.md
Outdated
| istio.ca_demo | true/false | false | NEEDS REVIEW | | ||
| istio.ingress.use_nodeport | true/false | false | Specifies whether to use nodeport or LB | | ||
| istio.ingress.nodeport_port | 1024 to 65535 | 32000 | If nodeport is used, specifies its port | | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you update this? This was changed yesterday to simplify things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated. Agree much simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on document this, please see some minor comments I put in.
@linsun Thanks for the review. I'm sort of consumed today and tomorrow. Will produce the requested changes Monday. By feature tracking, do you mean this document? https://docs.google.com/spreadsheets/d/1mUIrX82Z8mRuhNWyl5D_sd_Evg3KzeNotzRorPzQMko/edit?ts=5a03f88d#gid=351491887 Cheers |
I believe lin meant https://istio.io/about/feature-stages.html |
@ldemailly thanks - will address that in the next rev. |
that page itself being quite sensitive - I would just focus on getting working helm instructions and not so much about alpha status or updating the feature stage in this PR - that can be later I think |
Thank you @sdake and @ldemailly ! The plan sounds good. |
dd14d06
to
6bab1f2
Compare
This patch should be good to go. I am aware of istio/istio#2734, however, I had committed to @costinm to have these documentation changes in around the release of 0.4.0 (~3 weeks ago). With all of the refactoring related to Helm, it was difficult to find a good point at which to merge these changes. I think the above PR will take some time to sort out and will commit to revising this documentation when istio/istio#2734 merges. Cheers |
@linsun would you mind merging this? Cheers |
_docs/setup/kubernetes/helm.md
Outdated
title: Istio Helm Chart Instructions | ||
overview: Instructions for the setup and configuration of Istio using the Helm package manager. | ||
|
||
order: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order should not be the same as quickstart
can you attach a preview of the toc/nav once fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! Taught me something new.
_docs/setup/kubernetes/helm.md
Outdated
| global.pilot.tag | image tag | release unique hash | Specifies the TAG for the pilot image | | ||
| global.pilot.enabled | true/false | true | Specifies whether pilot is enabled/disabled | | ||
| istio.ca.hub | registry+namespace | release registry/namespace | Specifies the HUB for the ca image | | ||
| istio.ca.tag | image tag | release unique hash | Specifies the TAG for the ca image | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still have these two helm vars? think they are renamed to global.security.hub and tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes. Approve pending address the comments.
@linsun @ldemailly thanks for the quick reviews. I'll take a look today and push an update on security (if that has changed) and attach a nav page this evening. Cheers |
@ldemailly is there a place we can download a daily build? would be nice to validate the packaging is good in a pre-0.5 build. |
@linsun yes http://gcsweb.istio.io/gcs/istio-prerelease/daily-build/ that one may not yet have istioctl fixed, so I asked the team to kick a new daily you need to use the TEST tgz |
I noticed one small error after testing the instructions after rendering them (thanks @ldemailly for the hint :). I'll push an additional patch to fix that problem @6pm PST today. Please don't merge until a 1 liner update comes. Cheers |
_docs/setup/kubernetes/helm.md
Outdated
|
||
1. Create the Helm chart: | ||
```bash | ||
helm install install/kuberenetes/helm/istio --name istio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping ^ kuberenetes
can you try the command line too / does it work :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A sign I need to go to bed :-) - I have run this workflow about 10-15 times in the last 2 days, however, I was manually typing and using completion. I'll make the typo change, render the docs, and C&P the docs into a log - in the morning :) Thanks for the patience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use the "copy" button from the rendered version and paste in terminal to be sure it's the right command :-)
thx !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup - my 10-15 runs were for training purposes - so a CP isn't ideal in that model.
Looks like istio/istio#2734 merged sooner than I expected. I will need to change the table in this document considerably, so will need a bit more time on the docs. Should be able to wrap them up today however. Cheers |
This review is blocked on sorting out the Helm charts for the 0.5.0 release. I'll ping reviewers when it has been updated to match the new canonical version. |
/hold |
This adds documentation along with related configuration options for deploying Istio with the Helm charts.
Fix a typo in the install command and warning clause about automatic sidecar injection not being implemented.
@linsun My plan here is to merge these as is (since these instructions render properly and work today) and follow up with an update to the table as a separate patch. Could you merge, and I'll start working on the updated table contents as well as integrating the README.md in the istio repository related to Helm? Cheers |
This adds documentation along with related configuration options for
deploying Istio with the Helm charts.