Remove the error message driven client behavior

[ahrtr]

Background

In order to fix #12385, PR #14322 introduced a change in which the client side may retry based on the error message returned from server side.

This is not good, as it's too fragile and it's also changed the protocol between client and server. Please see the discussion in kubernetes/kubernetes#114403

Note: The issue #12385 only happens when auth is enabled, and client side reuse the same client to watch.

Solutions

Solution 1

Rollback the change. Reasons:

1. K8s doesn't enable auth at all. It has no any impact on K8s.
2. It's very easy for client application to workaround the issue. The client just needs to create a new client each time before watching.

Solution 2

Add a field something like ReasonCode in WatchResponse.

etcd/api/etcdserverpb/rpc.pb.go

Lines 2192 to 2221 in ee9db72

	type WatchResponse struct {
	Header *ResponseHeader `protobuf:"bytes,1,opt,name=header,proto3" json:"header,omitempty"`
	// watch_id is the ID of the watcher that corresponds to the response.
	WatchId int64 `protobuf:"varint,2,opt,name=watch_id,json=watchId,proto3" json:"watch_id,omitempty"`
	// created is set to true if the response is for a create watch request.
	// The client should record the watch_id and expect to receive events for
	// the created watcher from the same stream.
	// All events sent to the created watcher will attach with the same watch_id.
	Created bool `protobuf:"varint,3,opt,name=created,proto3" json:"created,omitempty"`
	// canceled is set to true if the response is for a cancel watch request.
	// No further events will be sent to the canceled watcher.
	Canceled bool `protobuf:"varint,4,opt,name=canceled,proto3" json:"canceled,omitempty"`
	// compact_revision is set to the minimum index if a watcher tries to watch
	// at a compacted index.
	//
	// This happens when creating a watcher at a compacted revision or the watcher cannot
	// catch up with the progress of the key-value store.
	//
	// The client should treat the watcher as canceled and should not try to create any
	// watcher with the same start_revision again.
	CompactRevision int64 `protobuf:"varint,5,opt,name=compact_revision,json=compactRevision,proto3" json:"compact_revision,omitempty"`
	// cancel_reason indicates the reason for canceling the watcher.
	CancelReason string `protobuf:"bytes,6,opt,name=cancel_reason,json=cancelReason,proto3" json:"cancel_reason,omitempty"`
	// framgment is true if large watch response was split over multiple responses.
	Fragment bool `protobuf:"varint,7,opt,name=fragment,proto3" json:"fragment,omitempty"`
	Events []*mvccpb.Event `protobuf:"bytes,11,rep,name=events,proto3" json:"events,omitempty"`
	XXX_NoUnkeyedLiteral struct{} `json:"-"`
	XXX_unrecognized []byte `json:"-"`
	XXX_sizecache int32 `json:"-"`
	}

. The client side uses the ReasonCode instead of a fragile error message.

Thoughts ?

Please let me know your thought.

cc @mitake @ptabor @serathius @spzala
@liggitt @dims @lavalamp @deads2k

[serathius]

Definitely first to do is rollback. This change should have never been backported to v3.5. We can continue the discussion about changing Watch api in v3.6.

[dims]

@serathius +1 to that plan of action, a fresh release on v3.5.x branch would be welcome as well.

[ahrtr]

FYI. #14995

[mitake]

So sorry for the trouble. I opened an issue for adding Code based member to WatchResponse: #15058

[lavacat]

@ahrtr We use etcd with auth for non-kube use-case. I got feedback that recreating client will be cumbersome. So, I'd like help contribute fix that will keep retry in the watch. Adding ReasonCode and not using Strings makes sense, but maybe retry logic can be added back with ReasonCode.

For wider community here are arguments why it will be useful to keep retry logic:

1. retry in the watch is consistent with etcd client that will retry in interceptor.
2. ErrAuthOldRevision requires user to understand etcd auth implementation details.
3. If kube starts using auth, I think it will require some non-trivial refactoring to recreate the client. apiserver initializes etcd client in factory package. But watch error handing is in etcd3 package.

[ahrtr]

Thanks @lavacat , the direction of adding ReasonCode looks good to me.

Please feel free to work on #15058 if @mitake hasn't get started.

[ahrtr]

Note that even K8s doesn't enable auth, it might still have some impact/concern on K8s because it changes the protocol between client and server. FYI. kubernetes/kubernetes#114403 (comment)

Let's follow the discussion in #15058