This is an automated email from the ASF dual-hosted git repository. tuhaihe pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/cloudberry-backup.git
commit de5a244925cada1ba94f2084336b9a830d3858d3 Author: chenqiang <[email protected]> AuthorDate: Wed Apr 29 15:35:38 2026 +0800 gpbackman: gate history-db env-var fallback behind --auto-load-history-db opt-in. Address review feedback on the original PR: * The env-var fallback ($COORDINATOR_DATA_DIRECTORY then $MASTER_DATA_DIRECTORY) is now opt-in via a new persistent flag --auto-load-history-db. The default behaviour (current-directory lookup) is unchanged from upstream main, so destructive subcommands (backup-delete, backup-clean, history-clean) cannot silently target the wrong cluster's history DB on a multi-cluster jump host. * Drop the "Greenplum" mention in the comment for getHistoryDBPath to keep wording aligned with the Cloudberry environment scripts. * Trim the verbose --history-db description in gpbackman/README.md and add a separate --auto-load-history-db line. * Refine the not-found error message in OpenHistoryDB to mention the new flag rather than implying that setting env vars alone helps. * Tests updated to cover the opt-in matrix: env vars are ignored when the flag is off, honoured when on, and an explicit --history-db still wins regardless. The OpenHistoryDB safe-open change (os.Stat pre-check + mode=rw URI to avoid silently creating an empty SQLite file) is preserved as-is. Signed-off-by: chenqiang <[email protected]> --- gpbackman/COMMANDS.md | 10 +++++----- gpbackman/README.md | 3 ++- gpbackman/cmd/backup_clean.go | 4 ++-- gpbackman/cmd/backup_delete.go | 4 ++-- gpbackman/cmd/backup_info.go | 4 ++-- gpbackman/cmd/constants.go | 1 + gpbackman/cmd/history_clean.go | 4 ++-- gpbackman/cmd/report_info.go | 4 ++-- gpbackman/cmd/root.go | 21 ++++++++++++++------- gpbackman/cmd/wrappers.go | 24 +++++++++++++----------- gpbackman/cmd/wrappers_test.go | 32 +++++++++++++++++++++----------- gpbackman/gpbckpconfig/utils_db.go | 8 ++++---- 12 files changed, 70 insertions(+), 49 deletions(-) diff --git a/gpbackman/COMMANDS.md b/gpbackman/COMMANDS.md index cf82aa8b..7cb15fd0 100644 --- a/gpbackman/COMMANDS.md +++ b/gpbackman/COMMANDS.md @@ -72,7 +72,7 @@ For non local backups the following logic are applied: The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`); if neither variable is set, the current directory is used as a last resort. +If the --history-db option is not specified, the history database is looked for in the current directory. Pass `--auto-load-history-db` to resolve it from `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`) instead. Usage: gpbackman backup-clean [flags] @@ -158,7 +158,7 @@ For non local backups the following logic are applied: The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`); if neither variable is set, the current directory is used as a last resort. +If the --history-db option is not specified, the history database is looked for in the current directory. Pass `--auto-load-history-db` to resolve it from `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`) instead. Usage: gpbackman backup-delete [flags] @@ -256,7 +256,7 @@ To display the "object filtering details" column for all backups without using - The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`); if neither variable is set, the current directory is used as a last resort. +If the --history-db option is not specified, the history database is looked for in the current directory. Pass `--auto-load-history-db` to resolve it from `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`) instead. Usage: gpbackman backup-info [flags] @@ -455,7 +455,7 @@ Only --older-than-days or --before-timestamp option must be specified, not both. The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`); if neither variable is set, the current directory is used as a last resort. +If the --history-db option is not specified, the history database is looked for in the current directory. Pass `--auto-load-history-db` to resolve it from `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`) instead. Usage: gpbackman history-clean [flags] @@ -524,7 +524,7 @@ It is not necessary to use the --plugin-report-file-path flag for the following The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`); if neither variable is set, the current directory is used as a last resort. +If the --history-db option is not specified, the history database is looked for in the current directory. Pass `--auto-load-history-db` to resolve it from `$COORDINATOR_DATA_DIRECTORY` (then `$MASTER_DATA_DIRECTORY`) instead. Usage: gpbackman report-info [flags] diff --git a/gpbackman/README.md b/gpbackman/README.md index 923964ad..930e570b 100644 --- a/gpbackman/README.md +++ b/gpbackman/README.md @@ -53,7 +53,8 @@ Available Commands: Flags: -h, --help help for gpbackman - --history-db string full path to the gpbackup_history.db file (if unset, falls back to $COORDINATOR_DATA_DIRECTORY/gpbackup_history.db, then $MASTER_DATA_DIRECTORY/gpbackup_history.db, then the current directory) + --auto-load-history-db resolve gpbackup_history.db from $COORDINATOR_DATA_DIRECTORY (or $MASTER_DATA_DIRECTORY) when --history-db is unset + --history-db string full path to the gpbackup_history.db file --log-file string full path to log file directory, if not specified, the log file will be created in the $HOME/gpAdminLogs directory --log-level-console string level for console logging (error, info, debug, verbose) (default "info") --log-level-file string level for file logging (error, info, debug, verbose) (default "info") diff --git a/gpbackman/cmd/backup_clean.go b/gpbackman/cmd/backup_clean.go index 1ccaf8f8..eacd3418 100644 --- a/gpbackman/cmd/backup_clean.go +++ b/gpbackman/cmd/backup_clean.go @@ -77,7 +77,7 @@ For non local backups the following logic are applied: The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, +If the --history-db option is not specified, the history database is looked for in the current directory. To resolve it from $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY) instead, pass the --auto-load-history-db flag.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) @@ -205,7 +205,7 @@ func doCleanBackup() { } func cleanBackup() error { - hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB)) + hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB, rootAutoLoadHistoryDB)) if err != nil { gplog.Error("%s", textmsg.ErrorTextUnableActionHistoryDB("open", err)) return err diff --git a/gpbackman/cmd/backup_delete.go b/gpbackman/cmd/backup_delete.go index 202d52b3..2d4b95f8 100644 --- a/gpbackman/cmd/backup_delete.go +++ b/gpbackman/cmd/backup_delete.go @@ -84,7 +84,7 @@ For non local backups the following logic are applied: The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, +If the --history-db option is not specified, the history database is looked for in the current directory. To resolve it from $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY) instead, pass the --auto-load-history-db flag.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) @@ -205,7 +205,7 @@ func doDeleteBackup() { } func deleteBackup() error { - hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB)) + hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB, rootAutoLoadHistoryDB)) if err != nil { gplog.Error("%s", textmsg.ErrorTextUnableActionHistoryDB("open", err)) return err diff --git a/gpbackman/cmd/backup_info.go b/gpbackman/cmd/backup_info.go index 0515e402..adb5c118 100644 --- a/gpbackman/cmd/backup_info.go +++ b/gpbackman/cmd/backup_info.go @@ -98,7 +98,7 @@ To display the "object filtering details" column for all backups without using - The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, +If the --history-db option is not specified, the history database is looked for in the current directory. To resolve it from $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY) instead, pass the --auto-load-history-db flag.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) @@ -226,7 +226,7 @@ func backupInfo() error { } t := tablewriter.NewWriter(os.Stdout) initTable(t, opts.ShowDetails) - hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB)) + hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB, rootAutoLoadHistoryDB)) if err != nil { gplog.Error("%s", textmsg.ErrorTextUnableActionHistoryDB("open", err)) return err diff --git a/gpbackman/cmd/constants.go b/gpbackman/cmd/constants.go index 14610c5a..7b5b59ed 100644 --- a/gpbackman/cmd/constants.go +++ b/gpbackman/cmd/constants.go @@ -35,6 +35,7 @@ const ( // Flags. historyDBFlagName = "history-db" + autoLoadHistoryDBFlagName = "auto-load-history-db" logFileFlagName = "log-file" logLevelConsoleFlagName = "log-level-console" logLevelFileFlagName = "log-level-file" diff --git a/gpbackman/cmd/history_clean.go b/gpbackman/cmd/history_clean.go index 836991b8..361f4ee0 100644 --- a/gpbackman/cmd/history_clean.go +++ b/gpbackman/cmd/history_clean.go @@ -50,7 +50,7 @@ Only --older-than-days or --before-timestamp option must be specified, not both. The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, +If the --history-db option is not specified, the history database is looked for in the current directory. To resolve it from $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY) instead, pass the --auto-load-history-db flag.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) @@ -106,7 +106,7 @@ func doCleanHistory() { } func cleanHistory() error { - hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB)) + hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB, rootAutoLoadHistoryDB)) if err != nil { gplog.Error("%s", textmsg.ErrorTextUnableActionHistoryDB("open", err)) return err diff --git a/gpbackman/cmd/report_info.go b/gpbackman/cmd/report_info.go index 168695ae..042a3da5 100644 --- a/gpbackman/cmd/report_info.go +++ b/gpbackman/cmd/report_info.go @@ -78,7 +78,7 @@ It is not necessary to use the --plugin-report-file-path flag for the following The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, +If the --history-db option is not specified, the history database is looked for in the current directory. To resolve it from $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY) instead, pass the --auto-load-history-db flag.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) @@ -174,7 +174,7 @@ func doReportInfo() { } func reportInfo() error { - hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB)) + hDB, err := gpbckpconfig.OpenHistoryDB(getHistoryDBPath(rootHistoryDB, rootAutoLoadHistoryDB)) if err != nil { gplog.Error("%s", textmsg.ErrorTextUnableActionHistoryDB("open", err)) return err diff --git a/gpbackman/cmd/root.go b/gpbackman/cmd/root.go index e8473269..50884771 100644 --- a/gpbackman/cmd/root.go +++ b/gpbackman/cmd/root.go @@ -33,10 +33,11 @@ var version string // Flags for the gpbackman command (rootCmd) var ( - rootHistoryDB string - rootLogFile string - rootLogLevelConsole string - rootLogLevelFile string + rootHistoryDB string + rootAutoLoadHistoryDB bool + rootLogFile string + rootLogLevelConsole string + rootLogLevelFile string ) var rootCmd = &cobra.Command{ @@ -50,9 +51,15 @@ func init() { &rootHistoryDB, historyDBFlagName, "", - "full path to the gpbackup_history.db file (if unset, falls back to "+ - "$COORDINATOR_DATA_DIRECTORY/gpbackup_history.db, then "+ - "$MASTER_DATA_DIRECTORY/gpbackup_history.db, then the current directory)", + "full path to the gpbackup_history.db file", + ) + rootCmd.PersistentFlags().BoolVar( + &rootAutoLoadHistoryDB, + autoLoadHistoryDBFlagName, + false, + "when --history-db is unset, look up gpbackup_history.db under "+ + "$COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY) before "+ + "falling back to the current directory", ) rootCmd.PersistentFlags().StringVar( &rootLogFile, diff --git a/gpbackman/cmd/wrappers.go b/gpbackman/cmd/wrappers.go index be1c5b59..f809ce4c 100644 --- a/gpbackman/cmd/wrappers.go +++ b/gpbackman/cmd/wrappers.go @@ -82,20 +82,22 @@ func setLogLevelFile(level string) error { } // getHistoryDBPath resolves the path to the gpbackup_history.db file. -// When the --history-db flag is empty, fall back (in order) to the -// COORDINATOR_DATA_DIRECTORY and MASTER_DATA_DIRECTORY environment variables -// that the standard Cloudberry/Greenplum environment scripts export, so that -// users running gpbackman from a sourced cluster shell do not need to repeat -// the path on every invocation. As a last resort, return the bare filename -// (resolved against the current working directory), preserving the previous -// behaviour. -func getHistoryDBPath(historyDBPath string) string { +// An explicit --history-db value always wins. Otherwise, when the caller +// asks for auto-load (--auto-load-history-db), look up the file under the +// COORDINATOR_DATA_DIRECTORY / MASTER_DATA_DIRECTORY environment variables +// exported by the standard Cloudberry environment scripts. As a final +// fallback, return the bare filename so it is resolved against the current +// working directory, preserving the original behaviour for the default +// invocation. +func getHistoryDBPath(historyDBPath string, autoLoad bool) string { if historyDBPath != "" { return historyDBPath } - for _, envVar := range historyDBEnvVars { - if dir := os.Getenv(envVar); dir != "" { - return filepath.Join(dir, historyDBNameConst) + if autoLoad { + for _, envVar := range historyDBEnvVars { + if dir := os.Getenv(envVar); dir != "" { + return filepath.Join(dir, historyDBNameConst) + } } } return historyDBNameConst diff --git a/gpbackman/cmd/wrappers_test.go b/gpbackman/cmd/wrappers_test.go index d7e16093..401f2962 100644 --- a/gpbackman/cmd/wrappers_test.go +++ b/gpbackman/cmd/wrappers_test.go @@ -55,33 +55,43 @@ var _ = Describe("wrappers tests", func() { } }) - It("returns default filename when input is empty and no env vars are set", func() { - Expect(getHistoryDBPath("")).To(Equal(historyDBNameConst)) + It("returns default filename when input is empty (auto-load off, no env)", func() { + Expect(getHistoryDBPath("", false)).To(Equal(historyDBNameConst)) }) It("returns input path when not empty", func() { - Expect(getHistoryDBPath("path/to/" + historyDBNameConst)).To(Equal("path/to/" + historyDBNameConst)) + Expect(getHistoryDBPath("path/to/"+historyDBNameConst, false)).To(Equal("path/to/" + historyDBNameConst)) }) - It("falls back to COORDINATOR_DATA_DIRECTORY when input is empty", func() { + It("ignores env vars by default (auto-load off)", func() { os.Setenv("COORDINATOR_DATA_DIRECTORY", "/coord/data") - Expect(getHistoryDBPath("")).To(Equal(filepath.Join("/coord/data", historyDBNameConst))) + os.Setenv("MASTER_DATA_DIRECTORY", "/master/data") + Expect(getHistoryDBPath("", false)).To(Equal(historyDBNameConst)) + }) + + It("falls back to COORDINATOR_DATA_DIRECTORY when auto-load is on", func() { + os.Setenv("COORDINATOR_DATA_DIRECTORY", "/coord/data") + Expect(getHistoryDBPath("", true)).To(Equal(filepath.Join("/coord/data", historyDBNameConst))) }) - It("falls back to MASTER_DATA_DIRECTORY when COORDINATOR is unset", func() { + It("falls back to MASTER_DATA_DIRECTORY when COORDINATOR is unset and auto-load is on", func() { os.Setenv("MASTER_DATA_DIRECTORY", "/master/data") - Expect(getHistoryDBPath("")).To(Equal(filepath.Join("/master/data", historyDBNameConst))) + Expect(getHistoryDBPath("", true)).To(Equal(filepath.Join("/master/data", historyDBNameConst))) }) - It("prefers COORDINATOR over MASTER when both are set", func() { + It("prefers COORDINATOR over MASTER when both are set and auto-load is on", func() { os.Setenv("COORDINATOR_DATA_DIRECTORY", "/coord/data") os.Setenv("MASTER_DATA_DIRECTORY", "/master/data") - Expect(getHistoryDBPath("")).To(Equal(filepath.Join("/coord/data", historyDBNameConst))) + Expect(getHistoryDBPath("", true)).To(Equal(filepath.Join("/coord/data", historyDBNameConst))) + }) + + It("returns the cwd default when auto-load is on but no env vars are set", func() { + Expect(getHistoryDBPath("", true)).To(Equal(historyDBNameConst)) }) - It("explicit input wins over env vars", func() { + It("explicit input wins over env vars even when auto-load is on", func() { os.Setenv("COORDINATOR_DATA_DIRECTORY", "/coord/data") - Expect(getHistoryDBPath("/explicit/path.db")).To(Equal("/explicit/path.db")) + Expect(getHistoryDBPath("/explicit/path.db", true)).To(Equal("/explicit/path.db")) }) }) diff --git a/gpbackman/gpbckpconfig/utils_db.go b/gpbackman/gpbckpconfig/utils_db.go index 9cf18df5..07e307d9 100644 --- a/gpbackman/gpbckpconfig/utils_db.go +++ b/gpbackman/gpbckpconfig/utils_db.go @@ -42,10 +42,10 @@ func OpenHistoryDB(historyDBPath string) (*sql.DB, error) { if errors.Is(err, os.ErrNotExist) { return nil, fmt.Errorf( "gpbackup history database file not found: %s. "+ - "Specify the path via --history-db, set "+ - "COORDINATOR_DATA_DIRECTORY (or MASTER_DATA_DIRECTORY), "+ - "or run gpbackman from the directory that contains "+ - "gpbackup_history.db", + "Specify the path via --history-db, run gpbackman from "+ + "the directory that contains gpbackup_history.db, or "+ + "pass --auto-load-history-db to resolve it from "+ + "$COORDINATOR_DATA_DIRECTORY (or $MASTER_DATA_DIRECTORY)", historyDBPath, ) } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
