Add option to use etcd managed by cilium-etcd-operator as kvstore #8629

olemarkus · 2020-02-25T18:17:19Z

This PR adds the possibility to use etcd as kvstore for cilium agent state. See https://docs.cilium.io/en/v1.7/gettingstarted/k8s-install-etcd-operator/ for the details.

Fixes #8465

Note that coredns 1.6+ currently has a bug that prevents etcd-operator from creating clusters, so if one wants to test this, downgrade coredns first. See coredns/coredns#3686

k8s-ci-robot · 2020-02-25T18:17:30Z

Hi @olemarkus. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

hakman · 2020-02-25T18:31:15Z

/ok-to-test

johngmyers · 2020-03-02T05:12:56Z

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template

 hostNetwork: true
+{{- if .EtcdManaged }}
+ # In managed etcd mode, Cilium must be able to resolve the DNS name of
+ # the etcd service


Did you forget to set the dnsPolicy here?

No, I removed that part on purpose. Changing the DNS policy breaks cilium because it can no longer look up the internal cluster DNS (since we use the public DNS entry for that). The change on line 14 ( "etcd.operator": "true") removes the need to change the DNS policy.
That particular change hasn't made it into the cilium helm templates yet, if you are comparing to what that one outputs.

You have a conditional wrapped around nothing but comments here. Perhaps remove the conditional and comments as well?

A thanks. I was certain I removed that one. 🤦‍♂️

johngmyers · 2020-03-02T05:18:55Z

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template

+ image: "cilium/cilium-etcd-operator:v2.0.7"
+ name: cilium-etcd-operator
+ dnsPolicy: ClusterFirst
+ hostNetwork: true


priorityClassName: system-cluster-critical

Not 100% sure this should qualify for system-cluster-critical. It technically isn't very critical even though the pods it spawns is critical.

If it gets preempted then it can't repair any problems with the Cilium etcds, such as their losing quorum.

johngmyers · 2020-03-02T05:28:08Z

Does cilium-etcd-operator run the etcd pods with host network? We forked etcd-operator to include coreos/etcd-operator#2094 and suspect we'll have to fork cilium-etcd-operator too.

Also, does cilium-etcd-operator run the etcd pods with the system-cluster-critical priority class?

Since etcd-operator appears to be unmaintained, I'm wondering if it would be practical to add a Cilium mode to etcd-manager.

olemarkus · 2020-03-02T06:51:01Z

Does cilium-etcd-operator run the etcd pods with host network? We forked etcd-operator to include coreos/etcd-operator#2094 and suspect we'll have to fork cilium-etcd-operator too.

No, it doens't. It is unclear to me why that would be necessary.

Also, does cilium-etcd-operator run the etcd pods with the system-cluster-critical priority class?

No, I and I am not sure it is correct to do so either. The consequence of losing etcd is that state propagation will be slower, but it won't lead to immediate critical failure

Since etcd-operator appears to be unmaintained, I'm wondering if it would be practical to add a Cilium mode to etcd-manager.

That could be interesting. Everything that talks to etcd uses hostNetworking, so etcd-manager's etcd should be reachable too. It would certainly the concerns above. Would be a pretty big change though.

johngmyers · 2020-03-02T07:13:09Z

With hundreds of nodes, "slower" may well be "completely unstable". If someone doesn't need etcd for state propagation it's likely they wouldn't enable it.

I suppose in the crd identity allocation mode an agent could bootstrap off of the apiserver, so running etcd on host network wouldn't be as important as for kvstore mode. It might even be able to do so without killing the apiserver when in a large cluster.

johngmyers · 2020-03-02T07:19:05Z

Shouldn't we bump the version in bootstrapchannelbuilder.go?

olemarkus · 2020-03-02T07:24:38Z

Cilium's stance on this is still that larger environments should use external etcd (https://docs.cilium.io/en/v1.7/gettingstarted/k8s-install-external-etcd/)
I think etcd-manager would qualify as that better be pretty damn stable.

Forking cilium-etcd-operator/etcd-operator etc isn't an option for me at least.

olemarkus · 2020-03-02T07:29:11Z

Shouldn't we bump the version in bootstrapchannelbuilder.go?

Yep. bumped.

johngmyers · 2020-03-02T07:45:24Z

External etcd would be expensive to manage. We run in-cluster etcd on host network with forked etcd-operator. When we are able to upgrade to 1.6 we'll look at crd identity allocation mode with etcd for propagation, assuming we're willing to stay under 250 nodes.

Elsewhere they've mentioned that pure crd is only good for up to 50 nodes.

Host network is not a blocker for this PR. I'm trying to decide whether I want to hold out for priorityClassName on cilium-etcd-operator. If cilium-etcd-operator happens to not put them on the etcd pods then it'd be a moot point unless/until that could be fixed upstream.

olemarkus · 2020-03-02T08:08:12Z

I think we should push this in without and then I can see if I can push cilium to propagate the priority class.

