From 4eaf306583f357353882666531b9fec82e00dc67 Mon Sep 17 00:00:00 2001 From: Spencer Judge Date: Tue, 30 Aug 2022 19:16:37 -0700 Subject: [PATCH 1/3] Fixes for graph logic --- service/matching/version_graph.go | 164 +++++++++++++++-------- service/matching/version_graph_test.go | 177 ++++++++++++++++++++++--- 2 files changed, 267 insertions(+), 74 deletions(-) diff --git a/service/matching/version_graph.go b/service/matching/version_graph.go index 4ee1d929ca3..a724d2090db 100644 --- a/service/matching/version_graph.go +++ b/service/matching/version_graph.go @@ -109,23 +109,26 @@ func depthLimiter(g *persistence.VersioningData, maxDepth int, noMutate bool) *w // See the API docs for more detail. In short, the graph looks like one long line of default versions, each of which // is incompatible with the previous, optionally with branching compatibility branches. Like so: // -// ─┬─1.0───2.0─┬─3.0───4.0 -// │ ├─3.1 -// │ └─3.2 -// ├─1.1 -// ├─1.2 -// └─1.3 +// ─┬─1.0───2.0─┬─3.0───4.0 +// │ ├─3.1 +// │ └─3.2 +// ├─1.1 +// ├─1.2 +// └─1.3 // // In the above graph, 4.0 is the current default, and [1.3, 3.2] is the set of current compatible leaves. Links // going left are incompatible relationships, and links going up are compatible relationships. // // A request may: -// 1. Add a new version to the graph, as a default version -// 2. Add a new version to the graph, compatible with some existing version. -// 3. Add a new version to the graph, compatible with some existing version and as the new default. -// 4. Unset a version as a default. It will be dropped and its previous incompatible version becomes default. -// 5. Unset a version as a compatible. It will be dropped and its previous compatible version will become the new -// compatible leaf for that branch. +// 1. Add a new version to the graph, as a default version +// 2. Add a new version to the graph, compatible with some existing version. +// 3. Add a new version to the graph, compatible with some existing version and as the new default. +// 4. Reorder an existing version (and it's whole compatible branch). We allow moving a node in the incompatible line +// to the front (along with its entire compatible branch), as long as the user has targeted the most recent +// version in that compatible branch. +// +// Deletions are not allowed, as it leads to confusion about what to do with open workflows who were operating on that +// version. It's better to simply add a new version (possibly with no associated workers) instead. func UpdateVersionsGraph(existingData *persistence.VersioningData, req *workflowservice.UpdateWorkerBuildIdOrderingRequest, maxSize int) error { if req.GetVersionId().GetWorkerBuildId() == "" { return serviceerror.NewInvalidArgument( @@ -155,6 +158,19 @@ func updateImpl(existingData *persistence.VersioningData, req *workflowservice.U " with any version other than the existing default is not allowed.") } if curDefault != nil { + if req.GetVersionId() == curDefault.GetVersion() { + // User is setting current default as... current default. Do nothing. + return nil + } + + didReorder, err := reorderExistingNodeIfNeeded(existingData, req.VersionId) + if err != nil { + return err + } + if didReorder { + return nil + } + // If the current default is going to be the previous compat version with the one we're adding, // then we need to skip over it when setting the previous *incompatible* version. if isCompatWithCurDefault { @@ -197,29 +213,6 @@ func updateImpl(existingData *persistence.VersioningData, req *workflowservice.U fmt.Sprintf("previous compatible version %v not found", req.GetPreviousCompatible())) } } else { - // Check if the version is already a default, and remove it from being one if it is. - curDefault := existingData.GetCurrentDefault() - if curDefault.GetVersion().Equal(req.GetVersionId()) { - existingData.CurrentDefault = nil - if curDefault.GetPreviousCompatible() != nil { - existingData.CurrentDefault = curDefault.GetPreviousCompatible() - } else if curDefault.GetPreviousIncompatible() != nil { - existingData.CurrentDefault = curDefault.GetPreviousIncompatible() - } - return nil - } - // Check if it's a compatible leaf, and remove it from being one if it is. - for i, def := range existingData.GetCompatibleLeaves() { - if def.GetVersion().Equal(req.GetVersionId()) { - existingData.CompatibleLeaves = - append(existingData.CompatibleLeaves[:i], existingData.CompatibleLeaves[i+1:]...) - if def.GetPreviousCompatible() != nil { - existingData.CompatibleLeaves = - append(existingData.CompatibleLeaves, def.GetPreviousCompatible()) - } - return nil - } - } return serviceerror.NewInvalidArgument( "requests to update build id ordering cannot create a new non-default version with no links") } @@ -227,7 +220,7 @@ func updateImpl(existingData *persistence.VersioningData, req *workflowservice.U return nil } -// Finds the node that the provided version should point at, given that it says it's compatible with the provided +// Finds the node that some new version should point at, given that it says it's compatible with the provided // version. Note that this does not necessary mean *that* node. If the version being targeted as compatible has nodes // which already point at it as their previous compatible version, that chain will be followed out to the leaf, which // will be returned. @@ -238,37 +231,104 @@ func findCompatibleNode( // First search down from all existing compatible leaves, as if any of those chains point at the desired version, // we will need to return that leaf. for ix, node := range existingData.GetCompatibleLeaves() { - if node.GetVersion().Equal(versionId) { - return node, ix - } - if findInNode(node, versionId) != nil { + if found, _, _ := findInNode(node, versionId, searchModeBoth); found != nil { return node, ix } } // Otherwise, this must be targeting some version in the default/incompatible chain, and it will become a new leaf curDefault := existingData.GetCurrentDefault() - if curDefault.GetVersion().Equal(versionId) { - return curDefault, -1 - } - if nn := findInNode(curDefault, versionId); nn != nil { + if nn, _, _ := findInNode(curDefault, versionId, searchModeBoth); nn != nil { return nn, -1 } return nil, -1 } +// The provided node wants to become the default, and thus move its compatible branch (which may be just itself) to the +// front of the incompatible list. If the node is the head of its compatible branch, this is acceptable, otherwise it is +// an error. +func reorderExistingNodeIfNeeded(existingData *persistence.VersioningData, versionId *taskqueuepb.VersionId) (bool, error) { + // First find if this node is inside a compatible branch. It could never be in more than one, since we + // do not allow forks. + for _, node := range existingData.GetCompatibleLeaves() { + if node.GetVersion().Equal(versionId) { + // We've got to find the node in the compatible branch which is pointed to by some node in the incompatible + // branch + entireCompatBranch := make(map[string]struct{}) + curCompatNode := node + for curCompatNode != nil { + entireCompatBranch[curCompatNode.GetVersion().GetWorkerBuildId()] = struct{}{} + curCompatNode = curCompatNode.GetPreviousCompatible() + } + found, nextIncompat, _ := findAnyInNode(existingData.GetCurrentDefault(), entireCompatBranch, searchModeIncompat) + if found != nil && nextIncompat != nil { + nextIncompat.PreviousIncompatible = found.PreviousIncompatible + found.PreviousIncompatible = nil + node.PreviousIncompatible = nextIncompat + existingData.CurrentDefault = node + return true, nil + } + return false, nil + } + if found, _, _ := findInNode(node, versionId, searchModeCompat); found != nil { + return false, serviceerror.NewInvalidArgument("A node which is already inside a compatible branch, but " + + "is not the leaf node of that branch, cannot be made default") + } + } + + // Now check in the incompatible branch, and if this node is found in it, shuffle its branch to the front. + found, nextIncompat, _ := findInNode(existingData.GetCurrentDefault(), versionId, searchModeIncompat) + if found != nil && nextIncompat != nil { + nextIncompat.PreviousIncompatible = found.PreviousIncompatible + found.PreviousIncompatible = existingData.GetCurrentDefault() + existingData.CurrentDefault = found + return true, nil + } + + return false, nil +} + +type searchMode int + +const ( + searchModeBoth searchMode = iota + searchModeCompat + searchModeIncompat +) + func findInNode( node *taskqueuepb.VersionIdNode, versionId *taskqueuepb.VersionId, -) *taskqueuepb.VersionIdNode { - if node.GetVersion().Equal(versionId) { - return node + mode searchMode, +) (*taskqueuepb.VersionIdNode, *taskqueuepb.VersionIdNode, *taskqueuepb.VersionIdNode) { + s := make(map[string]struct{}) + s[versionId.GetWorkerBuildId()] = struct{}{} + return _findInNode(node, s, mode, nil, nil) +} + +func findAnyInNode( + node *taskqueuepb.VersionIdNode, + versionIds map[string]struct{}, + mode searchMode, +) (*taskqueuepb.VersionIdNode, *taskqueuepb.VersionIdNode, *taskqueuepb.VersionIdNode) { + return _findInNode(node, versionIds, mode, nil, nil) +} + +func _findInNode( + node *taskqueuepb.VersionIdNode, + versionIds map[string]struct{}, + mode searchMode, + nextIncompat *taskqueuepb.VersionIdNode, + nextCompat *taskqueuepb.VersionIdNode, +) (*taskqueuepb.VersionIdNode, *taskqueuepb.VersionIdNode, *taskqueuepb.VersionIdNode) { + if _, ok := versionIds[node.GetVersion().GetWorkerBuildId()]; ok { + return node, nextIncompat, nextCompat } - if node.GetPreviousCompatible() != nil { - return findInNode(node.GetPreviousCompatible(), versionId) + if (mode == searchModeBoth || mode == searchModeCompat) && node.GetPreviousCompatible() != nil { + return _findInNode(node.GetPreviousCompatible(), versionIds, mode, nextIncompat, node) } - if node.GetPreviousIncompatible() != nil { - return findInNode(node.GetPreviousIncompatible(), versionId) + if (mode == searchModeBoth || mode == searchModeIncompat) && node.GetPreviousIncompatible() != nil { + return _findInNode(node.GetPreviousIncompatible(), versionIds, mode, node, nextCompat) } - return nil + return nil, nextIncompat, nextCompat } diff --git a/service/matching/version_graph_test.go b/service/matching/version_graph_test.go index 439144b848b..6d8bce30789 100644 --- a/service/matching/version_graph_test.go +++ b/service/matching/version_graph_test.go @@ -101,7 +101,7 @@ func TestNewDefaultGraphUpdateCompatWithCurDefault(t *testing.T) { } req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ - VersionId: mkVerId("2"), + VersionId: mkVerId("1.1"), PreviousCompatible: mkVerId("1"), BecomeDefault: true, } @@ -110,11 +110,13 @@ func TestNewDefaultGraphUpdateCompatWithCurDefault(t *testing.T) { assert.NoError(t, err) assert.True(t, data.CurrentDefault.Version.Equal(req.VersionId)) - assert.Equal(t, "2", data.CurrentDefault.Version.GetWorkerBuildId()) + assert.Equal(t, "1.1", data.CurrentDefault.Version.GetWorkerBuildId()) assert.Equal(t, n1, data.CurrentDefault.PreviousCompatible) assert.Equal(t, "1", data.CurrentDefault.PreviousCompatible.Version.GetWorkerBuildId()) assert.Equal(t, n0, data.CurrentDefault.PreviousIncompatible) assert.Equal(t, "0", data.CurrentDefault.PreviousIncompatible.Version.GetWorkerBuildId()) + // The node is *not* added to compatible leaves, since it replaces the current default. + assert.Empty(t, data.GetCompatibleLeaves()) } func TestNewDefaultGraphUpdateCompatWithSomethingElse(t *testing.T) { @@ -238,7 +240,7 @@ func TestAddingNewNodeWithNoLinksNotAllowed(t *testing.T) { assert.IsType(t, &serviceerror.InvalidArgument{}, err) } -func TestUnsetCurrentDefault(t *testing.T) { +func TestCannotUnsetDefault(t *testing.T) { n1 := mkVerIdNode("1") data := &persistencepb.VersioningData{ CurrentDefault: n1, @@ -249,56 +251,186 @@ func TestUnsetCurrentDefault(t *testing.T) { } err := UpdateVersionsGraph(data, req, 0) - assert.NoError(t, err) - - assert.Nil(t, data.CurrentDefault) + assert.Error(t, err) } -func TestUnsetCurrentDefaultPreviousIncompatBecomesDefault(t *testing.T) { +func TestDropMiddleIncomat(t *testing.T) { n0 := mkVerIdNode("0") n1 := mkVerIdNode("1") n1.PreviousIncompatible = n0 + n2 := mkVerIdNode("2") + n2.PreviousIncompatible = n1 data := &persistencepb.VersioningData{ - CurrentDefault: n1, + CurrentDefault: n2, } req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ VersionId: mkVerId("1"), } + err := UpdateVersionsGraph(data, req, 0) + assert.Error(t, err) +} + +func TestAddingSameVersionAsDefaultAgainIsIdempotent(t *testing.T) { + data := &persistencepb.VersioningData{} + + req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("1"), + BecomeDefault: true, + } err := UpdateVersionsGraph(data, req, 0) assert.NoError(t, err) - assert.True(t, data.CurrentDefault.Version.Equal(n0.Version)) - assert.Equal(t, "0", data.CurrentDefault.Version.GetWorkerBuildId()) + err = UpdateVersionsGraph(data, req, 0) + assert.NoError(t, err) + assert.Nil(t, data.CurrentDefault.GetPreviousIncompatible()) } -func TestUnsetCurrentDefaultPreviousCompatBecomesDefault(t *testing.T) { +func TestReorderMiddleIncompat(t *testing.T) { + n0 := mkVerIdNode("0") + n1 := mkVerIdNode("1") + n1.PreviousIncompatible = n0 + n2 := mkVerIdNode("2") + n2.PreviousIncompatible = n1 + data := &persistencepb.VersioningData{ + CurrentDefault: n2, + } + + req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("1"), + BecomeDefault: true, + } + + err := UpdateVersionsGraph(data, req, 0) + assert.NoError(t, err) + assert.Equal(t, "1", data.CurrentDefault.Version.GetWorkerBuildId()) + assert.Equal(t, "2", data.CurrentDefault.PreviousIncompatible.Version.GetWorkerBuildId()) + assert.Equal(t, "0", data.CurrentDefault.PreviousIncompatible.PreviousIncompatible.Version.GetWorkerBuildId()) + assert.Nil(t, data.CurrentDefault.PreviousIncompatible.PreviousIncompatible.PreviousIncompatible) +} + +func TestAddExistingVerInCompatBranchAsNewDefault(t *testing.T) { n0 := mkVerIdNode("0") n1 := mkVerIdNode("1") n1d1 := mkVerIdNode("1.1") n1.PreviousIncompatible = n0 n1d1.PreviousCompatible = n1 + n2 := mkVerIdNode("2") + n2.PreviousIncompatible = n1 data := &persistencepb.VersioningData{ - CurrentDefault: n1d1, + CurrentDefault: n2, + CompatibleLeaves: []*taskqueuepb.VersionIdNode{n1d1}, } req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ - VersionId: mkVerId("1.1"), + VersionId: mkVerId("1.1"), + BecomeDefault: true, } + err := UpdateVersionsGraph(data, req, 0) + assert.NoError(t, err) + + assert.Equal(t, "1.1", data.CurrentDefault.Version.GetWorkerBuildId()) + assert.Equal(t, "2", data.CurrentDefault.PreviousIncompatible.Version.GetWorkerBuildId()) + assert.Equal(t, "1", data.CurrentDefault.PreviousCompatible.Version.GetWorkerBuildId()) + assert.Equal(t, "0", data.CurrentDefault.PreviousIncompatible.PreviousIncompatible.Version.GetWorkerBuildId()) + assert.Nil(t, data.CurrentDefault.PreviousIncompatible.PreviousIncompatible.PreviousIncompatible) + req = &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("1.2"), + PreviousCompatible: mkVerId("1.1"), + } + err = UpdateVersionsGraph(data, req, 0) + assert.NoError(t, err) + assert.Equal(t, "1.1", data.CurrentDefault.Version.GetWorkerBuildId()) + assert.Equal(t, "1.2", data.CompatibleLeaves[0].Version.GetWorkerBuildId()) + assert.Equal(t, "1.1", data.CompatibleLeaves[0].PreviousCompatible.Version.GetWorkerBuildId()) +} + +func setupCross(t *testing.T) *persistencepb.VersioningData { + data := &persistencepb.VersioningData{} + req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("0"), + BecomeDefault: true, + } + err := UpdateVersionsGraph(data, req, 0) + assert.NoError(t, err) + req = &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("1"), + BecomeDefault: true, + } + err = UpdateVersionsGraph(data, req, 0) + assert.NoError(t, err) + req = &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("1.1"), + PreviousCompatible: mkVerId("1"), + BecomeDefault: true, + } + err = UpdateVersionsGraph(data, req, 0) + assert.NoError(t, err) + req = &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("2"), + BecomeDefault: true, + } + err = UpdateVersionsGraph(data, req, 0) + assert.NoError(t, err) + req = &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("1.2"), + PreviousCompatible: mkVerId("1.1"), + } + err = UpdateVersionsGraph(data, req, 0) + assert.NoError(t, err) + + assert.Equal(t, "2", data.CurrentDefault.Version.GetWorkerBuildId()) + assert.Equal(t, "1.1", data.CurrentDefault.PreviousIncompatible.Version.GetWorkerBuildId()) + assert.Equal(t, "1.2", data.CompatibleLeaves[0].Version.GetWorkerBuildId()) + return data +} + +// This test makes the following change to the graph: +// +// 1.0 1.0 +// │ │ +// │ │ +// 0.0────1.1────2.0 1.1 +// │ │ +// │ │ +// 1.2 0.0────2.0────1.2 +func TestMoveCompatBranchPointedInMiddleToBeDefault(t *testing.T) { + data := setupCross(t) + + // Move the 1.x compat branch to the front + req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("1.2"), + BecomeDefault: true, + } err := UpdateVersionsGraph(data, req, 0) assert.NoError(t, err) - assert.True(t, data.CurrentDefault.Version.Equal(n1.Version)) - assert.Equal(t, "1", data.CurrentDefault.Version.GetWorkerBuildId()) + assert.Equal(t, "1.2", data.CurrentDefault.Version.GetWorkerBuildId()) + assert.Equal(t, "1.1", data.CurrentDefault.PreviousCompatible.Version.GetWorkerBuildId()) + assert.Equal(t, "2", data.CurrentDefault.PreviousIncompatible.Version.GetWorkerBuildId()) + assert.Equal(t, "0", data.CurrentDefault.PreviousIncompatible.PreviousIncompatible.Version.GetWorkerBuildId()) +} + +func TestCannotMoveOlderCompatVersionToBeDefault(t *testing.T) { + data := setupCross(t) + + // It's not allowed to make 1.1 the default, since that would really make 1.2 the real default, because + // it's compat with 1.1, but that's not what the user said, and we want to be explicit. + req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: mkVerId("1.1"), + BecomeDefault: true, + } + err := UpdateVersionsGraph(data, req, 0) + assert.Error(t, err) } -func TestDropCompatibleLeaf(t *testing.T) { +func TestDoesNotForkCompatBranch(t *testing.T) { n0 := mkVerIdNode("0") n1 := mkVerIdNode("1") - n1d1 := mkVerIdNode("1.1") n1.PreviousIncompatible = n0 + n1d1 := mkVerIdNode("1.1") n1d1.PreviousCompatible = n1 data := &persistencepb.VersioningData{ CurrentDefault: n1, @@ -306,16 +438,15 @@ func TestDropCompatibleLeaf(t *testing.T) { } req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ - VersionId: mkVerId("1.1"), + VersionId: mkVerId("1.2"), + PreviousCompatible: mkVerId("1"), } err := UpdateVersionsGraph(data, req, 0) assert.NoError(t, err) - - assert.True(t, data.CurrentDefault.Version.Equal(n1.Version)) + assert.Equal(t, "1.2", data.CompatibleLeaves[0].Version.GetWorkerBuildId()) + assert.Equal(t, "1.1", data.CompatibleLeaves[0].PreviousCompatible.Version.GetWorkerBuildId()) assert.Equal(t, "1", data.CurrentDefault.Version.GetWorkerBuildId()) - assert.Equal(t, 1, len(data.CompatibleLeaves)) - assert.Equal(t, "1", data.CompatibleLeaves[0].Version.GetWorkerBuildId()) } func TestCompatibleTargetsNotFound(t *testing.T) { @@ -382,6 +513,8 @@ func TestLimitsMaxSize(t *testing.T) { assert.Equal(t, mkVerId("50.24"), lastNode.GetVersion()) } +// TODO: Fuzz no forks -- any node can only be pointed at by one incompatible and one compat node + func FuzzVersionGraphEnsureNoSameTypeDefaults(f *testing.F) { f.Fuzz(func(t *testing.T, numUpdates, willPickCompatMod, compatModTarget uint8) { addedNodes := make([]*taskqueuepb.VersionId, 0, numUpdates) From 0bf591fb99574a364a28927ea69994944a817870 Mon Sep 17 00:00:00 2001 From: Spencer Judge Date: Wed, 31 Aug 2022 13:30:00 -0700 Subject: [PATCH 2/3] Fuzz verification --- service/matching/version_graph.go | 35 ++++++++++- service/matching/version_graph_test.go | 83 ++++++++++++++++++++++++-- 2 files changed, 113 insertions(+), 5 deletions(-) diff --git a/service/matching/version_graph.go b/service/matching/version_graph.go index a724d2090db..c45872619aa 100644 --- a/service/matching/version_graph.go +++ b/service/matching/version_graph.go @@ -25,6 +25,7 @@ package matching import ( + "bytes" "encoding/binary" "fmt" @@ -154,7 +155,7 @@ func updateImpl(existingData *persistence.VersioningData, req *workflowservice.U // It does not make sense to introduce a version which is the new overall default, but is somehow also // supposed to be compatible with some existing version, as that would necessarily imply that the newly // added version is somehow both compatible and incompatible with the same target version. - return serviceerror.NewInvalidArgument("adding a new default version which is compatible " + + return serviceerror.NewInvalidArgument("adding a new default version which is compatible" + " with any version other than the existing default is not allowed.") } if curDefault != nil { @@ -332,3 +333,35 @@ func _findInNode( } return nil, nextIncompat, nextCompat } + +// For graph visualization purposes while debugging +func toDot(data *persistence.VersioningData) string { + var buf bytes.Buffer + buf.WriteString("digraph {\n") + buf.WriteString("// Default\n") + node := data.GetCurrentDefault() + buf.WriteString(fmt.Sprintf(" %s [label=\"%s\"];\n", node.GetVersion().GetWorkerBuildId(), node.GetVersion().GetWorkerBuildId())) + buf.WriteString("// Compatible leafs\n") + for _, node := range data.GetCompatibleLeaves() { + buf.WriteString(fmt.Sprintf(" %s [label=\"%s\"];\n", node.GetVersion().GetWorkerBuildId(), node.GetVersion().GetWorkerBuildId())) + } + + for _, node := range data.GetCompatibleLeaves() { + writeEdgesForNode(&buf, node) + } + writeEdgesForNode(&buf, data.GetCurrentDefault()) + + buf.WriteString("}\n") + return buf.String() +} + +func writeEdgesForNode(buf *bytes.Buffer, node *taskqueuepb.VersionIdNode) { + if node.GetPreviousCompatible() != nil { + buf.WriteString(fmt.Sprintf(" %s -> %s [label=\"compat\"];\n", node.GetVersion().GetWorkerBuildId(), node.GetPreviousCompatible().GetVersion().GetWorkerBuildId())) + writeEdgesForNode(buf, node.GetPreviousCompatible()) + } + if node.GetPreviousIncompatible() != nil { + buf.WriteString(fmt.Sprintf(" %s -> %s [label=\"incompat\"];\n", node.GetVersion().GetWorkerBuildId(), node.GetPreviousIncompatible().GetVersion().GetWorkerBuildId())) + writeEdgesForNode(buf, node.GetPreviousIncompatible()) + } +} diff --git a/service/matching/version_graph_test.go b/service/matching/version_graph_test.go index 6d8bce30789..48f98b5b133 100644 --- a/service/matching/version_graph_test.go +++ b/service/matching/version_graph_test.go @@ -513,11 +513,16 @@ func TestLimitsMaxSize(t *testing.T) { assert.Equal(t, mkVerId("50.24"), lastNode.GetVersion()) } -// TODO: Fuzz no forks -- any node can only be pointed at by one incompatible and one compat node +type pointedAtBy struct { + byIncompat bool + byCompat bool +} -func FuzzVersionGraphEnsureNoSameTypeDefaults(f *testing.F) { - f.Fuzz(func(t *testing.T, numUpdates, willPickCompatMod, compatModTarget uint8) { +func FuzzVersionGraphNeverHasForks(f *testing.F) { + f.Fuzz(func(t *testing.T, + numUpdates, willPickCompatMod, compatModTarget, numAddExistings, addExistingMod uint8, allowErrors bool) { addedNodes := make([]*taskqueuepb.VersionId, 0, numUpdates) + inSomeCompatBranch := make(map[string]struct{}) data := &persistencepb.VersioningData{} for i := uint8(0); i < numUpdates; i++ { @@ -530,12 +535,82 @@ func FuzzVersionGraphEnsureNoSameTypeDefaults(f *testing.F) { numUpdates%willPickCompatMod == 0 && uint8(len(addedNodes)) > numUpdates%compatModTarget { compatTarget := addedNodes[numUpdates%compatModTarget] + //t.Log(fmt.Sprintf("Setting %s to be compatible with %s", id, compatTarget)) req.PreviousCompatible = compatTarget + if !allowErrors { + inSomeCompatBranch[id.GetWorkerBuildId()] = struct{}{} + req.BecomeDefault = false + } } addedNodes = append(addedNodes, id) err := UpdateVersionsGraph(data, req, 0) - assert.NoError(t, err) + if !allowErrors { + assert.NoError(t, err) + } assert.NotNil(t, ToBuildIdOrderingResponse(data, 0)) } + + if len(addedNodes) > 0 { + for i := uint8(0); i < numAddExistings; i++ { + if addExistingMod == 0 { + continue + } + existing := addedNodes[uint8(len(addedNodes)-1)%addExistingMod] + req := &workflowservice.UpdateWorkerBuildIdOrderingRequest{ + VersionId: existing, + BecomeDefault: true, + } + if !allowErrors { + // Only nodes which are a leaf compat node, or not in a compat branch, can be shuffled to front + _, isInCompatBranch := inSomeCompatBranch[existing.GetWorkerBuildId()] + isLeaf := false + for _, node := range data.GetCompatibleLeaves() { + if node.GetVersion().GetWorkerBuildId() == existing.GetWorkerBuildId() { + isLeaf = true + break + } + } + //t.Log(existing.GetWorkerBuildId(), isInCompatBranch, isLeaf, toDot(data)) + if isInCompatBranch && !isLeaf { + continue + } + } + err := UpdateVersionsGraph(data, req, 0) + if !allowErrors { + assert.NoError(t, err) + } + } + } + + // Verify no node is pointed at by more than 1 incompat note and 1 compat node + pointedAt := make(map[string]*pointedAtBy) + visitor := func(node *taskqueuepb.VersionIdNode) { + bid := node.GetVersion().GetWorkerBuildId() + existingVal, inMap := pointedAt[bid] + if !inMap { + pointedAt[bid] = &pointedAtBy{ + byCompat: false, + byIncompat: false, + } + existingVal = pointedAt[bid] + } + if node.GetPreviousCompatible() != nil { + if existingVal.byCompat { + f.Errorf("Node %s is pointed at by more than 1 compat node", bid) + } + pointedAt[bid].byCompat = true + } + if node.GetPreviousIncompatible() != nil { + if existingVal.byIncompat { + f.Errorf("Node %s is pointed at by more than 1 incompat node", bid) + } + pointedAt[bid].byIncompat = true + } + } + visitor(data.GetCurrentDefault()) + for _, node := range data.GetCompatibleLeaves() { + visitor(node) + } + }) } From ae2e7069624d7c150bc2082676e159db2db144de Mon Sep 17 00:00:00 2001 From: Spencer Judge Date: Wed, 31 Aug 2022 13:41:16 -0700 Subject: [PATCH 3/3] Lint fix --- service/matching/version_graph.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/service/matching/version_graph.go b/service/matching/version_graph.go index c45872619aa..01dbdc0c0ec 100644 --- a/service/matching/version_graph.go +++ b/service/matching/version_graph.go @@ -335,6 +335,8 @@ func _findInNode( } // For graph visualization purposes while debugging +// +//lint:ignore U1000 Only needed for debugging purposes func toDot(data *persistence.VersioningData) string { var buf bytes.Buffer buf.WriteString("digraph {\n")