diff options
author | Sami Hiltunen <shiltunen@gitlab.com> | 2021-09-03 13:09:42 +0300 |
---|---|---|
committer | Sami Hiltunen <shiltunen@gitlab.com> | 2021-09-03 17:01:20 +0300 |
commit | 94f0e2eedc05fffa13ca8446e34deaec02ca3ec7 (patch) | |
tree | 209a27335563a9d4b4568c54af79e85f2a233706 | |
parent | 34cfd2c2b4964cf1c0ad1704fd9e2c25666bcf16 (diff) |
Workaround Rails filesystem ID tests in Praefect
Rails tests configure Praefect in front of the tests that exercise
the Rugged direct git access code. As Praefect is now deriving the
filesystem IDs from the names of the virtual storages, the filesystem
id checks fail and thus the test fail. This is not a problem in practice
as one wouldn't use rugged in a real-world setup with Praefect. This
commit worksaround the tests by returning the filesystem ID from the
Gitaly node if a virtual storage has only one Gitaly node configured.
This matches the setup the tests use and thus pass them. The workaround
and the filesystem ID code can be removed in 15.0 once the rugged patches
and NFS support are dropped.
-rw-r--r-- | internal/praefect/server_test.go | 30 | ||||
-rw-r--r-- | internal/praefect/service/server/info.go | 14 |
2 files changed, 40 insertions, 4 deletions
diff --git a/internal/praefect/server_test.go b/internal/praefect/server_test.go index 8ca2a913e..b02f466d3 100644 --- a/internal/praefect/server_test.go +++ b/internal/praefect/server_test.go @@ -8,6 +8,7 @@ import ( "net" "os" "path/filepath" + "sort" "strings" "sync" "testing" @@ -36,6 +37,7 @@ import ( serversvc "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/service/server" "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/service/transaction" "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/transactions" + "gitlab.com/gitlab-org/gitaly/v14/internal/storage" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/promtest" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert" @@ -117,9 +119,22 @@ func TestGitalyServerInfo(t *testing.T) { secondCfg := testcfg.Build(t, testcfg.WithStorages("praefect-internal-2")) secondCfg.SocketPath = testserver.RunGitalyServer(t, secondCfg, nil, setup.RegisterAll, testserver.WithDisablePraefect()) + require.NoError(t, storage.WriteMetadataFile(firstCfg.Storages[0].Path)) + firstMetadata, err := storage.ReadMetadataFile(firstCfg.Storages[0].Path) + require.NoError(t, err) + conf := config.Config{ VirtualStorages: []*config.VirtualStorage{ { + Name: "passthrough-filesystem-id", + Nodes: []*config.Node{ + { + Storage: firstCfg.Storages[0].Name, + Address: firstCfg.SocketPath, + }, + }, + }, + { Name: "virtual-storage", Nodes: []*config.Node{ { @@ -153,7 +168,14 @@ func TestGitalyServerInfo(t *testing.T) { StorageStatuses: []*gitalypb.ServerInfoResponse_StorageStatus{ { StorageName: conf.VirtualStorages[0].Name, - FilesystemId: serversvc.DeriveFilesystemID(conf.VirtualStorages[0].Name).String(), + FilesystemId: firstMetadata.GitalyFilesystemID, + Readable: true, + Writeable: true, + ReplicationFactor: 1, + }, + { + StorageName: conf.VirtualStorages[1].Name, + FilesystemId: serversvc.DeriveFilesystemID(conf.VirtualStorages[1].Name).String(), Readable: true, Writeable: true, ReplicationFactor: 2, @@ -167,6 +189,12 @@ func TestGitalyServerInfo(t *testing.T) { for _, ss := range actual.StorageStatuses { ss.FsType = "" } + + // sort the storages by name so they match the expected + sort.Slice(actual.StorageStatuses, func(i, j int) bool { + return actual.StorageStatuses[i].StorageName < actual.StorageStatuses[j].StorageName + }) + require.True(t, proto.Equal(expected, actual), "expected: %v, got: %v", expected, actual) }) diff --git a/internal/praefect/service/server/info.go b/internal/praefect/service/server/info.go index 1094a3ba7..1ca09a871 100644 --- a/internal/praefect/service/server/info.go +++ b/internal/praefect/service/server/info.go @@ -77,9 +77,6 @@ func (s *Server) ServerInfo(ctx context.Context, in *gitalypb.ServerInfoRequest) for _, storageStatus := range resp.GetStorageStatuses() { if storageStatus.StorageName == storage { storageStatuses[i] = storageStatus - // Each of the Gitaly nodes have a different filesystem ID they've generated. To have a stable filesystem - // ID for a given virtual storage, the filesystem ID is generated from the virtual storage's name. - storageStatuses[i].FilesystemId = DeriveFilesystemID(virtualStorage).String() // the storage name in the response needs to be rewritten to be the virtual storage name // because the praefect client has no concept of internal gitaly nodes that are behind praefect. // From the perspective of the praefect client, the primary internal gitaly node's storage status is equivalent @@ -87,6 +84,17 @@ func (s *Server) ServerInfo(ctx context.Context, in *gitalypb.ServerInfoRequest) storageStatuses[i].StorageName = virtualStorage storageStatuses[i].Writeable = storageStatus.Writeable storageStatuses[i].ReplicationFactor = uint32(len(storages)) + + // Rails tests configure Praefect in front of tests that drive the direct git access with Rugged patches. + // This is not a real world scenario and would not work. The tests only configure a single Gitaly node, + // so as a workaround for these tests, we only override the filesystem id if we have more than one Gitaly node. + // The filesystem ID and this workaround can be removed once the Rugged patches and NFS are gone in 15.0. + if len(storages) > 1 { + // Each of the Gitaly nodes have a different filesystem ID they've generated. To have a stable filesystem + // ID for a given virtual storage, the filesystem ID is generated from the virtual storage's name. + storageStatuses[i].FilesystemId = DeriveFilesystemID(storageStatus.StorageName).String() + } + break } } |