Merge branch 'ps-remove-ff-reads-distrubution' into 'master'

Removal of the feature flag: distributed_reads

Closes #3383

See merge request gitlab-org/gitaly!3271

9 files changed, 12 insertions, 84 deletions

diff --git a/.gitlab/issue_templates/Demo.md b/.gitlab/issue_templates/Demo.md
index 483c4c309..5daac1a5c 100644
--- a/.gitlab/issue_templates/Demo.md
+++ b/.gitlab/issue_templates/Demo.md
@@ -76,34 +76,6 @@ succeed.
 
 ## Features
 
-### Distributed reads with caching https://gitlab.com/gitlab-org/gitaly/-/issues/3053
-
-The goal of caching is to reduce load on the database and speed up defining up to date storages for distributing read operations among them.
-
-1. [ ] Prep:
-   - [ ] Create a repository in the demo cluster. This ensures we have a repository we can use for read/write.
-   - [ ] Verify the distributed reads feature is enabled. On the GitLab node run:
-     ```
-     gitlab-rails console
-     Feature.enabled?('gitaly_distributed_reads')
-     ```
-   - [ ] Verify the cache is enabled by running `grep 'reads distribution caching is' /var/log/gitlab/praefect/current`
-     the full message should be: `reads distribution caching is enabled by configuration`
-1. [ ] Demo:
-   - [ ] Make some random operations on the repository: files creation/modification etc.
-   - [ ] Verify the feature flag is set by observing `rate(gitaly_feature_flag_checks_total{flag="distributed_reads"}[5m])` metric.
-   - [ ] Verify the reads distribution is working by checking grafana dashboard: `sum by (storage) (rate(gitaly_praefect_read_distribution[5m]))`.
-   - [ ] Verify the cache is used by checking grafana dashboard: `sum by (type) (rate(gitaly_praefect_uptodate_storages_cache_access_total[5m]))`.
-   - [ ] Remember values of the `read_distribution` metric for future comparison.
-   - [ ] Jump on one of the gitaly nodes and terminate it with command `gitlab-ctl stop gitaly`.
-   - [ ] Make some random operations on the repository: files creation/modification etc.
-   - [ ] Query for the `read_distribution` metric and compare with previous result (the terminated node should have the same value).
-   - [ ] Remember values of `gitaly_praefect_uptodate_storages_cache_access_total` metric for future comparison.
-   - [ ] From one of the instances login into the Postgres with `/opt/gitlab/embedded/bin/psql -U praefect -d praefect_production -h <POSTGRESQL_SERVER_ADDRESS>` and execute the query: `NOTIFY repositories_updates, '{send';`.
-   - [ ] Verify each praefect instance has a log entry: `grep 'received payload can't be processed, cache disabled' /var/log/gitlab/praefect/current`.
-   - [ ] Make some random operations on the repository: files creation/modification etc.
-   - [ ] Query for the metric `gitaly_praefect_uptodate_storages_cache_access_total` and compare with the result saved previosuly. The `hit` metric value should not change.
-
 ### Variable Replication Factor #2971
 
 Previously Praefect has replicated repositories to every node in a virtual storage. This has made large clusters
