Log an error if praefect's server info fails to connect to a node

If an error occurs when praefect is trying to reach a gitaly node for
its server info, we don't to fail the entire rpc. We log an error and
move on, and the caller of the RPC can decide what to do when it sees
missing server info

3 files changed, 54 insertions, 5 deletions

diff --git a/changelogs/unreleased/jc-fix-server-info.yml b/changelogs/unreleased/jc-fix-server-info.yml
new file mode 100644
index 000000000..4fff455f0
--- /dev/null
+++ b/changelogs/unreleased/jc-fix-server-info.yml
@@ -0,0 +1,5 @@
+---
+title: Log an error if praefect's server info fails to connect to a node
+merge_request: 1651
+author:
+type: other
diff --git a/internal/praefect/server_test.go b/internal/praefect/server_test.go
index b0ecdb7de..e201bcbcb 100644
--- a/internal/praefect/server_test.go
+++ b/internal/praefect/server_test.go
@@ -16,9 +16,12 @@ import (
 	gconfig "gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/log"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
+	"gitlab.com/gitlab-org/gitaly/internal/praefect/conn"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/mock"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/models"
+	"gitlab.com/gitlab-org/gitaly/internal/praefect/protoregistry"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/internal/version"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -83,10 +86,12 @@ func TestGitalyServerInfo(t *testing.T) {
 					&models.Node{
 						Storage: "praefect-internal-2",
 						Token:   "xyz",
-					}},
+					},
+				},
 			},
 		},
 	}
+
 	cc, _, cleanup := runPraefectServerWithGitaly(t, conf)
 	defer cleanup()
 
@@ -109,6 +114,43 @@ func TestGitalyServerInfo(t *testing.T) {
 	}
 }
 
+func TestGitalyServerInfoBadNode(t *testing.T) {
+	conf := config.Config{
+		VirtualStorages: []*config.VirtualStorage{
+			&config.VirtualStorage{
+				Nodes: []*models.Node{
+					&models.Node{
+						Storage:        "praefect-internal-1",
+						Address:        "tcp://unreachable:1234",
+						DefaultPrimary: true,
+						Token:          "abc",
+					},
+				},
+			},
+		},
+	}
+
+	clientCC := conn.NewClientConnections()
+	clientCC.RegisterNode(conf.VirtualStorages[0].Nodes[0].Storage, conf.VirtualStorages[0].Nodes[0].Address, conf.VirtualStorages[0].Nodes[0].Token)
+	_, srv := setupServer(t, conf, clientCC, log.Default(), protoregistry.GitalyProtoFileDescriptors)
+
+	listener, port := listenAvailPort(t)
+	go func() {
+		srv.RegisterServices()
+		srv.Serve(listener, false)
+	}()
+
+	cc := dialLocalPort(t, port, false)
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	client := gitalypb.NewServerServiceClient(cc)
+
+	metadata, err := client.ServerInfo(ctx, &gitalypb.ServerInfoRequest{})
+	require.NoError(t, err)
+	require.Len(t, metadata.GetStorageStatuses(), 0)
+}
+
 func TestGitalyDiskStatistics(t *testing.T) {
 	conf := config.Config{
 		VirtualStorages: []*config.VirtualStorage{
diff --git a/internal/praefect/service/server/info.go b/internal/praefect/service/server/info.go
index e83394b04..bcda4e0de 100644
--- a/internal/praefect/service/server/info.go
+++ b/internal/praefect/service/server/info.go
@@ -2,14 +2,14 @@ package server
 
 import (
 	"context"
-	"fmt"
 	"sync"
 
-	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
+	grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	"golang.org/x/sync/errgroup"
 
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/models"
+	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
 // ServerInfo sends ServerInfoRequest to all of a praefect server's internal gitaly nodes and aggregates the results into
@@ -41,13 +41,15 @@ func (s *Server) ServerInfo(ctx context.Context, in *gitalypb.ServerInfoRequest)
 		node := node
 		cc, err := s.clientCC.GetConnection(node.Storage)
 		if err != nil {
-			return nil, helper.ErrInternalf("error getting client connection for %s: %v", node.Storage, err)
+			grpc_logrus.Extract(ctx).WithField("storage", node.Storage).WithError(err).Error("error getting client connection")
+			continue
 		}
 		g.Go(func() error {
 			client := gitalypb.NewServerServiceClient(cc)
 			resp, err := client.ServerInfo(ctx, &gitalypb.ServerInfoRequest{})
 			if err != nil {
-				return fmt.Errorf("error when requesting server info from internal storage %v", node.Storage)
+				grpc_logrus.Extract(ctx).WithField("storage", node.Storage).WithError(err).Error("error getting sever info")
+				return nil
 			}
 
 			storageStatuses[i] = resp.GetStorageStatuses()