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.

Sign up for GitHub

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

Jump to bottom

etcd watcher: centralize error handling #32907

Merged
merged 1 commit into from
Sep 19, 2016
Merged

etcd watcher: centralize error handling #32907

merged 1 commit into from
Sep 19, 2016
+17 −18

Conversation

hongchaodeng
Copy link
Contributor

@hongchaodeng hongchaodeng commented Sep 16, 2016 •
edited by jessfraz
Loading

We should centralize error handling in watcher in run(). Otherwise this could silently return.

Also we don't need the grpc code checking anymore. It's fixed.

etcd: watcher centralize error handling

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Sep 16, 2016
@hongchaodeng
Copy link
Contributor Author

cc @timothysc @xiang90

@hongchaodeng
Copy link
Contributor Author

@k8s-bot test this issue: #IGNORE

@hongchaodeng
Copy link
Contributor Author

no previous test result is found...

@k8s-bot
Copy link

k8s-bot commented Sep 17, 2016

GCE e2e build/test passed for commit 5a4a095.

@hongchaodeng
Copy link
Contributor Author

@k8s-bot unit test this issue: #32770

@hongchaodeng
Copy link
Contributor Author

hongchaodeng commented Sep 17, 2016 •
edited
Loading

Can we move forward this?
This could help narrow the problem scope of the watch issue.

@hongchaodeng
Copy link
Contributor Author

@wojtek-t Can you review this?

@hongchaodeng hongchaodeng mentioned this pull request Sep 19, 2016
Merged
@timothysc timothysc self-assigned this Sep 19, 2016
@timothysc timothysc added area/etcd release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Sep 19, 2016
timothysc
timothysc reviewed Sep 19, 2016
View reviewed changes
pkg/storage/etcd3/watcher.go
@@ -109,6 +107,10 @@ func (wc *watchChan) run() {

select {
case err := <-wc.errChan:
if err == context.Canceled {
wc.cancel() // just in case
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly confused why you would cancel the context when it already came back as canceled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is sort of last defense line -- what if we get context.Canceled but wc.ctx isn't canceled.
We want to make sure all goroutines in etcd3 watcher to stop regardlessly.

@wojtek-t
Copy link
Member

LGTM

@hongchaodeng - I'm OOO this week - please don't expect any actions from me this week in general.

@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2016
@timothysc timothysc added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Sep 19, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Sep 19, 2016

GCE e2e build/test passed for commit 5a4a095.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 4bfebf0 into kubernetes:master Sep 19, 2016
@hongchaodeng hongchaodeng deleted the etcd_fix branch September 19, 2016 20:10
@timothysc timothysc mentioned this pull request Sep 19, 2016
Closed
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@wojtek-t wojtek-t added this to the v1.4 milestone Oct 19, 2016
@wojtek-t wojtek-t mentioned this pull request Oct 20, 2016
Merged
@jessfraz jessfraz added release-note Denotes a PR that will be considered when it comes time to generate release notes. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 20, 2016
k8s-github-robot pushed a commit that referenced this pull request Oct 21, 2016
Merge pull request #35205 from wojtek-t/automated-cherry-pick-of-#31189-
bdbe2e1 
#31704-#32831-#32907-#33003-#33349-#31190-#33581-#34089-#34234-#32822-#33393-#34246-#34435-#32477-upstream-release-1.4

Automatic merge from submit-queue

Automated cherry pick of #31189 #31704 #32831 #32907 #33003 #33349 #31190 #33581 #34089 #34234 #32822 #33393 #34246 #34435 #32477 upstream release 1.4

We are going to release etcd v3 in 1.4.x release.

```
Cherrypick etcd v3-related bug fixes to 1.4 branch
```

Those PRs include:
- Updates of etcd Godeps
- Update to pkg/storage/etcd directory
- Two PR that were unnecessary to avoid conflicts: #31189 #31704

Automated cherry pick of #31189 #31704 #32831 #32907 #33003 #33349 #31190 #33581 #34089 #34234 #32822 #33393 #34246 #34435 #32477

@hongchaodeng @xiang90 @lavalamp @smarterclayton
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#35205 from wojtek-t/automated-cherry-pi…
711c6c6 
…ck-of-#31189-kubernetes#31704-kubernetes#32831-kubernetes#32907-kubernetes#33003-kubernetes#33349-kubernetes#31190-kubernetes#33581-kubernetes#34089-kubernetes#34234-kubernetes#32822-kubernetes#33393-kubernetes#34246-kubernetes#34435-kubernetes#32477-upstream-release-1.4

Automatic merge from submit-queue

Automated cherry pick of kubernetes#31189 kubernetes#31704 kubernetes#32831 kubernetes#32907 kubernetes#33003 kubernetes#33349 kubernetes#31190 kubernetes#33581 kubernetes#34089 kubernetes#34234 kubernetes#32822 kubernetes#33393 kubernetes#34246 kubernetes#34435 kubernetes#32477 upstream release 1.4

We are going to release etcd v3 in 1.4.x release.

```
Cherrypick etcd v3-related bug fixes to 1.4 branch
```

Those PRs include:
- Updates of etcd Godeps
- Update to pkg/storage/etcd directory
- Two PR that were unnecessary to avoid conflicts: kubernetes#31189 kubernetes#31704

Automated cherry pick of kubernetes#31189 kubernetes#31704 kubernetes#32831 kubernetes#32907 kubernetes#33003 kubernetes#33349 kubernetes#31190 kubernetes#33581 kubernetes#34089 kubernetes#34234 kubernetes#32822 kubernetes#33393 kubernetes#34246 kubernetes#34435 kubernetes#32477

@hongchaodeng @xiang90 @lavalamp @smarterclayton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timothysc timothysc timothysc left review comments

Assignees

@timothysc timothysc

@wojtek-t wojtek-t

Labels
area/etcd cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
8 participants
@hongchaodeng @k8s-bot @wojtek-t @k8s-github-robot @k8s-cherrypick-bot @timothysc @jessfraz @googlebot