Merge pull request etcd-io#17771 from serathius/robustness-bookmarkable

Validate bookmarkable checks the last event before progress notify
ahrtr · Apr 16, 2024 · 2ff45f8 · 2ff45f8
2 parents 6f66472 + dc187ce
commit 2ff45f8
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 15 deletions.
diff --git a/tests/robustness/validate/validate_test.go b/tests/robustness/validate/validate_test.go
@@ -493,8 +493,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("b", "2", 3, true),
 				putPersistedEvent("c", "3", 4, true),
 			},
-			// TODO: Should fail as it should guarantee that all events between requested revision and bookmark are delivered
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Bookmarkable - revision non decreasing - pass",
@@ -583,8 +582,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("b", "2", 3, true),
 				putPersistedEvent("c", "3", 4, true),
 			},
-			// TODO: This should fail as bookmark lowered revision
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Bookmarkable - progress precedes event - fail",
@@ -653,8 +651,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("b", "2", 3, true),
 				putPersistedEvent("c", "3", 4, true),
 			},
-			// TODO: This should fail as there is a missing event before bookmark
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Bookmarkable - missing event matching watch before bookmark - fail",
@@ -685,8 +682,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("a", "2", 3, false),
 				putPersistedEvent("c", "3", 4, true),
 			},
-			// TODO: This should fail as there is a missing event before bookmark
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Bookmarkable - missing event matching watch with prefix before bookmark - fail",
@@ -718,8 +714,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("ab", "2", 3, true),
 				putPersistedEvent("cc", "3", 4, true),
 			},
-			// TODO: This should fail as there is a missing event before bookmark
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Reliable - all events history - pass",

diff --git a/tests/robustness/validate/watch.go b/tests/robustness/validate/watch.go
@@ -17,6 +17,7 @@ package validate
 import (
 	"errors"
 
+	"github.com/google/go-cmp/cmp"
 	"go.uber.org/zap"
 
 	"go.etcd.io/etcd/tests/v3/robustness/model"
@@ -55,11 +56,11 @@ func validateWatch(lg *zap.Logger, cfg Config, reports []report.ClientReport, ev
 		if err != nil {
 			return err
 		}
-		err = validateBookmarkable(lg, r)
-		if err != nil {
-			return err
-		}
 		if eventHistory != nil {
+			err = validateBookmarkable(lg, eventHistory, r)
+			if err != nil {
+				return err
+			}
 			err = validateReliable(lg, eventHistory, r)
 			if err != nil {
 				return err
@@ -95,17 +96,37 @@ func validateFilter(lg *zap.Logger, report report.ClientReport) (err error) {
 	return err
 }
 
-func validateBookmarkable(lg *zap.Logger, report report.ClientReport) (err error) {
+func validateBookmarkable(lg *zap.Logger, eventHistory []model.PersistedEvent, report report.ClientReport) (err error) {
 	for _, op := range report.Watch {
 		var lastProgressNotifyRevision int64
+		var gotEventBeforeProgressNotify *model.PersistedEvent
 		for _, resp := range op.Responses {
 			for _, event := range resp.Events {
 				if event.Revision <= lastProgressNotifyRevision {
 					lg.Error("Broke watch guarantee", zap.String("guarantee", "bookmarkable"), zap.Int("client", report.ClientID), zap.Int64("revision", event.Revision))
 					err = errBrokeBookmarkable
 				}
+				gotEventBeforeProgressNotify = &event.PersistedEvent
 			}
 			if resp.IsProgressNotify {
+				if gotEventBeforeProgressNotify != nil || op.Request.Revision != 0 {
+					var wantEventBeforeProgressNotify *model.PersistedEvent
+					for _, ev := range eventHistory {
+						if ev.Revision < op.Request.Revision {
+							continue
+						}
+						if ev.Revision > resp.Revision {
+							break
+						}
+						if ev.Match(op.Request) {
+							wantEventBeforeProgressNotify = &ev
+						}
+					}
+					if diff := cmp.Diff(wantEventBeforeProgressNotify, gotEventBeforeProgressNotify); diff != "" {
+						lg.Error("Broke watch guarantee", zap.String("guarantee", "bookmarkable"), zap.Int("client", report.ClientID), zap.String("diff", diff))
+						err = errBrokeBookmarkable
+					}
+				}
 				lastProgressNotifyRevision = resp.Revision
 			}
 		}