diff --git a/changelogs/unreleased/ps-remove-ff-reads-distrubution.yml b/changelogs/unreleased/ps-remove-ff-reads-distrubution.yml
new file mode 100644
index 000000000..7dc80cb63
--- /dev/null
+++ b/changelogs/unreleased/ps-remove-ff-reads-distrubution.yml
@@ -0,0 +1,5 @@
+---
+title: 'Removal of the feature flag: distributed_reads'
+merge_request: 3271
+author:
+type: removed
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index ad4acd160..e5d1bd505 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -9,8 +9,6 @@ type FeatureFlag struct {
 // In order to support coverage of combined features usage all feature flags should be marked as enabled for the test.
 // NOTE: if you add a new feature flag please add it to the `All` list defined below.
 var (
-	// DistributedReads allows praefect to redirect accessor operations to up-to-date secondaries
-	DistributedReads = FeatureFlag{Name: "distributed_reads", OnByDefault: true}
 	// ReferenceTransactions will handle Git reference updates via the transaction service for strong consistency
 	ReferenceTransactions = FeatureFlag{Name: "reference_transactions", OnByDefault: true}
 	// LogCommandStats will log additional rusage stats for commands
@@ -44,7 +42,6 @@ var (
 
 // All includes all feature flags.
 var All = []FeatureFlag{
-	DistributedReads,
 	LogCommandStats,
 	ReferenceTransactions,
 	GoUserCherryPick,
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 19f838836..b71505572 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -118,8 +118,8 @@ var forcePrimaryRPCs = map[string]bool{
 	// GetObjectDirectorySize depends on a repository's on-disk state. It depends on when a
 	// repository was last packed and on git-pack-objects(1) producing deterministic results.
 	// Given that we can neither guarantee that replicas are always packed at the same time,
-	// nor that git-pack-objects(1) produces the same packs, reportings would be inconsistent
-	// if we used reads distribution here. Thus, we always report sizes for the primary node.
+	// nor that git-pack-objects(1) produces the same packs. We always report sizes for the
+	// primary node.
 	"/gitaly.RepositoryService/GetObjectDirectorySize": true,
 	// Same reasoning as for GetObjectDirectorySize.
 	"/gitaly.RepositoryService/RepositorySize": true,
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index dc4dd77bc..3a7130136 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -372,16 +372,14 @@ func TestStreamDirectorAccessor(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	ctx = featureflag.IncomingCtxWithDisabledFeatureFlag(ctx, featureflag.DistributedReads)
-
 	entry := testhelper.DiscardTestEntry(t)
+	rs := datastore.MockRepositoryStore{}
 
-	nodeMgr, err := nodes.NewManager(entry, conf, nil, nil, promtest.NewMockHistogramVec(), protoregistry.GitalyProtoPreregistered, nil)
+	nodeMgr, err := nodes.NewManager(entry, conf, nil, rs, promtest.NewMockHistogramVec(), protoregistry.GitalyProtoPreregistered, nil)
 	require.NoError(t, err)
 	nodeMgr.Start(0, time.Minute)
 
 	txMgr := transactions.NewManager(conf)
-	rs := datastore.MockRepositoryStore{}
 
 	coordinator := NewCoordinator(
 		queue,
diff --git a/internal/praefect/nodes/manager.go b/internal/praefect/nodes/manager.go
index 7bc4d2a43..cbd6b1c37 100644
--- a/internal/praefect/nodes/manager.go
+++ b/internal/praefect/nodes/manager.go
@@ -13,7 +13,6 @@ import (
 	"github.com/sirupsen/logrus"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
 	"gitlab.com/gitlab-org/gitaly/client"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/grpc-proxy/proxy"
@@ -222,15 +221,6 @@ func (n *Mgr) GetShard(ctx context.Context, virtualStorageName string) (Shard, e
 }
 
 func (n *Mgr) GetSyncedNode(ctx context.Context, virtualStorageName, repoPath string) (Node, error) {
-	if featureflag.IsDisabled(ctx, featureflag.DistributedReads) {
-		shard, err := n.GetShard(ctx, virtualStorageName)
-		if err != nil {
-			return nil, fmt.Errorf("get shard for %q: %w", virtualStorageName, err)
-		}
-
-		return shard.Primary, nil
-	}
-
 	upToDateStorages, err := n.csg.GetConsistentStorages(ctx, virtualStorageName, repoPath)
 	if err != nil {
 		return nil, err
diff --git a/internal/praefect/router_per_repository.go b/internal/praefect/router_per_repository.go
index 5deb8745c..eabf4fa61 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/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/nodes"
 	"google.golang.org/grpc"
@@ -139,7 +138,7 @@ func (r *PerRepositoryRouter) RouteRepositoryAccessor(ctx context.Context, virtu
 		return RouterNode{}, err
 	}
 
-	if forcePrimary || featureflag.IsDisabled(ctx, featureflag.DistributedReads) {
+	if forcePrimary {
 		primary, err := r.pg.GetPrimary(ctx, virtualStorage, relativePath)
 		if err != nil {
 			return RouterNode{}, fmt.Errorf("get primary: %w", err)
diff --git a/internal/praefect/router_per_repository_test.go b/internal/praefect/router_per_repository_test.go
index c17337574..d97431df8 100644
--- a/internal/praefect/router_per_repository_test.go
+++ b/internal/praefect/router_per_repository_test.go
@@ -6,7 +6,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/nodes"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -110,10 +109,6 @@ func TestPerRepositoryRouter_RouteStorageAccessor(t *testing.T) {
 }
 
 func TestPerRepositoryRouter_RouteRepositoryAccessor(t *testing.T) {
-	mdReadDistributionEnabled := map[string]string{
-		featureflag.HeaderKey(featureflag.DistributedReads.Name): "true",
-	}
-
 	for _, tc := range []struct {
 		desc           string
 		virtualStorage string
@@ -128,14 +123,12 @@ func TestPerRepositoryRouter_RouteRepositoryAccessor(t *testing.T) {
 		{
 			desc:           "unknown virtual storage",
 			virtualStorage: "unknown",
-			metadata:       mdReadDistributionEnabled,
 			error:          nodes.ErrVirtualStorageNotExist,
 		},
 		{
 			desc:           "no healthy nodes",
 			virtualStorage: "virtual-storage-1",
 			healthyNodes:   map[string][]string{},
-			metadata:       mdReadDistributionEnabled,
 			error:          ErrNoHealthyNodes,
 		},
 		{
@@ -144,7 +137,6 @@ func TestPerRepositoryRouter_RouteRepositoryAccessor(t *testing.T) {
 			healthyNodes: map[string][]string{
 				"virtual-storage-1": {"primary", "consistent-secondary"},
 			},
-			metadata:      mdReadDistributionEnabled,
 			numCandidates: 2,
 			pickCandidate: 0,
 			node:          "primary",
@@ -155,7 +147,6 @@ func TestPerRepositoryRouter_RouteRepositoryAccessor(t *testing.T) {
 			healthyNodes: map[string][]string{
 				"virtual-storage-1": {"primary", "consistent-secondary"},
 			},
-			metadata:      mdReadDistributionEnabled,
 			numCandidates: 2,
 			pickCandidate: 1,
 			node:          "consistent-secondary",
@@ -166,7 +157,6 @@ func TestPerRepositoryRouter_RouteRepositoryAccessor(t *testing.T) {
 			healthyNodes: map[string][]string{
 				"virtual-storage-1": {"consistent-secondary"},
 			},
-			metadata:      mdReadDistributionEnabled,
 			numCandidates: 1,
 			node:          "consistent-secondary",
 		},
@@ -176,8 +166,7 @@ func TestPerRepositoryRouter_RouteRepositoryAccessor(t *testing.T) {
 			healthyNodes: map[string][]string{
 				"virtual-storage-1": {"inconistent-secondary"},
 			},
-			metadata: mdReadDistributionEnabled,
-			error:    ErrNoSuitableNode,
+			error: ErrNoSuitableNode,
 		},
 		{
 			desc:           "primary force-picked",
@@ -197,28 +186,6 @@ func TestPerRepositoryRouter_RouteRepositoryAccessor(t *testing.T) {
 			forcePrimary: true,
 			error:        nodes.ErrPrimaryNotHealthy,
 		},
-		{
-			desc:           "primary picked if read distribution is disabled",
-			virtualStorage: "virtual-storage-1",
-			healthyNodes: map[string][]string{
-				"virtual-storage-1": {"primary", "consistent-secondary"},
-			},
-			metadata: map[string]string{
-				featureflag.HeaderKey(featureflag.DistributedReads.Name): "false",
-			},
-			node: "primary",
-		},
-		{
-			desc:           "returns error if primary is unhealthy with read distribution disabled",
-			virtualStorage: "virtual-storage-1",
-			healthyNodes: map[string][]string{
-				"virtual-storage-1": {"consistent-secondary"},
-			},
-			metadata: map[string]string{
-				featureflag.HeaderKey(featureflag.DistributedReads.Name): "false",
-			},
-			error: nodes.ErrPrimaryNotHealthy,
-		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			ctx, cancel := testhelper.Context()
diff --git a/internal/praefect/server_test.go b/internal/praefect/server_test.go
index 13d8737f7..bbf606b28 100644
--- a/internal/praefect/server_test.go
+++ b/internal/praefect/server_test.go
@@ -537,7 +537,7 @@ func TestRenameRepository(t *testing.T) {
 	renamedVirtualRepo.RelativePath = newName
 
 	// wait until replication jobs propagate changes to other storages
-	// as we don't know which one will be used to check because of read distribution
+	// as we don't know which one will be used to check because of reads distribution
 	canCheckRepo.Wait()
 
 	for _, oldLocation := range repoPaths {