Fix not retryable error codes from: Unavailable -> FailedPrecondition #12985

ptabor · 2021-05-17T11:02:33Z

ErrGRPCNotCapable("etcdserver: not capable") -> codes.FailedPrecondition (it will not autofix, it requires new version of server)
ErrGPRCNotSupportedForLearner("etcdserver: rpc not supported for learner") -> codes.FailedPrecondition (as long as its learner, the call will not work)
ErrGRPCClusterVersionUnavailable("etcdserver: cluster version not found during downgrade") -> codes.FailedPrecondition (backend does not contain the version (old etcd?) so retry will not help)

https://github.com/etcd-io/etcd/runs/2599598633?check_suite_focus=true

{"level":"warn","ts":"2021-05-17T09:55:30.246Z","logger":"etcd-client","caller":"v3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"etcd-endpoints://0xc000539880/#initially=[unix://localhost:m30]","attempt":0,"error":"rpc error: code = Unavailable desc = etcdserver: rpc not supported for learner"}
{"level":"warn","ts":"2021-05-17T09:55:30.270Z","logger":"etcd-client","caller":"v3/retry_interceptor.go:62","msg":"retrying
of unary invoker
failed","target":"etcd-endpoints://0xc000539880/#initially=[unix://localhost:m30]","attempt":1,"error":"rpc
error: code = Unavailable desc = etcdserver: rpc not supported for
learner"}`

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

ptabor · 2021-05-17T11:31:17Z

Seems its not as obvious as it seems:

TestBalancerSupportLearner tests that expects requests (not supported by learners) to be retried. This makes sense if the retry happens on another instance that is likely not a learner.
But if the learner(s) the only endpoint that client connects to, the retrying does not make sense.

xiang90 · 2021-05-17T16:52:01Z

LGTM.

Even for the learner case, this is not a transient error from the server point of view. So the client should not rely on Unavailable to trigger the rebalance to the actual etcd server.

We need to either fix the test or implement the learner switch in a more reliable way :).

gyuho

lgtm, thanks for the fix

ptabor · 2021-05-18T17:57:50Z

PTAL. Decided to add dedicated case to retry interceptor:
https://github.com/etcd-io/etcd/pull/12985/files#diff-4293a62cbd502b5fb268bfe85b304addfe3348237b9241f7b9218d1a0de5e237

xiang90 · 2021-05-18T22:02:10Z

api/v3rpc/rpctypes/error.go

 	ErrGRPCStopped                    = status.New(codes.Unavailable, "etcdserver: server stopped").Err()
 	ErrGRPCTimeout                    = status.New(codes.Unavailable, "etcdserver: request timed out").Err()
 	ErrGRPCTimeoutDueToLeaderFail     = status.New(codes.Unavailable, "etcdserver: request timed out, possibly due to previous leader failure").Err()
 	ErrGRPCTimeoutDueToConnectionLost = status.New(codes.Unavailable, "etcdserver: request timed out, possibly due to connection lost").Err()
 	ErrGRPCUnhealthy                  = status.New(codes.Unavailable, "etcdserver: unhealthy cluster").Err()
 	ErrGRPCCorrupt                    = status.New(codes.DataLoss, "etcdserver: corrupt cluster").Err()
-	ErrGPRCNotSupportedForLearner     = status.New(codes.Unavailable, "etcdserver: rpc not supported for learner").Err()
+	ErrGPRCNotSupportedForLearner     = status.New(codes.InvalidArgument, "etcdserver: rpc not supported for learner").Err()


It seems Unimplemented or Not Found status is better fit here?

How about FailedPrecondition ?

not found - sounds like a problem with 'resource' not 'semantic'.

not implemented - sounds like a way to fix the problem is to upgrade the server.

FailedPrecondition is the most neutral in my opinion

Not implemented can mean two things:

The operation is not implemented

The operation is not supported/enabled

This case can fall into the second one.

ref: https://grpc.github.io/grpc/core/md_doc_statuscodes.html.

The precondition failure indicates that the service is in a state that cannot execute the operation. Usually, it is an external triggered state and can be fixed externally (for example to remove an unempty director, client needs to remove all files under it). learner is an internal state of the service and cannot be changed by users, but it also makes sense in a border sense.

I have no strong opinion.

xiang90 · 2021-05-18T22:05:52Z

client/v3/retry_interceptor.go

 	if isContextError(err) {
 		return false
 	}
+
+	// Situation when learner refuses RPC its supposed to not serve is from server


Yea. We should have a more reliable way for client to retry on the learner case. The client should be able to tell if the server instance is a leaner or not.

- ErrGRPCNotCapable("etcdserver: not capable") -> codes.FailedPrecondition (it will not autofix, it requires new version of server) - ErrGPRCNotSupportedForLearner("etcdserver: rpc not supported for learner") -> codes.FailedPrecondition (as long as its learner, the call will not work) - ErrGRPCClusterVersionUnavailable("etcdserver: cluster version not found during downgrade") -> codes.FailedPrecondition (backend does not contain the version (old etcd?) so retry will not help) https://github.com/etcd-io/etcd/runs/2599598633?check_suite_focus=true ``` {"level":"warn","ts":"2021-05-17T09:55:30.246Z","logger":"etcd-client","caller":"v3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"etcd-endpoints://0xc000539880/#initially=[unix://localhost:m30]","attempt":0,"error":"rpc error: code = Unavailable desc = etcdserver: rpc not supported for learner"} {"level":"warn","ts":"2021-05-17T09:55:30.270Z","logger":"etcd-client","caller":"v3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"etcd-endpoints://0xc000539880/#initially=[unix://localhost:m30]","attempt":1,"error":"rpc error: code = Unavailable desc = etcdserver: rpc not supported for learner"}` ```

codecov-commenter · 2021-05-19T00:57:56Z

Codecov Report

Merging #12985 (16d51d8) into main (80ccb27) will decrease coverage by 4.76%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #12985      +/-   ##
==========================================
- Coverage   47.24%   42.47%   -4.77%     
==========================================
  Files         438      393      -45     
  Lines       34110    31699    -2411     
==========================================
- Hits        16116    13465    -2651     
- Misses      16088    16543     +455     
+ Partials     1906     1691     -215

Flag	Coverage Δ
all	`42.47% <0.00%> (-4.77%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/v3rpc/rpctypes/error.go	`47.05% <ø> (-17.65%)`	⬇️
client/v3/retry_interceptor.go	`0.00% <0.00%> (-23.25%)`	⬇️
client/v3/ctx.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/idutil/id.go	`0.00% <0.00%> (-100.00%)`	⬇️
raft/quorum/joint.go	`0.00% <0.00%> (-100.00%)`	⬇️
raft/quorum/quorum.go	`0.00% <0.00%> (-100.00%)`	⬇️
raft/tracker/state.go	`0.00% <0.00%> (-100.00%)`	⬇️
client/v3/compact_op.go	`0.00% <0.00%> (-100.00%)`	⬇️
raft/quorum/majority.go	`0.00% <0.00%> (-100.00%)`	⬇️
raft/raftpb/confstate.go	`0.00% <0.00%> (-100.00%)`	⬇️
... and 166 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80ccb27...16d51d8. Read the comment docs.

xiang90 · 2021-05-19T05:17:13Z

LGTM

gyuho · 2021-05-19T06:15:39Z

@ptabor This is a bug fix. Can we backport to 3.5?

ptabor assigned hexfusion and unassigned hexfusion May 17, 2021

gyuho approved these changes May 17, 2021

View reviewed changes

ptabor force-pushed the 20210517-fix-not-a-learner-retries branch 2 times, most recently from 888c4b2 to e591871 Compare May 18, 2021 17:41

ptabor requested a review from xiang90 May 18, 2021 17:57

xiang90 reviewed May 18, 2021

View reviewed changes

ptabor force-pushed the 20210517-fix-not-a-learner-retries branch from e591871 to 13b6791 Compare May 18, 2021 23:39

ptabor requested a review from xiang90 May 18, 2021 23:40

ptabor force-pushed the 20210517-fix-not-a-learner-retries branch from 13b6791 to 16d51d8 Compare May 19, 2021 00:09

gyuho approved these changes May 19, 2021

View reviewed changes

ptabor merged commit 159d191 into etcd-io:main May 19, 2021

ptabor deleted the 20210517-fix-not-a-learner-retries branch May 19, 2021 06:05

gyuho added the backport/v3.5 label May 19, 2021

This was referenced Mar 25, 2024

Plan to release etcd v3.5.13 #17633

Closed

[3.5] backport for fix retry requests when receiving ErrGPRCNotSupportedForLearner #17641

Merged

sheyt0 mentioned this pull request Apr 2, 2024

[3.4] backport for fix retry requests when receiving ErrGPRCNotSupportedForLearner #17692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix not retryable error codes from: Unavailable -> FailedPrecondition #12985

Fix not retryable error codes from: Unavailable -> FailedPrecondition #12985

ptabor commented May 17, 2021

ptabor commented May 17, 2021 •

edited

Loading

xiang90 commented May 17, 2021 •

edited

Loading

gyuho left a comment

ptabor commented May 18, 2021

xiang90 May 18, 2021

ptabor May 18, 2021

xiang90 May 19, 2021

xiang90 May 18, 2021

ptabor May 18, 2021

codecov-commenter commented May 19, 2021

xiang90 commented May 19, 2021

gyuho commented May 19, 2021

Fix not retryable error codes from: Unavailable -> FailedPrecondition #12985

Fix not retryable error codes from: Unavailable -> FailedPrecondition #12985

Conversation

ptabor commented May 17, 2021

ptabor commented May 17, 2021 • edited Loading

xiang90 commented May 17, 2021 • edited Loading

gyuho left a comment

Choose a reason for hiding this comment

ptabor commented May 18, 2021

xiang90 May 18, 2021

Choose a reason for hiding this comment

ptabor May 18, 2021

Choose a reason for hiding this comment

xiang90 May 19, 2021

Choose a reason for hiding this comment

xiang90 May 18, 2021

Choose a reason for hiding this comment

ptabor May 18, 2021

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2021

Codecov Report

xiang90 commented May 19, 2021

gyuho commented May 19, 2021

ptabor commented May 17, 2021 •

edited

Loading

xiang90 commented May 17, 2021 •

edited

Loading