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.

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

Document use of ambient wrapper chart in 1.23 install/upgrade guides #15279

Closed

Conversation

bleggett
Copy link
Contributor

Description

Fixes #15167

Note that until at least a 1.23 prerelease build is published, the ambient chart will not show up in the documented repo, see istio/release-builder#1849

To test locally in the meantime, you can use the local copies of the chart, so instead of doing:

helm install istio-ambient istio/ambient -n istio-system --create-namespace --wait

like the doc says, do

helm install ambient ./manifests/charts/ambient -n istio-system --create-namespace --wait

(you may need to run helm dep update ./manifests/charts/ambient first to populate the depcache)

I left both the individual and wrapper modes in both docs.

Reviewers

  • Ambient
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure
  • Localization/Translation

@bleggett bleggett requested a review from a team as a code owner June 13, 2024 20:45
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 13, 2024
@bleggett bleggett changed the title Document use of ambient wrapper chart in install/upgrade guides Document use of ambient wrapper chart in 1.23 install/upgrade guides Jun 13, 2024
@@ -28,7 +28,21 @@ we encourage the use of Helm to install Istio for use in ambient mode. Helm help

*See [helm repo](https://helm.sh/docs/helm/helm_repo/) for command documentation.*

## Install the components
{{< gloss "ambient" >}}Ambient{{< /gloss >}} is made up of multiple components - for production deployments, it is strongly recommended to install the components separately to allow for more control during upgrades. Consult the [operations guidelines](/docs/ops/deployment) for production deployment considerations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{< gloss "ambient" >}}Ambient{{< /gloss >}} is made up of multiple components - for production deployments, it is strongly recommended to install the components separately to allow for more control during upgrades. Consult the [operations guidelines](/docs/ops/deployment) for production deployment considerations.
Istio comprises many components (control plane, data plane, custom resources, etc). For production deployments, it is strongly recommended to install the components separately to allow for fine grained control during upgrades. Consult the [operations guidelines](/docs/ops/deployment) for production deployment considerations.

so is sidecar mode. At least three of the charts are shared?

Does the operations docs actually talk about what you should do differently? Is it a scaling suggestion? (You wouldn't scale istio-cni and I doubt you'd scale ztunnel)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so is sidecar mode. At least three of the charts are shared?

Yeah, this is sort of boilerplate. I'll enhance to mention that some components have different impacts on app traffic and node health than others (data plane vs control plane, really) and this impacts upgrade cadence considerations.

Does the operations docs actually talk about what you should do differently? Is it a scaling suggestion? (You wouldn't scale istio-cni and I doubt you'd scale ztunnel)

Scaling, and upgrade cadence.

Really, this should link to

https://preliminary.istio.io/latest/docs/ops/best-practices/deployment/

and that should be built out. I'll do that as part of this.

Copy link
Contributor Author

@bleggett bleggett Jun 17, 2024

Choose a reason for hiding this comment

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

Actually - what I was kinda hoping is that we could just link to wherever this #15247 ends up (under /ops/ or wherever) as the "recommended/opinionated production setup".

maybe even dropping the "individual install" instructions and just having

@therealmitchconnors thoughts?

Copy link
Contributor

@craigbox craigbox Jul 2, 2024

Choose a reason for hiding this comment

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

