Merge branch 'pks-drop-routing-with-additional-repository-ff' into 'master'

praefect: Drop feature flag that fixes routing with additional repos

Closes #5134

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5838

Merged-by: James Fargher <proglottis@gmail.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

4 files changed, 72 insertions, 178 deletions

diff --git a/internal/featureflag/ff_fix_routing_with_additional_repository.go b/internal/featureflag/ff_fix_routing_with_additional_repository.go
deleted file mode 100644
index 2cd2b63dd..000000000
--- a/internal/featureflag/ff_fix_routing_with_additional_repository.go
+++ /dev/null
@@ -1,13 +0,0 @@
-package featureflag
-
-// FixRoutingWithAdditionalRepository fixes routing of repository-creating RPCs that have an
-// additional repository set in their request that is to be rewritten. Previously, it could have
-// happened that the new repository was routed to nodes that didn't have the additional repo. With
-// the flag enabled, Praefect always routes repository-creating RPCs to the same node that the
-// additional repository is assigned to.
-var FixRoutingWithAdditionalRepository = NewFeatureFlag(
-	"fix_routing_with_additional_repository",
-	"v16.0.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/5134",
-	false,
-)
diff --git a/internal/praefect/router_per_repository.go b/internal/praefect/router_per_repository.go
index 63070ecfb..56726e4e5 100644
--- a/internal/praefect/router_per_repository.go
+++ b/internal/praefect/router_per_repository.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/nodes"
@@ -307,7 +306,7 @@ func (r *PerRepositoryRouter) assignRepositoryToNodes(
 	replicationFactor := r.defaultReplicationFactors[virtualStorage]
 
 	switch {
-	case featureflag.FixRoutingWithAdditionalRepository.IsEnabled(ctx) && additionalRepoMetadata != nil:
+	case additionalRepoMetadata != nil:
 		// RPCs that create repositories can have an additional repository. This repository
 		// is used as additional input and is expected to be directly accessible on the
 		// target node, or otherwise we wouldn't have to resolve its relative path.
diff --git a/internal/praefect/router_per_repository_test.go b/internal/praefect/router_per_repository_test.go
index e4c032aa9..0edebace0 100644
--- a/internal/praefect/router_per_repository_test.go
+++ b/internal/praefect/router_per_repository_test.go
@@ -7,7 +7,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/commonerr"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/datastore"
@@ -665,18 +664,8 @@ func TestPerRepositoryRouter_RouteRepositoryMaintenance(t *testing.T) {
 
 func TestPerRepositoryRouterRouteRepositoryCreation(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FixRoutingWithAdditionalRepository).Run(t, testPerRepositoryRouterRouteRepositoryCreation)
-}
 
-func withOrWithoutRoutingFix[T any](ctx context.Context, enabled, disabled T) T {
-	if featureflag.FixRoutingWithAdditionalRepository.IsEnabled(ctx) {
-		return enabled
-	}
-	return disabled
-}
-
-func testPerRepositoryRouterRouteRepositoryCreation(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	configuredNodes := map[string][]string{
 		"virtual-storage-1": {"primary", "secondary-1", "secondary-2"},
@@ -869,33 +858,21 @@ func testPerRepositoryRouterRouteRepositoryCreation(t *testing.T, ctx context.Co
 			setupRepoStore: func(t *testing.T, rs datastore.RepositoryStore) {
 				require.NoError(t, rs.CreateRepository(ctx, 1, "virtual-storage-1", "additional-relative-path", "something", "primary", nil, nil, true, true))
 			},
-			virtualStorage:            "virtual-storage-1",
-			additionalRelativePath:    "additional-relative-path",
-			healthyNodes:              StaticHealthChecker{"virtual-storage-1": {"secondary-1", "secondary-2"}},
-			primaryPick:               1,
-			expectedPrimaryCandidates: withOrWithoutRoutingFix(ctx, nil, []int{2}),
-			expectedErr:               withOrWithoutRoutingFix(ctx, nodes.ErrPrimaryNotHealthy, nil),
-			expectedRoute: withOrWithoutRoutingFix(ctx, nil, requireOneOf(
-				RepositoryMutatorRoute{
-					RepositoryID:          1,
-					ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-					AdditionalReplicaPath: "something",
-					Primary:               RouterNode{Storage: "secondary-2", Connection: secondary2Conn},
-					Secondaries:           []RouterNode{{Storage: "secondary-1", Connection: secondary1Conn}},
-					ReplicationTargets:    []string{"primary"},
-				},
-			)),
+			virtualStorage:         "virtual-storage-1",
+			additionalRelativePath: "additional-relative-path",
+			healthyNodes:           StaticHealthChecker{"virtual-storage-1": {"secondary-1", "secondary-2"}},
+			primaryPick:            1,
+			expectedErr:            nodes.ErrPrimaryNotHealthy,
 		},
 		{
 			desc: "additional repo without secondaries with explicit replication factor",
 			setupRepoStore: func(t *testing.T, rs datastore.RepositoryStore) {
 				require.NoError(t, rs.CreateRepository(ctx, 1, "virtual-storage-1", "additional-relative-path", "something", "primary", nil, nil, true, true))
 			},
-			virtualStorage:            "virtual-storage-1",
-			additionalRelativePath:    "additional-relative-path",
-			healthyNodes:              StaticHealthChecker{"virtual-storage-1": {"primary"}},
-			replicationFactor:         1,
-			expectedPrimaryCandidates: withOrWithoutRoutingFix(ctx, nil, []int{1}),
+			virtualStorage:         "virtual-storage-1",
+			additionalRelativePath: "additional-relative-path",
+			healthyNodes:           StaticHealthChecker{"virtual-storage-1": {"primary"}},
+			replicationFactor:      1,
 			expectedRoute: requireOneOf(
 				RepositoryMutatorRoute{
 					RepositoryID:          1,
@@ -910,26 +887,16 @@ func testPerRepositoryRouterRouteRepositoryCreation(t *testing.T, ctx context.Co
 			setupRepoStore: func(t *testing.T, rs datastore.RepositoryStore) {
 				require.NoError(t, rs.CreateRepository(ctx, 1, "virtual-storage-1", "additional-relative-path", "something", "primary", nil, nil, true, true))
 			},
-			virtualStorage:            "virtual-storage-1",
-			additionalRelativePath:    "additional-relative-path",
-			healthyNodes:              StaticHealthChecker{"virtual-storage-1": {"primary"}},
-			expectedPrimaryCandidates: withOrWithoutRoutingFix(ctx, nil, []int{1}),
+			virtualStorage:         "virtual-storage-1",
+			additionalRelativePath: "additional-relative-path",
+			healthyNodes:           StaticHealthChecker{"virtual-storage-1": {"primary"}},
 			expectedRoute: requireOneOf(
-				withOrWithoutRoutingFix(ctx,
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
-					},
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
-						ReplicationTargets:    []string{"secondary-1", "secondary-2"},
-					},
-				),
+				RepositoryMutatorRoute{
+					RepositoryID:          1,
+					ReplicaPath:           praefectutil.DeriveReplicaPath(1),
+					AdditionalReplicaPath: "something",
+					Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
+				},
 			),
 		},
 		{
@@ -939,34 +906,21 @@ func testPerRepositoryRouterRouteRepositoryCreation(t *testing.T, ctx context.Co
 					"secondary-1", "secondary-2",
 				}, nil, true, false))
 			},
-			virtualStorage:            "virtual-storage-1",
-			additionalRelativePath:    "additional-relative-path",
-			healthyNodes:              configuredNodes,
-			primaryPick:               1,
-			expectedPrimaryCandidates: withOrWithoutRoutingFix(ctx, nil, []int{3}),
+			virtualStorage:         "virtual-storage-1",
+			additionalRelativePath: "additional-relative-path",
+			healthyNodes:           configuredNodes,
+			primaryPick:            1,
 			expectedRoute: requireOneOf(
-				withOrWithoutRoutingFix(ctx,
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
-						Secondaries: []RouterNode{
-							{Storage: "secondary-1", Connection: secondary1Conn},
-							{Storage: "secondary-2", Connection: secondary2Conn},
-						},
-					},
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "secondary-1", Connection: secondary1Conn},
-						Secondaries: []RouterNode{
-							{Storage: "primary", Connection: primaryConn},
-							{Storage: "secondary-2", Connection: secondary2Conn},
-						},
+				RepositoryMutatorRoute{
+					RepositoryID:          1,
+					ReplicaPath:           praefectutil.DeriveReplicaPath(1),
+					AdditionalReplicaPath: "something",
+					Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
+					Secondaries: []RouterNode{
+						{Storage: "secondary-1", Connection: secondary1Conn},
+						{Storage: "secondary-2", Connection: secondary2Conn},
 					},
-				),
+				},
 			),
 		},
 		{
@@ -976,30 +930,19 @@ func testPerRepositoryRouterRouteRepositoryCreation(t *testing.T, ctx context.Co
 					"secondary-1", "secondary-2",
 				}, nil, true, true))
 			},
