From 84e2df45b39e9012c976a4fba55e6ec0343b4ae3 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Fri, 12 Nov 2021 13:19:59 +0200 Subject: [PATCH 1/2] praefect: Use PerRepositoryRouter in TestRemoveRepository TestRemoveRepository is using node manager with the old routing stack. This is not supported anymore, so this commit updates the test to use PerRepositoryRouter stack to better match the production setup. --- internal/praefect/server_test.go | 73 ++++++++++++++++---------------- 1 file changed, 37 insertions(+), 36 deletions(-) diff --git a/internal/praefect/server_test.go b/internal/praefect/server_test.go index 8e2ffd9ee1d..e5c81fc32c6 100644 --- a/internal/praefect/server_test.go +++ b/internal/praefect/server_test.go @@ -506,13 +506,23 @@ func TestWarnDuplicateAddrs(t *testing.T) { func TestRemoveRepository(t *testing.T) { t.Parallel() + + const ( + virtualStorage = "praefect" + relativePath = "test-repository" + ) + gitalyCfgs := make([]gconfig.Cfg, 3) repos := make([][]*gitalypb.Repository, 3) - praefectCfg := config.Config{VirtualStorages: []*config.VirtualStorage{{Name: "praefect"}}} + praefectCfg := config.Config{ + VirtualStorages: []*config.VirtualStorage{{Name: virtualStorage}}, + Failover: config.Failover{ElectionStrategy: config.ElectionStrategyPerRepository}, + } - for i, name := range []string{"gitaly-1", "gitaly-2", "gitaly-3"} { + storages := []string{"gitaly-1", "gitaly-2", "gitaly-3"} + for i, name := range storages { cfgBuilder := testcfg.NewGitalyCfgBuilder(testcfg.WithStorages(name)) - gitalyCfgs[i], repos[i] = cfgBuilder.BuildWithRepoAt(t, "test-repository") + gitalyCfgs[i], repos[i] = cfgBuilder.BuildWithRepoAt(t, relativePath) gitalyAddr := testserver.RunGitalyServer(t, gitalyCfgs[i], nil, setup.RegisterAll, testserver.WithDisablePraefect()) gitalyCfgs[i].SocketPath = gitalyAddr @@ -524,7 +534,16 @@ func TestRemoveRepository(t *testing.T) { }) } + ctx, cancel := testhelper.Context() + defer cancel() + + db := glsql.NewDB(t) + rs := datastore.NewPostgresRepositoryStore(db, praefectCfg.StorageNames()) + + require.NoError(t, rs.CreateRepository(ctx, 1, virtualStorage, relativePath, relativePath, storages[0], storages[1:], nil, true, false)) + verifyReposExistence := func(t *testing.T, code codes.Code) { + t.Helper() for i, gitalyCfg := range gitalyCfgs { locator := gconfig.NewLocator(gitalyCfg) _, err := locator.GetRepoPath(repos[i][0]) @@ -536,32 +555,24 @@ func TestRemoveRepository(t *testing.T) { verifyReposExistence(t, codes.OK) - queueInterceptor := datastore.NewReplicationEventQueueInterceptor(datastore.NewPostgresReplicationEventQueue(glsql.NewDB(t))) - repoStore := defaultRepoStore(praefectCfg) - txMgr := defaultTxMgr(praefectCfg) - nodeMgr, err := nodes.NewManager(testhelper.DiscardTestEntry(t), praefectCfg, nil, - repoStore, promtest.NewMockHistogramVec(), protoregistry.GitalyProtoPreregistered, - nil, backchannel.NewClientHandshaker( - testhelper.DiscardTestEntry(t), - NewBackchannelServerFactory(testhelper.DiscardTestEntry(t), transaction.NewServer(txMgr), nil), - ), nil, - ) + nodeSet, err := DialNodes(ctx, praefectCfg.VirtualStorages, protoregistry.GitalyProtoPreregistered, nil, nil, nil) require.NoError(t, err) - nodeMgr.Start(0, time.Hour) - defer nodeMgr.Stop() - - ctx, cancel := testhelper.Context() - defer cancel() + defer nodeSet.Close() + conns := nodeSet.Connections() cc, _, cleanup := runPraefectServer(t, ctx, praefectCfg, buildOptions{ - withQueue: queueInterceptor, - withRepoStore: datastore.MockRepositoryStore{ - GetConsistentStoragesFunc: func(ctx context.Context, virtualStorage, relativePath string) (string, map[string]struct{}, error) { - return relativePath, nil, nil - }, - }, - withNodeMgr: nodeMgr, - withTxMgr: txMgr, + withConnections: conns, + withRepoStore: rs, + withRouter: NewPerRepositoryRouter( + conns, + nodes.NewPerRepositoryElector(db), + StaticHealthChecker(praefectCfg.StorageNames()), + NewLockedRandom(rand.New(rand.NewSource(0))), + rs, + datastore.NewAssignmentStore(db, praefectCfg.StorageNames()), + rs, + praefectCfg.DefaultReplicationFactors(), + ), }) defer cleanup() @@ -573,16 +584,6 @@ func TestRemoveRepository(t *testing.T) { }) require.NoError(t, err) - require.NoError(t, queueInterceptor.Wait(time.Minute, func(i *datastore.ReplicationEventQueueInterceptor) bool { - var compl int - for _, ack := range i.GetAcknowledge() { - if ack.State == datastore.JobStateCompleted { - compl++ - } - } - return compl == 2 - })) - verifyReposExistence(t, codes.NotFound) } -- GitLab From 42d01da040d1bde2e1aa3bdb3a7da2028fa6389e Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Fri, 12 Nov 2021 13:21:14 +0200 Subject: [PATCH 2/2] praefect: Mark RemoveRepository as an intercepted method This commit marks RemoveRepository as an intercepted method which allows us to remove the annotations as they are not needed by the proxy anymore. This also removes the RemoveRepository handling from the Praefect's coordinator as its no longer needed with the RPC being intercepted. --- internal/praefect/coordinator.go | 3 -- proto/go/gitalypb/repository-service.pb.go | 62 +++++++++++----------- proto/repository-service.proto | 4 +- 3 files changed, 32 insertions(+), 37 deletions(-) diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go index 1f531d449e9..14b45b8026c 100644 --- a/internal/praefect/coordinator.go +++ b/internal/praefect/coordinator.go @@ -75,7 +75,6 @@ var transactionRPCs = map[string]transactionsCondition{ "/gitaly.RepositoryService/FetchBundle": transactionsEnabled, "/gitaly.RepositoryService/FetchRemote": transactionsEnabled, "/gitaly.RepositoryService/FetchSourceBranch": transactionsEnabled, - "/gitaly.RepositoryService/RemoveRepository": transactionsEnabled, "/gitaly.RepositoryService/ReplicateRepository": transactionsEnabled, "/gitaly.RepositoryService/SetFullPath": transactionsEnabled, "/gitaly.RepositoryService/WriteRef": transactionsEnabled, @@ -165,8 +164,6 @@ func shouldUseTransaction(ctx context.Context, method string) bool { // getReplicationDetails determines the type of job and additional details based on the method name and incoming message func getReplicationDetails(methodName string, m proto.Message) (datastore.ChangeType, datastore.Params, error) { switch methodName { - case "/gitaly.RepositoryService/RemoveRepository": - return datastore.DeleteRepo, nil, nil case "/gitaly.RepositoryService/CreateFork", "/gitaly.RepositoryService/CreateRepository", "/gitaly.RepositoryService/CreateRepositoryFromBundle", diff --git a/proto/go/gitalypb/repository-service.pb.go b/proto/go/gitalypb/repository-service.pb.go index 4b921c9bbfa..40699f786ed 100644 --- a/proto/go/gitalypb/repository-service.pb.go +++ b/proto/go/gitalypb/repository-service.pb.go @@ -5107,7 +5107,7 @@ var file_repository_service_proto_rawDesc = []byte{ 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x12, 0x12, 0x0a, 0x04, 0x70, 0x61, 0x74, 0x68, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x70, 0x61, 0x74, 0x68, 0x22, 0x15, 0x0a, 0x13, 0x53, 0x65, 0x74, 0x46, 0x75, 0x6c, 0x6c, 0x50, 0x61, 0x74, 0x68, 0x52, 0x65, 0x73, 0x70, 0x6f, - 0x6e, 0x73, 0x65, 0x32, 0xda, 0x1e, 0x0a, 0x11, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, + 0x6e, 0x73, 0x65, 0x32, 0xd8, 0x1e, 0x0a, 0x11, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x53, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x12, 0x5d, 0x0a, 0x10, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x45, 0x78, 0x69, 0x73, 0x74, 0x73, 0x12, 0x1f, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, @@ -5323,40 +5323,40 @@ var file_repository_service_proto_rawDesc = []byte{ 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x25, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x43, 0x6c, 0x6f, 0x6e, 0x65, 0x46, 0x72, 0x6f, 0x6d, 0x50, 0x6f, 0x6f, 0x6c, 0x49, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x06, 0xfa, - 0x97, 0x28, 0x02, 0x08, 0x01, 0x12, 0x5d, 0x0a, 0x10, 0x52, 0x65, 0x6d, 0x6f, 0x76, 0x65, 0x52, + 0x97, 0x28, 0x02, 0x08, 0x01, 0x12, 0x5b, 0x0a, 0x10, 0x52, 0x65, 0x6d, 0x6f, 0x76, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x12, 0x1f, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x52, 0x65, 0x6d, 0x6f, 0x76, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x20, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x52, 0x65, 0x6d, 0x6f, 0x76, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, - 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x06, 0xfa, 0x97, - 0x28, 0x02, 0x08, 0x01, 0x12, 0x5d, 0x0a, 0x10, 0x52, 0x65, 0x6e, 0x61, 0x6d, 0x65, 0x52, 0x65, - 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x12, 0x1f, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, - 0x79, 0x2e, 0x52, 0x65, 0x6e, 0x61, 0x6d, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, - 0x72, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x20, 0x2e, 0x67, 0x69, 0x74, 0x61, - 0x6c, 0x79, 0x2e, 0x52, 0x65, 0x6e, 0x61, 0x6d, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, - 0x6f, 0x72, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x06, 0xfa, 0x97, 0x28, - 0x02, 0x08, 0x01, 0x12, 0x66, 0x0a, 0x13, 0x52, 0x65, 0x70, 0x6c, 0x69, 0x63, 0x61, 0x74, 0x65, - 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x12, 0x22, 0x2e, 0x67, 0x69, 0x74, - 0x61, 0x6c, 0x79, 0x2e, 0x52, 0x65, 0x70, 0x6c, 0x69, 0x63, 0x61, 0x74, 0x65, 0x52, 0x65, 0x70, - 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x23, - 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x52, 0x65, 0x70, 0x6c, 0x69, 0x63, 0x61, 0x74, - 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, - 0x6e, 0x73, 0x65, 0x22, 0x06, 0xfa, 0x97, 0x28, 0x02, 0x08, 0x01, 0x12, 0x63, 0x0a, 0x12, 0x4f, - 0x70, 0x74, 0x69, 0x6d, 0x69, 0x7a, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, - 0x79, 0x12, 0x21, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x4f, 0x70, 0x74, 0x69, 0x6d, - 0x69, 0x7a, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, 0x71, - 0x75, 0x65, 0x73, 0x74, 0x1a, 0x22, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x4f, 0x70, - 0x74, 0x69, 0x6d, 0x69, 0x7a, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, - 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x06, 0xfa, 0x97, 0x28, 0x02, 0x08, 0x01, - 0x12, 0x4e, 0x0a, 0x0b, 0x53, 0x65, 0x74, 0x46, 0x75, 0x6c, 0x6c, 0x50, 0x61, 0x74, 0x68, 0x12, - 0x1a, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x53, 0x65, 0x74, 0x46, 0x75, 0x6c, 0x6c, - 0x50, 0x61, 0x74, 0x68, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x1b, 0x2e, 0x67, 0x69, - 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x53, 0x65, 0x74, 0x46, 0x75, 0x6c, 0x6c, 0x50, 0x61, 0x74, 0x68, - 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x06, 0xfa, 0x97, 0x28, 0x02, 0x08, 0x01, - 0x42, 0x34, 0x5a, 0x32, 0x67, 0x69, 0x74, 0x6c, 0x61, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x67, - 0x69, 0x74, 0x6c, 0x61, 0x62, 0x2d, 0x6f, 0x72, 0x67, 0x2f, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, - 0x2f, 0x76, 0x31, 0x34, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x67, 0x6f, 0x2f, 0x67, 0x69, - 0x74, 0x61, 0x6c, 0x79, 0x70, 0x62, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x04, 0x80, 0x98, + 0x28, 0x01, 0x12, 0x5d, 0x0a, 0x10, 0x52, 0x65, 0x6e, 0x61, 0x6d, 0x65, 0x52, 0x65, 0x70, 0x6f, + 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x12, 0x1f, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, + 0x52, 0x65, 0x6e, 0x61, 0x6d, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, + 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x20, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, + 0x2e, 0x52, 0x65, 0x6e, 0x61, 0x6d, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, + 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x06, 0xfa, 0x97, 0x28, 0x02, 0x08, + 0x01, 0x12, 0x66, 0x0a, 0x13, 0x52, 0x65, 0x70, 0x6c, 0x69, 0x63, 0x61, 0x74, 0x65, 0x52, 0x65, + 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x12, 0x22, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, + 0x79, 0x2e, 0x52, 0x65, 0x70, 0x6c, 0x69, 0x63, 0x61, 0x74, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, + 0x69, 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x23, 0x2e, 0x67, + 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x52, 0x65, 0x70, 0x6c, 0x69, 0x63, 0x61, 0x74, 0x65, 0x52, + 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, + 0x65, 0x22, 0x06, 0xfa, 0x97, 0x28, 0x02, 0x08, 0x01, 0x12, 0x63, 0x0a, 0x12, 0x4f, 0x70, 0x74, + 0x69, 0x6d, 0x69, 0x7a, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x12, + 0x21, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x4f, 0x70, 0x74, 0x69, 0x6d, 0x69, 0x7a, + 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, 0x71, 0x75, 0x65, + 0x73, 0x74, 0x1a, 0x22, 0x2e, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x4f, 0x70, 0x74, 0x69, + 0x6d, 0x69, 0x7a, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x52, 0x65, + 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x06, 0xfa, 0x97, 0x28, 0x02, 0x08, 0x01, 0x12, 0x4e, + 0x0a, 0x0b, 0x53, 0x65, 0x74, 0x46, 0x75, 0x6c, 0x6c, 0x50, 0x61, 0x74, 0x68, 0x12, 0x1a, 0x2e, + 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2e, 0x53, 0x65, 0x74, 0x46, 0x75, 0x6c, 0x6c, 0x50, 0x61, + 0x74, 0x68, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x1b, 0x2e, 0x67, 0x69, 0x74, 0x61, + 0x6c, 0x79, 0x2e, 0x53, 0x65, 0x74, 0x46, 0x75, 0x6c, 0x6c, 0x50, 0x61, 0x74, 0x68, 0x52, 0x65, + 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x06, 0xfa, 0x97, 0x28, 0x02, 0x08, 0x01, 0x42, 0x34, + 0x5a, 0x32, 0x67, 0x69, 0x74, 0x6c, 0x61, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x67, 0x69, 0x74, + 0x6c, 0x61, 0x62, 0x2d, 0x6f, 0x72, 0x67, 0x2f, 0x67, 0x69, 0x74, 0x61, 0x6c, 0x79, 0x2f, 0x76, + 0x31, 0x34, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x67, 0x6f, 0x2f, 0x67, 0x69, 0x74, 0x61, + 0x6c, 0x79, 0x70, 0x62, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( diff --git a/proto/repository-service.proto b/proto/repository-service.proto index 61416421fa8..f7808d5f245 100644 --- a/proto/repository-service.proto +++ b/proto/repository-service.proto @@ -218,9 +218,7 @@ service RepositoryService { // eventually remove it. This ensures that even on networked filesystems the // data is actually removed even if there's someone still handling the data. rpc RemoveRepository(RemoveRepositoryRequest) returns (RemoveRepositoryResponse) { - option (op_type) = { - op: MUTATOR - }; + option (intercepted_method) = true; } rpc RenameRepository(RenameRepositoryRequest) returns (RenameRepositoryResponse) { option (op_type) = { -- GitLab