Merge ef61fb604e5e3d5463cdcae8e324502e3416572d into c86c93ca2951338115159dcdd20711603044e1f1

tests/robustness/validate/validate_test.go

@@ -1782,6 +1782,67 @@ func TestValidateWatch(t *testing.T) {
 			},
 			expectError: errBrokePrevKV.Error(),
 		},
+		{
+			name: "PrevKV - all previous values, with compaction, followed by a deletion, delete event should have prevKV=nil - pass",
+			reports: []report.ClientReport{
+				{
+					Watch: []model.WatchOperation{
+						{
+							Request: model.WatchRequest{
+								Key:        "a",
+								WithPrevKV: true,
+							},
+							Responses: []model.WatchResponse{
+								{
+									Events: []model.WatchEvent{
+										putWatchEvent("a", "1", 2, true),
+										putWatchEventWithPrevKV("a", "2", 3, false, "1", 2),
+										// < compaction occurs here >
+										deleteWatchEvent("a", 4), // create delete without prevKV because compaction
+										putWatchEvent("a", "4", 5, true),
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			persistedRequests: []model.EtcdRequest{
+				putRequest("a", "1"),
+				putRequest("a", "2"),
+				compactRequest(3),
+				deleteRequest("a"),
+				putRequest("a", "4"),
+			},
+		},
+		{
+			name: "PrevKV - no compaction, prevKV is non-nil and event is create event - fail",
+			reports: []report.ClientReport{
+				{
+					Watch: []model.WatchOperation{
+						{
+							Request: model.WatchRequest{
+								Key:        "a",
+								WithPrevKV: true,
+							},
+							Responses: []model.WatchResponse{
+								{
+									Events: []model.WatchEvent{
+										putWatchEvent("a", "1", 2, true),
+										putWatchEventWithPrevKV("a", "2", 3, true, "1", 2),
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			persistedRequests: []model.EtcdRequest{
+				putRequest("a", "1"),
+				putRequest("a", "2"),
+			},
+			expectError: errBrokePrevKV.Error(),
+		},
 		{
 			name: "Filter - event not matching the watch - fail",
 			reports: []report.ClientReport{
@@ -1959,3 +2020,12 @@ func deleteRequest(key string) model.EtcdRequest {
 		Defragment: nil,
 	}
 }
+
+func compactRequest(rv int64) model.EtcdRequest {
+	return model.EtcdRequest{
+		Type: model.Compact,
+		Compact: &model.CompactRequest{
+			Revision: rv,
+		},
+	}
+}

tests/robustness/validate/watch.go

@@ -245,26 +245,31 @@ func validatePrevKV(lg *zap.Logger, replay *model.EtcdReplay, report report.Clie
 		}
 		for _, resp := range op.Responses {
 			for _, event := range resp.Events {
-				// Get state state just before the current event.
+				// Get state just before the current event.
-				state, err2 := replay.StateForRevision(event.Revision - 1)
+				prevState, err2 := replay.StateForRevision(event.Revision - 1)
 				if err2 != nil {
 					panic(err2)
 				}
-				// TODO(MadhavJivrajani): check if compaction has been run as part
-				// of failpoint injection. If compaction has run, prevKV can be nil
-				// even if it is not a create event.
-				//
-				// Considering that Kubernetes opens watches to etcd using WithPrevKV()
-				// option, ideally we would want to explicitly check the condition that
-				// Kubernetes does while parsing events received from etcd:
-				// https://github.com/kubernetes/kubernetes/blob/a9e4f5b7862e84c4152eabe2e960f3f6fb9a4867/staging/src/k8s.io/apiserver/pkg/storage/etcd3/event.go#L59
-				// i.e. prevKV is nil iff the event is a create event, we cannot reliably
-				// check that without knowing if compaction has run.
 
-				// We allow PrevValue to be nil since in the face of compaction, etcd does not
+				// Considering that Kubernetes opens watches to etcd using WithPrevKV() option, we explicitly
-				// guarantee its presence.
+				// check the condition that Kubernetes does while parsing events received from etcd:
-				if event.PrevValue != nil && *event.PrevValue != state.KeyValues[event.Key] {
+				// https://github.com/kubernetes/kubernetes/blob/a9e4f5b7862e84c4152eabe2e960f3f6fb9a4867/staging/src/k8s.io/apiserver/pkg/storage/etcd3/event.go#L59
-					lg.Error("Incorrect event prevValue field", zap.Int("client", report.ClientID), zap.Any("event", event), zap.Any("previousValue", state.KeyValues[event.Key]))
+				// i.e. for create events, prevKV is nil.
+				if event.IsCreate && event.PrevValue != nil {
+					lg.Error("prevValue field shold be nil in create event", zap.Int("client", report.ClientID), zap.Any("event", event), zap.Any("previousValue", prevState.KeyValues[event.Key]))
+					err = errBrokePrevKV
+				}
+
+				// If compaction took place in the previous state, prevKV should be nil for all types of events in the current state:
+				// https://github.com/etcd-io/etcd/blob/43d675997772bee9cf9d6bd4804ef05cc0cecb01/api/etcdserverpb/rpc.proto#L799-L801
+				if prevState.CompactRevision >= 0 && event.PrevValue != nil {
+					lg.Error("prevValue field should be nil after compaction in any event", zap.Int("client", report.ClientID), zap.Any("event", event), zap.Any("previousValue", prevState.KeyValues[event.Key]), zap.Int64("CompactionRevision", prevState.CompactRevision))
+					err = errBrokePrevKV
+				}
+
+				// Validate that PrevValue actually matches value of the key in the previous state.
+				if event.PrevValue != nil && *event.PrevValue != prevState.KeyValues[event.Key] {
+					lg.Error("Incorrect event prevValue field", zap.Int("client", report.ClientID), zap.Any("event", event), zap.Any("previousValue", prevState.KeyValues[event.Key]))
 					err = errBrokePrevKV
 				}
 			}