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

docs: refactor AKS installation instructions #23304

Merged
merged 1 commit into from Feb 1, 2023

Conversation

nbusseneau
Copy link
Member

@nbusseneau nbusseneau commented Jan 24, 2023

BYOCNI was initially introduced as the preferred installation method for Cilium on AKS clusters in d8259c1, at the cost of doubling the number of AKS tabs in Getting Started and Helm guides.

Since then:

  • More tabs have been added, making it even more complex to navigate the options.
  • BYOCNI is now GA (Azure CLI version 2.39.0).
  • Azure CNI Powered by Cilium has been announced, further complexifying the Cilium on AKS landscape.

In order to reduce bloat and streamline AKS installation instructions, we refactor the AKS instructions in a single tab. We use this opportunity to strongly encourage users to use BYOCNI, and prepare for Azure IPAM legacy retirement.

Even though we could very add it into the current structure, Azure CNI Powered by Cilium has not been introduced as another installation option here, because the Cilium distribution used in this case is maintained and controlled by AKS, and not by the Cilium community. We felt this was sensible considering there is already a similar situation with GKE's Dataplane V2 and it is not listed in Cilium documentation either.

Full details of edits since the diff is a bit hard to parse:

  • Getting Started Guides:
    • We had 2 separate AKS tabs for creating AKS clusters (one for BYOCNI and one for Azure IPAM), now we have only one AKS tab and it only explains how to create a cluster for BYOCNI. This is the only set of instructions that was removed, and it was done intentionally so as to just silently encourage users that don't have a cluster yet to use BYOCNI.
    • We had 2 separate AKS tabs for installing Cilium in an AKS cluster (one for BYOCNI and one for Azure IPAM) but they actually contained the exact same installation instructions. This is because the Cilium CLI is responsible for automatically detecting which mode to use based on the cluster type. Now we have only one AKS tab with the installation instructions up front, and then sub-tabs for both modes with the rest of the previous info we had (requirements + limitations).
    • So putting the 2 together, if it happens that someone already had an AKS cluster and did not create it with BYOCNI, it'll still work, and if someone actually does want to use Azure IPAM intentionally, they can still figure it out based on the requirements.
  • Helm:
    • We had 2 separate AKS tabs for installing Cilium in an AKS cluster (one for BYOCNI and one for Azure IPAM). Now we have only one AKS tab with sub-tabs for both modes with all the previous info we had (installation instructions + requirements + limitations).
  • Both:
    • BYOCNI is made even more explicit as the preferred option for installing Cilium on AKS, since it's now GA on AKS.
    • Azure IPAM has been re-dubbed Legacy Azure IPAM to double down on that, and also in preparation for the fact we might want to stop maintaining it.

@nbusseneau nbusseneau added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jan 24, 2023
@nbusseneau nbusseneau requested review from a team as code owners January 24, 2023 18:12
@nbusseneau nbusseneau mentioned this pull request Jan 24, 2023
20 tasks
@nbusseneau
Copy link
Member Author

I am adding the backport label for 1.13 just in case it makes sense to have this in before release. If it does not, feel free to remove.

@nbusseneau nbusseneau added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Jan 24, 2023
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks!

@qmonnet
Copy link
Member

qmonnet commented Jan 25, 2023

I am adding the backport label for 1.13 just in case it makes sense to have this in before release.

Backport Criteria for documentation changes

Changes to Cilium’s documentation should generally be subject to backports for all supported branches to which they apply (all supported branches containing the parent features to which the modified sections relate).

The motivation is that users can then simply look at the branch of the documentation related to the version they are deploying, and find the latest correct instructions for their version.

