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

Proposal to support multi-node etcd clusters #115

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

amshuman-kr
Copy link
Collaborator

@amshuman-kr amshuman-kr commented Nov 11, 2020

What this PR does / why we need it:
Proposal to support multi-node etcd clusters.

Credits:
@abdasgupta @shreyas-s-rao for extensive discussions and content.
@swapnilgm for initial discussions and first draft of implementation.
@istvanballok for many interesting ideas, especially, tmpfs.
@vlerenc for many interesting inputs, including (but not limited to 🙂) ephemeral persistence and etcd container and backup-restore sidecar interaction.

Which issue(s) this PR fixes:

Special notes for your reviewer:
This is the proposal part of #107. The implementation will be done in further PRs.
Please find the human-friendly rendering of the document here (note: point in time).

Release note:

NONE

@amshuman-kr amshuman-kr requested a review from a team as a code owner November 11, 2020 05:03
@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Nov 11, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 11, 2020
@abdasgupta
Copy link
Contributor

LGTM

@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@amshuman-kr amshuman-kr mentioned this pull request Nov 11, 2020
34 tasks
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2020
@dguendisch
Copy link
Member

Very nice and thorough write up, thanks!
I have two questions:

  1. On backup failure, is it required to fail the readiness probe, or could ectd as well be put in some read-only mode? Rationale would be to "only" fail writes, but still allow for reads.

  2. Out of interest: do you have experience how fast leader fail-over performs on etcd? From own experience I know that e.g. MongoDB is super fast (new primary elected in <5secs after old primary breaks), while e.g. Elasticsearch is super slow (often up to one 1min). For Elasticsearch thus very careful tuning of readinessProbe, initialDelaySeconds, and so on is necessary if a rolling update should be somewhat HA. And I guess etcd will not write without a leader, i.e. during a new election?

@amshuman-kr
Copy link
Collaborator Author

amshuman-kr commented Nov 11, 2020

  • On backup failure, is it required to fail the readiness probe, or could ectd as well be put in some read-only mode? Rationale would be to "only" fail writes, but still allow for reads.

@dguendisch Very interesting idea. I hadn't thought of that. I just checked the etcd configuration and unfortunately, there doesn't seem to be any explicit configuration that can put etcd clusters into read-only mode short of losing quorum. Forcibly losing quorum on backup failure sounds too drastic.

Out of interest: do you have experience how fast leader fail-over performs on etcd? From own experience I know that e.g. MongoDB is super fast (new primary elected in <5secs after old primary breaks), while e.g. Elasticsearch is super slow (often up to one 1min). For Elasticsearch thus very careful tuning of readinessProbe, initialDelaySeconds, and so on is necessary if a rolling update should be somewhat HA. And I guess etcd will not write without a leader, i.e. during a new election?

I am afraid I do not have too much experience. Only from local playing around, I would say leader election is quite fast. The heartbeats are 100ms and default leader election timeout is 1000ms according to the documentation. https://etcd.io/docs/v3.4.0/tuning/#time-parameters. That is quite fast.

Also, during leader election the writes are queued instead of rejected. So, writes experience more latency at worst. They may timeout, of course. https://etcd.io/docs/v3.1.12/op-guide/failures/#leader-failure.

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 17, 2020
Copy link
Member

@vlerenc vlerenc left a comment

Choose a reason for hiding this comment

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

Awesome, thank you very much, very helpful. I have only a few nitpicks and proposals, but otherwise I found it very enlightening.

docs/proposals/multi-node/README.md Outdated Show resolved Hide resolved
docs/proposals/multi-node/README.md Show resolved Hide resolved
docs/proposals/multi-node/README.md Show resolved Hide resolved
docs/proposals/multi-node/README.md Outdated Show resolved Hide resolved
docs/proposals/multi-node/README.md Outdated Show resolved Hide resolved
docs/proposals/multi-node/README.md Outdated Show resolved Hide resolved
docs/proposals/multi-node/README.md Outdated Show resolved Hide resolved
docs/proposals/multi-node/README.md Outdated Show resolved Hide resolved
docs/proposals/multi-node/README.md Outdated Show resolved Hide resolved
docs/proposals/multi-node/README.md Show resolved Hide resolved
@gardener-robot gardener-robot removed the needs/review Needs review label Nov 17, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 25, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 25, 2020
@amshuman-kr
Copy link
Collaborator Author

@vlerenc Thanks a lot for the suggestions! I have applied almost all of them 🙂

@amshuman-kr amshuman-kr force-pushed the multi-node-proposal branch from 570f460 to dd7852f Compare March 9, 2021 09:05
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 9, 2021
@amshuman-kr amshuman-kr force-pushed the multi-node-proposal branch from dd7852f to e06139c Compare March 9, 2021 09:19
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 9, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 9, 2021
@amshuman-kr
Copy link
Collaborator Author

I have addressed the review comments so far. I will merge this PR now. If there are more concerns, please do raise an issue :-)

@amshuman-kr amshuman-kr merged commit a3fdf13 into gardener:master Mar 9, 2021
@amshuman-kr amshuman-kr deleted the multi-node-proposal branch March 9, 2021 09:23
@amshuman-kr amshuman-kr restored the multi-node-proposal branch March 9, 2021 09:23
@ashwani2k ashwani2k added the release/beta Planned for Beta release of the Feature label Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/second-opinion Needs second review by someone else release/beta Planned for Beta release of the Feature size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants