From 1424b53fe7ebb508563d0d9b5ade7030dcd1fddb Mon Sep 17 00:00:00 2001 From: James Liu Date: Wed, 12 Mar 2025 11:51:29 +1100 Subject: [PATCH 1/7] grpc: Remove FailOnNonTempDialError options A subsequent commit will use `grpc.NewClient` instead of the deprecated `grpc.DialContext` function to create a client connection. One caveat with switching to `grpc.NewClient` is it doesn't support the grpc.WithBlock and grpc.FailOnNonTempDialError options, as unlike `grpc.DialContext`, it doesn't perform any I/O until the connection is actually used. Those options were originally added [1] to prevent `grpc.DialContext` from blocking indefinitely when given an invalid address, and are no longer needed because blocking will no longer occur during the instantiation of the client connection. Remove the options and the associated test case. The options are no-ops when `grpc.NewClient` is used. We don't remove the function from the public `client` package, as it may still be used by consumers of the package; we simply return an empty slice instead. [1] https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2849 --- client/dial.go | 2 +- client/dial_test.go | 6 ------ internal/cli/gitaly/serve.go | 5 ++--- internal/gitaly/transaction/manager.go | 5 ++--- internal/grpc/client/dial.go | 8 -------- 5 files changed, 5 insertions(+), 21 deletions(-) diff --git a/client/dial.go b/client/dial.go index ed4e02a667..0de36fa002 100644 --- a/client/dial.go +++ b/client/dial.go @@ -47,7 +47,7 @@ func DialSidechannel(ctx context.Context, rawAddress string, sr *SidechannelRegi // FailOnNonTempDialError helps to identify if remote listener is ready to accept new connections. func FailOnNonTempDialError() []grpc.DialOption { - return client.FailOnNonTempDialError() + return []grpc.DialOption{} } // HealthCheckDialer uses provided dialer as an actual dialer, but issues a health check request to the remote diff --git a/client/dial_test.go b/client/dial_test.go index 72738c23bc..62a7701853 100644 --- a/client/dial_test.go +++ b/client/dial_test.go @@ -117,12 +117,6 @@ func TestDial(t *testing.T) { rawAddress: "", expectDialFailure: true, }, - { - name: "dial fail if there is no listener on address", - rawAddress: "tcp://invalid.address", - dialOpts: FailOnNonTempDialError(), - expectDialFailure: true, - }, } for _, tc := range tests { diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go index d989396336..1bd51d5d6f 100644 --- a/internal/cli/gitaly/serve.go +++ b/internal/cli/gitaly/serve.go @@ -277,10 +277,9 @@ func run(appCtx *cli.Context, cfg config.Cfg, logger log.Logger) error { return client.Dial(ctx, address, client.WithGrpcOptions(opts)) }, )), - client.WithDialOptions(append( - client.FailOnNonTempDialError(), + client.WithDialOptions( client.UnaryInterceptor(), - client.StreamInterceptor())..., + client.StreamInterceptor(), ), ) defer func() { diff --git a/internal/gitaly/transaction/manager.go b/internal/gitaly/transaction/manager.go index e25d5cb546..54db2dff81 100644 --- a/internal/gitaly/transaction/manager.go +++ b/internal/gitaly/transaction/manager.go @@ -67,10 +67,9 @@ func NewManager(cfg config.Cfg, logger log.Logger, backchannels *backchannel.Reg return &PoolManager{ logger: logger.WithField("component", "transaction.PoolManager"), backchannels: backchannels, - conns: client.NewPool(client.WithDialOptions(append( - client.FailOnNonTempDialError(), + conns: client.NewPool(client.WithDialOptions( client.UnaryInterceptor(), - client.StreamInterceptor())..., + client.StreamInterceptor(), )), votingDelayMetric: prometheus.NewHistogram( prometheus.HistogramOpts{ diff --git a/internal/grpc/client/dial.go b/internal/grpc/client/dial.go index 4aad0e23cc..4b9219c6af 100644 --- a/internal/grpc/client/dial.go +++ b/internal/grpc/client/dial.go @@ -280,14 +280,6 @@ func defaultServiceConfig() string { return string(configJSON) } -// FailOnNonTempDialError helps to identify if remote listener is ready to accept new connections. -func FailOnNonTempDialError() []grpc.DialOption { - return []grpc.DialOption{ - grpc.WithBlock(), - grpc.FailOnNonTempDialError(true), - } -} - // HealthCheckDialer uses provided dialer as an actual dialer, but issues a health check request to the remote // to verify the connection was set properly and could be used with no issues. func HealthCheckDialer(base Dialer) Dialer { -- GitLab From ffd5938fc49dbb3da7e4a1d819012632d2ebecb3 Mon Sep 17 00:00:00 2001 From: James Liu Date: Fri, 7 Mar 2025 14:59:31 +1100 Subject: [PATCH 2/7] grpc: Enable dns lookup by default with NewClient Gitaly uses the deprecated grpc.DialContext [1] function to instantiate new client connections. This function relies on being provided a target address starting with the `dns:` scheme in order to configure DNS resolution. Before we provide the target address, we perform some formatting depending on the scheme provided by the user [2]. Gitaly addresses starting with `dns:` remain as-is, whereas addresses using the `tcp:` and `tls:` schemes are stripped of the scheme. For example: - `dns://mygitalynode.test` remains as so. This represents a non-TLS connection. - `tls://127.0.0.1:1234` is transformed to `127.0.0.1:1234`. - `tls://mygitalynode.test:1234` is transformed in a similar way. This transformed address is then provided to grpc.DialContext(). When the `dns:` prefixed address abovce is provided, it will cause the DNS resolver to be configured and used to resolve mygitalynode.test to the IP of the node. When the second, non-prefixed IP is provided, no DNS resolver is configured, and it's in fact not needed. When the third non-prefixed address is provided, despite requiring DNS resolution, no DNS resolver is configured due to the lack of the `dns:` scheme. The problem is that while plain TCP connections to a hostname can be configured via the `dns:` scheme, TLS connections to a hostname cannot, since that requires using the `tls:` prefix instead of `dns:`. The new grpc.NewClient() constructor resolves this problem by enabling DNS resolution by default. It's no longer required to use the `dns:` prefix; you can simply provide a `tls:` prefixed address with a hostname. For example, `tls://mygitalynode.test:1234` transforms into `mygitalynode.test:1234` before being provided to grpc.NewClient(). Despite the lack of an explicit `dns:` prefix, the DNS resolver is still used, and the IP will be resolved correctly. The one caveat is that addresses containing a DNS authority must still use the `dns:` prefix. `tls://mydnsserver.test/mygitalynode.test:1234` is invalid for example. Change the invocation of grpc.DialContext to grpc.NewClient. We wrap a lot of the gRPC connection code, so there's only one place we need to update the call. Also add missing integration tests to the gitaly-ssh command to exercise the various schemes we support. [1] https://pkg.go.dev/google.golang.org/grpc#DialContext [2] https://gitlab.com/gitlab-org/gitaly/-/blob/922659f937508f6e92c5d0f5a2f456bbf866c6d6/internal/grpc/client/dial.go#L113 --- client/dial.go | 12 ++++-- cmd/gitaly-ssh/auth_test.go | 39 +++++++++++++++++++ internal/cli/praefect/subcmd_test.go | 4 +- internal/gitaly/server/server_factory_test.go | 4 +- internal/grpc/client/dial.go | 6 +-- internal/grpc/middleware/cache/cache_test.go | 3 +- .../customfields_handler_test.go | 2 +- .../requestinfohandler_test.go | 2 +- .../sentryhandler/sentryhandler_test.go | 2 +- internal/grpc/proxy/handler_ext_test.go | 4 +- .../grpc/proxy/proxy_test_testhelper_test.go | 6 +-- internal/praefect/get_object_pool_test.go | 2 +- internal/praefect/remove_repository_test.go | 2 +- internal/praefect/repository_exists_test.go | 2 +- internal/praefect/server_factory_test.go | 2 +- internal/praefect/walkrepos_test.go | 2 +- 16 files changed, 66 insertions(+), 28 deletions(-) diff --git a/client/dial.go b/client/dial.go index 0de36fa002..15296b864f 100644 --- a/client/dial.go +++ b/client/dial.go @@ -18,11 +18,15 @@ import ( // DefaultDialOpts hold the default DialOptions for connection to Gitaly over UNIX-socket var DefaultDialOpts = []grpc.DialOption{} -// DialContext dials the Gitaly at the given address with the provided options. Valid address formats are -// 'unix:' for Unix sockets, 'tcp://' for insecure TCP connections and 'tls://' -// for TCP+TLS connections. +// DialContext creates a client connection to a Gitaly at the given address with the provided options. Valid address +// formats are +// - 'unix:' for Unix sockets +// - 'tcp://' for insecure TCP connections to an IP or hostname (resolved via DNS). +// - 'tls://' for TCP+TLS connections to an IP or hostname (resolved via DNS). +// - 'dns:///' for insecure TCP connections that should be resolved by the +// specified authoritative DNS server. Note that it's not possible to use TLS in conjunction with a DNS authority. // -// The returned ClientConns are configured with tracing and correlation id interceptors to ensure they are propagated +// The returned ClientConn is configured with tracing and correlation id interceptors to ensure they are propagated // correctly. They're also configured to send Keepalives with settings matching what Gitaly expects. // // connOpts should not contain `grpc.WithInsecure` as DialContext determines whether it is needed or not from the diff --git a/cmd/gitaly-ssh/auth_test.go b/cmd/gitaly-ssh/auth_test.go index 54d165ed79..e44467dbf4 100644 --- a/cmd/gitaly-ssh/auth_test.go +++ b/cmd/gitaly-ssh/auth_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/miekg/dns" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" @@ -102,6 +103,44 @@ func TestConnectivity(t *testing.T) { return runGitaly(t, cfg), certificate.CertPath }, }, + { + name: "dns", + addr: func(t *testing.T, cfg config.Cfg) (string, string) { + // Configure a Gitaly server that listens over TCP. + cfg.ListenAddr = "localhost:0" + gitalyAddr := runGitaly(t, cfg) + gitalyPort := strings.Split(gitalyAddr, ":")[2] + + // Start a DNS server that responds to anything with the loopback address. + dnsServer := testhelper.NewFakeDNSServer(t).WithHandler(dns.TypeA, func(host string) []string { + return []string{"127.0.0.1"} + }).Start() + + return fmt.Sprintf("dns://%s/%s", dnsServer.Addr(), "localhost:"+gitalyPort), "" + }, + }, + { + name: "dns (no authority)", + addr: func(t *testing.T, cfg config.Cfg) (string, string) { + // Configure a Gitaly server that listens over TCP. + cfg.ListenAddr = "localhost:0" + gitalyAddr := runGitaly(t, cfg) + gitalyPort := strings.Split(gitalyAddr, ":")[2] + + return "dns:///localhost:" + gitalyPort, "" + }, + }, + { + name: "tcp with dns address (no authority)", + addr: func(t *testing.T, cfg config.Cfg) (string, string) { + // Configure a Gitaly server that listens over TCP. + cfg.ListenAddr = "localhost:0" + gitalyAddr := runGitaly(t, cfg) + gitalyPort := strings.Split(gitalyAddr, ":")[2] + + return fmt.Sprintf("tcp://localhost:%s", gitalyPort), "" + }, + }, } payload, err := protojson.Marshal(&gitalypb.SSHUploadPackWithSidechannelRequest{ diff --git a/internal/cli/praefect/subcmd_test.go b/internal/cli/praefect/subcmd_test.go index 6b56040427..d99068a2ab 100644 --- a/internal/cli/praefect/subcmd_test.go +++ b/internal/cli/praefect/subcmd_test.go @@ -55,11 +55,9 @@ func listenAndServe(tb testing.TB, svcs []svcRegistrar) (net.Listener, testhelpe errCh := make(chan error, 1) go func() { errCh <- srv.Serve(ln) }() - ctx := testhelper.Context(tb) - // verify the service is up addr := fmt.Sprintf("%s://%s", ln.Addr().Network(), ln.Addr()) - cc, err := grpc.DialContext(ctx, addr, grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials())) + cc, err := grpc.NewClient(addr, grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(tb, err) require.NoError(tb, cc.Close()) diff --git a/internal/gitaly/server/server_factory_test.go b/internal/gitaly/server/server_factory_test.go index c6e27d3721..65f3231fa3 100644 --- a/internal/gitaly/server/server_factory_test.go +++ b/internal/gitaly/server/server_factory_test.go @@ -48,7 +48,7 @@ func TestGitalyServerFactory(t *testing.T) { creds := cert.TransportCredentials(t) - cc, err = grpc.DialContext(ctx, listener.Addr().String(), grpc.WithTransportCredentials(creds)) + cc, err = grpc.NewClient(listener.Addr().String(), grpc.WithTransportCredentials(creds)) require.NoError(t, err) } else { srv, err := sf.CreateExternal(false) @@ -296,7 +296,7 @@ func TestGitalyServerFactory_closeOrder(t *testing.T) { go testhelper.MustServe(t, server, ln) - conn, err := grpc.DialContext(ctx, ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + conn, err := grpc.NewClient(ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) t.Cleanup(func() { testhelper.MustClose(t, conn) }) *builder.conn = conn diff --git a/internal/grpc/client/dial.go b/internal/grpc/client/dial.go index 4b9219c6af..eb1f35de6f 100644 --- a/internal/grpc/client/dial.go +++ b/internal/grpc/client/dial.go @@ -31,6 +31,7 @@ const ( tlsConnection unixConnection dnsConnection + dnsPlusTLSConnection ) func getConnectionType(rawAddress string) connectionType { @@ -79,7 +80,7 @@ func WithHandshaker(handshaker Handshaker) DialOption { } // WithGrpcOptions will set up the given gRPC dial options so that they will be used when calling -// `grpc.DialContext()`. +// `grpc.NewClient()`. func WithGrpcOptions(opts []grpc.DialOption) DialOption { return func(cfg *dialConfig) { cfg.grpcOpts = append(cfg.grpcOpts, opts...) @@ -131,7 +132,6 @@ func Dial(ctx context.Context, rawAddress string, opts ...DialOption) (*grpc.Cli return nil, fmt.Errorf("failed to parse target for 'dns' connection: %w", err) } canonicalAddress = rawAddress // DNS Resolver will handle this - case unixConnection: canonicalAddress = rawAddress // This will be overridden by the custom dialer... connOpts = append( @@ -208,7 +208,7 @@ func Dial(ctx context.Context, rawAddress string, opts ...DialOption) (*grpc.Cli grpc.WithDefaultServiceConfig(defaultServiceConfig()), ) - conn, err := grpc.DialContext(ctx, canonicalAddress, connOpts...) + conn, err := grpc.NewClient(canonicalAddress, connOpts...) if err != nil { return nil, fmt.Errorf("failed to dial %q connection: %w", canonicalAddress, err) } diff --git a/internal/grpc/middleware/cache/cache_test.go b/internal/grpc/middleware/cache/cache_test.go index e911fd85d8..73c4a67591 100644 --- a/internal/grpc/middleware/cache/cache_test.go +++ b/internal/grpc/middleware/cache/cache_test.go @@ -130,8 +130,7 @@ func TestInvalidators(t *testing.T) { }() t.Cleanup(server.Stop) - conn, err := grpc.DialContext( - ctx, + conn, err := grpc.NewClient( listener.Addr().String(), grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials()), diff --git a/internal/grpc/middleware/customfieldshandler/customfields_handler_test.go b/internal/grpc/middleware/customfieldshandler/customfields_handler_test.go index 1ad7cdc319..f47207f65e 100644 --- a/internal/grpc/middleware/customfieldshandler/customfields_handler_test.go +++ b/internal/grpc/middleware/customfieldshandler/customfields_handler_test.go @@ -132,7 +132,7 @@ func TestInterceptor(t *testing.T) { t.Run(tt.name, func(t *testing.T) { hook.Reset() - conn, err := grpc.DialContext(ctx, "", grpc.WithContextDialer(getBufDialer(listener)), grpc.WithTransportCredentials(insecure.NewCredentials())) + conn, err := grpc.NewClient("passthrough://", grpc.WithContextDialer(getBufDialer(listener)), grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) defer conn.Close() diff --git a/internal/grpc/middleware/requestinfohandler/requestinfohandler_test.go b/internal/grpc/middleware/requestinfohandler/requestinfohandler_test.go index 2b72eb4c0e..6d82abde34 100644 --- a/internal/grpc/middleware/requestinfohandler/requestinfohandler_test.go +++ b/internal/grpc/middleware/requestinfohandler/requestinfohandler_test.go @@ -845,7 +845,7 @@ func setupServer(tb testing.TB, ctx context.Context) (*mockServer, mockClient) { listener := bufconn.Listen(1) go testhelper.MustServe(tb, server, listener) - conn, err := grpc.DialContext(ctx, listener.Addr().String(), + conn, err := grpc.NewClient("passthrough://"+listener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithContextDialer(func(ctx context.Context, _ string) (net.Conn, error) { return listener.DialContext(ctx) diff --git a/internal/grpc/middleware/sentryhandler/sentryhandler_test.go b/internal/grpc/middleware/sentryhandler/sentryhandler_test.go index 61d67ed106..9ca48ebe4d 100644 --- a/internal/grpc/middleware/sentryhandler/sentryhandler_test.go +++ b/internal/grpc/middleware/sentryhandler/sentryhandler_test.go @@ -415,7 +415,7 @@ func (s *mockServiceServer) setup(tb testing.TB, ctx context.Context) grpc_testi listener := bufconn.Listen(1) go testhelper.MustServe(tb, server, listener) - conn, err := grpc.DialContext(ctx, listener.Addr().String(), + conn, err := grpc.NewClient("passthrough://"+listener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithContextDialer(func(ctx context.Context, _ string) (net.Conn, error) { return listener.DialContext(ctx) diff --git a/internal/grpc/proxy/handler_ext_test.go b/internal/grpc/proxy/handler_ext_test.go index ed99af19a0..11cebbecaf 100644 --- a/internal/grpc/proxy/handler_ext_test.go +++ b/internal/grpc/proxy/handler_ext_test.go @@ -367,7 +367,7 @@ func TestProxyErrorPropagation(t *testing.T) { }() defer backendServer.Stop() - backendClientConn, err := grpc.DialContext(ctx, "unix://"+backendListener.Addr().String(), + backendClientConn, err := grpc.NewClient("unix://"+backendListener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithDefaultCallOptions(grpc.ForceCodec(proxy.NewCodec())), ) @@ -399,7 +399,7 @@ func TestProxyErrorPropagation(t *testing.T) { }() defer proxyServer.Stop() - proxyClientConn, err := grpc.DialContext(ctx, "unix://"+proxyListener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + proxyClientConn, err := grpc.NewClient("unix://"+proxyListener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) defer testhelper.MustClose(t, proxyClientConn) diff --git a/internal/grpc/proxy/proxy_test_testhelper_test.go b/internal/grpc/proxy/proxy_test_testhelper_test.go index 3fef99bda1..6bbf09b656 100644 --- a/internal/grpc/proxy/proxy_test_testhelper_test.go +++ b/internal/grpc/proxy/proxy_test_testhelper_test.go @@ -34,8 +34,7 @@ func newBackendPinger(tb testing.TB, ctx context.Context) (*grpc.ClientConn, *in require.NoError(tb, srvr.Serve(listener)) }() - cc, err := grpc.DialContext( - ctx, + cc, err := grpc.NewClient( listener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock(), @@ -72,8 +71,7 @@ func newProxy(tb testing.TB, ctx context.Context, director proxy.StreamDirector, require.NoError(tb, proxySrvr.Serve(listener)) }() - proxyCC, err := grpc.DialContext( - ctx, + proxyCC, err := grpc.NewClient( listener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock(), diff --git a/internal/praefect/get_object_pool_test.go b/internal/praefect/get_object_pool_test.go index 989f96e0cb..4292730675 100644 --- a/internal/praefect/get_object_pool_test.go +++ b/internal/praefect/get_object_pool_test.go @@ -96,7 +96,7 @@ func TestGetObjectPoolHandler(t *testing.T) { go testhelper.MustServe(t, srv, ln) - clientConn, err := grpc.DialContext(ctx, "unix:"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + clientConn, err := grpc.NewClient("unix:"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) t.Cleanup(func() { testhelper.MustClose(t, clientConn) diff --git a/internal/praefect/remove_repository_test.go b/internal/praefect/remove_repository_test.go index 50792ddbc5..89ca0a3e19 100644 --- a/internal/praefect/remove_repository_test.go +++ b/internal/praefect/remove_repository_test.go @@ -131,7 +131,7 @@ func TestRemoveRepositoryHandler(t *testing.T) { go testhelper.MustServe(t, srv, ln) - clientConn, err := grpc.DialContext(ctx, "unix:"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + clientConn, err := grpc.NewClient("unix:"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) defer clientConn.Close() diff --git a/internal/praefect/repository_exists_test.go b/internal/praefect/repository_exists_test.go index 6c4ed1f92c..ca36e54246 100644 --- a/internal/praefect/repository_exists_test.go +++ b/internal/praefect/repository_exists_test.go @@ -98,7 +98,7 @@ func TestRepositoryExistsHandler(t *testing.T) { go testhelper.MustServe(t, srv, ln) - clientConn, err := grpc.DialContext(ctx, "unix://"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + clientConn, err := grpc.NewClient("unix://"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) defer testhelper.MustClose(t, clientConn) diff --git a/internal/praefect/server_factory_test.go b/internal/praefect/server_factory_test.go index 5910de7c7a..81137cf4fd 100644 --- a/internal/praefect/server_factory_test.go +++ b/internal/praefect/server_factory_test.go @@ -237,7 +237,7 @@ func TestServerFactory(t *testing.T) { creds := certificate.TransportCredentials(t) - cc, err := grpc.DialContext(ctx, listener.Addr().String(), grpc.WithTransportCredentials(creds)) + cc, err := grpc.NewClient(listener.Addr().String(), grpc.WithTransportCredentials(creds)) require.NoError(t, err) defer func() { require.NoError(t, cc.Close()) }() diff --git a/internal/praefect/walkrepos_test.go b/internal/praefect/walkrepos_test.go index 63301caf58..7b02583867 100644 --- a/internal/praefect/walkrepos_test.go +++ b/internal/praefect/walkrepos_test.go @@ -65,7 +65,7 @@ func TestWalkReposHandler(t *testing.T) { go testhelper.MustServe(t, srv, ln) - clientConn, err := grpc.DialContext(ctx, "unix://"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + clientConn, err := grpc.NewClient("unix://"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) defer testhelper.MustClose(t, clientConn) -- GitLab From a71c83298745d4139e260aec5a40c362b435aee7 Mon Sep 17 00:00:00 2001 From: James Liu Date: Wed, 12 Mar 2025 12:19:24 +1100 Subject: [PATCH 3/7] grpc: Rename client.Dial to client.New The change to `grpc.NewClient` means that we're not actually dialing the remote. Change the name of the internal `client.Dial` function to reflect this new behaviour. I don't think we should change the function names in the public `client` package yet, as that would require more client-side changes. --- client/dial.go | 2 +- cmd/gitaly-hooks/hooks.go | 2 +- internal/backup/backup_test.go | 4 ++-- internal/backup/repository_test.go | 4 ++-- internal/cli/gitaly/serve.go | 2 +- internal/cli/gitaly/subcmd_hooks.go | 2 +- internal/cli/gitaly/subcmd_hooks_test.go | 2 +- internal/cli/praefect/subcmd.go | 2 +- .../praefect/subcmd_list_untracked_repositories_test.go | 2 +- internal/cli/praefect/subcmd_remove_repository_test.go | 6 +++--- internal/cli/praefect/subcmd_track_repositories_test.go | 2 +- internal/cli/praefect/subcmd_track_repository_test.go | 4 ++-- internal/git/gittest/repo.go | 2 +- internal/gitaly/server/server_factory_test.go | 2 +- internal/gitaly/service/operations/testhelper_test.go | 2 +- internal/gitaly/service/partition/testhelper_test.go | 2 +- internal/gitaly/service/repository/testhelper_test.go | 4 ++-- internal/gitaly/service/smarthttp/testhelper_test.go | 2 +- internal/gitaly/service/smarthttp/upload_pack_test.go | 2 +- internal/gitaly/service/ssh/upload_pack_test.go | 2 +- internal/gitaly/storage/storagemgr/middleware_ext_test.go | 8 ++++---- internal/gitaly/storage/storagemgr/middleware_test.go | 4 ++-- internal/gitaly/transaction/manager_test.go | 4 ++-- internal/grpc/client/address_parser.go | 2 +- internal/grpc/client/dial.go | 8 ++++---- internal/grpc/client/dial_ext_test.go | 2 +- internal/grpc/client/dial_test.go | 4 ++-- internal/grpc/client/pool.go | 4 ++-- internal/grpc/grpcstats/stats_test.go | 2 +- internal/grpc/proxy/handler_ext_test.go | 2 +- internal/grpc/sidechannel/conn.go | 2 +- internal/praefect/coordinator_test.go | 2 +- internal/praefect/nodes/manager.go | 2 +- internal/praefect/nodes/manager_test.go | 2 +- internal/praefect/nodes/ping.go | 2 +- internal/praefect/remove_repository_test.go | 4 ++-- internal/praefect/replicator_pg_test.go | 2 +- internal/praefect/server_factory_test.go | 4 ++-- internal/praefect/testserver.go | 2 +- internal/testhelper/testserver/gitaly.go | 2 +- tools/test-boot/main.go | 2 +- 41 files changed, 59 insertions(+), 59 deletions(-) diff --git a/client/dial.go b/client/dial.go index 15296b864f..af69c37352 100644 --- a/client/dial.go +++ b/client/dial.go @@ -32,7 +32,7 @@ var DefaultDialOpts = []grpc.DialOption{} // connOpts should not contain `grpc.WithInsecure` as DialContext determines whether it is needed or not from the // scheme. `grpc.TransportCredentials` should not be provided either as those are handled internally as well. func DialContext(ctx context.Context, rawAddress string, connOpts []grpc.DialOption) (*grpc.ClientConn, error) { - return client.Dial(ctx, rawAddress, client.WithGrpcOptions(connOpts)) + return client.New(ctx, rawAddress, client.WithGrpcOptions(connOpts)) } // Dial calls DialContext with the provided arguments and context.Background. Refer to DialContext's documentation diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go index aa53056fc5..c10ae9d9a9 100644 --- a/cmd/gitaly-hooks/hooks.go +++ b/cmd/gitaly-hooks/hooks.go @@ -218,7 +218,7 @@ func dialGitaly(ctx context.Context, payload gitcmd.HooksPayload) (*grpc.ClientC dialOpts = append(dialOpts, grpc.WithChainUnaryInterceptor(unaryInterceptors...)) dialOpts = append(dialOpts, grpc.WithChainStreamInterceptor(streamInterceptors...)) - conn, err := client.Dial(ctx, "unix://"+payload.InternalSocket, client.WithGrpcOptions(dialOpts)) + conn, err := client.New(ctx, "unix://"+payload.InternalSocket, client.WithGrpcOptions(dialOpts)) if err != nil { return nil, fmt.Errorf("error when dialing: %w", err) } diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 0ab3045aa5..7d97ace61d 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -484,7 +484,7 @@ func TestManager_Restore_latest(t *testing.T) { ctx := testhelper.Context(t) - cc, err := client.Dial(ctx, cfg.SocketPath) + cc, err := client.New(ctx, cfg.SocketPath) require.NoError(t, err) defer testhelper.MustClose(t, cc) @@ -849,7 +849,7 @@ func TestManager_Restore_specific(t *testing.T) { ctx := testhelper.Context(t) - cc, err := client.Dial(ctx, cfg.SocketPath) + cc, err := client.New(ctx, cfg.SocketPath) require.NoError(t, err) defer testhelper.MustClose(t, cc) diff --git a/internal/backup/repository_test.go b/internal/backup/repository_test.go index cc2d1d1f07..3ca54168d0 100644 --- a/internal/backup/repository_test.go +++ b/internal/backup/repository_test.go @@ -292,7 +292,7 @@ func TestCreateBundlePatterns_HandleEOF(t *testing.T) { cfg.SocketPath = testserver.RunGitalyServer(t, cfg, setup.RegisterAll) - conn, err := client.Dial(ctx, cfg.SocketPath) + conn, err := client.New(ctx, cfg.SocketPath) require.NoError(t, err) defer testhelper.MustClose(t, conn) @@ -311,7 +311,7 @@ func TestRemoteRepository_ResetRefs_HandleEOF(t *testing.T) { cfg.SocketPath = testserver.RunGitalyServer(t, cfg, setup.RegisterAll) ctx := testhelper.Context(t) - conn, err := client.Dial(ctx, cfg.SocketPath) + conn, err := client.New(ctx, cfg.SocketPath) require.NoError(t, err) defer testhelper.MustClose(t, conn) diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go index 1bd51d5d6f..9440308cb4 100644 --- a/internal/cli/gitaly/serve.go +++ b/internal/cli/gitaly/serve.go @@ -274,7 +274,7 @@ func run(appCtx *cli.Context, cfg config.Cfg, logger log.Logger) error { conns := client.NewPool( client.WithDialer(client.HealthCheckDialer( func(ctx context.Context, address string, opts []grpc.DialOption) (*grpc.ClientConn, error) { - return client.Dial(ctx, address, client.WithGrpcOptions(opts)) + return client.New(ctx, address, client.WithGrpcOptions(opts)) }, )), client.WithDialOptions( diff --git a/internal/cli/gitaly/subcmd_hooks.go b/internal/cli/gitaly/subcmd_hooks.go index 9b81003f35..9e7f32e20b 100644 --- a/internal/cli/gitaly/subcmd_hooks.go +++ b/internal/cli/gitaly/subcmd_hooks.go @@ -156,7 +156,7 @@ func dial(ctx context.Context, addr, token string, timeout time.Duration, opts . ) } - return client.Dial(ctx, addr, client.WithGrpcOptions(opts)) + return client.New(ctx, addr, client.WithGrpcOptions(opts)) } func getAddressWithScheme(cfg config.Cfg) (string, error) { diff --git a/internal/cli/gitaly/subcmd_hooks_test.go b/internal/cli/gitaly/subcmd_hooks_test.go index 01b4942a49..c647c5701b 100644 --- a/internal/cli/gitaly/subcmd_hooks_test.go +++ b/internal/cli/gitaly/subcmd_hooks_test.go @@ -223,7 +223,7 @@ func newRepositoryClient(tb testing.TB, ctx context.Context, cfg config.Cfg, ser if cfg.Auth.Token != "" { connOpts = append(connOpts, grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(cfg.Auth.Token))) } - conn, err := client.Dial(ctx, serverSocketPath, client.WithGrpcOptions(connOpts)) + conn, err := client.New(ctx, serverSocketPath, client.WithGrpcOptions(connOpts)) require.NoError(tb, err) tb.Cleanup(func() { require.NoError(tb, conn.Close()) }) diff --git a/internal/cli/praefect/subcmd.go b/internal/cli/praefect/subcmd.go index bb171b2a32..da93cc1aa9 100644 --- a/internal/cli/praefect/subcmd.go +++ b/internal/cli/praefect/subcmd.go @@ -73,7 +73,7 @@ func subCmdDial(ctx context.Context, addr, token string, timeout time.Duration, ) } - return client.Dial(ctx, addr, client.WithGrpcOptions(opts)) + return client.New(ctx, addr, client.WithGrpcOptions(opts)) } type requiredParameterError string diff --git a/internal/cli/praefect/subcmd_list_untracked_repositories_test.go b/internal/cli/praefect/subcmd_list_untracked_repositories_test.go index 7f53020326..3a026a0986 100644 --- a/internal/cli/praefect/subcmd_list_untracked_repositories_test.go +++ b/internal/cli/praefect/subcmd_list_untracked_repositories_test.go @@ -56,7 +56,7 @@ func TestListUntrackedRepositoriesCommand(t *testing.T) { praefectServer := testserver.StartPraefect(t, conf) - cc, err := client.Dial(ctx, praefectServer.Address()) + cc, err := client.New(ctx, praefectServer.Address()) require.NoError(t, err) defer func() { require.NoError(t, cc.Close()) }() repoClient := gitalypb.NewRepositoryServiceClient(cc) diff --git a/internal/cli/praefect/subcmd_remove_repository_test.go b/internal/cli/praefect/subcmd_remove_repository_test.go index 3f0a6b62b2..44e76b16da 100644 --- a/internal/cli/praefect/subcmd_remove_repository_test.go +++ b/internal/cli/praefect/subcmd_remove_repository_test.go @@ -38,11 +38,11 @@ func TestRemoveRepositorySubcommand(t *testing.T) { gitalyOneAddr := testserver.RunGitalyServer(t, gitalyOneCfg, setup.RegisterAll, testserver.WithDisablePraefect()) gitalyTwoSrv := testserver.StartGitalyServer(t, gitalyTwoCfg, setup.RegisterAll, testserver.WithDisablePraefect()) - gitalyOneConfig, err := client.Dial(ctx, gitalyOneAddr) + gitalyOneConfig, err := client.New(ctx, gitalyOneAddr) require.NoError(t, err) defer testhelper.MustClose(t, gitalyOneConfig) - gitalyTwoConfig, err := client.Dial(ctx, gitalyTwoSrv.Address()) + gitalyTwoConfig, err := client.New(ctx, gitalyTwoSrv.Address()) require.NoError(t, err) defer testhelper.MustClose(t, gitalyTwoConfig) @@ -70,7 +70,7 @@ func TestRemoveRepositorySubcommand(t *testing.T) { praefectServer := testserver.StartPraefect(t, conf) - cc, err := client.Dial(ctx, praefectServer.Address()) + cc, err := client.New(ctx, praefectServer.Address()) require.NoError(t, err) t.Cleanup(func() { require.NoError(t, cc.Close()) }) repoClient := gitalypb.NewRepositoryServiceClient(cc) diff --git a/internal/cli/praefect/subcmd_track_repositories_test.go b/internal/cli/praefect/subcmd_track_repositories_test.go index 77a2718635..75fabc7cc5 100644 --- a/internal/cli/praefect/subcmd_track_repositories_test.go +++ b/internal/cli/praefect/subcmd_track_repositories_test.go @@ -69,7 +69,7 @@ func TestTrackRepositoriesSubcommand(t *testing.T) { } confPath := writeConfigToFile(t, conf) - gitalyOneConn, err := client.Dial(ctx, gitalyOneAddr) + gitalyOneConn, err := client.New(ctx, gitalyOneAddr) require.NoError(t, err) defer testhelper.MustClose(t, gitalyOneConn) diff --git a/internal/cli/praefect/subcmd_track_repository_test.go b/internal/cli/praefect/subcmd_track_repository_test.go index 332988d801..53509a1e86 100644 --- a/internal/cli/praefect/subcmd_track_repository_test.go +++ b/internal/cli/praefect/subcmd_track_repository_test.go @@ -64,11 +64,11 @@ func TestTrackRepositorySubcommand(t *testing.T) { } confPath := writeConfigToFile(t, conf) - gitalyOneConn, err := client.Dial(ctx, gitalyOneAddr) + gitalyOneConn, err := client.New(ctx, gitalyOneAddr) require.NoError(t, err) defer testhelper.MustClose(t, gitalyOneConn) - gitalyTwoConn, err := client.Dial(ctx, gitalyTwoSrv.Address()) + gitalyTwoConn, err := client.New(ctx, gitalyTwoSrv.Address()) require.NoError(t, err) defer testhelper.MustClose(t, gitalyTwoConn) diff --git a/internal/git/gittest/repo.go b/internal/git/gittest/repo.go index f3a5dd42e9..fb15ffff94 100644 --- a/internal/git/gittest/repo.go +++ b/internal/git/gittest/repo.go @@ -118,7 +118,7 @@ func DialService(tb testing.TB, ctx context.Context, cfg config.Cfg) *grpc.Clien require.FailNow(tb, "cannot dial service without configured address") } - conn, err := client.Dial(ctx, addr, client.WithGrpcOptions(dialOptions)) + conn, err := client.New(ctx, addr, client.WithGrpcOptions(dialOptions)) require.NoError(tb, err) tb.Cleanup(func() { testhelper.MustClose(tb, conn) }) return conn diff --git a/internal/gitaly/server/server_factory_test.go b/internal/gitaly/server/server_factory_test.go index 65f3231fa3..eee8b0b657 100644 --- a/internal/gitaly/server/server_factory_test.go +++ b/internal/gitaly/server/server_factory_test.go @@ -64,7 +64,7 @@ func TestGitalyServerFactory(t *testing.T) { endpoint, err := starter.ComposeEndpoint(schema, listener.Addr().String()) require.NoError(t, err) - cc, err = client.Dial(ctx, endpoint) + cc, err = client.New(ctx, endpoint) require.NoError(t, err) } t.Cleanup(func() { assert.NoError(t, cc.Close()) }) diff --git a/internal/gitaly/service/operations/testhelper_test.go b/internal/gitaly/service/operations/testhelper_test.go index a7037dd9f1..8ace9b6178 100644 --- a/internal/gitaly/service/operations/testhelper_test.go +++ b/internal/gitaly/service/operations/testhelper_test.go @@ -103,7 +103,7 @@ func newOperationClient(tb testing.TB, serverSocketPath string) (gitalypb.Operat } func newMuxedOperationClient(t *testing.T, ctx context.Context, serverSocketPath, authToken string, handshaker client.Handshaker) gitalypb.OperationServiceClient { - conn, err := client.Dial(ctx, serverSocketPath, client.WithGrpcOptions([]grpc.DialOption{grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(authToken))}), client.WithHandshaker(handshaker)) + conn, err := client.New(ctx, serverSocketPath, client.WithGrpcOptions([]grpc.DialOption{grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(authToken))}), client.WithHandshaker(handshaker)) require.NoError(t, err) t.Cleanup(func() { conn.Close() }) return gitalypb.NewOperationServiceClient(conn) diff --git a/internal/gitaly/service/partition/testhelper_test.go b/internal/gitaly/service/partition/testhelper_test.go index e020b7e695..1698be1f96 100644 --- a/internal/gitaly/service/partition/testhelper_test.go +++ b/internal/gitaly/service/partition/testhelper_test.go @@ -26,7 +26,7 @@ func setupServices(tb testing.TB, opt ...testserver.GitalyServerOpt) (config.Cfg addr := testserver.RunGitalyServer(tb, cfg, setup.RegisterAll, opt...) cfg.SocketPath = addr - cc, err := client.Dial(testhelper.Context(tb), cfg.SocketPath) + cc, err := client.New(testhelper.Context(tb), cfg.SocketPath) require.NoError(tb, err) tb.Cleanup(func() { testhelper.MustClose(tb, cc) }) diff --git a/internal/gitaly/service/repository/testhelper_test.go b/internal/gitaly/service/repository/testhelper_test.go index 39b618ce59..329a05ee4e 100644 --- a/internal/gitaly/service/repository/testhelper_test.go +++ b/internal/gitaly/service/repository/testhelper_test.go @@ -35,7 +35,7 @@ func newRepositoryClient(tb testing.TB, cfg config.Cfg, serverSocketPath string) if cfg.Auth.Token != "" { connOpts = append(connOpts, grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(cfg.Auth.Token))) } - conn, err := client.Dial(testhelper.Context(tb), serverSocketPath, client.WithGrpcOptions(connOpts)) + conn, err := client.New(testhelper.Context(tb), serverSocketPath, client.WithGrpcOptions(connOpts)) require.NoError(tb, err) tb.Cleanup(func() { require.NoError(tb, conn.Close()) }) @@ -43,7 +43,7 @@ func newRepositoryClient(tb testing.TB, cfg config.Cfg, serverSocketPath string) } func newMuxedRepositoryClient(t *testing.T, ctx context.Context, cfg config.Cfg, serverSocketPath string, handshaker client.Handshaker) gitalypb.RepositoryServiceClient { - conn, err := client.Dial(ctx, serverSocketPath, + conn, err := client.New(ctx, serverSocketPath, client.WithGrpcOptions([]grpc.DialOption{ grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(cfg.Auth.Token)), }), diff --git a/internal/gitaly/service/smarthttp/testhelper_test.go b/internal/gitaly/service/smarthttp/testhelper_test.go index 251fdacb81..d1f34d8d90 100644 --- a/internal/gitaly/service/smarthttp/testhelper_test.go +++ b/internal/gitaly/service/smarthttp/testhelper_test.go @@ -63,7 +63,7 @@ func newSmartHTTPClient(t *testing.T, serverSocketPath, token string) gitalypb.S func newMuxedSmartHTTPClient(t *testing.T, ctx context.Context, serverSocketPath, token string, serverFactory backchannel.ServerFactory) gitalypb.SmartHTTPServiceClient { t.Helper() - conn, err := client.Dial( + conn, err := client.New( ctx, serverSocketPath, client.WithGrpcOptions([]grpc.DialOption{grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(token))}), diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go index 8fab391132..07949ca4ac 100644 --- a/internal/gitaly/service/smarthttp/upload_pack_test.go +++ b/internal/gitaly/service/smarthttp/upload_pack_test.go @@ -273,7 +273,7 @@ func (m mockStreamCache) Fetch(ctx context.Context, key string, dst io.Writer, c } func newRepositoryClient(tb testing.TB, cfg config.Cfg) gitalypb.RepositoryServiceClient { - conn, err := client.Dial(testhelper.Context(tb), cfg.SocketPath) + conn, err := client.New(testhelper.Context(tb), cfg.SocketPath) require.NoError(tb, err) tb.Cleanup(func() { require.NoError(tb, conn.Close()) }) diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go index db750f5d65..edf4970eb7 100644 --- a/internal/gitaly/service/ssh/upload_pack_test.go +++ b/internal/gitaly/service/ssh/upload_pack_test.go @@ -685,7 +685,7 @@ func (m mockStreamCache) Fetch(ctx context.Context, key string, dst io.Writer, c } func newRepositoryClient(tb testing.TB, cfg config.Cfg) gitalypb.RepositoryServiceClient { - conn, err := client.Dial(testhelper.Context(tb), cfg.SocketPath) + conn, err := client.New(testhelper.Context(tb), cfg.SocketPath) require.NoError(tb, err) tb.Cleanup(func() { require.NoError(tb, conn.Close()) }) diff --git a/internal/gitaly/storage/storagemgr/middleware_ext_test.go b/internal/gitaly/storage/storagemgr/middleware_ext_test.go index 86467d0d7f..5beca35d7a 100644 --- a/internal/gitaly/storage/storagemgr/middleware_ext_test.go +++ b/internal/gitaly/storage/storagemgr/middleware_ext_test.go @@ -42,7 +42,7 @@ func TestMiddleware_partitioning_hint(t *testing.T) { { desc: "implicit CreateFork hint", createFork: func(t *testing.T, ctx context.Context, cfg config.Cfg, alternate *gitalypb.Repository) *gitalypb.Repository { - cc, err := client.Dial(ctx, cfg.ListenAddr) + cc, err := client.New(ctx, cfg.ListenAddr) require.NoError(t, err) defer testhelper.MustClose(t, cc) @@ -65,7 +65,7 @@ func TestMiddleware_partitioning_hint(t *testing.T) { { desc: "explicit partitioning hint with implicit CreateFork hint", createFork: func(t *testing.T, ctx context.Context, cfg config.Cfg, alternate *gitalypb.Repository) *gitalypb.Repository { - cc, err := client.Dial(ctx, cfg.ListenAddr) + cc, err := client.New(ctx, cfg.ListenAddr) require.NoError(t, err) defer testhelper.MustClose(t, cc) @@ -96,7 +96,7 @@ func TestMiddleware_partitioning_hint(t *testing.T) { { desc: "explicit partitioning hint with an additional repository fails", createFork: func(t *testing.T, ctx context.Context, cfg config.Cfg, alternate *gitalypb.Repository) *gitalypb.Repository { - cc, err := client.Dial(ctx, cfg.ListenAddr) + cc, err := client.New(ctx, cfg.ListenAddr) require.NoError(t, err) defer testhelper.MustClose(t, cc) @@ -133,7 +133,7 @@ func TestMiddleware_partitioning_hint(t *testing.T) { RelativePath: gittest.NewObjectPoolName(t), }) - cc, err := client.Dial(ctx, cfg.ListenAddr) + cc, err := client.New(ctx, cfg.ListenAddr) require.NoError(t, err) defer testhelper.MustClose(t, cc) diff --git a/internal/gitaly/storage/storagemgr/middleware_test.go b/internal/gitaly/storage/storagemgr/middleware_test.go index 39a6843a02..0cd3ca080f 100644 --- a/internal/gitaly/storage/storagemgr/middleware_test.go +++ b/internal/gitaly/storage/storagemgr/middleware_test.go @@ -634,7 +634,7 @@ messages and behavior by erroring out the requests before they even hit this int testserver.WithLogger(log.FromLogrusEntry(entry)), ) - clientConn, err := client.Dial(ctx, serverAddress) + clientConn, err := client.New(ctx, serverAddress) require.NoError(t, err) defer clientConn.Close() @@ -815,7 +815,7 @@ messages and behavior by erroring out the requests before they even hit this int }) }) - clientConn, err := client.Dial(ctx, serverAddress) + clientConn, err := client.New(ctx, serverAddress) require.NoError(t, err) defer clientConn.Close() diff --git a/internal/gitaly/transaction/manager_test.go b/internal/gitaly/transaction/manager_test.go index c37257ee5e..d4c5b84523 100644 --- a/internal/gitaly/transaction/manager_test.go +++ b/internal/gitaly/transaction/manager_test.go @@ -48,7 +48,7 @@ func TestPoolManager_Vote(t *testing.T) { logger := testhelper.NewLogger(t) registry := backchannel.NewRegistry() - backchannelConn, err := client.Dial(ctx, transactionServerAddr) + backchannelConn, err := client.New(ctx, transactionServerAddr) require.NoError(t, err) defer backchannelConn.Close() @@ -198,7 +198,7 @@ func TestPoolManager_Stop(t *testing.T) { logger := testhelper.NewLogger(t) registry := backchannel.NewRegistry() - backchannelConn, err := client.Dial(ctx, transactionServerAddr) + backchannelConn, err := client.New(ctx, transactionServerAddr) require.NoError(t, err) defer backchannelConn.Close() diff --git a/internal/grpc/client/address_parser.go b/internal/grpc/client/address_parser.go index 1d0d560cb0..05d266eb14 100644 --- a/internal/grpc/client/address_parser.go +++ b/internal/grpc/client/address_parser.go @@ -7,7 +7,7 @@ import ( ) // extractHostFromRemoteURL will convert Gitaly-style URL addresses of the form -// scheme://host:port to the "host:port" addresses used by `grpc.Dial` +// scheme://host:port to the "host:port" addresses used by `grpc.NewClient` func extractHostFromRemoteURL(rawAddress string) (hostAndPort string, err error) { u, err := url.Parse(rawAddress) if err != nil { diff --git a/internal/grpc/client/dial.go b/internal/grpc/client/dial.go index eb1f35de6f..4219d43b5d 100644 --- a/internal/grpc/client/dial.go +++ b/internal/grpc/client/dial.go @@ -67,7 +67,7 @@ type dialConfig struct { creds credentials.TransportCredentials } -// DialOption is an option that can be passed to Dial. +// DialOption is an option that can be passed to NewClient. type DialOption func(*dialConfig) // WithHandshaker sets up the given handshaker so that it's passed as the transport credentials @@ -96,9 +96,9 @@ func WithTransportCredentials(creds credentials.TransportCredentials) DialOption } } -// Dial dials a Gitaly node serving at the given address. Dial is used by the public 'client' package -// and the expected behavior is mostly documented there. -func Dial(ctx context.Context, rawAddress string, opts ...DialOption) (*grpc.ClientConn, error) { +// New creates a dormant connection to a Gitaly node serving at the given address. New is used by the public 'client' +// package and the expected behavior is mostly documented there. +func New(ctx context.Context, rawAddress string, opts ...DialOption) (*grpc.ClientConn, error) { var dialCfg dialConfig for _, opt := range opts { opt(&dialCfg) diff --git a/internal/grpc/client/dial_ext_test.go b/internal/grpc/client/dial_ext_test.go index 2be6d3e01f..f4a73221cf 100644 --- a/internal/grpc/client/dial_ext_test.go +++ b/internal/grpc/client/dial_ext_test.go @@ -106,7 +106,7 @@ func TestRetryPolicy(t *testing.T) { SkipSnapshotInvalidation: true, }) - conn, err := client.Dial(ctx, cfg.SocketPath) + conn, err := client.New(ctx, cfg.SocketPath) require.NoError(t, err) defer testhelper.MustClose(t, conn) diff --git a/internal/grpc/client/dial_test.go b/internal/grpc/client/dial_test.go index 39636f213b..6f3ba367c9 100644 --- a/internal/grpc/client/dial_test.go +++ b/internal/grpc/client/dial_test.go @@ -47,7 +47,7 @@ func TestDial(t *testing.T) { ctx := testhelper.Context(t) t.Run("non-muxed conn", func(t *testing.T) { - nonMuxedConn, err := Dial(ctx, "tcp://"+ln.Addr().String()) + nonMuxedConn, err := New(ctx, "tcp://"+ln.Addr().String()) require.NoError(t, err) defer func() { require.NoError(t, nonMuxedConn.Close()) }() @@ -57,7 +57,7 @@ func TestDial(t *testing.T) { t.Run("muxed conn", func(t *testing.T) { handshaker := backchannel.NewClientHandshaker(logger, func() backchannel.Server { return grpc.NewServer() }, backchannel.DefaultConfiguration()) - nonMuxedConn, err := Dial(ctx, "tcp://"+ln.Addr().String(), WithHandshaker(handshaker)) + nonMuxedConn, err := New(ctx, "tcp://"+ln.Addr().String(), WithHandshaker(handshaker)) require.NoError(t, err) defer func() { require.NoError(t, nonMuxedConn.Close()) }() diff --git a/internal/grpc/client/pool.go b/internal/grpc/client/pool.go index a79112dc8c..26a350332c 100644 --- a/internal/grpc/client/pool.go +++ b/internal/grpc/client/pool.go @@ -28,7 +28,7 @@ func WithDialer(dialer Dialer) PoolOption { } } -// WithDialOptions sets gRPC options to use for the gRPC Dial call. +// WithDialOptions sets gRPC options to use for the gRPC NewClient call. func WithDialOptions(dialOptions ...grpc.DialOption) PoolOption { return func(options *poolConfig) { options.dialOptions = dialOptions @@ -50,7 +50,7 @@ type Pool struct { func NewPool(opts ...PoolOption) *Pool { cfg := poolConfig{ dialer: func(ctx context.Context, address string, opts []grpc.DialOption) (*grpc.ClientConn, error) { - return Dial(ctx, address, WithGrpcOptions(opts)) + return New(ctx, address, WithGrpcOptions(opts)) }, } for _, opt := range opts { diff --git a/internal/grpc/grpcstats/stats_test.go b/internal/grpc/grpcstats/stats_test.go index 04a393737d..19c5ef0307 100644 --- a/internal/grpc/grpcstats/stats_test.go +++ b/internal/grpc/grpcstats/stats_test.go @@ -101,7 +101,7 @@ func TestPayloadBytes(t *testing.T) { t.Cleanup(srv.GracefulStop) go func() { assert.NoError(t, srv.Serve(lis)) }() - cc, err := client.Dial(ctx, "unix://"+sock.Name()) + cc, err := client.New(ctx, "unix://"+sock.Name()) require.NoError(t, err) t.Cleanup(func() { require.NoError(t, cc.Close()) }) diff --git a/internal/grpc/proxy/handler_ext_test.go b/internal/grpc/proxy/handler_ext_test.go index 11cebbecaf..b4a834e32e 100644 --- a/internal/grpc/proxy/handler_ext_test.go +++ b/internal/grpc/proxy/handler_ext_test.go @@ -509,7 +509,7 @@ func TestRegisterStreamHandlers(t *testing.T) { go testhelper.MustServe(t, server, listener) defer server.Stop() - conn, err := client.Dial(ctx, "tcp://"+listener.Addr().String(), client.WithGrpcOptions([]grpc.DialOption{grpc.WithBlock()})) + conn, err := client.New(ctx, "tcp://"+listener.Addr().String(), client.WithGrpcOptions([]grpc.DialOption{grpc.WithBlock()})) require.NoError(t, err) defer conn.Close() client := grpc_testing.NewTestServiceClient(conn) diff --git a/internal/grpc/sidechannel/conn.go b/internal/grpc/sidechannel/conn.go index 7e8830bd8e..e03a3f5297 100644 --- a/internal/grpc/sidechannel/conn.go +++ b/internal/grpc/sidechannel/conn.go @@ -188,5 +188,5 @@ func (cc *ClientConn) CloseWrite() error { // also injects sr as a sidechannel registry, so that Gitaly can establish sidechannels back to the client. func Dial(ctx context.Context, registry *Registry, logger log.Logger, rawAddress string, connOpts []grpc.DialOption) (*grpc.ClientConn, error) { clientHandshaker := NewClientHandshaker(logger, registry) - return client.Dial(ctx, rawAddress, client.WithGrpcOptions(connOpts), client.WithHandshaker(clientHandshaker)) + return client.New(ctx, rawAddress, client.WithGrpcOptions(connOpts), client.WithHandshaker(clientHandshaker)) } diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go index 88ab9dbad5..39956d6713 100644 --- a/internal/praefect/coordinator_test.go +++ b/internal/praefect/coordinator_test.go @@ -2180,7 +2180,7 @@ func TestCoordinator_grpcErrorHandling(t *testing.T) { gitalypb.RegisterOperationServiceServer(srv, operationServer) }, testserver.WithDiskCache(&mockDiskCache{}), testserver.WithDisablePraefect()) - conn, err := client.Dial(ctx, addr, client.WithGrpcOptions([]grpc.DialOption{ + conn, err := client.New(ctx, addr, client.WithGrpcOptions([]grpc.DialOption{ grpc.WithDefaultCallOptions(grpc.ForceCodec(proxy.NewCodec())), })) require.NoError(t, err) diff --git a/internal/praefect/nodes/manager.go b/internal/praefect/nodes/manager.go index 9c7e908637..58c5f1365c 100644 --- a/internal/praefect/nodes/manager.go +++ b/internal/praefect/nodes/manager.go @@ -160,7 +160,7 @@ func Dial( client.StreamInterceptor(), } - return client.Dial(ctx, node.Address, client.WithGrpcOptions(dialOpts), client.WithHandshaker(handshaker)) + return client.New(ctx, node.Address, client.WithGrpcOptions(dialOpts), client.WithHandshaker(handshaker)) } // NewManager creates a new NodeMgr based on virtual storage configs diff --git a/internal/praefect/nodes/manager_test.go b/internal/praefect/nodes/manager_test.go index 0d29c752c8..eb42be71ef 100644 --- a/internal/praefect/nodes/manager_test.go +++ b/internal/praefect/nodes/manager_test.go @@ -472,7 +472,7 @@ func TestNodeStatus_IsHealthy(t *testing.T) { healthSrv := testhelper.NewServerWithHealth(t, socket) - clientConn, err := client.Dial(ctx, address) + clientConn, err := client.New(ctx, address) require.NoError(t, err) defer func() { require.NoError(t, clientConn.Close()) }() diff --git a/internal/praefect/nodes/ping.go b/internal/praefect/nodes/ping.go index e6e56180f0..fa0fd5e0d8 100644 --- a/internal/praefect/nodes/ping.go +++ b/internal/praefect/nodes/ping.go @@ -80,7 +80,7 @@ func (p *Ping) dial(ctx context.Context) (*grpc.ClientConn, error) { opts = append(opts, grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(p.token))) } - return client.Dial(ctx, p.address, client.WithGrpcOptions(opts)) + return client.New(ctx, p.address, client.WithGrpcOptions(opts)) } func (p *Ping) healthCheck(ctx context.Context, cc *grpc.ClientConn) (grpc_health_v1.HealthCheckResponse_ServingStatus, error) { diff --git a/internal/praefect/remove_repository_test.go b/internal/praefect/remove_repository_test.go index 89ca0a3e19..d89692f109 100644 --- a/internal/praefect/remove_repository_test.go +++ b/internal/praefect/remove_repository_test.go @@ -67,7 +67,7 @@ func TestRemoveRepositoryHandler(t *testing.T) { gitalyOneRepoPath := filepath.Join(gitalyOneCfg.Storages[0].Path, relativePath) gitalyOneAddr := testserver.RunGitalyServer(t, gitalyOneCfg, setup.RegisterAll, testserver.WithDisablePraefect()) - gitalyOneConn, err := client.Dial(ctx, gitalyOneAddr) + gitalyOneConn, err := client.New(ctx, gitalyOneAddr) require.NoError(t, err) defer testhelper.MustClose(t, gitalyOneConn) @@ -79,7 +79,7 @@ func TestRemoveRepositoryHandler(t *testing.T) { gitalyTwoRepoPath := filepath.Join(gitalyTwoCfg.Storages[0].Path, relativePath) gitalyTwoAddr := testserver.RunGitalyServer(t, gitalyTwoCfg, setup.RegisterAll, testserver.WithDisablePraefect()) - gitalyTwoConn, err := client.Dial(ctx, gitalyTwoAddr) + gitalyTwoConn, err := client.New(ctx, gitalyTwoAddr) require.NoError(t, err) defer testhelper.MustClose(t, gitalyTwoConn) diff --git a/internal/praefect/replicator_pg_test.go b/internal/praefect/replicator_pg_test.go index 02f59bf6ab..eb99aed806 100644 --- a/internal/praefect/replicator_pg_test.go +++ b/internal/praefect/replicator_pg_test.go @@ -36,7 +36,7 @@ func TestReplicatorInvalidSourceRepository(t *testing.T) { defer srv.Stop() go testhelper.MustServe(t, srv, ln) - targetCC, err := client.Dial(ctx, ln.Addr().Network()+":"+ln.Addr().String()) + targetCC, err := client.New(ctx, ln.Addr().Network()+":"+ln.Addr().String()) require.NoError(t, err) defer testhelper.MustClose(t, targetCC) diff --git a/internal/praefect/server_factory_test.go b/internal/praefect/server_factory_test.go index 81137cf4fd..992f82751b 100644 --- a/internal/praefect/server_factory_test.go +++ b/internal/praefect/server_factory_test.go @@ -208,7 +208,7 @@ func TestServerFactory(t *testing.T) { creds := insecure.NewCredentials() - cc, err := client.Dial(ctx, praefectAddr) + cc, err := client.New(ctx, praefectAddr) require.NoError(t, err) defer func() { require.NoError(t, cc.Close()) }() ctx := testhelper.Context(t) @@ -291,7 +291,7 @@ func TestServerFactory(t *testing.T) { address, err := starter.ComposeEndpoint(listener.Addr().Network(), listener.Addr().String()) require.NoError(t, err) - conn, err := client.Dial(ctx, address, cfg.dialOpts...) + conn, err := client.New(ctx, address, cfg.dialOpts...) require.NoError(t, err) defer func() { require.NoError(t, conn.Close()) }() diff --git a/internal/praefect/testserver.go b/internal/praefect/testserver.go index 15bc462246..29fcfcbee2 100644 --- a/internal/praefect/testserver.go +++ b/internal/praefect/testserver.go @@ -124,7 +124,7 @@ func dialLocalPort(tb testing.TB, ctx context.Context, port int, backend bool) * ) } - cc, err := client.Dial( + cc, err := client.New( ctx, fmt.Sprintf("tcp://localhost:%d", port), client.WithGrpcOptions(opts), diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go index 88054eb602..d3da9df5b6 100644 --- a/internal/testhelper/testserver/gitaly.go +++ b/internal/testhelper/testserver/gitaly.go @@ -153,7 +153,7 @@ func waitHealthy(tb testing.TB, ctx context.Context, addr string, authToken stri grpcOpts = append(grpcOpts, grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(authToken))) } - conn, err := client.Dial(ctx, addr, client.WithGrpcOptions(grpcOpts)) + conn, err := client.New(ctx, addr, client.WithGrpcOptions(grpcOpts)) require.NoError(tb, err) defer testhelper.MustClose(tb, conn) diff --git a/tools/test-boot/main.go b/tools/test-boot/main.go index a9739b00b7..8baa2c8a2b 100644 --- a/tools/test-boot/main.go +++ b/tools/test-boot/main.go @@ -112,7 +112,7 @@ func spawnAndWait(ctx context.Context, gitalyBin, configPath, socketPath string) for i := 0; i < 300; i++ { ctx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) - conn, err := client.Dial(ctx, "unix://"+socketPath, client.WithGrpcOptions([]grpc.DialOption{ + conn, err := client.New(ctx, "unix://"+socketPath, client.WithGrpcOptions([]grpc.DialOption{ grpc.WithBlock(), })) -- GitLab From 44859cb904beb37013e4bc1780967ae21db87623 Mon Sep 17 00:00:00 2001 From: James Liu Date: Wed, 12 Mar 2025 15:20:45 +1100 Subject: [PATCH 4/7] grpc: Remove grpc.WithBlock This option is not supported by `grpc.NewClient`, so remove it from our tests. --- internal/cli/gitaly/subcmd_hooks.go | 1 - internal/cli/praefect/subcmd.go | 1 - internal/cli/praefect/subcmd_test.go | 2 +- internal/grpc/middleware/cache/cache_test.go | 1 - internal/grpc/proxy/handler_ext_test.go | 2 +- internal/grpc/proxy/proxy_test_testhelper_test.go | 2 -- internal/praefect/nodes/ping.go | 1 - internal/praefect/testserver.go | 1 - internal/testhelper/testserver/gitaly.go | 1 - tools/test-boot/main.go | 5 +---- 10 files changed, 3 insertions(+), 14 deletions(-) diff --git a/internal/cli/gitaly/subcmd_hooks.go b/internal/cli/gitaly/subcmd_hooks.go index 9e7f32e20b..16e8e02f79 100644 --- a/internal/cli/gitaly/subcmd_hooks.go +++ b/internal/cli/gitaly/subcmd_hooks.go @@ -143,7 +143,6 @@ func dial(ctx context.Context, addr, token string, timeout time.Duration, opts . defer cancel() opts = append(opts, - grpc.WithBlock(), client.UnaryInterceptor(), client.StreamInterceptor(), ) diff --git a/internal/cli/praefect/subcmd.go b/internal/cli/praefect/subcmd.go index da93cc1aa9..1568151706 100644 --- a/internal/cli/praefect/subcmd.go +++ b/internal/cli/praefect/subcmd.go @@ -60,7 +60,6 @@ func subCmdDial(ctx context.Context, addr, token string, timeout time.Duration, defer cancel() opts = append(opts, - grpc.WithBlock(), client.UnaryInterceptor(), client.StreamInterceptor(), ) diff --git a/internal/cli/praefect/subcmd_test.go b/internal/cli/praefect/subcmd_test.go index d99068a2ab..53612e39f1 100644 --- a/internal/cli/praefect/subcmd_test.go +++ b/internal/cli/praefect/subcmd_test.go @@ -57,7 +57,7 @@ func listenAndServe(tb testing.TB, svcs []svcRegistrar) (net.Listener, testhelpe // verify the service is up addr := fmt.Sprintf("%s://%s", ln.Addr().Network(), ln.Addr()) - cc, err := grpc.NewClient(addr, grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials())) + cc, err := grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(tb, err) require.NoError(tb, cc.Close()) diff --git a/internal/grpc/middleware/cache/cache_test.go b/internal/grpc/middleware/cache/cache_test.go index 73c4a67591..9251fbfe47 100644 --- a/internal/grpc/middleware/cache/cache_test.go +++ b/internal/grpc/middleware/cache/cache_test.go @@ -132,7 +132,6 @@ func TestInvalidators(t *testing.T) { conn, err := grpc.NewClient( listener.Addr().String(), - grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials()), ) require.NoError(t, err) diff --git a/internal/grpc/proxy/handler_ext_test.go b/internal/grpc/proxy/handler_ext_test.go index b4a834e32e..d66b1d3335 100644 --- a/internal/grpc/proxy/handler_ext_test.go +++ b/internal/grpc/proxy/handler_ext_test.go @@ -509,7 +509,7 @@ func TestRegisterStreamHandlers(t *testing.T) { go testhelper.MustServe(t, server, listener) defer server.Stop() - conn, err := client.New(ctx, "tcp://"+listener.Addr().String(), client.WithGrpcOptions([]grpc.DialOption{grpc.WithBlock()})) + conn, err := client.New(ctx, "tcp://"+listener.Addr().String()) require.NoError(t, err) defer conn.Close() client := grpc_testing.NewTestServiceClient(conn) diff --git a/internal/grpc/proxy/proxy_test_testhelper_test.go b/internal/grpc/proxy/proxy_test_testhelper_test.go index 6bbf09b656..957b990b4e 100644 --- a/internal/grpc/proxy/proxy_test_testhelper_test.go +++ b/internal/grpc/proxy/proxy_test_testhelper_test.go @@ -37,7 +37,6 @@ func newBackendPinger(tb testing.TB, ctx context.Context) (*grpc.ClientConn, *in cc, err := grpc.NewClient( listener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()), - grpc.WithBlock(), grpc.WithDefaultCallOptions( grpc.ForceCodec(proxy.NewCodec()), ), @@ -74,7 +73,6 @@ func newProxy(tb testing.TB, ctx context.Context, director proxy.StreamDirector, proxyCC, err := grpc.NewClient( listener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()), - grpc.WithBlock(), ) require.NoError(tb, err) diff --git a/internal/praefect/nodes/ping.go b/internal/praefect/nodes/ping.go index fa0fd5e0d8..5a04ca58ab 100644 --- a/internal/praefect/nodes/ping.go +++ b/internal/praefect/nodes/ping.go @@ -71,7 +71,6 @@ func (p *Ping) Address() string { func (p *Ping) dial(ctx context.Context) (*grpc.ClientConn, error) { opts := []grpc.DialOption{ - grpc.WithBlock(), client.UnaryInterceptor(), client.StreamInterceptor(), } diff --git a/internal/praefect/testserver.go b/internal/praefect/testserver.go index 29fcfcbee2..a7b33afba9 100644 --- a/internal/praefect/testserver.go +++ b/internal/praefect/testserver.go @@ -113,7 +113,6 @@ func listenAvailPort(tb testing.TB) (net.Listener, int) { func dialLocalPort(tb testing.TB, ctx context.Context, port int, backend bool) *grpc.ClientConn { opts := []grpc.DialOption{ - grpc.WithBlock(), grpc.WithUnaryInterceptor(correlation.UnaryClientCorrelationInterceptor()), grpc.WithStreamInterceptor(correlation.StreamClientCorrelationInterceptor()), } diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go index d3da9df5b6..3e1dc546de 100644 --- a/internal/testhelper/testserver/gitaly.go +++ b/internal/testhelper/testserver/gitaly.go @@ -145,7 +145,6 @@ func (gs GitalyServer) Address() string { // waitHealthy waits until the server hosted at address becomes healthy. func waitHealthy(tb testing.TB, ctx context.Context, addr string, authToken string) { grpcOpts := []grpc.DialOption{ - grpc.WithBlock(), client.UnaryInterceptor(), client.StreamInterceptor(), } diff --git a/tools/test-boot/main.go b/tools/test-boot/main.go index 8baa2c8a2b..9d4dffeb9d 100644 --- a/tools/test-boot/main.go +++ b/tools/test-boot/main.go @@ -15,7 +15,6 @@ import ( "github.com/urfave/cli/v2" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/client" - "google.golang.org/grpc" ) type gitalyConfig struct { @@ -112,9 +111,7 @@ func spawnAndWait(ctx context.Context, gitalyBin, configPath, socketPath string) for i := 0; i < 300; i++ { ctx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) - conn, err := client.New(ctx, "unix://"+socketPath, client.WithGrpcOptions([]grpc.DialOption{ - grpc.WithBlock(), - })) + conn, err := client.New(ctx, "unix://"+socketPath) cancel() -- GitLab From 103748926161016e805709d168ffe512ae69488d Mon Sep 17 00:00:00 2001 From: James Liu Date: Thu, 13 Mar 2025 11:39:50 +1100 Subject: [PATCH 5/7] test: Limit package concurrency to 4 --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index e56a4cc21a..9dff578434 100644 --- a/Makefile +++ b/Makefile @@ -224,7 +224,7 @@ GIT_FILTER_REPO_SOURCE_DIR ?= ${DEPENDENCY_DIR}/git-filter-repo ## Go packages to test when using the test-go target. TEST_PACKAGES ?= ${SOURCE_DIR}/... ## Test options passed to `go test`. -TEST_OPTIONS ?= -count=1 +TEST_OPTIONS ?= -count=1 -p=4 ## Specify the output format used to print tests ["standard-verbose", "standard-quiet", "short"] TEST_FORMAT ?= short ## Specify the location where the JUnit-style format shall be written to. -- GitLab From fb75a2f118a9f6b95aa496d3bbb1a38a71fff72c Mon Sep 17 00:00:00 2001 From: James Liu Date: Thu, 13 Mar 2025 11:59:18 +1100 Subject: [PATCH 6/7] test: Enable the --rerun-fails flag --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 9dff578434..fd62790f50 100644 --- a/Makefile +++ b/Makefile @@ -267,7 +267,7 @@ find_go_sources = $(shell find ${SOURCE_DIR} -type d \( -path "${SO run_go_tests = PATH='${SOURCE_DIR}/internal/testhelper/testdata/home/bin:${PATH}' \ TEST_TMP_DIR='${TEST_TMP_DIR}' \ TEST_LOG_DIR='${TEST_LOG_DIR}' \ - ${GOTESTSUM} --format ${TEST_FORMAT} --junitfile '${TEST_JUNIT_REPORT}' --jsonfile '${TEST_JSON_REPORT}' -- -ldflags '${GO_LDFLAGS}' -tags '${SERVER_BUILD_TAGS}' ${TEST_OPTIONS} ${TEST_PACKAGES} + ${GOTESTSUM} --rerun-fails --packages $(TEST_PACKAGES) --format ${TEST_FORMAT} --junitfile '${TEST_JUNIT_REPORT}' --jsonfile '${TEST_JSON_REPORT}' -- -ldflags '${GO_LDFLAGS}' -tags '${SERVER_BUILD_TAGS}' ${TEST_OPTIONS} ${TEST_PACKAGES} ## Test options passed to `dlv test`. DEBUG_OPTIONS ?= $(patsubst -%,-test.%,${TEST_OPTIONS}) -- GitLab From fea94d68eaf12eef0ac251e08940f50195e35610 Mon Sep 17 00:00:00 2001 From: Quang-Minh Nguyen Date: Thu, 13 Mar 2025 08:40:06 +0700 Subject: [PATCH 7/7] test: Quarantine Praefect flaky tests on MacOS --- internal/praefect/coordinator_test.go | 2 ++ internal/praefect/server_test.go | 1 + 2 files changed, 3 insertions(+) diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go index 39956d6713..bc85d9aa9e 100644 --- a/internal/praefect/coordinator_test.go +++ b/internal/praefect/coordinator_test.go @@ -1828,6 +1828,8 @@ func TestAbsentCorrelationID(t *testing.T) { } func TestCoordinatorEnqueueFailure(t *testing.T) { + testhelper.SkipWithMacOS(t, "this test is extremely flaky on macos platform") + t.Parallel() conf := config.Config{ VirtualStorages: []*config.VirtualStorage{ diff --git a/internal/praefect/server_test.go b/internal/praefect/server_test.go index 70916f0dc5..6de18fd257 100644 --- a/internal/praefect/server_test.go +++ b/internal/praefect/server_test.go @@ -936,6 +936,7 @@ func TestProxyWrites(t *testing.T) { } func TestErrorThreshold(t *testing.T) { + testhelper.SkipWithMacOS(t, "this test is extremely flaky on macos platform") t.Parallel() backendToken := "" backend, cleanup := newMockDownstream(t, backendToken, func(srv *grpc.Server) { -- GitLab