kubeadm: reduce the backoff time of AddMember for etcd #104134

ihgann · 2021-08-04T19:06:05Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This change optimizes the kubeadm/etcd AddMember client-side function by stopping early in the backoff loop when a peer conflict is found (indicating the member has already been added to the etcd cluster). In this situation, the function will stop early and relay a call to ListMembers to fetch the current list of members to return. With this optimization, front-loading a ListMembers call is no longer necessary, as this functionally returns the equivalent response.

This helps reduce the amount of time taken in situational cases where an initial client request to add a member is accepted by the server, but fails client-side.

This situation is possible situationally, such as if network latency causes the request to timeout after it was sent and accepted by the cluster. In this situation, the following loop would occur and fail with an ErrPeerURLExist response, and would be stuck until the backoff timeout was met (roughly ~2min30sec currently).

Which issue(s) this PR fixes:

I have not opened an issue publicly for this, but the issue is described above.

Special notes for your reviewer:

n/a

Does this PR introduce a user-facing change?

kubeadm: When adding an etcd peer to an existing cluster, if an error is returned indicating the peer has already been added, this is accepted and a ListMembers call is used instead to return the existing cluster. This helps diminish the exponential backoff when the first AddMember call times out, while still retaining a similar performance when the peer had already been added from a previous call.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

k8s-ci-robot · 2021-08-04T19:06:14Z

Hi @ihgann. 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.

neolit123

/ok-to-test
/priority backlog
/triage accepted

thanks for the PR @ihgann
i will have a look tomorrow.

randomvariable · 2021-08-05T09:21:32Z

looks like a flake
/retest

dims · 2021-08-05T12:43:53Z

/assign @neolit123

randomvariable · 2021-08-05T14:27:48Z

I think this should be fine, and won't break Cluster API in the same way as it did prior to the ListMembers code being added. it should be caught by conformance main branch testing in any case.

neolit123 · 2021-08-05T16:28:06Z

go.mod

@@ -81,6 +81,7 @@ require (
 	github.com/stretchr/testify v1.7.0
 	github.com/vishvananda/netlink v1.1.0
 	github.com/vmware/govmomi v0.20.3
+	go.etcd.io/etcd/api/v3 v3.5.0


note, i cannot approve changes in the go.mod.
once we are done with the review here we can assign one of the maintainers for go.mod changes.

cmd/kubeadm/app/util/etcd/testing/mock_etcd_cluster.go

cmd/kubeadm/app/util/etcd/etcd_test.go

cmd/kubeadm/app/util/etcd/testing/mock_etcd_cluster.go

cmd/kubeadm/app/util/etcd/etcd.go

neolit123 · 2021-08-05T17:30:52Z

/retitle kubeadm: reduce the backoff time of AddMember for etcd

@ihgann , under:

Does this PR introduce a user-facing change?

instead of None, could provide a one/two sentence summary of what problem we are trying to fix in this PR and how it would benefit them. should be prefixed with kubeadm: ....

neolit123 · 2021-08-05T19:57:04Z

ok, this SGTM. please squash the commits to one and it can be merged.

This change optimizes the kubeadm/etcd `AddMember` client-side function by stopping early in the backoff loop when a peer conflict is found (indicating the member has already been added to the etcd cluster). In this situation, the function will stop early and relay a call to `ListMembers` to fetch the current list of members to return. With this optimization, front-loading a `ListMembers` call is no longer necessary, as this functionally returns the equivalent response. This helps reduce the amount of time taken in situational cases where an initial client request to add a member is accepted by the server, but fails client-side. This situation is possible situationally, such as if network latency causes the request to timeout after it was sent and accepted by the cluster. In this situation, the following loop would occur and fail with an `ErrPeerURLExist` response, and would be stuck until the backoff timeout was met (roughly ~2min30sec currently). Testing Done: * Manual testing with an etcd cluster. Initial "AddMember` call was successful, and the etcd manifest file was identical to prior version of these files. Subsequent calls to add the same member succeeded immediately (retaining idempotency), and the resulting manifest file remains identical to previous version as well. The difference, this time, is the call finished ~2min25sec faster in an identical test in the environment tested with.

neolit123 · 2021-08-05T20:17:40Z

/lgtm
/approve

thanks @ihgann
if for some reason we see problems for users in the wild after this change, i will give you ping.
(hopefully unlikely)

neolit123 · 2021-08-05T20:18:49Z

/assign @liggitt
for the go.mod change (we are already vendoring go.etcd.io/etcd/api/v3)

liggitt · 2021-08-05T20:21:04Z

/approve
for dep change

k8s-ci-robot · 2021-08-05T20:21:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ihgann, liggitt, neolit123

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~~ [liggitt]

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 requested review from neolit123 and yagonobre August 4, 2021 19:06

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 4, 2021

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 4, 2021

neolit123 reviewed Aug 4, 2021

View reviewed changes

k8s-ci-robot assigned neolit123 Aug 5, 2021