From 8fbd235f9e1e1b348ab5379164832467b1d9c694 Mon Sep 17 00:00:00 2001 From: James Fargher Date: Thu, 22 Apr 2021 09:35:25 +1200 Subject: [PATCH] Return errors from FetchInternalRemote For historical reasons FetchInternalRemote returns a boolean to indicate if the fetch succeeded or not. The only caller of FetchInternalRemote is ReplicateRepository. To make this call easier to diagnose we should return the errors directly. --- .../service/remote/fetch_internal_remote.go | 16 ++++++++++++--- .../remote/fetch_internal_remote_test.go | 20 +++++++++++++------ .../metadata/featureflag/feature_flags.go | 3 +++ 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/internal/gitaly/service/remote/fetch_internal_remote.go b/internal/gitaly/service/remote/fetch_internal_remote.go index 114efe41cc7..d5050a52f4f 100644 --- a/internal/gitaly/service/remote/fetch_internal_remote.go +++ b/internal/gitaly/service/remote/fetch_internal_remote.go @@ -11,6 +11,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ref" "gitlab.com/gitlab-org/gitaly/internal/gitalyssh" "gitlab.com/gitlab-org/gitaly/internal/helper" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -55,9 +56,18 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt return nil, fmt.Errorf("create git fetch: %w", err) } if err := cmd.Wait(); err != nil { - // Design quirk: if the fetch fails, this RPC returns Result: false, but no error. - ctxlogrus.Extract(ctx).WithError(err).WithField("stderr", stderr.String()).Warn("git fetch failed") - return &gitalypb.FetchInternalRemoteResponse{Result: false}, nil + if featureflag.IsDisabled(ctx, featureflag.FetchInternalRemoteErrors) { + // Design quirk: if the fetch fails, this RPC returns Result: false, but no error. + ctxlogrus.Extract(ctx).WithError(err).WithField("stderr", stderr.String()).Warn("git fetch failed") + return &gitalypb.FetchInternalRemoteResponse{Result: false}, nil + } + + errMsg := stderr.String() + if errMsg != "" { + return nil, fmt.Errorf("FetchInternalRemote: fetch: %w, stderr: %q", err, errMsg) + } + + return nil, fmt.Errorf("FetchInternalRemote: fetch: %w", err) } remoteDefaultBranch, err := s.getRemoteDefaultBranch(ctx, req.RemoteRepository) diff --git a/internal/gitaly/service/remote/fetch_internal_remote_test.go b/internal/gitaly/service/remote/fetch_internal_remote_test.go index 23f2f19b920..d6e70dbbf83 100644 --- a/internal/gitaly/service/remote/fetch_internal_remote_test.go +++ b/internal/gitaly/service/remote/fetch_internal_remote_test.go @@ -21,13 +21,13 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ref" "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ssh" "gitlab.com/gitlab-org/gitaly/internal/helper" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/internal/testhelper/testserver" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc" "google.golang.org/grpc/codes" - "google.golang.org/grpc/metadata" ) type mockHookManager struct { @@ -220,8 +220,7 @@ func TestFailedFetchInternalRemote(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() - md := testhelper.GitalyServersMetadataFromCfg(t, cfg) - ctx = metadata.NewOutgoingContext(ctx, md) + ctx = testhelper.MergeOutgoingMetadata(ctx, testhelper.GitalyServersMetadataFromCfg(t, cfg)) // Non-existing remote repo remoteRepo := &gitalypb.Repository{StorageName: repo.GetStorageName(), RelativePath: "fake.git"} @@ -231,9 +230,18 @@ func TestFailedFetchInternalRemote(t *testing.T) { 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()) + t.Run("fetch_internal_remote_errors enabled", func(t *testing.T) { + _, err := client.FetchInternalRemote(ctx, request) + require.Error(t, err, "FetchInternalRemote is supposed to return an error when 'git fetch' fails") + }) + + t.Run("fetch_internal_remote_errors disabled", func(t *testing.T) { + ctx := featureflag.OutgoingCtxWithDisabledFeatureFlags(ctx, featureflag.FetchInternalRemoteErrors) + + 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/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 69d3baf5bbc..ca870f25047 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -30,6 +30,8 @@ var ( GrpcTreeEntryNotFound = FeatureFlag{Name: "grpc_tree_entry_not_found", OnByDefault: false} // BackchannelVoting enables voting via the backchannel connection. BackchannelVoting = FeatureFlag{Name: "backchannel_voting", OnByDefault: false} + // FetchInternalRemoteErrors makes FetchInternalRemote return actual errors instead of a boolean + FetchInternalRemoteErrors = FeatureFlag{Name: "fetch_internal_remote_errors", OnByDefault: false} ) // All includes all feature flags. @@ -44,4 +46,5 @@ var All = []FeatureFlag{ GrpcTreeEntryNotFound, GoUpdateRemoteMirror, BackchannelVoting, + FetchInternalRemoteErrors, } -- GitLab