From 581475e69cccbda5e0b2721accc7c8a2c5af5856 Mon Sep 17 00:00:00 2001 From: James Fargher Date: Thu, 17 Jun 2021 14:03:49 +1200 Subject: [PATCH 1/2] Add env var to disable gitaly_fetch_internal_remote_errors feature Setting GITALY_DISABLE_FETCH_INTERNAL_REMOTE_ERRORS to 1 will disable the feature. --- .../service/remote/fetch_internal_remote.go | 9 +++++++- .../remote/fetch_internal_remote_test.go | 21 +++++++++++++++++++ internal/gitaly/service/remote/server.go | 9 ++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/internal/gitaly/service/remote/fetch_internal_remote.go b/internal/gitaly/service/remote/fetch_internal_remote.go index 5c72264c194..eee66218ef8 100644 --- a/internal/gitaly/service/remote/fetch_internal_remote.go +++ b/internal/gitaly/service/remote/fetch_internal_remote.go @@ -107,7 +107,7 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt if err := FetchInternalRemote(ctx, s.cfg, s.conns, repo, req.RemoteRepository); err != nil { var fetchErr fetchFailedError - if featureflag.IsDisabled(ctx, featureflag.FetchInternalRemoteErrors) && errors.As(err, &fetchErr) { + if s.isFetchInternalRemoteErrorsDisabled(ctx) && errors.As(err, &fetchErr) { // Design quirk: if the fetch fails, this RPC returns Result: false, but no error. ctxlogrus.Extract(ctx).WithError(fetchErr.err).WithField("stderr", fetchErr.stderr).Warn("git fetch failed") return &gitalypb.FetchInternalRemoteResponse{Result: false}, nil @@ -119,6 +119,13 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt return &gitalypb.FetchInternalRemoteResponse{Result: true}, nil } +func (s *server) isFetchInternalRemoteErrorsDisabled(ctx context.Context) bool { + // Since feature flags can only be turned on through requests from + // gitlab-rails we need a way to explicitly disable the feature without + // triggering a new release. + return featureflag.IsDisabled(ctx, featureflag.FetchInternalRemoteErrors) || s.disableFetchInternalRemoteErrors +} + // getRemoteDefaultBranch gets the default branch of a repository hosted on another Gitaly node. func getRemoteDefaultBranch(ctx context.Context, repo *gitalypb.Repository, conns *client.Pool) ([]byte, error) { serverInfo, err := helper.ExtractGitalyServer(ctx, repo.StorageName) diff --git a/internal/gitaly/service/remote/fetch_internal_remote_test.go b/internal/gitaly/service/remote/fetch_internal_remote_test.go index d593ca7223c..db784800021 100644 --- a/internal/gitaly/service/remote/fetch_internal_remote_test.go +++ b/internal/gitaly/service/remote/fetch_internal_remote_test.go @@ -260,6 +260,27 @@ func TestFailedFetchInternalRemote(t *testing.T) { require.NoError(t, err, "FetchInternalRemote is not supposed to return an error when 'git fetch' fails") require.False(t, c.GetResult()) }) + + t.Run("GITALY_DISABLE_FETCH_INTERNAL_REMOTE_ERRORS set", func(t *testing.T) { + cleanup := testhelper.ModifyEnvironment(t, "GITALY_DISABLE_FETCH_INTERNAL_REMOTE_ERRORS", "1") + t.Cleanup(cleanup) + + cfg, repo, _, client := setupRemoteService(t) + + ctx := testhelper.MergeOutgoingMetadata(ctx, testhelper.GitalyServersMetadataFromCfg(t, cfg)) + + // Non-existing remote repo + remoteRepo := &gitalypb.Repository{StorageName: repo.GetStorageName(), RelativePath: "fake.git"} + + request := &gitalypb.FetchInternalRemoteRequest{ + Repository: repo, + RemoteRepository: remoteRepo, + } + + c, err := client.FetchInternalRemote(ctx, request) + require.NoError(t, err, "FetchInternalRemote is not supposed to return an error when 'git fetch' fails") + require.False(t, c.GetResult()) + }) } func TestFailedFetchInternalRemoteDueToValidations(t *testing.T) { diff --git a/internal/gitaly/service/remote/server.go b/internal/gitaly/service/remote/server.go index 2d0bd3dafe0..c7ca4fadf7b 100644 --- a/internal/gitaly/service/remote/server.go +++ b/internal/gitaly/service/remote/server.go @@ -9,6 +9,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/rubyserver" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction" + "gitlab.com/gitlab-org/gitaly/v14/internal/helper/env" "gitlab.com/gitlab-org/gitaly/v14/internal/storage" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" ) @@ -23,6 +24,8 @@ type server struct { txManager transaction.Manager conns *client.Pool + + disableFetchInternalRemoteErrors bool } // NewServer creates a new instance of a grpc RemoteServiceServer @@ -34,6 +37,11 @@ func NewServer( catfileCache catfile.Cache, txManager transaction.Manager, ) gitalypb.RemoteServiceServer { + disableFetchInternalRemoteErrors, err := env.GetBool("GITALY_DISABLE_FETCH_INTERNAL_REMOTE_ERRORS", false) + if err != nil { + panic(err) + } + return &server{ cfg: cfg, ruby: rs, @@ -45,6 +53,7 @@ func NewServer( client.WithDialer(client.HealthCheckDialer(client.DialContext)), client.WithDialOptions(client.FailOnNonTempDialError()...), ), + disableFetchInternalRemoteErrors: disableFetchInternalRemoteErrors, } } -- GitLab From 091b5bdb4ae25c7a5807949acacc6cd0fd71106e Mon Sep 17 00:00:00 2001 From: James Fargher Date: Thu, 17 Jun 2021 14:04:40 +1200 Subject: [PATCH 2/2] Make FetchInternalRemote return errors by default Historically FetchInternalRemote returned a boolean to indicate if the fetch failed. This makes debugging ReplicateRepository more difficult than it needs to be. Instead we return the error so it can be diagnosed without needing to scrape logs. Changelog: changed --- internal/metadata/featureflag/feature_flags.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 4b8d05bf671..1d2767d4d80 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -12,7 +12,7 @@ var ( // GoUpdateRemoteMirror enables the Go implementation of UpdateRemoteMirror GoUpdateRemoteMirror = FeatureFlag{Name: "go_update_remote_mirror", OnByDefault: false} // FetchInternalRemoteErrors makes FetchInternalRemote return actual errors instead of a boolean - FetchInternalRemoteErrors = FeatureFlag{Name: "fetch_internal_remote_errors", OnByDefault: false} + FetchInternalRemoteErrors = FeatureFlag{Name: "fetch_internal_remote_errors", OnByDefault: true} // TxConfig enables transactional voting for SetConfig and DeleteConfig RPCs. TxConfig = FeatureFlag{Name: "tx_config", OnByDefault: true} // TxRemote enables transactional voting for AddRemote and DeleteRemote. -- GitLab