Merge branch 'jc-list-repo-to-use-shorter-grace-period' into 'master'

repocleaner: Allow NewWalker to receive grace period parameter

Closes #4164

See merge request gitlab-org/gitaly!4474

3 files changed, 42 insertions, 32 deletions

diff --git a/cmd/praefect/subcmd_list_untracked_repositories.go b/cmd/praefect/subcmd_list_untracked_repositories.go
index 39d6cd41c..dd6563f24 100644
--- a/cmd/praefect/subcmd_list_untracked_repositories.go
+++ b/cmd/praefect/subcmd_list_untracked_repositories.go
@@ -26,9 +26,10 @@ const (
 var errNoConnectionToGitalies = errors.New("no connection established to gitaly nodes")
 
 type listUntrackedRepositories struct {
-	logger    logrus.FieldLogger
-	delimiter string
-	out       io.Writer
+	logger               logrus.FieldLogger
+	onlyIncludeOlderThan time.Duration
+	delimiter            string
+	out                  io.Writer
 }
 
 func newListUntrackedRepositories(logger logrus.FieldLogger, out io.Writer) *listUntrackedRepositories {
@@ -38,6 +39,12 @@ func newListUntrackedRepositories(logger logrus.FieldLogger, out io.Writer) *lis
 func (cmd *listUntrackedRepositories) FlagSet() *flag.FlagSet {
 	fs := flag.NewFlagSet(listUntrackedRepositoriesName, flag.ExitOnError)
 	fs.StringVar(&cmd.delimiter, "delimiter", "\n", "string used as a delimiter in output")
+	fs.DurationVar(
+		&cmd.onlyIncludeOlderThan,
+		"older-than",
+		6*time.Hour,
+		"only include repositories created before this duration",
+	)
 	fs.Usage = func() {
 		printfErr("Description:\n" +
 			"	This command checks if all repositories on all gitaly nodes tracked by praefect.\n" +
@@ -81,7 +88,7 @@ func (cmd listUntrackedRepositories) Exec(flags *flag.FlagSet, cfg config.Config
 	defer func() { _ = db.Close() }()
 	logger.Debug("connected to praefect database")
 
-	walker := repocleaner.NewWalker(nodeSet.Connections(), 16)
+	walker := repocleaner.NewWalker(nodeSet.Connections(), 16, cmd.onlyIncludeOlderThan)
 	reporter := reportUntrackedRepositories{
 		ctx:         ctx,
 		checker:     datastore.NewStorageCleanup(db),
diff --git a/cmd/praefect/subcmd_list_untracked_repositories_test.go b/cmd/praefect/subcmd_list_untracked_repositories_test.go
index a2c251a81..aa6fb8848 100644
--- a/cmd/praefect/subcmd_list_untracked_repositories_test.go
+++ b/cmd/praefect/subcmd_list_untracked_repositories_test.go
@@ -3,7 +3,6 @@ package main
 import (
 	"bytes"
 	"context"
-	"flag"
 	"fmt"
 	"os"
 	"strings"
@@ -31,20 +30,20 @@ func TestListUntrackedRepositories_FlagSet(t *testing.T) {
 		exp  []interface{}
 	}{
 		{
-			desc: "custom value",
-			args: []string{"--delimiter", ","},
-			exp:  []interface{}{","},
+			desc: "custom values",
+			args: []string{"--delimiter", ",", "--older-than", "1s"},
+			exp:  []interface{}{",", time.Second},
 		},
 		{
-			desc: "default value",
+			desc: "default values",
 			args: nil,
-			exp:  []interface{}{"\n"},
+			exp:  []interface{}{"\n", 6 * time.Hour},
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			fs := cmd.FlagSet()
 			require.NoError(t, fs.Parse(tc.args))
-			require.ElementsMatch(t, tc.exp, []interface{}{cmd.delimiter})
+			require.ElementsMatch(t, tc.exp, []interface{}{cmd.delimiter, cmd.onlyIncludeOlderThan})
 		})
 	}
 }
@@ -54,20 +53,6 @@ func TestListUntrackedRepositories_Exec(t *testing.T) {
 	g1Cfg := testcfg.Build(t, testcfg.WithStorages("gitaly-1"))
 	g2Cfg := testcfg.Build(t, testcfg.WithStorages("gitaly-2"))
 
-	// Repositories not managed by praefect.
-	repo1, repo1Path := gittest.InitRepo(t, g1Cfg, g1Cfg.Storages[0])
-	repo2, repo2Path := gittest.InitRepo(t, g1Cfg, g1Cfg.Storages[0])
-	_, _ = gittest.InitRepo(t, g2Cfg, g2Cfg.Storages[0])
-
-	require.NoError(t, os.Chtimes(
-		repo1Path,
-		time.Now().Add(-(24*time.Hour+1*time.Second)),
-		time.Now().Add(-(24*time.Hour+1*time.Second))))
-	require.NoError(t, os.Chtimes(
-		repo2Path,
-		time.Now().Add(-(24*time.Hour+1*time.Second)),
-		time.Now().Add(-(24*time.Hour+1*time.Second))))
-
 	g1Addr := testserver.RunGitalyServer(t, g1Cfg, nil, setup.RegisterAll, testserver.WithDisablePraefect())
 	g2Addr := testserver.RunGitalyServer(t, g2Cfg, nil, setup.RegisterAll, testserver.WithDisablePraefect())
 
@@ -104,7 +89,24 @@ func TestListUntrackedRepositories_Exec(t *testing.T) {
 	createRepo(t, ctx, repoClient, praefectStorage, "path/to/test/repo")
 	out := &bytes.Buffer{}
 	cmd := newListUntrackedRepositories(testhelper.NewDiscardingLogger(t), out)
-	require.NoError(t, cmd.Exec(flag.NewFlagSet("", flag.PanicOnError), conf))
+	fs := cmd.FlagSet()
+	require.NoError(t, fs.Parse([]string{}))
+
+	// Repositories not managed by praefect.
+	repo1, repo1Path := gittest.InitRepo(t, g1Cfg, g1Cfg.Storages[0])
+	repo2, repo2Path := gittest.InitRepo(t, g1Cfg, g1Cfg.Storages[0])
+	_, _ = gittest.InitRepo(t, g2Cfg, g2Cfg.Storages[0])
+
+	require.NoError(t, os.Chtimes(
+		repo1Path,
+		time.Now().Add(-(6*time.Hour+1*time.Second)),
+		time.Now().Add(-(6*time.Hour+1*time.Second))))
+	require.NoError(t, os.Chtimes(
+		repo2Path,
+		time.Now().Add(-(6*time.Hour+1*time.Second)),
+		time.Now().Add(-(6*time.Hour+1*time.Second))))
+
+	require.NoError(t, cmd.Exec(fs, conf))
 
 	exp := []string{
 		"The following repositories were found on disk, but missing from the tracking database:",
diff --git a/internal/praefect/repocleaner/repository.go b/internal/praefect/repocleaner/repository.go
index aeff3ac7d..afc8fd317 100644
--- a/internal/praefect/repocleaner/repository.go
+++ b/internal/praefect/repocleaner/repository.go
@@ -65,7 +65,7 @@ func NewRunner(cfg Cfg, logger logrus.FieldLogger, healthChecker praefect.Health
 		logger:        logger.WithField("component", "repocleaner.repository_existence"),
 		healthChecker: healthChecker,
 		conns:         conns,
-		walker:        NewWalker(conns, cfg.RepositoriesInBatch),
+		walker:        NewWalker(conns, cfg.RepositoriesInBatch, 24*time.Hour),
 		stateOwner:    stateOwner,
 		acquirer:      acquirer,
 		action:        action,
@@ -155,13 +155,14 @@ func (gs *Runner) loggerWith(virtualStorage, storage string) logrus.FieldLogger
 
 // Walker allows walk by the repositories of the gitaly storage.
 type Walker struct {
-	conns     praefect.Connections
-	batchSize int
+	conns       praefect.Connections
+	gracePeriod time.Duration
+	batchSize   int
 }
 
 // NewWalker returns a new *Walker instance.
-func NewWalker(conns praefect.Connections, batchSize int) *Walker {
-	return &Walker{conns: conns, batchSize: batchSize}
+func NewWalker(conns praefect.Connections, batchSize int, gracePeriod time.Duration) *Walker {
+	return &Walker{conns: conns, batchSize: batchSize, gracePeriod: gracePeriod}
 }
 
 // ExecOnRepositories runs through all the repositories on a Gitaly storage and executes the provided action.
@@ -189,7 +190,7 @@ func (wr *Walker) ExecOnRepositories(ctx context.Context, virtualStorage, storag
 
 		// repositories that are in the process of being created, where
 		// they do not yet have a record in Praefect.
-		if res.GetModificationTime().AsTime().After(time.Now().Add(-24 * time.Hour)) {
+		if res.GetModificationTime().AsTime().After(time.Now().Add(-wr.gracePeriod)) {
 			continue
 		}