I agree that external etcd is too expensive, but I want to see how hard it is to add another etcd-manager cluster. That will have to come later though

johngmyers · 2020-03-02T15:00:06Z

/lgtm
/retest

johngmyers · 2020-03-02T16:04:52Z

/retest

johngmyers · 2020-03-02T16:33:43Z

/retest

johngmyers · 2020-03-03T16:32:48Z

It looks like cilium/cilium-etcd-operator#67 is going to be a significant defect now that kops rolling update can drain nodes in parallel.

olemarkus · 2020-03-03T18:02:36Z

Yep. I think I am going to have a look at etcd-manager sooner than I thought.

johngmyers · 2020-03-14T03:48:03Z

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template

+ path: etcd.config
+ name: cilium-config
+ name: etcd-config-path
+ # To read the k8s etcd secrets in case the user might want to use TLS


These are the Cilium etcd secrets, not the k8s ones, right?

Yes. etcd-operator creates the secret cilium-etcd-secret.

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template

…1.12.yaml.template Co-Authored-By: John Gardiner Myers <[email protected]>

johngmyers · 2020-03-14T23:20:18Z

/lgtm

rifelpet · 2020-03-14T23:41:05Z

/approve

k8s-ci-robot · 2020-03-14T23:41:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, rifelpet

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:

~~OWNERS~~ [rifelpet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 25, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 25, 2020

k8s-ci-robot requested review from joshbranham and robinpercy February 25, 2020 18:19

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 25, 2020

olemarkus force-pushed the cilium-etcd-operator branch from 88ab2b1 to 641ab56 Compare February 25, 2020 18:33

johngmyers requested changes Mar 2, 2020

View reviewed changes

k8s-ci-robot assigned johngmyers Mar 2, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2020

Ole Markus With added 3 commits March 13, 2020 20:17

Add option to use etcd managed by cilium-etcd-operator as kvstore

2274045

Remove conditional with only comments

e5ecce6

Bump cilium addon version

4c5bef8

olemarkus force-pushed the cilium-etcd-operator branch from d827401 to 4c5bef8 Compare March 13, 2020 19:18

k8s-ci-robot added area/api and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 13, 2020

johngmyers reviewed Mar 14, 2020

View reviewed changes

johngmyers requested changes Mar 14, 2020

View reviewed changes

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template Outdated Show resolved Hide resolved

Update upup/models/cloudup/resources/addons/networking.cilium.io/k8s-…

0c27aa3

…1.12.yaml.template Co-Authored-By: John Gardiner Myers <[email protected]>

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2020

k8s-ci-robot merged commit 8860040 into kubernetes:master Mar 15, 2020

k8s-ci-robot added this to the v1.18 milestone Mar 15, 2020

olemarkus deleted the cilium-etcd-operator branch March 25, 2020 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to use etcd managed by cilium-etcd-operator as kvstore #8629

Add option to use etcd managed by cilium-etcd-operator as kvstore #8629

olemarkus commented Feb 25, 2020

k8s-ci-robot commented Feb 25, 2020

hakman commented Feb 25, 2020

johngmyers Mar 2, 2020

olemarkus Mar 2, 2020

johngmyers Mar 2, 2020

olemarkus Mar 2, 2020

johngmyers Mar 2, 2020

olemarkus Mar 2, 2020

johngmyers Mar 2, 2020

johngmyers commented Mar 2, 2020

olemarkus commented Mar 2, 2020

johngmyers commented Mar 2, 2020

johngmyers commented Mar 2, 2020

olemarkus commented Mar 2, 2020

olemarkus commented Mar 2, 2020

johngmyers commented Mar 2, 2020

olemarkus commented Mar 2, 2020

johngmyers commented Mar 2, 2020

johngmyers commented Mar 2, 2020

johngmyers commented Mar 2, 2020

johngmyers commented Mar 3, 2020

olemarkus commented Mar 3, 2020

johngmyers Mar 14, 2020

olemarkus Mar 14, 2020

johngmyers commented Mar 14, 2020

rifelpet commented Mar 14, 2020

k8s-ci-robot commented Mar 14, 2020

Add option to use etcd managed by cilium-etcd-operator as kvstore #8629

Add option to use etcd managed by cilium-etcd-operator as kvstore #8629

Conversation

olemarkus commented Feb 25, 2020

k8s-ci-robot commented Feb 25, 2020

hakman commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers commented Mar 2, 2020

olemarkus commented Mar 2, 2020

johngmyers commented Mar 2, 2020

johngmyers commented Mar 2, 2020

olemarkus commented Mar 2, 2020

olemarkus commented Mar 2, 2020

johngmyers commented Mar 2, 2020

olemarkus commented Mar 2, 2020

johngmyers commented Mar 2, 2020

johngmyers commented Mar 2, 2020

johngmyers commented Mar 2, 2020

johngmyers commented Mar 3, 2020

olemarkus commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers commented Mar 14, 2020

rifelpet commented Mar 14, 2020

k8s-ci-robot commented Mar 14, 2020