Add a Code based member for cancel reason in WatchResponse

[mitake]

What would you like to be added?

Currently WatchResponse message has a string type member CancelReason. The member is used for storing an error which happened during watch.
It's better to add a new Code based member for representing an error in the watch API.

Why is this needed?

Using the existing member for error handling code isn't good because of fragility and hard to track behavior of the etcd wire protocol.

See also:

• Switch vendored etcd to v3.5.6 kubernetes/kubernetes#114403
• [3.5] clientv3: revert the client side change in 14547 #14995
• Remove the error message driven client behavior #14992

[lavacat]

@mitake seams like you've been working on this code before, are you planning to contribute this change if approved?

How are you planning to handle different values of CancelReason? I don't think they map to grpc codes directly.

[mitake]

@lavacat Yes I'm planning to work on, but if other people can work on this issue, it's fine for me.
I checked grpc codes again, and yeah it doesn't seem suitable for the purpose. Having a new int32 member and define etcd specific error codes might be good? If you have suggestions, I'd like to know.

[ahrtr]

The direction of adding a Code (e.g. ReasonCode) looks good to me. Please evaluate the impact and provide a design firstly.

Since it changes the protocol between client and server, so we need K8s folks to review the changes as well. cc @ptabor @serathius @liggitt @dims @lavalamp @deads2k as well.

[liggitt]

a new optional (since old servers won't send it) code field the client reacts to if present seems like a minimal and safe approach

[ahrtr]

a new optional (since old servers won't send it) code field the client reacts to if present seems like a minimal and safe approach

Agree. It would be something like below,

$ git diff
diff --git a/api/etcdserverpb/rpc.proto b/api/etcdserverpb/rpc.proto
index 9cdc0b37f..7b37b1556 100644
--- a/api/etcdserverpb/rpc.proto
+++ b/api/etcdserverpb/rpc.proto
@@ -834,6 +834,8 @@ message WatchResponse {
   // framgment is true if large watch response was split over multiple responses.
   bool fragment = 7 [(versionpb.etcd_version_field)="3.4"];
 
+  optional uint32 reason_code = 8 [(gogoproto.nullable) = true];
+
   repeated mvccpb.Event events = 11;
 }

[ahrtr]

@mitake @lavacat or anyone else please feel free to work on this task if you have bandwidth and be interested. Please drop a message before you get started.

[lavacat]

@mitake if you don't mind, I'll work on this task, since we have an internal use case for it and we also might contribute retry logic to jetcd client.

[mitake]

@lavacat Thanks a lot! If you can work on it, it's really helpful.

[lavacat]

@mitake @ahrtr I've tried implementing reason codes, but it feels cumbersome to have both reasonCode and cancelReason string. Here are some concerns:

1. All other response objects have string error. StatusResponse, LeaseGrantResponse. So adding codes for WatchResponse isn't consistent.
2. errors defined in mvcc/watcher.go I've marked with InternalWatchError code. Not sure it's worth defining a code for each of those.

How about we add retry flag to WatchResponse to signal that watch can be retried without exposing codes?

I've added both reason_code and retry in this branch for demo. I'm ok with proceeding with original plan but want to run retry flag idea by you.

[ahrtr]

My immediate feeling is adding retry is a good idea because It's much simpler. It should have no any impact on K8s either. It's false by default, and only explicitly set to true on etcdserver side when needed.

[mitake]

I also feel retry might be fine, but adding reasonCode to the response and making cancelReason not a suggested thing to access will be good for letting clients not rely on cancelReason in future (I'm not sure it's valuable to introduce similar change to other response messages). What do you think @lavacat @ahrtr ?

[ahrtr]

I also feel retry might be fine, but adding reasonCode to the response and making cancelReason not a suggested thing to access will be good for letting clients not rely on cancelReason in future (I'm not sure it's valuable to introduce similar change to other response messages). What do you think @lavacat @ahrtr ?

I think adding field something like errorCode for all messages may be a long-term goal, but it's a big change, and we should document all the available Codes as well, just in the same way as HTTP does, e.g 4xx means bad request, and 5xx means service side internal errors.

Note Kubernetes describes working against a storage layer that implements the etcd wire protocol. FYI. kubernetes/kubernetes#114403 (comment)

Even we only add reasonCode for WatchResponse, we also need to expose and clearly document the reason Codes. Once we add them, then we can't easily change them in order to be backward compatible. The good side is it's extensible and flexible. On the contrary, adding retry is a good workaround for this case, because it's much simpler and just a minor change.

In summary, if we follow the direction of adding field something like errorCode or reasonCode for all messages, we need to evaluate the impact and efforts, and also whether it's really needed or necessary. Otherwise, we'd better add retry or reasonCode only for WatchResponse for now.

[lavacat]

@mitake main problem with codes is that they add extra duplication to exposed rpctypes. For example there will be new AuthOldRevision code and existing ErrAuthOldRevision. Also errors from mvcc/watcher.go will have to get new codes or be aggregated under InternalWatchError. See lavacat@e4e4e71

From client perspective the only actionable use-case is retry on InvalidAuthToken and AuthOldRevision, so exposing that as a flag is simpler. I agree, it's less flexible, but are there other hypothetical scenarios when code will be useful?

I'm open to reasonCode route, but just want to call out extra boilerplate it will add.