Allow uneven etcd zones #6641
Allow uneven etcd zones #6641
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @adammw. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
the
size/XS
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
I'm not sure this is a good idea. If I'm not mistaken, etcd operates via quorum. In the case of 3 etcd in 2 AZ, if the AZ hosting 2 instances goes out, then you've lost quorum. I believe the intent of kops HA design is that if an AZ is lost, the cluster doesnt go out. Operating in 2 AZs is not a very HA design. Perhaps an argument which ignores the check would be a better solution. |
|
Etcd can operate stand-alone which it would if the az with two instances fail. Etcd is also capable to run with two nodes, just that the consensus protocol wouldn’t work and thus performance would degrade badly. Personally I see this as quite a good approach to get a more HA etcd without being required to use more AZ |
|
/lgtm It’s a small change so unsure if really suitable for 1.12 still but definitely a thing for 1.13 |
k8s-ci-robot
added
the
lgtm
|
So the problem I was trying to solve here was to create a HA cluster in us-west-1 which only has two AZs available. However, even after solving this issue, I ran into other issues where having multiple etcd members in the same instance group, or having multiple instance groups in the same zone, just doesn't work. |
|
@chrisz100 https://coreos.com/etcd/docs/latest/op-guide/failures.html according to this, in the scenario of 1 AZ going down that has 2 instances, this would be considered a majority failure and cause the cluster to cease performing writes. I personally would not consider this to be an ideal HA scenario. I completely appreciate that in some cases (as in the authors case) it may be the best you have to work with. But I dont feel kops should lower its bar for a good HA design without the user willingly agreeing to it. And it also sounds as if there should be additional work which goes into this PR to make it work correctly in those scenarios to begin with. |
Do you have more information about these issues? Before this is sorted out: |
k8s-ci-robot
removed
the
lgtm
| @@ -157,10 +157,11 @@ func (c *populateClusterSpec) run(clientset simple.Clientset) error { | |||
| //if clusterSubnets[zone] == nil { | |||
| // return fmt.Errorf("EtcdMembers for %q is configured in zone %q, but that is not configured at the k8s-cluster level", etcd.Name, m.Zone) | |||
| //} | |||
| etcdNames[m.Name] = m | |||
There was a problem hiding this comment.
Good fix here - we weren't actually checking for duplicate names!
| etcdInstanceGroups[instanceGroupName] = m | ||
| } | ||
|
|
||
| if (len(etcdInstanceGroups) % 2) == 0 { | ||
| if (len(etcdNames) % 2) == 0 { |
There was a problem hiding this comment.
We could also look at the size of the etcd.Members map, but this is fine..
|
I would classify this as a bug fix, because the code wasn't checking correctly for duplicate names (for example). However, yes, if you're putting 2 of 3 nodes in the same AZ, it's arguably not much more reliable than running non-HA. I'm happy to go either way - on the one hand the previous checks were wrong, on the other hand it was helpfully wrong in that it stopped some configurations that probably weren't ideal. And yes, it should work with multiple etcd members in the same group / AZ (though it gets confusing because they can mount each other's volumes). It sounds like we need a test for that case though... |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve Thanks @adammw 👍 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adammw, KashifSaadat 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Cherry pick of #6641 onto release-1.12
Previously trying to use multiple etcd members on two AZs would result in the "there should be an odd number of master-zones" error, even if there was an odd number of etcd members.
This change fixes the etcdNames check by actually storing the values in the map, and uses that to check that the number of etcd members is an odd number, even if the zones are reused (there is an existing warning for that).