-			virtualStorage:            "virtual-storage-1",
-			additionalRelativePath:    "additional-relative-path",
-			healthyNodes:              StaticHealthChecker{"virtual-storage-1": {"primary", "secondary-2"}},
-			primaryPick:               1,
-			expectedPrimaryCandidates: withOrWithoutRoutingFix(ctx, nil, []int{2}),
+			virtualStorage:         "virtual-storage-1",
+			additionalRelativePath: "additional-relative-path",
+			healthyNodes:           StaticHealthChecker{"virtual-storage-1": {"primary", "secondary-2"}},
+			primaryPick:            1,
 			expectedRoute: requireOneOf(
-				withOrWithoutRoutingFix(ctx,
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
-						Secondaries:           []RouterNode{{Storage: "secondary-2", Connection: secondary2Conn}},
-						ReplicationTargets:    []string{"secondary-1"},
-					},
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "secondary-2", Connection: secondary2Conn},
-						Secondaries:           []RouterNode{{Storage: "primary", Connection: primaryConn}},
-						ReplicationTargets:    []string{"secondary-1"},
-					},
-				),
+				RepositoryMutatorRoute{
+					RepositoryID:          1,
+					ReplicaPath:           praefectutil.DeriveReplicaPath(1),
+					AdditionalReplicaPath: "something",
+					Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
+					Secondaries:           []RouterNode{{Storage: "secondary-2", Connection: secondary2Conn}},
+					ReplicationTargets:    []string{"secondary-1"},
+				},
 			),
 		},
 		{
@@ -1014,32 +957,19 @@ func testPerRepositoryRouterRouteRepositoryCreation(t *testing.T, ctx context.Co
 				require.NoError(t, rs.SetGeneration(ctx, 1, "secondary-1", "additional-relative-path", 3))
 				require.NoError(t, rs.SetGeneration(ctx, 1, "secondary-2", "additional-relative-path", 10))
 			},
-			virtualStorage:            "virtual-storage-1",
-			additionalRelativePath:    "additional-relative-path",
-			healthyNodes:              configuredNodes,
-			primaryPick:               0,
-			expectedPrimaryCandidates: withOrWithoutRoutingFix(ctx, nil, []int{3}),
+			virtualStorage:         "virtual-storage-1",
+			additionalRelativePath: "additional-relative-path",
+			healthyNodes:           configuredNodes,
+			primaryPick:            0,
 			expectedRoute: requireOneOf(
-				withOrWithoutRoutingFix(ctx,
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
-						Secondaries:           []RouterNode{{Storage: "secondary-2", Connection: secondary2Conn}},
-						ReplicationTargets:    []string{"secondary-1"},
-					},
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
-						Secondaries: []RouterNode{
-							{Storage: "secondary-1", Connection: secondary1Conn},
-							{Storage: "secondary-2", Connection: secondary2Conn},
-						},
-					},
-				),
+				RepositoryMutatorRoute{
+					RepositoryID:          1,
+					ReplicaPath:           praefectutil.DeriveReplicaPath(1),
+					AdditionalReplicaPath: "something",
+					Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
+					Secondaries:           []RouterNode{{Storage: "secondary-2", Connection: secondary2Conn}},
+					ReplicationTargets:    []string{"secondary-1"},
+				},
 			),
 		},
 		{
@@ -1050,42 +980,22 @@ func testPerRepositoryRouterRouteRepositoryCreation(t *testing.T, ctx context.Co
 					"secondary-2",
 				}, nil, true, true))
 			},
