diff options
author | Toon Claes <toon@gitlab.com> | 2023-07-04 13:39:05 +0300 |
---|---|---|
committer | Toon Claes <toon@gitlab.com> | 2023-07-04 13:39:05 +0300 |
commit | 6dac0d3b720261ef4f774f6727c1b74112809ffd (patch) | |
tree | 39a549e78c1b473122e36cd514d6929d801464a1 /internal | |
parent | abacd8fcffda22087c9f3b3ef479bc44a73ba80e (diff) | |
parent | 7b5689f42873d3a13921b2c008359c599451ceac (diff) |
Merge branch 'qmnguyen0711/reserve-usage-of-unavailable-codes' into 'master'
Reserve Unavailable response code for service-level unavailability
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5951
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Diffstat (limited to 'internal')
-rw-r--r-- | internal/gitaly/service/blob/get_blob.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/blob/get_blobs.go | 10 | ||||
-rw-r--r-- | internal/gitaly/service/commit/raw_blame.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/commit/tree_entry.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/diff/commit.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/diff/commit_test.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/diff/find_changed_paths.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/diff/numstat.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/diff/numstat_test.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/diff/raw.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/receive_pack.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/upload_pack.go | 6 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/upload_pack_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_pack.go | 2 | ||||
-rw-r--r-- | internal/grpc/middleware/limithandler/rate_limiter.go | 2 | ||||
-rw-r--r-- | internal/structerr/error.go | 18 |
16 files changed, 52 insertions, 38 deletions
diff --git a/internal/gitaly/service/blob/get_blob.go b/internal/gitaly/service/blob/get_blob.go index bde49365a..ea4d294d7 100644 --- a/internal/gitaly/service/blob/get_blob.go +++ b/internal/gitaly/service/blob/get_blob.go @@ -31,7 +31,7 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic if err != nil { if catfile.IsNotFound(err) { if err := stream.Send(&gitalypb.GetBlobResponse{}); err != nil { - return structerr.NewUnavailable("sending empty response: %w", err) + return structerr.NewAborted("sending empty response: %w", err) } return nil } @@ -40,7 +40,7 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic if blob.Type != "blob" { if err := stream.Send(&gitalypb.GetBlobResponse{}); err != nil { - return structerr.NewUnavailable("sending empty response: %w", err) + return structerr.NewAborted("sending empty response: %w", err) } return nil @@ -57,7 +57,7 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic if readLimit == 0 { if err := stream.Send(firstMessage); err != nil { - return structerr.NewUnavailable("sending empty blob: %w", err) + return structerr.NewAborted("sending empty blob: %w", err) } return nil @@ -75,7 +75,7 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic _, err = io.CopyN(sw, blob, readLimit) if err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } return nil diff --git a/internal/gitaly/service/blob/get_blobs.go b/internal/gitaly/service/blob/get_blobs.go index e27b54ae8..bb6042370 100644 --- a/internal/gitaly/service/blob/get_blobs.go +++ b/internal/gitaly/service/blob/get_blobs.go @@ -46,7 +46,7 @@ func sendGetBlobsResponse( if treeEntry == nil || len(treeEntry.Oid) == 0 { if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } continue @@ -60,7 +60,7 @@ func sendGetBlobsResponse( response.Type = gitalypb.ObjectType_COMMIT if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } continue @@ -82,7 +82,7 @@ func sendGetBlobsResponse( if response.Type != gitalypb.ObjectType_BLOB { if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } continue } @@ -115,7 +115,7 @@ func sendBlobTreeEntry( // blobObj. if readLimit == 0 { if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } return nil } @@ -147,7 +147,7 @@ func sendBlobTreeEntry( _, err = io.CopyN(sw, blobObj, readLimit) if err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } return nil diff --git a/internal/gitaly/service/commit/raw_blame.go b/internal/gitaly/service/commit/raw_blame.go index 366e243fd..aeb74bd32 100644 --- a/internal/gitaly/service/commit/raw_blame.go +++ b/internal/gitaly/service/commit/raw_blame.go @@ -46,7 +46,7 @@ func (s *server) RawBlame(in *gitalypb.RawBlameRequest, stream gitalypb.CommitSe _, err = io.Copy(sw, cmd) if err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } if err := cmd.Wait(); err != nil { diff --git a/internal/gitaly/service/commit/tree_entry.go b/internal/gitaly/service/commit/tree_entry.go index 881bedcfc..b1cf25ba1 100644 --- a/internal/gitaly/service/commit/tree_entry.go +++ b/internal/gitaly/service/commit/tree_entry.go @@ -38,7 +38,7 @@ func sendTreeEntry( Oid: treeEntry.Oid, } if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } return nil @@ -58,7 +58,7 @@ func sendTreeEntry( } if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("sending response: %w", err) + return structerr.NewAborted("sending response: %w", err) } return nil @@ -97,7 +97,7 @@ func sendTreeEntry( } if dataLength == 0 { if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("sending response: %w", err) + return structerr.NewAborted("sending response: %w", err) } return nil @@ -115,7 +115,7 @@ func sendTreeEntry( response.Data = p if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } // Use a new response so we don't send other fields (Size, ...) over and over diff --git a/internal/gitaly/service/diff/commit.go b/internal/gitaly/service/diff/commit.go index 35c5aa4fc..961c9f165 100644 --- a/internal/gitaly/service/diff/commit.go +++ b/internal/gitaly/service/diff/commit.go @@ -109,7 +109,7 @@ func (s *server) CommitDiff(in *gitalypb.CommitDiffRequest, stream gitalypb.Diff response.EndOfPatch = true if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } } else { patch := diff.Patch @@ -125,7 +125,7 @@ func (s *server) CommitDiff(in *gitalypb.CommitDiffRequest, stream gitalypb.Diff } if err := stream.Send(response); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } // Use a new response so we don't send other fields (FromPath, ...) over and over @@ -177,7 +177,7 @@ func (s *server) CommitDelta(in *gitalypb.CommitDeltaRequest, stream gitalypb.Di } if err := stream.Send(&gitalypb.CommitDeltaResponse{Deltas: batch}); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } return nil @@ -249,7 +249,7 @@ func (s *server) eachDiff(ctx context.Context, repo *gitalypb.Repository, subCmd } if err := cmd.Wait(); err != nil { - return structerr.NewUnavailable("%w", err) + return structerr.NewFailedPrecondition("%w", err) } return nil diff --git a/internal/gitaly/service/diff/commit_test.go b/internal/gitaly/service/diff/commit_test.go index aa23953d7..dc9bf8f79 100644 --- a/internal/gitaly/service/diff/commit_test.go +++ b/internal/gitaly/service/diff/commit_test.go @@ -1353,7 +1353,7 @@ func TestFailedCommitDiffRequestWithNonExistentCommit(t *testing.T) { require.NoError(t, err) err = drainCommitDiffResponse(c) - testhelper.RequireGrpcCode(t, err, codes.Unavailable) + testhelper.RequireGrpcCode(t, err, codes.FailedPrecondition) } func TestSuccessfulCommitDeltaRequest(t *testing.T) { @@ -1562,7 +1562,7 @@ func TestFailedCommitDeltaRequestWithNonExistentCommit(t *testing.T) { require.NoError(t, err) err = drainCommitDeltaResponse(c) - testhelper.RequireGrpcCode(t, err, codes.Unavailable) + testhelper.RequireGrpcCode(t, err, codes.FailedPrecondition) } func drainCommitDiffResponse(c gitalypb.DiffService_CommitDiffClient) error { diff --git a/internal/gitaly/service/diff/find_changed_paths.go b/internal/gitaly/service/diff/find_changed_paths.go index 9e3e6ada2..440fa82df 100644 --- a/internal/gitaly/service/diff/find_changed_paths.go +++ b/internal/gitaly/service/diff/find_changed_paths.go @@ -90,7 +90,7 @@ func (s *server) FindChangedPaths(in *gitalypb.FindChangedPathsRequest, stream g } if err := cmd.Wait(); err != nil { - return structerr.NewUnavailable("cmd wait err: %w", err) + return structerr.NewFailedPrecondition("cmd wait err: %w", err) } return diffChunker.Flush() diff --git a/internal/gitaly/service/diff/numstat.go b/internal/gitaly/service/diff/numstat.go index e0fbbaff5..d4bf2dae9 100644 --- a/internal/gitaly/service/diff/numstat.go +++ b/internal/gitaly/service/diff/numstat.go @@ -57,7 +57,7 @@ func (s *server) DiffStats(in *gitalypb.DiffStatsRequest, stream gitalypb.DiffSe } if err := cmd.Wait(); err != nil { - return structerr.NewUnavailable("%w", err) + return structerr.NewFailedPrecondition("%w", err) } return sendStats(batch, stream) @@ -69,7 +69,7 @@ func sendStats(batch []*gitalypb.DiffStats, stream gitalypb.DiffService_DiffStat } if err := stream.Send(&gitalypb.DiffStatsResponse{Stats: batch}); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } return nil diff --git a/internal/gitaly/service/diff/numstat_test.go b/internal/gitaly/service/diff/numstat_test.go index bd95e6559..38f55b576 100644 --- a/internal/gitaly/service/diff/numstat_test.go +++ b/internal/gitaly/service/diff/numstat_test.go @@ -188,28 +188,28 @@ func TestFailedDiffStatsRequest(t *testing.T) { repo: repo, leftCommitID: "invalidinvalidinvalid", rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab", - expectedErr: status.Error(codes.Unavailable, "exit status 128"), + expectedErr: status.Error(codes.FailedPrecondition, "exit status 128"), }, { desc: "invalid right commit", repo: repo, leftCommitID: "e4003da16c1c2c3fc4567700121b17bf8e591c6c", rightCommitID: "invalidinvalidinvalid", - expectedErr: status.Error(codes.Unavailable, "exit status 128"), + expectedErr: status.Error(codes.FailedPrecondition, "exit status 128"), }, { desc: "left commit not found", repo: repo, leftCommitID: "a4003da16c1c2c3fc4567700121b17bf8e591c6c", rightCommitID: "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab", - expectedErr: status.Error(codes.Unavailable, "exit status 128"), + expectedErr: status.Error(codes.FailedPrecondition, "exit status 128"), }, { desc: "right commit not found", repo: repo, leftCommitID: "e4003da16c1c2c3fc4567700121b17bf8e591c6c", rightCommitID: "a4003da16c1c2c3fc4567700121b17bf8e591c6c", - expectedErr: status.Error(codes.Unavailable, "exit status 128"), + expectedErr: status.Error(codes.FailedPrecondition, "exit status 128"), }, } diff --git a/internal/gitaly/service/diff/raw.go b/internal/gitaly/service/diff/raw.go index c6927b531..0882f0302 100644 --- a/internal/gitaly/service/diff/raw.go +++ b/internal/gitaly/service/diff/raw.go @@ -53,7 +53,7 @@ func sendRawOutput(ctx context.Context, gitCmdFactory git.CommandFactory, repo * } if _, err := io.Copy(sender, cmd); err != nil { - return structerr.NewUnavailable("send: %w", err) + return structerr.NewAborted("send: %w", err) } return cmd.Wait() diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go index 7b6a21f63..d440bc8db 100644 --- a/internal/gitaly/service/smarthttp/receive_pack.go +++ b/internal/gitaly/service/smarthttp/receive_pack.go @@ -63,11 +63,11 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac git.WithConfig(config...), ) if err != nil { - return structerr.NewUnavailable("spawning receive-pack: %w", err) + return structerr.NewFailedPrecondition("spawning receive-pack: %w", err) } if err := cmd.Wait(); err != nil { - return structerr.NewUnavailable("waiting for receive-pack: %w", err) + return structerr.NewFailedPrecondition("waiting for receive-pack: %w", err) } // In cases where all reference updates are rejected by git-receive-pack(1), we would end up diff --git a/internal/gitaly/service/smarthttp/upload_pack.go b/internal/gitaly/service/smarthttp/upload_pack.go index 5f004c9dd..589e8456e 100644 --- a/internal/gitaly/service/smarthttp/upload_pack.go +++ b/internal/gitaly/service/smarthttp/upload_pack.go @@ -119,13 +119,13 @@ func (s *server) runUploadPack(ctx context.Context, req *gitalypb.PostUploadPack Args: []string{repoPath}, }, commandOpts...) if err != nil { - return nil, structerr.NewUnavailable("spawning upload-pack: %w", err) + return nil, structerr.NewFailedPrecondition("spawning upload-pack: %w", err) } // Use a custom buffer size to minimize the number of system calls. respBytes, err := io.CopyBuffer(stdout, cmd, make([]byte, 64*1024)) if err != nil { - return nil, structerr.NewUnavailable("copying stdout from upload-pack: %w", err) + return nil, structerr.NewFailedPrecondition("copying stdout from upload-pack: %w", err) } if err := cmd.Wait(); err != nil { @@ -137,7 +137,7 @@ func (s *server) runUploadPack(ctx context.Context, req *gitalypb.PostUploadPack return stats, nil } - return nil, structerr.NewUnavailable("waiting for upload-pack: %w", err) + return nil, structerr.NewFailedPrecondition("waiting for upload-pack: %w", err) } ctxlogrus.Extract(ctx).WithField("request_sha", fmt.Sprintf("%x", h.Sum(nil))).WithField("response_bytes", respBytes).Info("request details") diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go index f3ca4f7d1..368d515b4 100644 --- a/internal/gitaly/service/smarthttp/upload_pack_test.go +++ b/internal/gitaly/service/smarthttp/upload_pack_test.go @@ -157,7 +157,7 @@ func testServerPostUploadPackGitConfigOptions(t *testing.T, ctx context.Context, }, } response, err := makeRequest(t, ctx, cfg.SocketPath, cfg.Auth.Token, rpcRequest, bytes.NewReader(requestBody.Bytes())) - testhelper.RequireGrpcError(t, structerr.NewUnavailable("running upload-pack: waiting for upload-pack: exit status 128"), err) + testhelper.RequireGrpcError(t, structerr.NewFailedPrecondition("running upload-pack: waiting for upload-pack: exit status 128"), err) // The failure message proves that upload-pack failed because of // GitConfigOptions, and that proves that passing GitConfigOptions works. diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go index aac712b91..c5815536a 100644 --- a/internal/gitaly/service/ssh/upload_pack.go +++ b/internal/gitaly/service/ssh/upload_pack.go @@ -206,7 +206,7 @@ func (rf *largeBufferReaderFrom) ReadFrom(r io.Reader) (int64, error) { func (s *server) SSHUploadPackWithSidechannel(ctx context.Context, req *gitalypb.SSHUploadPackWithSidechannelRequest) (*gitalypb.SSHUploadPackWithSidechannelResponse, error) { conn, err := sidechannel.OpenSidechannel(ctx) if err != nil { - return nil, structerr.NewUnavailable("opennig sidechannel: %w", err) + return nil, structerr.NewAborted("opennig sidechannel: %w", err) } defer conn.Close() diff --git a/internal/grpc/middleware/limithandler/rate_limiter.go b/internal/grpc/middleware/limithandler/rate_limiter.go index be09a1f77..577e68b1d 100644 --- a/internal/grpc/middleware/limithandler/rate_limiter.go +++ b/internal/grpc/middleware/limithandler/rate_limiter.go @@ -51,7 +51,7 @@ func (r *RateLimiter) Limit(ctx context.Context, lockKey string, f LimitedFunc) // of traffic. r.requestsDroppedMetric.Inc() - return nil, structerr.NewUnavailable("%w", ErrRateLimit).WithDetail( + return nil, structerr.NewResourceExhausted("%w", ErrRateLimit).WithDetail( &gitalypb.LimitError{ ErrorMessage: ErrRateLimit.Error(), RetryAfter: durationpb.New(0), diff --git a/internal/structerr/error.go b/internal/structerr/error.go index 541bec49e..184b0da90 100644 --- a/internal/structerr/error.go +++ b/internal/structerr/error.go @@ -177,8 +177,22 @@ func NewResourceExhausted(format string, a ...any) Error { return newError(codes.ResourceExhausted, format, a...) } -// NewUnavailable constructs a new error code with the Unavailable error code. Please refer to New -// for further details. +// NewUnavailable constructs a new error code with the Unavailable error code. Please refer to New for further details. +// Please note that the Unavailable status code is a signal telling clients to retry automatically. This auto-retry +// mechanism is handled at the library layer, without client consensus. Typically, it is used for the situations where +// the gRPC is not available or is not responding. Here are some discrete examples: +// +// - Server downtime: The server hosting the gRPC service is down for maintenance or has crashed. +// - Network issues: Connectivity problems between the client and server, like network congestion or a broken connection, +// can cause the service to appear unavailable. +// - Load balancing failure: In a distributed system, the load balancer may be unable to route the client's request to a +// healthy instance of the gRPC service. This can happen if all instances are down or if the load balancer is +// misconfigured. +// - TLS/SSL handshake failure: If there's a problem during the TLS/SSL handshake between the client and the server, the +// connection may fail, leading to an UNAVAILABLE status code. +// +// Thus, this status code should be used by interceptors or network-related components. gRPC handlers should use another +// status code instead. func NewUnavailable(format string, a ...any) Error { return newError(codes.Unavailable, format, a...) } |