Refactor and reorder validation to avoid reporting multiple corelated failures

It doesn't make sense to report watch failure if key value operations
are not linearizable.

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>

tests/robustness/validate/operations.go

@@ -28,28 +28,21 @@ import (
 	"go.etcd.io/etcd/tests/v3/robustness/model"
 )
 
-func validateOperationsAndVisualize(t *testing.T, lg *zap.Logger, operations []porcupine.Operation, eventHistory []model.WatchEvent) func(basepath string) error {
+func validateLinearizableOperationsAndVisualize(lg *zap.Logger, operations []porcupine.Operation) (result porcupine.CheckResult, visualize func(basepath string) error) {
 	const timeout = 5 * time.Minute
 	lg.Info("Validating linearizable operations", zap.Duration("timeout", timeout))
-	result, visualize := validateLinearizableOperationAndVisualize(lg, operations, timeout)
+	result, info := porcupine.CheckOperationsVerbose(model.NonDeterministicModel, operations, timeout)
 	switch result {
 	case porcupine.Illegal:
-		t.Error("Linearization failed")
+		lg.Error("Linearization failed")
 	case porcupine.Unknown:
-		t.Error("Linearization has timed out")
+		lg.Error("Linearization has timed out")
 	case porcupine.Ok:
-		t.Log("Linearization success")
+		lg.Info("Linearization success")
-		lg.Info("Validating serializable operations")
-		validateSerializableOperations(t, operations, eventHistory)
 	default:
-		t.Fatalf("Unknown Linearization")
+		panic(fmt.Sprintf("Unknown Linearization result %s", result))
 	}
-	return visualize
+	return result, func(path string) error {
-}
-
-func validateLinearizableOperationAndVisualize(lg *zap.Logger, operations []porcupine.Operation, timeout time.Duration) (result porcupine.CheckResult, visualize func(basepath string) error) {
-	linearizable, info := porcupine.CheckOperationsVerbose(model.NonDeterministicModel, operations, timeout)
-	return linearizable, func(path string) error {
 		lg.Info("Saving visualization", zap.String("path", path))
 		err := porcupine.VisualizePath(model.NonDeterministicModel, info, path)
 		if err != nil {
@@ -59,7 +52,8 @@ func validateLinearizableOperationAndVisualize(lg *zap.Logger, operations []porc
 	}
 }
 
-func validateSerializableOperations(t *testing.T, operations []porcupine.Operation, totalEventHistory []model.WatchEvent) {
+func validateSerializableOperations(t *testing.T, lg *zap.Logger, operations []porcupine.Operation, totalEventHistory []model.WatchEvent) {
+	lg.Info("Validating serializable operations")
 	staleReads := filterSerializableReads(operations)
 	if len(staleReads) == 0 {
 		return

tests/robustness/validate/validate.go

@@ -15,20 +15,93 @@
 package validate
 
 import (
+	"fmt"
+	"sort"
 	"testing"
 
+	"github.com/anishathalye/porcupine"
+	"github.com/google/go-cmp/cmp"
 	"go.uber.org/zap"
 
+	"go.etcd.io/etcd/tests/v3/robustness/model"
 	"go.etcd.io/etcd/tests/v3/robustness/report"
 )
 
 // ValidateAndReturnVisualize returns visualize as porcupine.linearizationInfo used to generate visualization is private.
 func ValidateAndReturnVisualize(t *testing.T, lg *zap.Logger, cfg Config, reports []report.ClientReport) (visualize func(basepath string) error) {
-	eventHistory := validateWatch(t, cfg, reports)
 	patchedOperations := patchedOperationHistory(reports)
-	return validateOperationsAndVisualize(t, lg, patchedOperations, eventHistory)
+	linearizable, visualize := validateLinearizableOperationsAndVisualize(lg, patchedOperations)
+	if linearizable != porcupine.Ok {
+		lg.Error("Failed linearization, skipping further validation")
+		return visualize
+	}
+	// TODO: Don't use watch events to get event history.
+	eventHistory, err := mergeWatchEventHistory(reports)
+	if err != nil {
+		t.Errorf("Failed merging watch history to create event history, skipping further validation, err: %s", err)
+		return visualize
+	}
+	validateWatch(t, lg, cfg, reports, eventHistory)
+	validateSerializableOperations(t, lg, patchedOperations, eventHistory)
+	return visualize
 }
 
 type Config struct {
 	ExpectRevisionUnique bool
 }
+
+func mergeWatchEventHistory(reports []report.ClientReport) ([]model.WatchEvent, error) {
+	type revisionEvents struct {
+		events   []model.WatchEvent
+		revision int64
+		clientId int
+	}
+	revisionToEvents := map[int64]revisionEvents{}
+	var lastClientId = 0
+	var lastRevision int64
+	events := []model.WatchEvent{}
+	for _, r := range reports {
+		for _, op := range r.Watch {
+			for _, resp := range op.Responses {
+				for _, event := range resp.Events {
+					if event.Revision == lastRevision && lastClientId == r.ClientId {
+						events = append(events, event)
+					} else {
+						if prev, found := revisionToEvents[lastRevision]; found {
+							// This assumes that there are txn that would be observed differently by two watches.
+							// TODO: Implement merging events from multiple watches about single revision based on operations.
+							if diff := cmp.Diff(prev.events, events); diff != "" {
+								return nil, fmt.Errorf("events between clients %d and %d don't match, revision: %d, diff: %s", prev.clientId, lastClientId, lastRevision, diff)
+							}
+						} else {
+							revisionToEvents[lastRevision] = revisionEvents{clientId: lastClientId, events: events, revision: lastRevision}
+						}
+						lastClientId = r.ClientId
+						lastRevision = event.Revision
+						events = []model.WatchEvent{event}
+					}
+				}
+			}
+		}
+	}
+	if prev, found := revisionToEvents[lastRevision]; found {
+		if diff := cmp.Diff(prev.events, events); diff != "" {
+			return nil, fmt.Errorf("events between clients %d and %d don't match, revision: %d, diff: %s", prev.clientId, lastClientId, lastRevision, diff)
+		}
+	} else {
+		revisionToEvents[lastRevision] = revisionEvents{clientId: lastClientId, events: events, revision: lastRevision}
+	}
+
+	var allRevisionEvents []revisionEvents
+	for _, revEvents := range revisionToEvents {
+		allRevisionEvents = append(allRevisionEvents, revEvents)
+	}
+	sort.Slice(allRevisionEvents, func(i, j int) bool {
+		return allRevisionEvents[i].revision < allRevisionEvents[j].revision
+	})
+	var eventHistory []model.WatchEvent
+	for _, revEvents := range allRevisionEvents {
+		eventHistory = append(eventHistory, revEvents.events...)
+	}
+	return eventHistory, nil
+}

tests/robustness/validate/validate_test.go

@@ -211,7 +211,11 @@ func TestValidateWatch(t *testing.T) {
 	}
 	for _, tc := range tcs {
 		t.Run(tc.name, func(t *testing.T) {
-			validateWatch(t, Config{ExpectRevisionUnique: true}, tc.reports)
+			eventHistory, err := mergeWatchEventHistory(tc.reports)
+			if err != nil {
+				t.Fatal(err)
+			}
+			validateWatch(t, zaptest.NewLogger(t), Config{ExpectRevisionUnique: true}, tc.reports, eventHistory)
 		})
 	}
 }

tests/robustness/validate/watch.go

@@ -15,27 +15,22 @@
 package validate
 
 import (
-	"sort"
 	"testing"
 
-	"github.com/google/go-cmp/cmp"
+	"go.uber.org/zap"
 
 	"go.etcd.io/etcd/tests/v3/robustness/model"
 	"go.etcd.io/etcd/tests/v3/robustness/report"
 )
 
-func validateWatch(t *testing.T, cfg Config, reports []report.ClientReport) []model.WatchEvent {
+func validateWatch(t *testing.T, lg *zap.Logger, cfg Config, reports []report.ClientReport, eventHistory []model.WatchEvent) []model.WatchEvent {
+	lg.Info("Validating watch")
 	// Validate etcd watch properties defined in https://etcd.io/docs/v3.6/learning/api_guarantees/#watch-apis
 	for _, r := range reports {
 		validateOrdered(t, r)
 		validateUnique(t, cfg.ExpectRevisionUnique, r)
 		validateAtomic(t, r)
 		validateBookmarkable(t, r)
-	}
-	// TODO: Use linearization result instead of event history to get order of events
-	// This is currently impossible as porcupine doesn't expose operation order created during linearization.
-	eventHistory := mergeWatchEventHistory(t, reports)
-	for _, r := range reports {
 		validateReliable(t, eventHistory, r)
 		validateResumable(t, eventHistory, r)
 	}
@@ -165,59 +160,3 @@ func firstWatchEvent(op model.WatchOperation) *model.WatchEvent {
 	}
 	return nil
 }
-
-func mergeWatchEventHistory(t *testing.T, reports []report.ClientReport) []model.WatchEvent {
-	type revisionEvents struct {
-		events   []model.WatchEvent
-		revision int64
-		clientId int
-	}
-	revisionToEvents := map[int64]revisionEvents{}
-	var lastClientId = 0
-	var lastRevision int64
-	events := []model.WatchEvent{}
-	for _, r := range reports {
-		for _, op := range r.Watch {
-			for _, resp := range op.Responses {
-				for _, event := range resp.Events {
-					if event.Revision == lastRevision && lastClientId == r.ClientId {
-						events = append(events, event)
-					} else {
-						if prev, found := revisionToEvents[lastRevision]; found {
-							// This assumes that there are txn that would be observed differently by two watches.
-							// TODO: Implement merging events from multiple watches about single revision based on operations.
-							if diff := cmp.Diff(prev.events, events); diff != "" {
-								t.Errorf("Events between clients %d and %d don't match, revision: %d, diff: %s", prev.clientId, lastClientId, lastRevision, diff)
-							}
-						} else {
-							revisionToEvents[lastRevision] = revisionEvents{clientId: lastClientId, events: events, revision: lastRevision}
-						}
-						lastClientId = r.ClientId
-						lastRevision = event.Revision
-						events = []model.WatchEvent{event}
-					}
-				}
-			}
-		}
-	}
-	if prev, found := revisionToEvents[lastRevision]; found {
-		if diff := cmp.Diff(prev.events, events); diff != "" {
-			t.Errorf("Events between clients %d and %d don't match, revision: %d, diff: %s", prev.clientId, lastClientId, lastRevision, diff)
-		}
-	} else {
-		revisionToEvents[lastRevision] = revisionEvents{clientId: lastClientId, events: events, revision: lastRevision}
-	}
-
-	var allRevisionEvents []revisionEvents
-	for _, revEvents := range revisionToEvents {
-		allRevisionEvents = append(allRevisionEvents, revEvents)
-	}
-	sort.Slice(allRevisionEvents, func(i, j int) bool {
-		return allRevisionEvents[i].revision < allRevisionEvents[j].revision
-	})
-	var eventHistory []model.WatchEvent
-	for _, revEvents := range allRevisionEvents {
-		eventHistory = append(eventHistory, revEvents.events...)
-	}
-	return eventHistory
-}