diff --git a/backup/delete_backup.go b/backup/delete_backup.go index 113f780e..abcffba2 100644 --- a/backup/delete_backup.go +++ b/backup/delete_backup.go @@ -117,6 +117,7 @@ func DoDeleteBackupInit(cmd *cobra.Command) { func DoDeleteBackup(timestamp string) { SetLoggerVerbosity() + ValidatePluginConfigFlag() if !filepath.IsValidTimestamp(timestamp) { gplog.Fatal(errors.Errorf("Invalid timestamp: %s", timestamp), "") diff --git a/backup/delete_backup_test.go b/backup/delete_backup_test.go index 6ed11541..12d429e6 100644 --- a/backup/delete_backup_test.go +++ b/backup/delete_backup_test.go @@ -11,6 +11,7 @@ import ( "github.com/greenplum-db/gpbackup/history" "github.com/greenplum-db/gpbackup/utils" + "github.com/spf13/cobra" "github.com/warehouse-pg/common-go-libs/cluster" "github.com/warehouse-pg/common-go-libs/testhelper" @@ -442,6 +443,38 @@ var _ = Describe("delete-backup internal tests", func() { }) }) + Describe("ValidatePluginConfigFlag", func() { + // delete-backup and delete-backups-before never run DoFlagValidation, so + // they call this guard themselves; exercise it through their own flag set. + runWithArgs := func(args []string) error { + testCmd := &cobra.Command{ + Use: "delete-backup", + Args: cobra.NoArgs, + Run: func(cmd *cobra.Command, _ []string) { + UseCmdFlags(cmd.Flags()) + ValidatePluginConfigFlag() + }} + RegisterDeleteBackupFlags(testCmd.Flags()) + testCmd.SetArgs(args) + return testCmd.Execute() + } + + It("fails when --plugin-config is specified with an empty value", func() { + defer testhelper.ShouldPanicWithMessage("The --plugin-config flag was specified with an empty value") + if err := runWithArgs([]string{"--plugin-config", ""}); err == nil { + Fail("empty --plugin-config value passed validation check") + } + }) + + It("passes when --plugin-config is specified with a path", func() { + Expect(runWithArgs([]string{"--plugin-config", "/etc/ddboost_config.yaml"})).To(Succeed()) + }) + + It("passes when --plugin-config is not specified", func() { + Expect(runWithArgs([]string{})).To(Succeed()) + }) + }) + Describe("newlyAddedDependents", func() { It("returns nothing when recheck matches the original set", func() { original := []*history.BackupConfig{{Timestamp: "20260101000000"}, {Timestamp: "20260101010000"}} diff --git a/backup/delete_backups_before.go b/backup/delete_backups_before.go index a6c99748..2149049a 100644 --- a/backup/delete_backups_before.go +++ b/backup/delete_backups_before.go @@ -29,6 +29,7 @@ func DoDeleteBackupsBeforeInit(cmd *cobra.Command) { // delete-backup --cascade. func DoDeleteBackupsBefore(cutoff string) { SetLoggerVerbosity() + ValidatePluginConfigFlag() if !filepath.IsValidTimestamp(cutoff) { gplog.Fatal(errors.Errorf("Invalid timestamp: %s", cutoff), "") diff --git a/backup/validate.go b/backup/validate.go index d87f1466..b368576f 100644 --- a/backup/validate.go +++ b/backup/validate.go @@ -164,7 +164,26 @@ func validateFlagCombinations(flags *pflag.FlagSet) { } } +/* + * ValidatePluginConfigFlag rejects --plugin-config given with an empty value. + * ValidateFullPath accepts the empty string and every command that takes this + * flag treats an empty plugin config as "no plugin at all", so an unset or + * mistyped shell variable would otherwise silently write the backup to local + * disk instead of to the plugin's storage -- which can fill up the segment + * hosts -- or delete only the local half of a backup set. + * + * Call this from any command that registers --plugin-config: gpbackup itself + * (via validateFlagValues), delete-backup and delete-backups-before. It must + * run after UseCmdFlags has pointed cmdFlags at the subcommand's flag set. + */ +func ValidatePluginConfigFlag() { + if FlagChanged(options.PLUGIN_CONFIG) && MustGetFlagString(options.PLUGIN_CONFIG) == "" { + gplog.Fatal(errors.Errorf("The --plugin-config flag was specified with an empty value. Specify the absolute path to the plugin configuration file, or omit the flag to use local storage."), "") + } +} + func validateFlagValues() { + ValidatePluginConfigFlag() err := utils.ValidateFullPath(MustGetFlagString(options.BACKUP_DIR)) gplog.FatalOnError(err) err = utils.ValidateFullPath(MustGetFlagString(options.PLUGIN_CONFIG)) diff --git a/backup/validate_test.go b/backup/validate_test.go index 248fffc0..acde8325 100644 --- a/backup/validate_test.go +++ b/backup/validate_test.go @@ -215,6 +215,8 @@ var _ = Describe("backup/validate tests", func() { } }, Entry("--backup-dir combo", "--backup-dir /tmp --plugin-config /tmp/config", false), + Entry("--plugin-config with an empty value", "--plugin-config ", false), + Entry("--plugin-config with a path", "--plugin-config /tmp/config", true), /* * Below are all the different filter combinations diff --git a/restore/remote.go b/restore/remote.go index bb6ff279..c31c9b34 100644 --- a/restore/remote.go +++ b/restore/remote.go @@ -17,25 +17,54 @@ import ( * Functions to run commands on entire cluster during restore */ -func VerifyBackupDirectoriesExistOnAllHosts() { - _, err := globalCluster.ExecuteLocalCommand(fmt.Sprintf("test -d %s", globalFPInfo.GetDirForContent(-1))) - gplog.FatalOnError(err, "Backup directory %s missing or inaccessible", globalFPInfo.GetDirForContent(-1)) - if MustGetFlagString(options.PLUGIN_CONFIG) == "" || backupConfig.SingleDataFile { - origSize, destSize, isResizeRestore, _ := GetResizeClusterInfo() - - remoteOutput := globalCluster.GenerateAndExecuteCommand("Verifying backup directories exist", cluster.ON_SEGMENTS, func(contentID int) string { - if isResizeRestore { // Map origin content to destination content to find where the original files have been placed - if contentID >= origSize { // Don't check for directories for contents that aren't part of the backup set - return "" - } - contentID = contentID % destSize - } - return fmt.Sprintf("test -d %s", globalFPInfo.GetDirForContent(contentID)) - }) - globalCluster.CheckClusterError(remoteOutput, "Backup directories missing or inaccessible", func(contentID int) string { - return fmt.Sprintf("Backup directory %s missing or inaccessible", globalFPInfo.GetDirForContent(contentID)) - }) +/* + * EnsureBackupDirectoriesExistOnAllHosts checks, or creates, the backup + * directories the restore is about to use. + * + * Without a plugin those directories hold the backup itself, so a missing one + * means the data is gone and the restore must stop. With a plugin the backup + * lives on the plugin's storage and the local directories are only staging + * areas for files gprestore downloads into them, so they are created here + * instead of being required. That is what lets a backup be restored onto a + * host that was rebuilt or replaced after the backup was taken, without having + * to reconstruct the old directory tree by hand first. + */ +func EnsureBackupDirectoriesExistOnAllHosts() { + usingPlugin := MustGetFlagString(options.PLUGIN_CONFIG) != "" + + coordinatorDir := globalFPInfo.GetDirForContent(-1) + if usingPlugin { + _, err := globalCluster.ExecuteLocalCommand(fmt.Sprintf("mkdir -p %s", coordinatorDir)) + gplog.FatalOnError(err, "Unable to create backup directory %s", coordinatorDir) + } else { + _, err := globalCluster.ExecuteLocalCommand(fmt.Sprintf("test -d %s", coordinatorDir)) + gplog.FatalOnError(err, "Backup directory %s missing or inaccessible", coordinatorDir) } + + // The segment directories are needed for every plugin restore, not just the + // single-data-file ones: a resize restore stages the helper's pipes, oid + // files and segment TOCs there as well. + dirCommand, verboseMsg, errMsg := "test -d", "Verifying backup directories exist", "Backup directories missing or inaccessible" + if usingPlugin { + dirCommand, verboseMsg, errMsg = "mkdir -p", "Creating backup directories", "Unable to create backup directories" + } + origSize, destSize, isResizeRestore, _ := GetResizeClusterInfo() + + remoteOutput := globalCluster.GenerateAndExecuteCommand(verboseMsg, cluster.ON_SEGMENTS, func(contentID int) string { + if isResizeRestore { // Map origin content to destination content to find where the original files have been placed + if contentID >= origSize { // Don't check for directories for contents that aren't part of the backup set + return "" + } + contentID = contentID % destSize + } + return fmt.Sprintf("%s %s", dirCommand, globalFPInfo.GetDirForContent(contentID)) + }) + globalCluster.CheckClusterError(remoteOutput, errMsg, func(contentID int) string { + if usingPlugin { + return fmt.Sprintf("Unable to create backup directory %s", globalFPInfo.GetDirForContent(contentID)) + } + return fmt.Sprintf("Backup directory %s missing or inaccessible", globalFPInfo.GetDirForContent(contentID)) + }) } func VerifyBackupFileCountOnSegments() { diff --git a/restore/remote_test.go b/restore/remote_test.go index d05a188d..e5f2f453 100644 --- a/restore/remote_test.go +++ b/restore/remote_test.go @@ -117,4 +117,49 @@ var _ = Describe("restore/remote tests", func() { restore.VerifyBackupFileCountOnSegments() }) }) + Describe("EnsureBackupDirectoriesExistOnAllHosts", func() { + BeforeEach(func() { + testExecutor.ClusterOutput = &cluster.RemoteOutput{NumErrors: 0} + testCluster.Executor = testExecutor + restore.SetCluster(testCluster) + restore.SetBackupConfig(&history.BackupConfig{SegmentCount: 2}) + }) + It("requires the directories to exist when no plugin is in use", func() { + restore.EnsureBackupDirectoriesExistOnAllHosts() + + Expect(testExecutor.LocalCommands[0]).To(Equal("test -d /data/gpseg-1/backups/20170101/20170101010101")) + cc := testExecutor.ClusterCommands[0] + Expect(cc).To(HaveLen(2)) + Expect(cc[0].CommandString).To(ContainSubstring("test -d /data/gpseg0/backups/20170101/20170101010101")) + Expect(cc[1].CommandString).To(ContainSubstring("test -d /data/gpseg1/backups/20170101/20170101010101")) + }) + It("creates the directories when a plugin is in use", func() { + // With a plugin these directories only stage files downloaded from the + // plugin's storage, so a host rebuilt since the backup must not fail here. + cmdFlags.Set(options.PLUGIN_CONFIG, "/tmp/plugin_config.yaml") + restore.SetBackupConfig(&history.BackupConfig{SegmentCount: 2, SingleDataFile: true}) + + restore.EnsureBackupDirectoriesExistOnAllHosts() + + Expect(testExecutor.LocalCommands[0]).To(Equal("mkdir -p /data/gpseg-1/backups/20170101/20170101010101")) + cc := testExecutor.ClusterCommands[0] + Expect(cc).To(HaveLen(2)) + Expect(cc[0].CommandString).To(ContainSubstring("mkdir -p /data/gpseg0/backups/20170101/20170101010101")) + Expect(cc[1].CommandString).To(ContainSubstring("mkdir -p /data/gpseg1/backups/20170101/20170101010101")) + }) + It("creates the segment directories for a plugin backup taken without --single-data-file", func() { + // This combination used to skip the segments entirely, leaving the staging + // directory missing for the helper files a resize restore puts there. + cmdFlags.Set(options.PLUGIN_CONFIG, "/tmp/plugin_config.yaml") + restore.SetBackupConfig(&history.BackupConfig{SegmentCount: 2, SingleDataFile: false}) + + restore.EnsureBackupDirectoriesExistOnAllHosts() + + Expect(testExecutor.NumClusterExecutions).To(Equal(1)) + cc := testExecutor.ClusterCommands[0] + Expect(cc).To(HaveLen(2)) + Expect(cc[0].CommandString).To(ContainSubstring("mkdir -p /data/gpseg0/backups/20170101/20170101010101")) + Expect(cc[1].CommandString).To(ContainSubstring("mkdir -p /data/gpseg1/backups/20170101/20170101010101")) + }) + }) }) diff --git a/restore/restore.go b/restore/restore.go index 2f7bcfcb..15eaf976 100644 --- a/restore/restore.go +++ b/restore/restore.go @@ -39,6 +39,7 @@ func DoInit(cmd *cobra.Command) { */ func DoValidation(cmd *cobra.Command) { ValidateFlagCombinations(cmd) + ValidatePluginConfigFlag() err := utils.ValidateFullPath(MustGetFlagString(options.BACKUP_DIR)) gplog.FatalOnError(err) err = utils.ValidateFullPath(MustGetFlagString(options.PLUGIN_CONFIG)) @@ -93,8 +94,10 @@ func DoSetup() { // Get restore metadata from plugin if MustGetFlagString(options.PLUGIN_CONFIG) != "" { + gplog.Info("Restoring metadata files using plugin config %s", MustGetFlagString(options.PLUGIN_CONFIG)) RecoverMetadataFilesUsingPlugin() } else { + gplog.Info("No plugin configured; expecting all backup files on local disk under %s", globalFPInfo.GetDirForContent(-1)) InitializeBackupConfig() } diff --git a/restore/validate.go b/restore/validate.go index 4c18e839..b4451059 100644 --- a/restore/validate.go +++ b/restore/validate.go @@ -282,10 +282,29 @@ func ValidateBackupFlagCombinations() { if !backupConfig.SingleDataFile && FlagChanged(options.COPY_QUEUE_SIZE) { gplog.Fatal(errors.Errorf("The --copy-queue-size flag can only be used if the backup was taken with --single-data-file"), "") } - validateBackupFlagPluginCombinations() } -func validateBackupFlagPluginCombinations() { +/* + * ValidatePluginConfigFlag rejects --plugin-config given with an empty value. + * ValidateFullPath accepts the empty string, and DoSetup treats an empty plugin + * config as "no plugin at all", so without this check an unset or mistyped shell + * variable silently downgrades a plugin restore to a local-filesystem restore + * and then fails on the first backup file that is not on local disk. + */ +func ValidatePluginConfigFlag() { + if FlagChanged(options.PLUGIN_CONFIG) && MustGetFlagString(options.PLUGIN_CONFIG) == "" { + gplog.Fatal(errors.Errorf("The --plugin-config flag was specified with an empty value. Specify the absolute path to the plugin configuration file, or omit the flag to restore from local disk."), "") + } +} + +/* + * ValidateBackupFlagPluginCombinations must be called before any check that + * assumes the backup files are on local disk. A backup taken with a plugin + * stores its files on the plugin's storage, so if the plugin is missing here the + * useful error is "the --plugin-config flag must be used", not a report of a + * missing local directory or file. + */ +func ValidateBackupFlagPluginCombinations() { if MustGetFlagBool(options.IGNORE_PLUGIN_CONFIG) { // No-op against a backup taken without a plugin; the flag just means // "do not invoke any plugin during restore", which is already the diff --git a/restore/validate_test.go b/restore/validate_test.go index 31b2b124..79105e49 100644 --- a/restore/validate_test.go +++ b/restore/validate_test.go @@ -421,7 +421,7 @@ var _ = Describe("restore/validate tests", func() { Use: "flag validation", Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { - restore.ValidateBackupFlagCombinations() + restore.ValidateBackupFlagPluginCombinations() }} testCmd.SetArgs([]string{"--ignore-plugin-config"}) restore.SetCmdFlags(testCmd.Flags()) @@ -435,11 +435,72 @@ var _ = Describe("restore/validate tests", func() { Use: "flag validation", Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { - restore.ValidateBackupFlagCombinations() + restore.ValidateBackupFlagPluginCombinations() }} testCmd.SetArgs([]string{"--ignore-plugin-config"}) restore.SetCmdFlags(testCmd.Flags()) + err := testCmd.Execute() + Expect(err).ToNot(HaveOccurred()) + }) + It("should fatal when the backup was taken with a plugin but no plugin config is given", func() { + restore.SetBackupConfig(&history.BackupConfig{Plugin: "/tmp/gpbackup_fake_plugin"}) + testCmd := &cobra.Command{ + Use: "flag validation", + Args: cobra.NoArgs, + Run: func(cmd *cobra.Command, args []string) { + restore.ValidateBackupFlagPluginCombinations() + }} + testCmd.SetArgs([]string{}) + restore.SetCmdFlags(testCmd.Flags()) + + defer testhelper.ShouldPanicWithMessage("The --plugin-config flag must be used to restore") + err := testCmd.Execute() + if err == nil { + Fail("restore of a plugin backup without --plugin-config passed validation check") + } + }) + }) + Describe("ValidatePluginConfigFlag", func() { + It("should fatal when --plugin-config is specified with an empty value", func() { + testCmd := &cobra.Command{ + Use: "flag validation", + Args: cobra.NoArgs, + Run: func(cmd *cobra.Command, args []string) { + restore.ValidatePluginConfigFlag() + }} + testCmd.SetArgs([]string{"--plugin-config", ""}) + restore.SetCmdFlags(testCmd.Flags()) + + defer testhelper.ShouldPanicWithMessage("The --plugin-config flag was specified with an empty value") + err := testCmd.Execute() + if err == nil { + Fail("empty --plugin-config value passed validation check") + } + }) + It("should pass when --plugin-config is specified with a path", func() { + testCmd := &cobra.Command{ + Use: "flag validation", + Args: cobra.NoArgs, + Run: func(cmd *cobra.Command, args []string) { + restore.ValidatePluginConfigFlag() + }} + testCmd.SetArgs([]string{"--plugin-config", "/tmp/plugin_config.yaml"}) + restore.SetCmdFlags(testCmd.Flags()) + + err := testCmd.Execute() + Expect(err).ToNot(HaveOccurred()) + }) + It("should pass when --plugin-config is not specified at all", func() { + testCmd := &cobra.Command{ + Use: "flag validation", + Args: cobra.NoArgs, + Run: func(cmd *cobra.Command, args []string) { + restore.ValidatePluginConfigFlag() + }} + testCmd.SetArgs([]string{}) + restore.SetCmdFlags(testCmd.Flags()) + err := testCmd.Execute() Expect(err).ToNot(HaveOccurred()) }) diff --git a/restore/wrappers.go b/restore/wrappers.go index ebd50904..f7716a07 100644 --- a/restore/wrappers.go +++ b/restore/wrappers.go @@ -163,9 +163,14 @@ func InitializeBackupConfig() { } func BackupConfigurationValidation() { + // This must run first: with no plugin in effect the checks below assume the + // backup files are on local disk, and would report a missing directory or + // file instead of the missing --plugin-config flag. + ValidateBackupFlagPluginCombinations() + if !backupConfig.MetadataOnly { gplog.Verbose("Gathering information on backup directories") - VerifyBackupDirectoriesExistOnAllHosts() + EnsureBackupDirectoriesExistOnAllHosts() } VerifyMetadataFilePaths(MustGetFlagBool(options.WITH_STATS))