From 1ed2f9ff420cbb93fd2cd5418cb6144901d603c6 Mon Sep 17 00:00:00 2001 From: Jia Shiang Gao Date: Tue, 5 Apr 2022 01:36:22 -0700 Subject: [PATCH 1/5] cmd/praefect: Display full replication status by default in 'dataloss' subcommand Because dataloss command is used to show the full state of repository replication is more common than only showing the repository with an unhealthy primary. So make the default command behavior show all repositories with full unavailable and partially unavailable replication status. This commit keep the dataloss command's partially-unavailable flag available for backward compatibility. And introduce the fully-unavailable flag for only showing the repositories with an unhealthy primary. --- cmd/praefect/subcmd_dataloss.go | 33 ++++++++++++++++++++++++---- cmd/praefect/subcmd_dataloss_test.go | 32 ++++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/cmd/praefect/subcmd_dataloss.go b/cmd/praefect/subcmd_dataloss.go index 5eb88f4e686..621b0f994ff 100644 --- a/cmd/praefect/subcmd_dataloss.go +++ b/cmd/praefect/subcmd_dataloss.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "flag" "fmt" "io" @@ -16,9 +17,18 @@ import ( const datalossCmdName = "dataloss" +var errDatalossFlagsConflict = errors.New("fully-unavailable flag and partially-unavailable flag " + + "cannot be used together") + type datalossSubcommand struct { - output io.Writer - virtualStorage string + output io.Writer + virtualStorage string + onlyIncludeFullyUnAvailable bool + /* + includePartiallyAvailable is just a variable for holding a deprecated partially-unavailable + flag's parsed value. + But we don't need this variable. + */ includePartiallyAvailable bool } @@ -28,8 +38,19 @@ func newDatalossSubcommand() *datalossSubcommand { func (cmd *datalossSubcommand) FlagSet() *flag.FlagSet { fs := flag.NewFlagSet(datalossCmdName, flag.ContinueOnError) + fs.Usage = func() { + printfErr("Description:\n" + + " This command checks whether all repositories in specified virtual storage are available or not.\n" + + " The default behavior is to show all partially unavailable repositories.\n" + + " Partially unavailable repositories are ones where some replicas are unavailable." + + " These repositories are available but not fully replicated.") + fs.PrintDefaults() + } fs.StringVar(&cmd.virtualStorage, "virtual-storage", "", "virtual storage to check for data loss") + fs.BoolVar(&cmd.onlyIncludeFullyUnAvailable, "fully-unavailable", false, strings.TrimSpace(` +Only show repositories whose primaries are unavailable.`)) fs.BoolVar(&cmd.includePartiallyAvailable, "partially-unavailable", false, strings.TrimSpace(` +(Deprecated, this is the default behavior of the command) Additionally include repositories which are available but some assigned replicas are unavailable. Such repositories are available but are not fully replicated. This increases the chance of data loss on primary failure`)) @@ -46,6 +67,10 @@ func (cmd *datalossSubcommand) Exec(flags *flag.FlagSet, cfg config.Config) erro if flags.NArg() > 0 { return unexpectedPositionalArgsError{Command: flags.Name()} } + if cmd.onlyIncludeFullyUnAvailable && cmd.includePartiallyAvailable { + return errDatalossFlagsConflict + } + includePartiallyAvailable := !cmd.onlyIncludeFullyUnAvailable virtualStorages := []string{cmd.virtualStorage} if cmd.virtualStorage == "" { @@ -77,7 +102,7 @@ func (cmd *datalossSubcommand) Exec(flags *flag.FlagSet, cfg config.Config) erro for _, vs := range virtualStorages { resp, err := client.DatalossCheck(ctx, &gitalypb.DatalossCheckRequest{ VirtualStorage: vs, - IncludePartiallyReplicated: cmd.includePartiallyAvailable, + IncludePartiallyReplicated: includePartiallyAvailable, }) if err != nil { return fmt.Errorf("error checking: %v", err) @@ -86,7 +111,7 @@ func (cmd *datalossSubcommand) Exec(flags *flag.FlagSet, cfg config.Config) erro cmd.println(0, "Virtual storage: %s", vs) if len(resp.Repositories) == 0 { msg := "All repositories are available!" - if cmd.includePartiallyAvailable { + if includePartiallyAvailable { msg = "All repositories are fully available on all assigned storages!" } diff --git a/cmd/praefect/subcmd_dataloss_test.go b/cmd/praefect/subcmd_dataloss_test.go index 949c7919b06..2e8f3747adc 100644 --- a/cmd/praefect/subcmd_dataloss_test.go +++ b/cmd/praefect/subcmd_dataloss_test.go @@ -93,9 +93,35 @@ func TestDatalossSubcommand(t *testing.T) { args: []string{"-virtual-storage=virtual-storage-1", "positional-arg"}, error: unexpectedPositionalArgsError{Command: "dataloss"}, }, + { + desc: "the fully-available flag is not permitted to appear with a partially-unavailable flag", + args: []string{"-virtual-storage=virtual-storage-1", "-fully-unavailable", "-partially-unavailable"}, + error: errDatalossFlagsConflict, + }, + { + desc: "check deprecated partially-unavailable flag could work for backward compatibility", + args: []string{"-virtual-storage=virtual-storage-1", "-partially-unavailable"}, + output: `Virtual storage: virtual-storage-1 + Repositories: + repository-1: + Primary: gitaly-1 + In-Sync Storages: + gitaly-1, assigned host + Outdated Storages: + gitaly-2 is behind by 1 change or less, assigned host, unhealthy + gitaly-3 is behind by 1 change or less + repository-2 (unavailable): + Primary: gitaly-3 + In-Sync Storages: + gitaly-2, unhealthy + Outdated Storages: + gitaly-1 is behind by 2 changes or less, assigned host + gitaly-3 is behind by 1 change or less, assigned host +`, + }, { desc: "data loss with unavailable repositories", - args: []string{"-virtual-storage=virtual-storage-1"}, + args: []string{"-virtual-storage=virtual-storage-1", "-fully-unavailable"}, output: `Virtual storage: virtual-storage-1 Repositories: repository-2 (unavailable): @@ -109,7 +135,7 @@ func TestDatalossSubcommand(t *testing.T) { }, { desc: "data loss with partially unavailable repositories", - args: []string{"-virtual-storage=virtual-storage-1", "-partially-unavailable"}, + args: []string{"-virtual-storage=virtual-storage-1"}, output: `Virtual storage: virtual-storage-1 Repositories: repository-1: @@ -131,6 +157,7 @@ func TestDatalossSubcommand(t *testing.T) { { desc: "multiple virtual storages with unavailable repositories", virtualStorages: []*config.VirtualStorage{{Name: "virtual-storage-2"}, {Name: "virtual-storage-1"}}, + args: []string{"-fully-unavailable"}, output: `Virtual storage: virtual-storage-1 Repositories: repository-2 (unavailable): @@ -146,7 +173,6 @@ Virtual storage: virtual-storage-2 }, { desc: "multiple virtual storages with partially unavailable repositories", - args: []string{"-partially-unavailable"}, virtualStorages: []*config.VirtualStorage{{Name: "virtual-storage-2"}, {Name: "virtual-storage-1"}}, output: `Virtual storage: virtual-storage-1 Repositories: -- GitLab From e0efdb025add74e1581f49628965b8c7ea0103ff Mon Sep 17 00:00:00 2001 From: John Cai Date: Thu, 7 Apr 2022 13:18:49 +0000 Subject: [PATCH 2/5] Apply 1 suggestion(s) to 1 file(s) --- cmd/praefect/subcmd_dataloss.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/praefect/subcmd_dataloss.go b/cmd/praefect/subcmd_dataloss.go index 621b0f994ff..95bd0156f9e 100644 --- a/cmd/praefect/subcmd_dataloss.go +++ b/cmd/praefect/subcmd_dataloss.go @@ -27,7 +27,7 @@ type datalossSubcommand struct { /* includePartiallyAvailable is just a variable for holding a deprecated partially-unavailable flag's parsed value. - But we don't need this variable. + This variable is not read, and will be removed in the future. */ includePartiallyAvailable bool } -- GitLab From bdff5fde51148c8f006518b931e5f8a846094887 Mon Sep 17 00:00:00 2001 From: Evan Read Date: Wed, 13 Apr 2022 07:57:12 +0000 Subject: [PATCH 3/5] Apply 4 suggestion(s) to 1 file(s) --- cmd/praefect/subcmd_dataloss.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/cmd/praefect/subcmd_dataloss.go b/cmd/praefect/subcmd_dataloss.go index 95bd0156f9e..091186e7b98 100644 --- a/cmd/praefect/subcmd_dataloss.go +++ b/cmd/praefect/subcmd_dataloss.go @@ -17,8 +17,7 @@ import ( const datalossCmdName = "dataloss" -var errDatalossFlagsConflict = errors.New("fully-unavailable flag and partially-unavailable flag " + - "cannot be used together") +var errDatalossFlagsConflict = errors.New("fully-unavailable option conflicts with partially-unavailable option." + type datalossSubcommand struct { output io.Writer @@ -40,20 +39,16 @@ func (cmd *datalossSubcommand) FlagSet() *flag.FlagSet { fs := flag.NewFlagSet(datalossCmdName, flag.ContinueOnError) fs.Usage = func() { printfErr("Description:\n" + - " This command checks whether all repositories in specified virtual storage are available or not.\n" + - " The default behavior is to show all partially unavailable repositories.\n" + - " Partially unavailable repositories are ones where some replicas are unavailable." + - " These repositories are available but not fully replicated.") + " Displays unavailable repositories in the specified virtual storage.\n" + + " By default, also shows all partially-unavailable repositories. Partially-unavailable repositories\n" + + " have some unavailable and unreplicated nodes.") fs.PrintDefaults() } fs.StringVar(&cmd.virtualStorage, "virtual-storage", "", "virtual storage to check for data loss") fs.BoolVar(&cmd.onlyIncludeFullyUnAvailable, "fully-unavailable", false, strings.TrimSpace(` -Only show repositories whose primaries are unavailable.`)) +Display only fully-unavailable repositories. Partially-unavailable repositories are not displayed.`)) fs.BoolVar(&cmd.includePartiallyAvailable, "partially-unavailable", false, strings.TrimSpace(` -(Deprecated, this is the default behavior of the command) -Additionally include repositories which are available but some assigned replicas -are unavailable. Such repositories are available but are not fully replicated. This -increases the chance of data loss on primary failure`)) +Display both fully-unavailable repositories and partially-unavailable repositories. Default behavior of command.`)) return fs } -- GitLab From b7f110b3a32fdc9e37545966e063de458b2698c8 Mon Sep 17 00:00:00 2001 From: Jia Shiang Gao Date: Wed, 13 Apr 2022 08:14:42 +0000 Subject: [PATCH 4/5] fix apply suggestions error. --- cmd/praefect/subcmd_dataloss.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/praefect/subcmd_dataloss.go b/cmd/praefect/subcmd_dataloss.go index 091186e7b98..8d524fbd2e9 100644 --- a/cmd/praefect/subcmd_dataloss.go +++ b/cmd/praefect/subcmd_dataloss.go @@ -17,7 +17,7 @@ import ( const datalossCmdName = "dataloss" -var errDatalossFlagsConflict = errors.New("fully-unavailable option conflicts with partially-unavailable option." + +var errDatalossFlagsConflict = errors.New("fully-unavailable option conflicts with partially-unavailable option.") type datalossSubcommand struct { output io.Writer -- GitLab From 9e08748e01e7b3a62f7afc66068a81b245af80e5 Mon Sep 17 00:00:00 2001 From: Jia Shiang Gao Date: Wed, 13 Apr 2022 09:41:20 +0000 Subject: [PATCH 5/5] remove period in the end of error message --- cmd/praefect/subcmd_dataloss.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/praefect/subcmd_dataloss.go b/cmd/praefect/subcmd_dataloss.go index 8d524fbd2e9..bc654d998fa 100644 --- a/cmd/praefect/subcmd_dataloss.go +++ b/cmd/praefect/subcmd_dataloss.go @@ -17,7 +17,7 @@ import ( const datalossCmdName = "dataloss" -var errDatalossFlagsConflict = errors.New("fully-unavailable option conflicts with partially-unavailable option.") +var errDatalossFlagsConflict = errors.New("fully-unavailable option conflicts with partially-unavailable option") type datalossSubcommand struct { output io.Writer -- GitLab