Skip to content

Tolerate temporary errors from etcdserver#11401

Merged
hickeyma merged 1 commit intohelm:mainfrom
dims:retry-for-issue-9502
Oct 5, 2022
Merged

Tolerate temporary errors from etcdserver#11401
hickeyma merged 1 commit intohelm:mainfrom
dims:retry-for-issue-9502

Conversation

@dims
Copy link
Copy Markdown
Contributor

@dims dims commented Sep 30, 2022 •
edited
Loading

What this PR does / why we need it:

There are cases when the etcdserver is temporarily unavailable and the
errors that we get back from kube-apiserver reflect that error. It looks
like we bail out immediately when these errors happen currently. We
should retry until timeout is reached when this sort of errors happen.

Fixes #9502
Fixes #7637

Signed-off-by: Davanum Srinivas davanum@gmail.com

Special notes for your reviewer:

With this patch, temporary errors like the etcdserver leader changes are not treated as terminal. We continue to retry until the specified timeout.

Note that there are things that can be done on the k8s side, discussion is going on there as well:
kubernetes/kubernetes#112152

If applicable:

  • This PR does not change any functionality, so no updates to the documentation
  • Currently there aren't any unit tests in this package, but i added one for just the new method isServiceUnavailable
  • No changes to functionality, so no incompatibilities

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 30, 2022
@dims dims force-pushed the retry-for-issue-9502 branch 2 times, most recently from c2bbcb0 to 3ca36c0 Compare October 1, 2022 21:29
@dims dims changed the title [WIP][IGNORE ME] Retry a few times, do not give up so soon! Tolerate temporary errors from etcdserver Oct 1, 2022
@dims dims marked this pull request as ready for review October 1, 2022 21:36
@dims dims force-pushed the retry-for-issue-9502 branch from 3ca36c0 to 0635219 Compare October 1, 2022 22:23
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 1, 2022
@dims dims force-pushed the retry-for-issue-9502 branch 2 times, most recently from dc81bb9 to 86defa7 Compare October 2, 2022 00:10
@dims
Tolerate temporary errors from etcdserver
ebc79fa 
There are cases when the etcdserver is temporarily unavailable and the
errors that we get back from kube-apiserver reflect that error. It looks
like we bail out immediately when these errors happen currently. We
should retry until timeout is reached when this sort of errors happen.

Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims dims force-pushed the retry-for-issue-9502 branch from 86defa7 to ebc79fa Compare October 2, 2022 00:12
@dims
Copy link
Copy Markdown
Contributor Author

dims commented Oct 3, 2022

@hickeyma this is ready now!

@mattfarina
Copy link
Copy Markdown
Collaborator

mattfarina commented Oct 3, 2022

@dims thanks for looking into the issues here.

I the Kubernetes API supposed to be a leaky abstraction? Are clients expected to work with etcd? I'm asking about intent and mid-term intent. I'm wondering whether this code is something we will need to maintain long term or if this is a short term situation.

@dims
Copy link
Copy Markdown
Contributor Author

dims commented Oct 3, 2022

@mattfarina we'll need a KEP in upstream, i've requested some folks who were pushing for this earlier to do more in 1.27 cycle (not 1.26), So until that KEP is discussed/reviewed/approved we will need this.

we'll also need this until versions of kubernetes supported by helm has the old style leaky abstraction.

technosophos
technosophos approved these changes Oct 4, 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.

This seems to be the appropriate stop-gap for this error.

hakuna-matatah
hakuna-matatah reviewed Oct 4, 2022
View reviewed changes
hickeyma
hickeyma approved these changes Oct 5, 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.

Thanks @dims for tracking these issues down and providing this interim solution. It will be helpful to users.

@hickeyma hickeyma added this to the 3.10.1 milestone Oct 5, 2022
@hickeyma hickeyma merged commit 2baf68f into helm:main Oct 5, 2022
@dims
Copy link
Copy Markdown
Contributor Author

dims commented Oct 5, 2022

thanks @hickeyma !

@hickeyma hickeyma added needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. picked Indicates that a PR has been cherry-picked into the next release candidate. and removed needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. labels Oct 5, 2022
@cenkalti cenkalti mentioned this pull request Oct 11, 2022
Merged
3 tasks
@cenkalti
Copy link
Copy Markdown
Contributor

cenkalti commented Oct 18, 2022

Hey @technosophos @hickeyma. Unfortunately, this fix does not solve the issue. Can you take a look at my new fix? #11426

@mattfarina mattfarina mentioned this pull request Jan 12, 2023
Closed
@sruthiwander
Copy link
Copy Markdown

sruthiwander commented Feb 23, 2023 •
edited
Loading

@dims Can this be backported to 3.2 please?

@dims
Copy link
Copy Markdown
Contributor Author

dims commented Feb 23, 2023 •
edited
Loading

@sruthiwander not this one! it was reverted, you will need #11426

Also https://github.com/helm/helm/releases/tag/v3.2.0 is practically ancient, i don't know/think that helm maintainers will go back that far https://helm.sh/docs/topics/release_policy/

@yuxiang-zhang yuxiang-zhang mentioned this pull request Feb 13, 2024
Open
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

@hakuna-matatah hakuna-matatah hakuna-matatah left review comments

@hickeyma hickeyma hickeyma approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

picked Indicates that a PR has been cherry-picked into the next release candidate. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

3.10.1

Development

Successfully merging this pull request may close these issues.

helm upgrade --wait command failded because of etcdserver: leader changed helm upgrade --atomic etcdserver: leader changed

7 participants

@dims @mattfarina @cenkalti @sruthiwander @technosophos @hickeyma @hakuna-matatah