Started reviewing this a bit further and realised the same thing - we need to treat all of these holistically (and think about how we're going to include this content in the general Istio install/upgrade docs as ambient moves to GA.) I'll stand down, but it would be great to get this chart included in the Getting Started guide (at least for 1.23, if not 1.22)

I don't think I meant that last part

content/en/docs/ambient/install/helm-installation/index.md Outdated Show resolved Hide resolved
content/en/docs/ambient/install/helm-installation/index.md Outdated Show resolved Hide resolved
content/en/docs/ambient/install/helm-installation/index.md Outdated Show resolved Hide resolved
content/en/docs/ambient/install/helm-installation/index.md Outdated Show resolved Hide resolved
content/en/docs/ambient/install/helm-installation/index.md Outdated Show resolved Hide resolved
content/en/docs/ambient/install/helm-installation/index.md Outdated Show resolved Hide resolved
@bleggett bleggett requested a review from a team as a code owner June 17, 2024 23:23
@bleggett
Copy link
Contributor Author

/test gencheck

@bleggett
Copy link
Contributor Author

/test doc.test.profile-default

@craigbox
Copy link
Contributor

craigbox commented Jul 2, 2024

/cc @costinm for opinions per comment

@craigbox
Copy link
Contributor

craigbox commented Jul 2, 2024

/retest-required

@linsun
Copy link
Member

linsun commented Jul 10, 2024

Do we want to have this in get started guide as the wrapper chart is mainly to show how simple it is to install Istio for demo purpose?

Just to recap some discussion from Monday's TOC, unfortunately the attendance is relative low, but folks on the call @therealmitchconnors @keithmattix @howardjohn and myself prefer to have the simple 1 helm install for demo purpose (or consider using helmfile as an alternative if we choose not to publish the wrapper chart). One primary concern is not to show people multiple ways to do Install within 1 install method (which is helm here) and the existing helm install instructions offer more flexibility than the wrapper chart. Please correct me if I am wrong here in capturing the discussion.

@bleggett
Copy link
Contributor Author

bleggett commented Jul 10, 2024

Do we want to have this in get started guide as the wrapper chart is mainly to show how simple it is to install Istio for demo purpose?

👍 I'll make that change, it makes sense.

@howardjohn
Copy link
Member

The other key part of the TOC discussion was to move this chart wrapper under samples/. We can still publish to OCI (and probably should!), but under "samples" in some way (could be gcr.io/istio-release/samples/...)

@bleggett
Copy link
Contributor Author

The other key part of the TOC discussion was to move this chart wrapper under samples/. We can still publish to OCI (and probably should!), but under "samples" in some way (could be gcr.io/istio-release/samples/...)

👍 I assume that's release-builder changes, will do.

@bleggett bleggett force-pushed the bleggett/ambient-helm-wrapperchart branch 2 times, most recently from 77f6e1e to dae5c5e Compare July 15, 2024 23:02
@bleggett
Copy link
Contributor Author

bleggett commented Jul 15, 2024

@linsun @craigbox et al - PTAL:

  • Moved the simple Helm instructions to gettting started
  • Left the full Helm instructions in helm install

tweaked wording and such.

Chart move PR: istio/istio#52013

Chart publish under "samples" PR: istio/release-builder#1901

@bleggett bleggett requested a review from craigbox July 19, 2024 16:48
@bleggett bleggett mentioned this pull request Jul 19, 2024
11 tasks
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 23, 2024
@bleggett bleggett mentioned this pull request Jul 24, 2024
11 tasks
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
@bleggett bleggett force-pushed the bleggett/ambient-helm-wrapperchart branch from 3207e05 to 8f6c1a6 Compare July 25, 2024 17:17
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 25, 2024
@istio-testing
Copy link
Contributor

@bleggett: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
doc.test.profile-ambient_istio.io 8f6c1a6 link true /test doc.test.profile-ambient

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@@ -28,7 +28,21 @@ we encourage the use of Helm to install Istio for use in ambient mode. Helm help

*See [helm repo](https://helm.sh/docs/helm/helm_repo/) for command documentation.*

## Install the components
Istio is made up of multiple components which belong to either the {{< gloss >}}data plane{{< /gloss >}} or {{< gloss >}}control plane{{< /gloss >}}, and different components may have different effects on your applications when upgraded - for production deployments, it is strongly recommended to install the components separately to allow for more control during upgrades. Consult the [operations guidelines](/docs/ops/best-practices/deployment/) for production deployment considerations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Istio is made up of multiple components which belong to either the {{< gloss >}}data plane{{< /gloss >}} or {{< gloss >}}control plane{{< /gloss >}}, and different components may have different effects on your applications when upgraded - for production deployments, it is strongly recommended to install the components separately to allow for more control during upgrades. Consult the [operations guidelines](/docs/ops/best-practices/deployment/) for production deployment considerations.
Istio comprises multiple components in its {{< gloss >}}data plane{{< /gloss >}} and {{< gloss >}}control plane{{< /gloss >}}. Upgrading these components can impact the availability of your applications. For production deployments, it is strongly recommended to install the Istio components separately, which allows you to control the order of upgrades.
Consult the [operations guidelines](/docs/ops/best-practices/deployment/) for production deployment considerations.

## Install the components
Istio is made up of multiple components which belong to either the {{< gloss >}}data plane{{< /gloss >}} or {{< gloss >}}control plane{{< /gloss >}}, and different components may have different effects on your applications when upgraded - for production deployments, it is strongly recommended to install the components separately to allow for more control during upgrades. Consult the [operations guidelines](/docs/ops/best-practices/deployment/) for production deployment considerations.

For installing Istio with support for the {{< gloss >}}ambient{{< /gloss >}} data plane mode in a non-production cluster, we provide a simple Helm chart which bundles all the required Helm charts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For installing Istio with support for the {{< gloss >}}ambient{{< /gloss >}} data plane mode in a non-production cluster, we provide a simple Helm chart which bundles all the required Helm charts.
For installing Istio in a non-production cluster with support for the {{< gloss >}}ambient{{< /gloss >}} data plane mode, we provide a simple Helm chart which bundles all the required component charts together.

@@ -28,7 +28,21 @@ we encourage the use of Helm to install Istio for use in ambient mode. Helm help

*See [helm repo](https://helm.sh/docs/helm/helm_repo/) for command documentation.*

## Install the components
{{< gloss "ambient" >}}Ambient{{< /gloss >}} is made up of multiple components - for production deployments, it is strongly recommended to install the components separately to allow for more control during upgrades. Consult the [operations guidelines](/docs/ops/deployment) for production deployment considerations.
Copy link
Contributor

@craigbox craigbox Jul 2, 2024

Choose a reason for hiding this comment

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

Started reviewing this a bit further and realised the same thing - we need to treat all of these holistically (and think about how we're going to include this content in the general Istio install/upgrade docs as ambient moves to GA.) I'll stand down, but it would be great to get this chart included in the Getting Started guide (at least for 1.23, if not 1.22)

I don't think I meant that last part

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh hello, I don't think we should be using helm in the Getting Started guide. This has to be zero to installed in as quick a time as possible.

I'd love to get updates to the Helm page in before making some updates of my own though. Could we remove this for now and just get the others?

Copy link
Contributor Author

@bleggett bleggett Jul 29, 2024

Choose a reason for hiding this comment

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

This has to be zero to installed in as quick a time as possible.

Yes, that was the intent behind this PR. helm install istio-ambient istio/samples/ambient -n istio-system --create-namespace --wait being analogous to istioctl install --profile=ambient

I was looking at the getting started doc as the simplest possible/most normative set of steps for the non-Istio user (who probably already uses Helm).

I had the wrapper chart in the regular Helm install guide before, and was asked to move it out: #15279 (comment)

The options here are:

  1. Drop the wrapper chart people have asked for, and (IMO) continue to have a needlessly substandard Helm experience, because we don't recommend/test/use/compose Helm charts the way our users do in any capacity.
  2. Put the wrapper chart guide in quickstart (where this PR puts it now)
  3. Put the wrapper chart guide in Helm install (where this PR originally had it, before the TOC asked for it not to be there)

@linsun @howardjohn and @craigbox you all decide, and I'll wait for that.

Copy link
Member

Choose a reason for hiding this comment

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

I think @craigbox's comment was just not to put anything 'helm' in quickstart, likely on a disagreement that its the 'simplest set of steps for the non-Istio user'.

I do tend to agree for a quickstart Helm is likely to only cause friction. istioctl is already something a user will install as part of the getting started (used in other parts), so its a decent choice -- as is kubectl apply which is what we used to do.

I don't think that means we need to drop the wrapper chart, though, we can just say you can install with helm in a aggregated or decomposed manner based on your needs

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a disconnect here between the TOC and the "docs WG" (me), who is asleep when TOC meetings happen. I'm sorry to Ben for it having dragged him in two directions.

The goal of the "Getting Started" page is the easiest possible experience for someone looking at Istio for the first time. We're debating on Slack if this is helm or if it is istioctl, but until now we've assumed it was istioctl. Peter and I have put some effort into improving this recently as unfortunately it is a measure by which the "complexity of Istio" has been reported.

There is also a meme of "Istio has 7 Helm charts", which I am all in favour of resolving. I think that the Helm install page should point out that you can install everything with one chart, or use separate charts, and lay out the case for why you should actually do the more complex thing.

I would like the TOC (or whoever; Environments WG, Ben?) to decide on the order of recommendation for helm and istioctl, and that might dictate the choice in the Getting Started guide.

Copy link
Member

Choose a reason for hiding this comment

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

The question is -- recommendation for what goal?

For a serious production install, where we are optimizing for reliability and safety, Helm is very likely the best choice at this point. istioctl isn't bad or anything, just doesn't integrate with tooling.

For "just give me an env with it deployed so I can try it out", helm is probably not the best...

To put it another way -- when I develop on Istio, I don't use helm since it is tedious to use.

Copy link
Contributor Author

@bleggett bleggett Jul 30, 2024

Choose a reason for hiding this comment

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

To put it another way -- when I develop on Istio, I don't use helm since it is tedious to use.

I don't use it with Istio either because it is tedious to use with Istio for reasons that have to do with Istio - I have and do use it in dev flows for other things just fine - Helm probably has more daily users than Istio does.

But all this brings up a very good point - the people developing Istio day-to-day will shave the edges off of any install mechanism we pick, no matter which is picked, in much the same way that water wears down rocks.

If we don't subject our Helm to that direct daily wear it will continue to be an afterthought and tedious to use. The same is true of istioctl.

I am convinced that the people developing Istio day-to-day (and I am including myself in this) are probably not qualified to make this decision for those reasons, and are too close to the problem to make a good assessment - so I'm gonna defer to TOC/docs WG/community asks.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for weigh in late here, my preference is to keep istioctl for getting started with ambient (same as sidecars), and add the helm wrapper also in getting started as a 2nd tab for existing helm user. That way, if people already has helm and don't want to download istioctl to get started on ambient, they can.

We do see some users asking for helm wrapper chart in ambient slack so folks will likely be using it when evaluating ambient.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the PR again, there are more lines (2X roughly i think) to read for users in the helm instruction with wrapper chart than istioctl... so I'd definitely continue to vote for istioctl. After reading the helm instruction, I lean towards to put helm get started instruction using wrapper chart as a link to be linked from the get started with ambient page.

@craigbox
Copy link
Contributor

Test failure is because the extend-wasm guide uses the Gateway API install docs from the getting started page, which were removed when we went to Helm.

@bleggett
Copy link
Contributor Author

bleggett commented Jul 29, 2024

Test failure is because the extend-wasm guide uses the Gateway API install docs from the getting started page, which were removed when we went to Helm.

Yeah I fixed some of this in #15474 - that one should probably go in before this one, and I'll resync this one pending the outcome of #15279 (comment)

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 30, 2024
@istio-testing
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@linsun
Copy link
Member

linsun commented Jul 31, 2024

FYI, I added this PR to ambient meeting agenda for tmr.

@bleggett
Copy link
Contributor Author

bleggett commented Jul 31, 2024

Aight so current consensus: https://docs.google.com/document/d/1SMlwliEnthgq7r2PjpLl1kCq3t8rAMbgu6r_lDAXJ0w/edit#heading=h.ttmnrtyxr98j

  • we aren't going to document the wrapper chart (gonna close this PR)
  • The wrapper chart is published under samples: Move wrapper to samples (part 2/2) release-builder#1901 but I am not comfortable publishing an artifact we do not document or doctest (which is why this PR was opened) - for now there seems to be a desire to leave it published but not document or doctest it, so I will leave it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ambient area/environments kind/docs needs-rebase Indicates a PR needs to be rebased before being merged size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redo ambient install guides to use ambient wrapper charts for 1.23
6 participants