From 21373c6e00ed20713c6bd42d032ea7ca4e71fe9c Mon Sep 17 00:00:00 2001
From: Sami Hiltunen <shiltunen@gitlab.com>
Date: Mon, 13 Sep 2021 17:38:07 +0200
Subject: [PATCH] Link created records with repository ID in
 SetAuthoritativeReplica

SetAuthoritativeReplica marks a given replica of a repository as the
latest version. This enables an administrator to accept data loss and
make the repository available again if all of the up to date replicas
are gone. 14.3 is introducing repository IDs to Praefect. In order to
allow us to make a migration to link all existing records in 14.4 via
the repository ID, 14.3 should create all records with repository ID
in place to avoid any issues with concurrent creations running with the
migration. SetAuthoritativeReplica is currently not doing that and would
cause problems if it was ran concurrently with the migration. This commit
updates the method to link the 'storage_repositories' entries it may create
via the repository ID to the corresponding 'repositories' table record.
This was missed earlier as there was no test case that exercised the part of
the code that created new records. This commit includes a test for the
functionality.
---
 .../praefect/datastore/repository_store.go    |  9 ++--
 .../datastore/repository_store_test.go        | 45 +++++++++++++++++--
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/internal/praefect/datastore/repository_store.go b/internal/praefect/datastore/repository_store.go
index 49bb6e5478..9c8dcf30e1 100644
--- a/internal/praefect/datastore/repository_store.go
+++ b/internal/praefect/datastore/repository_store.go
@@ -261,14 +261,15 @@ WITH updated_repository AS (
 	SET generation = generation + 1
 	WHERE virtual_storage = $1
 	AND   relative_path   = $2
-	RETURNING virtual_storage, relative_path, generation
+	RETURNING repository_id, virtual_storage, relative_path, generation
 )
 
-INSERT INTO storage_repositories
-SELECT virtual_storage, relative_path, $3, generation
+INSERT INTO storage_repositories (repository_id, virtual_storage, relative_path, storage, generation)
+SELECT repository_id, virtual_storage, relative_path, $3, generation
 FROM updated_repository
 ON CONFLICT (virtual_storage, relative_path, storage) DO UPDATE
-	SET generation = EXCLUDED.generation
+	SET repository_id = EXCLUDED.repository_id,
+	    generation = EXCLUDED.generation
 	`, virtualStorage, relativePath, storage)
 	if err != nil {
 		return fmt.Errorf("exec: %w", err)
diff --git a/internal/praefect/datastore/repository_store_test.go b/internal/praefect/datastore/repository_store_test.go
index caee75ce83..f8bd2085d0 100644
--- a/internal/praefect/datastore/repository_store_test.go
+++ b/internal/praefect/datastore/repository_store_test.go
@@ -438,16 +438,18 @@ func testRepositoryStore(t *testing.T, newStore repositoryStoreFactory) {
 	})
 
 	t.Run("SetAuthoritativeReplica", func(t *testing.T) {
-		rs, requireState := newStore(t, nil)
-
 		t.Run("fails when repository doesnt exist", func(t *testing.T) {
+			rs, _ := newStore(t, nil)
+
 			require.Equal(t,
 				commonerr.NewRepositoryNotFoundError(vs, repo),
 				rs.SetAuthoritativeReplica(ctx, vs, repo, stor),
 			)
 		})
 
-		t.Run("sets the given replica as the latest", func(t *testing.T) {
+		t.Run("sets an existing replica as the latest", func(t *testing.T) {
+			rs, requireState := newStore(t, nil)
+
 			require.NoError(t, rs.CreateRepository(ctx, 1, vs, repo, "storage-1", []string{"storage-2"}, nil, false, false))
 			requireState(t, ctx,
 				virtualStorageState{
@@ -482,6 +484,43 @@ func testRepositoryStore(t *testing.T, newStore repositoryStoreFactory) {
 				},
 			)
 		})
+
+		t.Run("sets a new replica as the latest", func(t *testing.T) {
+			rs, requireState := newStore(t, nil)
+
+			require.NoError(t, rs.CreateRepository(ctx, 1, vs, repo, "storage-1", nil, nil, false, false))
+			requireState(t, ctx,
+				virtualStorageState{
+					"virtual-storage-1": {
+						"repository-1": {repositoryID: 1, replicaPath: "repository-1"},
+					},
+				},
+				storageState{
+					"virtual-storage-1": {
+						"repository-1": {
+							"storage-1": {repositoryID: 1, generation: 0},
+						},
+					},
+				},
+			)
+
+			require.NoError(t, rs.SetAuthoritativeReplica(ctx, vs, repo, "storage-2"))
+			requireState(t, ctx,
+				virtualStorageState{
+					"virtual-storage-1": {
+						"repository-1": {repositoryID: 1, replicaPath: "repository-1"},
+					},
+				},
+				storageState{
+					"virtual-storage-1": {
+						"repository-1": {
+							"storage-1": {repositoryID: 1, generation: 0},
+							"storage-2": {repositoryID: 1, generation: 1},
+						},
+					},
+				},
+			)
+		})
 	})
 
 	t.Run("GetGeneration", func(t *testing.T) {
-- 
GitLab