(https://docs.cilium.io/en/latest/contributing/release/backports/#backport-criteria-for-documentation-changes)

So makes sense to me to backport.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good to me, although it's a bit difficult to review and understand what has moved, what has disappeared, and what has simply been reformatted with a smaller paragraph width.

The Azure IPAM instructions are now gone from k8s-install-default.rst, everything else is still here but moved around to other tabs etc., am I correct?

@nbusseneau
Copy link
Member Author

nbusseneau commented Jan 25, 2023

Looks good to me, although it's a bit difficult to review and understand what has moved, what has disappeared, and what has simply been reformatted with a smaller paragraph width.

The Azure IPAM instructions are now gone from k8s-install-default.rst, everything else is still here but moved around to other tabs etc., am I correct?

Sorry about that, such is the bane of indentation-based edits. I gave a high level intent in the commit description but here is a more detailed report of what happened in terms of edits, I will also add this back in the commit description:

  • Getting Started Guides:
    • We had 2 separate AKS tabs for creating AKS clusters (one for BYOCNI and one for Azure IPAM), now we have only one AKS tab and it only explains how to create a cluster for BYOCNI. This is the only set of instructions that was removed, and it was done intentionally so as to just silently encourage users that don't have a cluster yet to use BYOCNI.
    • We had 2 separate AKS tabs for installing Cilium in an AKS cluster (one for BYOCNI and one for Azure IPAM) but they actually contained the exact same installation instructions. This is because the Cilium CLI is responsible for automatically detecting which mode to use based on the cluster type. Now we have only one AKS tab with the installation instructions up front, and then sub-tabs for both modes with the rest of the previous info we had (requirements + limitations).
    • So putting the 2 together, if it happens that someone already had an AKS cluster and did not create it with BYOCNI, it'll still work, and if someone actually does want to use Azure IPAM intentionally, they can still figure it out based on the requirements.
  • Helm:
    • We had 2 separate AKS tabs for installing Cilium in an AKS cluster (one for BYOCNI and one for Azure IPAM). Now we have only one AKS tab with sub-tabs for both modes with all the previous info we had (installation instructions + requirements + limitations).
  • Both:
    • BYOCNI is made even more explicit as the preferred option for installing Cilium on AKS, since it's now GA on AKS.
    • Azure IPAM has been re-dubbed Legacy Azure IPAM to double down on that, and also in preparation for the fact we might want to stop maintaining it.

BYOCNI was initially introduced as the preferred installation method for
Cilium on AKS clusters in d8259c1, at
the cost of doubling the number of AKS tabs in Getting Started and Helm
guides.

Since then:

- More tabs have been added, making it even more complex to navigate the
  options.
- BYOCNI is now GA (Azure CLI version 2.39.0).
- [Azure CNI Powered by
  Cilium](https://learn.microsoft.com/en-us/azure/aks/azure-cni-powered-by-cilium)
  has been announced, further complexifying the Cilium on AKS landscape.

In order to reduce bloat and streamline AKS installation instructions,
we refactor the AKS instructions in a single tab. We use this
opportunity to strongly encourage users to use BYOCNI, and prepare for
Azure IPAM legacy retirement.

Even though we could very add it into the current structure, Azure CNI
Powered by Cilium has not been introduced as another installation option
here, because the Cilium distribution used in this case is maintained
and controlled by AKS, and not by the Cilium community. We felt this was
sensible considering there is already a similar situation with GKE's
Dataplane V2 and it is not listed in Cilium documentation either.

[
Full details of edits since the diff is a bit hard to parse:

- Getting Started Guides:
  - We had 2 separate AKS tabs for creating AKS clusters (one for BYOCNI
    and one for Azure IPAM), now we have only one AKS tab and it only
    explains how to create a cluster for BYOCNI. This is the only set of
    instructions that was removed, and it was done intentionally so as
    to just silently encourage users that don't have a cluster yet to
    use BYOCNI.
  - We had 2 separate AKS tabs for installing Cilium in an AKS cluster
    (one for BYOCNI and one for Azure IPAM) but they actually contained
    the exact same installation instructions. This is because the Cilium
    CLI is responsible for automatically detecting which mode to use
    based on the cluster type. Now we have only one AKS tab with the
    installation instructions up front, and then sub-tabs for both modes
    with the rest of the previous info we had (requirements +
    limitations).
  - So putting the 2 together, if it happens that someone already had an
    AKS cluster and did not create it with BYOCNI, it'll still work, and
    if someone actually does want to use Azure IPAM intentionally, they
    can still figure it out based on the requirements.
- Helm:
  - We had 2 separate AKS tabs for installing Cilium in an AKS cluster
    (one for BYOCNI and one for Azure IPAM). Now we have only one AKS
    tab with sub-tabs for both modes with all the previous info we had
    (installation instructions + requirements + limitations).
- Both:
  - BYOCNI is made even more explicit as the preferred option for
    installing Cilium on AKS, since it's now GA on AKS.
  - Azure IPAM has been re-dubbed Legacy Azure IPAM to double down on
    that, and also in preparation for the fact we might want to stop
    maintaining it.
]

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@pchaigno
Copy link
Member

pchaigno commented Feb 1, 2023

Sorry about that, such is the bane of indentation-based edits. I gave a high level intent in the commit description but here is a more detailed report of what happened in terms of edits, I will also add this back in the commit description:

That's where having more than one commit usually helps.

@nbusseneau
Copy link
Member Author

nbusseneau commented Feb 1, 2023

That's where having more than one commit usually helps.

In this case we would end up with a first commit where changes are done but the doc does not compile (due to indentation) and lines go over the line width, and then a second one to fix both. I personally don't like it much :/

@pchaigno
Copy link
Member

pchaigno commented Feb 1, 2023

That's where having more than one commit usually helps.

In this case we would end up with a first commit where changes are done but the doc does not compile (due to indentation) and lines go over the line width, and then a second one to fix both. I personally don't like it much :/

I would prefer it over bad reviewing experience. I doubt we often need to bisect on docs changes so temporarily breaking the lint checks there is probably okay.

@pchaigno
Copy link
Member

pchaigno commented Feb 1, 2023

Merging given reviews and passing docs test.

@pchaigno pchaigno merged commit 26909e7 into cilium:master Feb 1, 2023
@pchaigno pchaigno mentioned this pull request Feb 12, 2023
29 tasks
@pchaigno pchaigno added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Feb 12, 2023
@aanm aanm added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants