Merge branch 'ps-praefect-cli-dial-nodes' into 'master'

praefect: migrate dial-nodes sub-command

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5676

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Pavlo Strokov <pstrokov@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Pavlo Strokov <pstrokov@gitlab.com>

8 files changed, 79 insertions, 74 deletions

diff --git a/internal/cli/praefect/main.go b/internal/cli/praefect/main.go
index 6d453ab4c..a665d9445 100644
--- a/internal/cli/praefect/main.go
+++ b/internal/cli/praefect/main.go
@@ -26,13 +26,6 @@
 //
 //	praefect -config PATH_TO_CONFIG sql-migrate-status
 //
-// # Dial Nodes
-//
-// The subcommand "dial-nodes" helps diagnose connection problems to Gitaly or
-// Praefect. The subcommand works by sourcing the connection information from
-// the config file, and then dialing and health checking the remote nodes.
-//
-//	praefect -config PATH_TO_CONFIG dial-nodes
 
 package praefect
 
@@ -81,6 +74,7 @@ func NewApp() *cli.App {
 			newAcceptDatalossCommand(),
 			newCheckCommand(service.AllChecks()),
 			newDatalossCommand(),
+			newDialNodesCommand(),
 		},
 		Flags: []cli.Flag{
 			&cli.StringFlag{
diff --git a/internal/cli/praefect/main_test.go b/internal/cli/praefect/main_test.go
index cff3d62cd..dd8878236 100644
--- a/internal/cli/praefect/main_test.go
+++ b/internal/cli/praefect/main_test.go
@@ -3,8 +3,11 @@ package praefect
 import (
 	"errors"
 	"fmt"
+	"os"
+	"path/filepath"
 	"testing"
 
+	"github.com/pelletier/go-toml/v2"
 	"github.com/prometheus/client_golang/prometheus"
 	dto "github.com/prometheus/client_model/go"
 	"github.com/stretchr/testify/assert"
@@ -12,6 +15,7 @@ import (
 	"github.com/urfave/cli/v2"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/bootstrap"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/bootstrap/starter"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
@@ -235,3 +239,13 @@ func TestExcludeDatabaseMetricsFromDefaultMetrics(t *testing.T) {
 		})
 	}
 }
+
+func writeConfigToFile(tb testing.TB, conf config.Config) string {
+	tb.Helper()
+	confData, err := toml.Marshal(conf)
+	require.NoError(tb, err)
+	tmpDir := testhelper.TempDir(tb)
+	confPath := filepath.Join(tmpDir, "config.toml")
+	require.NoError(tb, os.WriteFile(confPath, confData, perm.PublicFile))
+	return confPath
+}
diff --git a/internal/cli/praefect/subcmd.go b/internal/cli/praefect/subcmd.go
index a594ae369..fa8bbed28 100644
--- a/internal/cli/praefect/subcmd.go
+++ b/internal/cli/praefect/subcmd.go
@@ -35,7 +35,6 @@ func subcommands(logger *logrus.Entry) map[string]subcmd {
 	return map[string]subcmd{
 		sqlPingCmdName:                &sqlPingSubcommand{},
 		sqlMigrateCmdName:             newSQLMigrateSubCommand(os.Stdout),
-		dialNodesCmdName:              newDialNodesSubcommand(os.Stdout),
 		sqlMigrateDownCmdName:         &sqlMigrateDownSubcommand{},
 		sqlMigrateStatusCmdName:       &sqlMigrateStatusSubcommand{},
 		setReplicationFactorCmdName:   newSetReplicatioFactorSubcommand(os.Stdout),
diff --git a/internal/cli/praefect/subcmd_accept_dataloss_test.go b/internal/cli/praefect/subcmd_accept_dataloss_test.go
index b2244f480..5348b8f5b 100644
--- a/internal/cli/praefect/subcmd_accept_dataloss_test.go
+++ b/internal/cli/praefect/subcmd_accept_dataloss_test.go
@@ -2,14 +2,10 @@ package praefect
 
 import (
 	"context"
-	"os"
-	"path/filepath"
 	"testing"
 
-	"github.com/pelletier/go-toml/v2"
 	"github.com/stretchr/testify/require"
 	"github.com/urfave/cli/v2"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/service/info"
@@ -66,11 +62,7 @@ func TestAcceptDatalossSubcommand(t *testing.T) {
 	defer clean()
 
 	conf.SocketPath = ln.Addr().String()
-	tmpDir := testhelper.TempDir(t)
-	confData, err := toml.Marshal(conf)
-	require.NoError(t, err)
-	confPath := filepath.Join(tmpDir, "config.toml")
-	require.NoError(t, os.WriteFile(confPath, confData, perm.PublicFile))
+	confPath := writeConfigToFile(t, conf)
 
 	type errorMatcher func(t *testing.T, err error)
 
diff --git a/internal/cli/praefect/subcmd_check_test.go b/internal/cli/praefect/subcmd_check_test.go
index aaedc3846..a37bb5b6f 100644
--- a/internal/cli/praefect/subcmd_check_test.go
+++ b/internal/cli/praefect/subcmd_check_test.go
@@ -5,15 +5,10 @@ import (
 	"context"
 	"errors"
 	"io"
-	"os"
-	"path/filepath"
 	"testing"
 
-	"github.com/pelletier/go-toml/v2"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 	"github.com/urfave/cli/v2"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/service"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
@@ -33,11 +28,7 @@ func TestCheckSubcommand(t *testing.T) {
 		},
 	}
 
-	tmpDir := testhelper.TempDir(t)
-	confData, err := toml.Marshal(conf)
-	require.NoError(t, err)
-	confPath := filepath.Join(tmpDir, "config.toml")
-	require.NoError(t, os.WriteFile(confPath, confData, perm.PublicFile))
+	confPath := writeConfigToFile(t, conf)
 
 	testCases := []struct {
 		desc                string
diff --git a/internal/cli/praefect/subcmd_dataloss_test.go b/internal/cli/praefect/subcmd_dataloss_test.go
index de62e8ae6..cb6dd19d6 100644
--- a/internal/cli/praefect/subcmd_dataloss_test.go
+++ b/internal/cli/praefect/subcmd_dataloss_test.go
@@ -2,14 +2,10 @@ package praefect
 
 import (
 	"bytes"
-	"os"
-	"path/filepath"
 	"testing"
 
-	"github.com/pelletier/go-toml/v2"
 	"github.com/stretchr/testify/require"
 	"github.com/urfave/cli/v2"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/service/info"
@@ -182,12 +178,7 @@ Virtual storage: virtual-storage-2
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			cfg.VirtualStorages = tc.virtualStorages
-
-			confData, err := toml.Marshal(cfg)
-			require.NoError(t, err)
-			tmpDir := testhelper.TempDir(t)
-			confPath := filepath.Join(tmpDir, "config.toml")
-			require.NoError(t, os.WriteFile(confPath, confData, perm.PublicFile))
+			confPath := writeConfigToFile(t, cfg)
 
 			var stdout bytes.Buffer
 			app := cli.App{
@@ -203,7 +194,7 @@ Virtual storage: virtual-storage-2
 				},
 			}
 
-			err = app.Run(append([]string{progname, "dataloss"}, tc.args...))
+			err := app.Run(append([]string{progname, "dataloss"}, tc.args...))
 			require.Equal(t, tc.error, err, err)
 			if tc.error == nil {
 				require.Equal(t, tc.output, stdout.String())
diff --git a/internal/cli/praefect/subcmd_dial_nodes.go b/internal/cli/praefect/subcmd_dial_nodes.go
index 3608a9be7..debf1f908 100644
--- a/internal/cli/praefect/subcmd_dial_nodes.go
+++ b/internal/cli/praefect/subcmd_dial_nodes.go
@@ -2,46 +2,51 @@ package praefect
 
 import (
 	"context"
-	"flag"
-	"io"
-	"time"
 
-	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
+	"github.com/urfave/cli/v2"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/nodes"
 )
 
-const dialNodesCmdName = "dial-nodes"
-
-func newDialNodesSubcommand(w io.Writer) *dialNodesSubcommand {
-	return &dialNodesSubcommand{
-		w: w,
+func newDialNodesCommand() *cli.Command {
+	return &cli.Command{
+		Name:  "dial-nodes",
+		Usage: "checks connection with remote nodes",
+		Description: "The subcommand \"dial-nodes\" helps diagnose connection problems to Gitaly or\n" +
+			"Praefect. The subcommand works by sourcing the connection information from\n" +
+			"the config file, and then dialing and health checking the remote nodes.",
+		Action: dialNodesAction,
+		Flags: []cli.Flag{
+			&cli.DurationFlag{
+				Name:  "timeout",
+				Usage: "timeout for dialing gitaly nodes",
+				Value: 0,
+			},
+		},
+		Before: func(ctx *cli.Context) error {
+			if ctx.Args().Present() {
+				_ = cli.ShowSubcommandHelp(ctx)
+				return cli.Exit(unexpectedPositionalArgsError{Command: ctx.Command.Name}, 1)
+			}
+			return nil
+		},
 	}
 }
 
-type dialNodesSubcommand struct {
-	w       io.Writer
-	timeout time.Duration
-}
-
-func (s *dialNodesSubcommand) FlagSet() *flag.FlagSet {
-	fs := flag.NewFlagSet(dialNodesCmdName, flag.ExitOnError)
-	fs.DurationVar(&s.timeout, "timeout", 0, "timeout for dialing gitaly nodes")
-	fs.Usage = func() {
-		printfErr("Description:\n" +
-			"	This command attempts to reach all Gitaly nodes.\n")
-		fs.PrintDefaults()
+func dialNodesAction(ctx *cli.Context) error {
+	logger := log.Default()
+	conf, err := getConfig(logger, ctx.String(configFlagName))
+	if err != nil {
+		return err
 	}
 
-	return fs
-}
-
-func (s *dialNodesSubcommand) Exec(flags *flag.FlagSet, conf config.Config) error {
-	if s.timeout == 0 {
-		s.timeout = defaultDialTimeout
+	timeout := ctx.Duration("timeout")
+	if timeout == 0 {
+		timeout = defaultDialTimeout
 	}
 
-	ctx, cancel := context.WithTimeout(context.Background(), s.timeout)
+	timeCtx, cancel := context.WithTimeout(ctx.Context, timeout)
 	defer cancel()
 
-	return nodes.PingAll(ctx, conf, nodes.NewTextPrinter(s.w), false)
+	return nodes.PingAll(timeCtx, conf, nodes.NewTextPrinter(ctx.App.Writer), false)
 }
diff --git a/internal/cli/praefect/subcmd_dial_nodes_test.go b/internal/cli/praefect/subcmd_dial_nodes_test.go
index f511c8872..ffb977f74 100644
--- a/internal/cli/praefect/subcmd_dial_nodes_test.go
+++ b/internal/cli/praefect/subcmd_dial_nodes_test.go
@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"github.com/urfave/cli/v2"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
@@ -23,6 +24,7 @@ func (m mockServerService) ServerInfo(ctx context.Context, r *gitalypb.ServerInf
 }
 
 func TestSubCmdDialNodes(t *testing.T) {
+	t.Parallel()
 	var resp *gitalypb.ServerInfoResponse
 	mockSvc := &mockServerService{
 		serverInfoFunc: func(_ context.Context, _ *gitalypb.ServerInfoRequest) (*gitalypb.ServerInfoResponse, error) {
@@ -46,14 +48,21 @@ func TestSubCmdDialNodes(t *testing.T) {
 
 	for _, tt := range []struct {
 		name   string
+		args   []string
 		conf   config.Config
 		resp   *gitalypb.ServerInfoResponse
 		logs   string
 		errMsg string
 	}{
 		{
+			name:   "positional arguments",
+			args:   []string{"positional-arg"},
+			errMsg: cli.Exit(unexpectedPositionalArgsError{Command: "dial-nodes"}, 1).Error(),
+		},
+		{
 			name: "2 virtuals, 2 storages, 1 node",
 			conf: config.Config{
+				SocketPath: ln.Addr().String(),
 				VirtualStorages: []*config.VirtualStorage{
 					{
 						Name: "default",
@@ -104,6 +113,7 @@ func TestSubCmdDialNodes(t *testing.T) {
 		{
 			name: "node unreachable",
 			conf: config.Config{
+				SocketPath: ln.Addr().String(),
 				VirtualStorages: []*config.VirtualStorage{
 					{
 						Name: "default",
@@ -123,17 +133,26 @@ func TestSubCmdDialNodes(t *testing.T) {
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			resp = tt.resp
-			tt.conf.SocketPath = ln.Addr().String()
-
-			output := &bytes.Buffer{}
-
-			cmd := dialNodesSubcommand{w: output, timeout: 1 * time.Second}
+			confPath := writeConfigToFile(t, tt.conf)
 
-			err := cmd.Exec(nil, tt.conf)
+			var stdout bytes.Buffer
+			app := cli.App{
+				Writer: &stdout,
+				Commands: []*cli.Command{
+					newDialNodesCommand(),
+				},
+				Flags: []cli.Flag{
+					&cli.StringFlag{
+						Name:  "config",
+						Value: confPath,
+					},
+				},
+			}
 
+			err := app.Run(append([]string{progname, "dial-nodes", "-timeout", time.Second.String()}, tt.args...))
 			if tt.errMsg == "" {
 				require.NoError(t, err)
-				require.Equal(t, tt.logs, output.String())
+				require.Equal(t, tt.logs, stdout.String())
 				return
 			}