Skip to content

Fix handling of "leader changed" errors#11426

Merged
hickeyma merged 2 commits intohelm:mainfrom
cenkalti:leader-changed
Nov 16, 2022
Merged

Fix handling of "leader changed" errors#11426
hickeyma merged 2 commits intohelm:mainfrom
cenkalti:leader-changed

Conversation

@cenkalti
Copy link
Copy Markdown
Contributor

@cenkalti cenkalti commented Oct 11, 2022

What this PR does / why we need it:
This PR aims to fix temporary "etcdserver: leader changed" errors from kube-apiserver that is previously attempted by #11401 by adding a single retry when this kind of error is detected.

/cc @dims

Special notes for your reviewer:
I also reverted the previous fix that didn't solve the issue. One commit is the revert, the other one is the new fix. It's better if you review commits separately.

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 11, 2022
@dims
Copy link
Copy Markdown
Contributor

dims commented Oct 11, 2022

@hickeyma @technosophos @mattfarina Folks, @cenkalti tested a bunch of scenarios recreating the problem(s) with etcd with this patch.

dims
dims reviewed Oct 11, 2022
View reviewed changes
@cenkalti
Revert "Tolerate temporary errors from etcdserver"
d32c623 
This reverts commit ebc79fa.

Signed-off-by: Cenk Alti <cenkalti@gmail.com>
@cenkalti
retry http request on temporary errors
b5378b3 
Signed-off-by: Cenk Alti <cenkalti@gmail.com>
@cenkalti cenkalti mentioned this pull request Oct 18, 2022
Merged
@cenkalti
Copy link
Copy Markdown
Contributor Author

cenkalti commented Oct 26, 2022

Hey @technosophos @hickeyma. Bumping up this for visibility. It's a small fix. Can you take a look please?

hickeyma
hickeyma reviewed Nov 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

@cenkalti thanks for pushing the PR. The code looks good in principle. However, we don't have any easy way of testing this and this is the 2nd fix following #11401. Would it be possible to provide some tests for this?

@cenkalti
Copy link
Copy Markdown
Contributor Author

cenkalti commented Nov 8, 2022

@hickeyma I tested this fix manually by setting up a custom k8s cluster with 3 etcd nodes and running etcdctl move-leader with random member ID continuously to trigger the error case. Even with this special setup, the error does not occur every time when I run helm install command as the timing of API calls are very critical that they must overlap with the leader change event.

I checked https://github.com/helm/acceptance-testing project to see If I can write this scenario as an acceptance test but I couldn't find a solution as the clusters are created with kind and there is no easy way to trigger a leader change event.

hickeyma
hickeyma approved these changes Nov 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @cenkalti

@hickeyma
Copy link
Copy Markdown
Contributor

hickeyma commented Nov 8, 2022

Needs another maintainer approval for merge.

@hickeyma hickeyma requested a review from dims November 8, 2022 16:22
dims
dims approved these changes Nov 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

@dims
Copy link
Copy Markdown
Contributor

dims commented Nov 8, 2022

thanks @cenkalti @hickeyma

@cenkalti
Copy link
Copy Markdown
Contributor Author

cenkalti commented Nov 11, 2022

@technosophos @mattfarina Can you take a look please?

technosophos
technosophos approved these changes Nov 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@technosophos technosophos left a comment

Choose a reason for hiding this comment

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

Looks good.

@technosophos
Copy link
Copy Markdown
Member

technosophos commented Nov 16, 2022

@hickeyma feel free to merge whenever you feel good about it.

@hickeyma hickeyma merged commit 3974136 into helm:main Nov 16, 2022
@hickeyma hickeyma added this to the 3.10.3 milestone Nov 16, 2022
@hickeyma hickeyma added the needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. label Nov 16, 2022
@dims dims mentioned this pull request Nov 18, 2022
Closed
3 tasks
@hickeyma hickeyma modified the milestones: 3.10.3, 3.11.0 Dec 14, 2022
@hickeyma hickeyma removed the needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. label Dec 14, 2022
@sruthiwander
Copy link
Copy Markdown

sruthiwander commented Feb 23, 2023

Hi, Any chance this could be patched to the 3.2.x release.

@yuxiang-zhang yuxiang-zhang mentioned this pull request Feb 13, 2024
Open
@DeyvsonL DeyvsonL mentioned this pull request May 20, 2024
Closed
1 task
@Valgueiro Valgueiro mentioned this pull request May 21, 2024
Closed
luisdavim added a commit to luisdavim/helm-controller that referenced this pull request Oct 8, 2024
@luisdavim
fix: handle "leader changed" errors
cc9ec38 
Fixes fluxcd/flux2/#4804 by copying the solution used in helm/helm#11426
@luisdavim luisdavim mentioned this pull request Oct 8, 2024
Merged
luisdavim added a commit to luisdavim/helm-controller that referenced this pull request Oct 8, 2024
@luisdavim
fix: handle "leader changed" errors
1d34d7f 
Fixes fluxcd/flux2/#4804 by copying the solution used in helm/helm#11426

Signed-off-by: Luis Davim <luis.davim@gmail.com>
luisdavim added a commit to luisdavim/helm-controller that referenced this pull request Nov 16, 2024
@luisdavim
fix: handle "leader changed" errors
a519331 
Fixes fluxcd/flux2/#4804 by copying the solution used in helm/helm#11426

Signed-off-by: Luis Davim <luis.davim@gmail.com>
luisdavim added a commit to luisdavim/helm-controller that referenced this pull request Nov 16, 2024
@luisdavim
fix: handle "leader changed" errors
26bdaec 
Fixes fluxcd/flux2/#4804 by copying the solution used in helm/helm#11426

Signed-off-by: Luis Davim <luis.davim@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@technosophos technosophos technosophos approved these changes

+2 more reviewers

@dims dims dims approved these changes

@hickeyma hickeyma hickeyma approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

3.11.0

Development

Successfully merging this pull request may close these issues.

5 participants

@cenkalti @dims @hickeyma @technosophos @sruthiwander