-			virtualStorage:              "virtual-storage-1",
-			additionalRelativePath:      "additional-relative-path",
-			replicationFactor:           2,
-			primaryPick:                 1,
-			expectedPrimaryCandidates:   withOrWithoutRoutingFix(ctx, nil, []int{3}),
-			expectedSecondaryCandidates: withOrWithoutRoutingFix(ctx, nil, []int{2}),
-			healthyNodes:                configuredNodes,
-			expectedRoute: withOrWithoutRoutingFix(ctx,
-				requireOneOf(
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
-						Secondaries: []RouterNode{
-							{Storage: "secondary-1", Connection: secondary1Conn},
-							{Storage: "secondary-2", Connection: secondary2Conn},
-						},
-					},
-				),
-				requireOneOf(
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "secondary-1", Connection: secondary1Conn},
-						Secondaries:           []RouterNode{{Storage: "primary", Connection: primaryConn}},
-					},
-					RepositoryMutatorRoute{
-						RepositoryID:          1,
-						ReplicaPath:           praefectutil.DeriveReplicaPath(1),
-						AdditionalReplicaPath: "something",
-						Primary:               RouterNode{Storage: "secondary-1", Connection: secondary1Conn},
-						Secondaries:           []RouterNode{{Storage: "secondary-2", Connection: secondary2Conn}},
+			virtualStorage:         "virtual-storage-1",
+			additionalRelativePath: "additional-relative-path",
+			replicationFactor:      2,
+			primaryPick:            1,
+			healthyNodes:           configuredNodes,
+			expectedRoute: requireOneOf(
+				RepositoryMutatorRoute{
+					RepositoryID:          1,
+					ReplicaPath:           praefectutil.DeriveReplicaPath(1),
+					AdditionalReplicaPath: "something",
+					Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
+					Secondaries: []RouterNode{
+						{Storage: "secondary-1", Connection: secondary1Conn},
+						{Storage: "secondary-2", Connection: secondary2Conn},
 					},
-				),
+				},
 			),
 		},
 	} {
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 5c90e89a2..af54831d9 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -194,8 +194,6 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context {
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, true)
 	// Randomly enable the use of the catfile cache in localrepo.ReadObject.
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.LocalrepoReadObjectCached, rnd.Int()%2 == 0)
-	// Randomly enable the use of the catfile cache in localrepo.ReadObject.
-	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.FixRoutingWithAdditionalRepository, rnd.Int()%2 == 0)
 
 	for _, opt := range opts {
 		ctx = opt(ctx)