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

Improve Cilium integration with managed Kubernetes providers #16631

Merged
merged 5 commits into from Jul 1, 2021

Conversation

aanm
Copy link
Member

@aanm aanm commented Jun 23, 2021

This PR introduces steps on how to install Cilium in cloud providers. The reason behind this PR is described in detail in #16602. Initially, the changes in this PR were supposed to be a few but the commit "pkg/k8s: replace GetNode instances with a k8s store" is necessary to make sure nodes that have the taint node.cilium.io/agent-not-ready set after Cilium started, will also have that taint removed without restart Cilium, by watching for Node events.

Marked this PR as needs-backport/1.10 as it tremendously improves the deployment of Cilium in cloud providers.

It should be easier to review the changes on a per-commit basis.

Provide new installation steps to deploy Cilium in managed kubernetes providers (GKE, EKS, AKS) to allow scale up and down node pools.

Fixes #7404
Fixes #16602
Fixes #15177
Fixes #16542

⚠️ Note for reviewers, this PR was ran with a test commit to test the new changes into the conformance GH workflows. All of them have passed except multicluster which is currently broken on master.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 23, 2021
@aanm aanm added the release-note/major This PR introduces major new functionality to Cilium. label Jun 23, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 23, 2021
@aanm aanm force-pushed the pr/fix-clouds branch 3 times, most recently from 75481c4 to 8c0df17 Compare June 23, 2021 04:00
@aanm
Copy link
Member Author

aanm commented Jun 23, 2021

test-me-please

@aanm aanm force-pushed the pr/fix-clouds branch 2 times, most recently from 3968932 to 4e4db06 Compare June 23, 2021 14:01
@aanm
Copy link
Member Author

aanm commented Jun 23, 2021

test-runtime

@aanm
Copy link
Member Author

aanm commented Jun 23, 2021

test-runtime

1 similar comment
@aanm
Copy link
Member Author

aanm commented Jun 23, 2021

test-runtime

@aanm
Copy link
Member Author

aanm commented Jun 24, 2021

test-runtime

@aanm
Copy link
Member Author

aanm commented Jun 24, 2021

test-me-please

@aanm aanm force-pushed the pr/fix-clouds branch 4 times, most recently from b34e3c5 to 8403809 Compare June 25, 2021 00:40
@aanm aanm closed this Jun 25, 2021
@aanm aanm reopened this Jun 25, 2021
aanm added 3 commits July 1, 2021 22:48
As Cilium will remove the node taint
'node.cilium.io/agent-not-ready=true:NoSchedule' once it is up and
ready, the documentation has all the necessary steps for users to create
clusters using that taint. Having nodes created with this taint will
prevent pods from being scheduled into those nodes until Cilium had
configured the node where it's being deployed.

Signed-off-by: André Martins <andre@cilium.io>
To replicate the same steps as users do, GH workflows will now create
clusters with node taints for which Cilium will remove them once it's
ready in that node.

Signed-off-by: André Martins <andre@cilium.io>
With a node taint setup on node creation, users will no longer be
required to restart application pods since application pods will only
start when Cilium is deployed and running in the cluster.

Signed-off-by: André Martins <andre@cilium.io>
pkg/k8s/node.go Show resolved Hide resolved
@aanm aanm merged commit 703b38f into master Jul 1, 2021
@aanm aanm deleted the pr/fix-clouds branch July 1, 2021 22:41
@thejosephstevens
Copy link
Contributor

I'm holding off on deep diving in the workflow changes as I have a bit of a high-level concern to address first: I feel like this solution, while neat, feels like a complete kludge from a user perspective.
I feel like it complicates cluster setup a lot, especially on AKS and EKS where nodepools are cumbersome to manage, while also restricting the options available for setting up the clusters (e.g. the load balancer choices on AKS).
Are we really OK with these drawbacks? Do we want this to be the default Cilium installation experience?

I see it as an unfortunate price to pay. Right now the AKS integration is broken and although usability is worse with this PR, it guarantees that there won't be any Cilium issues in it. Once we have a better integration in AKS, and / or this is fixed Azure/aks-engine#4476 the usability will improve.

I would advocate for having this as an optional thing for advanced users, and not the default installation. In particular, the simple getting started guide suddenly becomes a whole lot more complex :/

The issue I see with it is that people assume the GSG is enough to run in production, which kind of should it be, and if they only follow the "simple" GSG, they might face the issues we are trying to avoid with this PR.

Speaking as a user of AKS and Cilium, I think @aanm 's assessment here aligns with what I look for as an operator. We've had to deal with weird operational issues around the default pool that they force you to launch for as long as we've been on AKS (almost two years now), so having to work around it while setting up a CNI is not a big surprise.

It's a lot more important to me that I can follow the GSG and get a prod-ready cluster, we already had to build tooling to deal with the managed cluster bootstrap anyway.

@aanm aanm added this to Needs backport from master in 1.10.3 Jul 2, 2021
@aanm aanm removed this from Needs backport from master in 1.10.2 Jul 2, 2021
@smnmtzgr
Copy link

smnmtzgr commented Jul 7, 2021

@aanm / @thejosephstevens / @nbusseneau / @christarazi

We are also users of AKS and Cilium together.

In my opinion this change makes it very "hard" to use Cilium with AKS. It will also become very hard to update Cilium (or get the backport for 1.10) within an already running AKS-cluster. Will there be a documentation-part on how to do this? We deploy AKS-clusters using terraform. This workaround will be very cumbersome to implement with terraform.

In my opinion improving the STARTUP_SCRIPT would have been the better method/thing to fix these issues because it doesnt break so much in the deployment/operation tasks the operators have. E.g. #16356 for AKS.

The best solution would be a better integration of Cilium into the cloud providers. For example it should be possible to deploy AKS Clusters with CNI=None. So that one can choose a "custom" CNI like Cilium which will be installed/used. So no problems would occur with Azure-CNI being active before Cilium is ready. E.g. like suggested here: Azure/AKS#2092
Sure, thats something you would have to discuss with the cloud providers, but it would be the best one.

In my eyes this big change/merge/backport is not really a good thing for us operators. Sure it makes Cilium more stable on cloud providers managed kubernetes offerings. But the price to pay is too high. As long as there is no direct integration into the cloud provider investigating time into the STARTUP_SCRIPT and make it a lot more fail-safe would have been the better way in my eyes.

@aanm with Azure/aks-engine#4476 you refer to "aks-engine". I think you should open the issue here: https://github.com/Azure/AKS/issues ... aks-engine is not the "managed AKS solution".

@mattstam
Copy link

mattstam commented Apr 5, 2022

This should be far easier to set up on AKS now: https://docs.microsoft.com/en-us/azure/aks/use-byo-cni?tabs=azure-cli and won't need the node taint hack.

@thejosephstevens
Copy link
Contributor

This should be far easier to set up on AKS now: https://docs.microsoft.com/en-us/azure/aks/use-byo-cni?tabs=azure-cli and won't need the node taint hack.

How does this avoid needing the node taint? As best I can tell AKS is still expecting you to deploy a daemonset for the CNI so you would still have race conditions for scheduling on node scaleup.

@aanm
Copy link
Member Author

aanm commented Apr 23, 2022

FYI @mattstam @thejosephstevens the documentation for the BYO CNI is being done in #19379

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. release-note/major This PR introduces major new functionality to Cilium. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
No open projects
1.10.3
Backport done to v1.10