etcd3/watcher: cancelling context shouldn't return error #24638
Conversation
|
This is blocking other people running PR tests. Please prioritize it. |
|
LGTM |
xiang90
added
the
priority/important-soon
k8s-github-robot
added
size/S
gmarek
added
lgtm
k8s-github-robot
removed
the
lgtm
|
Updated the test in the PR.
select {
case wc.errChan <- err:
case <-wc.ctx.Done():
}But in the new test I can repro the failure more easily (without the fix, of course). |
|
@hongchaodeng Can we make the test itself reliable? |
|
GCE e2e build/test passed for commit a77ddadc1c0ba3347af2f5bde20c6140517cb25a. |
|
GCE e2e build/test passed for commit dcc3cb0773b99c2ead2d12d68c2187fc5009c70f. |
|
Seems like context.Canceled isn't the expected error. gRPC wraps it: https://github.com/grpc/grpc-go/blob/61e92eacc3bc5ec3b227ed739d691cfb5034b8a4/rpc_util.go#L319-L327 |
|
@xiang90 |
| @@ -276,6 +281,11 @@ func parseError(err error) *watch.Event { | |||
| } | |||
|
|
|||
| func (wc *watchChan) sendError(err error) { | |||
| // Context.canceled is an expected behavior. | |||
| // We should just stop all goroutines in watchChan without returning error. | |||
| if err == context.Canceled { | |||
There was a problem hiding this comment.
probably
- type assert on grpc error and then compare the code.
- if not grpc, compare with context.Canceled.
I think this is kind of etcd/clientv3 issue. We should not leak gRPC error. But this is P1 for k8s, so let's fix this in k8s for now as a work around.
/cc @heyitsanthony
|
Ah.. I think we can use |
k8s-github-robot
added
size/M
| // Context.canceled is an expected behavior. | ||
| // We should just stop all goroutines in watchChan without returning error. | ||
| // TODO: etcd client should return context.Canceled instead of grpc specific error. | ||
| if grpc.Code(err) == codes.Canceled { |
There was a problem hiding this comment.
if grpc.Code(err) == codes.Canceled || err == context.Canceled
|
GCE e2e build/test passed for commit b0eddb8e835f7d7bdcf4a2d5edf5c1f7e8b53c91. |
|
GCE e2e build/test passed for commit 4dcb017f46bc6f73e6cdaee6fef6908882745c97. |
xiang90
added
lgtm
k8s-github-robot
removed
the
lgtm
|
GCE e2e build/test passed for commit 4dcb017f46bc6f73e6cdaee6fef6908882745c97. |
|
GCE e2e build/test passed for commit b0f4517. |
| // Context.canceled is an expected behavior. | ||
| // We should just stop all goroutines in watchChan without returning error. | ||
| // TODO: etcd client should return context.Canceled instead of grpc specific error. | ||
| if grpc.Code(err) == codes.Canceled || err == context.Canceled { |
There was a problem hiding this comment.
Why do we need grpc dependency?
We are still not using grpc anywhere in kubernetes.
There was a problem hiding this comment.
See comment.
Currently etcd client returns grpc error. It shouldn't though. We will fix it later by return "context.Canceled"
There was a problem hiding this comment.
The comment explains the problem (which I understand) not why you are using grpc.
Currently etcd client returns grpc error. It shouldn't though. We will fix it later by return "context.Canceled"
ok - that makes sense.
wojtek-t
added
the
lgtm
|
LGTM Travis seems to be broken today. I'm not sure we can do much about it. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit b0f4517. |
|
Automatic merge from submit-queue |
Fixes #24528