fix: reject an empty --plugin-config and stage plugin restore directories - #48
Open
Bonartze wants to merge 3 commits into
Open
fix: reject an empty --plugin-config and stage plugin restore directories#48Bonartze wants to merge 3 commits into
Bonartze wants to merge 3 commits into
Conversation
…ly restoring from local disk
gprestore accepted `--plugin-config ""` and fell back to a local-filesystem
restore of a backup set that only exists on the plugin's storage.
utils.ValidateFullPath short-circuits on len(path) > 0, so the empty value passed
validation, and DoSetup treats an empty plugin config as "no plugin at all"
(restore/restore.go:95). The run then failed on the first backup file that is not
on local disk. Reported against DD Boost as
open .../gpbackup_<ts>_config.yaml: no such file or directory
Backup directories missing or inaccessible on 1 segment
both of which point at the local filesystem rather than at the dropped plugin,
and neither of which mentions that no plugin was in effect.
- Reject an explicitly-set-but-empty --plugin-config in gprestore
(ValidatePluginConfigFlag, called from DoValidation) and in gpbackup
(validateFlagValues), so the same mistake at backup time cannot silently
produce a backup on local disk instead of on the plugin's storage.
- Run the plugin-combination check before any check that assumes the backup
files are on local disk: ValidateBackupFlagPluginCombinations (renamed from
the unexported validateBackupFlagPluginCombinations) now runs at the top of
BackupConfigurationValidation, ahead of VerifyBackupDirectoriesExistOnAllHosts.
A plugin backup restored without --plugin-config now reports "Backup was taken
with plugin ... The --plugin-config flag must be used to restore." instead of a
missing local directory. BackupConfigurationValidation was the only production
caller, so this is a reordering, not a behavior change.
- Log at INFO whether a plugin is in effect and which config path is used.
…g them when a plugin is in use A restore through a plugin failed if the local backup directory was missing on the coordinator or on any segment, even though the backup itself lives on the plugin's storage and those directories only ever hold files gprestore downloads into them. A host rebuilt or replaced since the backup was taken therefore could not be restored onto without first recreating the old directory tree by hand (the reported workaround was to NFS-mount the Data Domain storage unit and copy the files back). VerifyBackupDirectoriesExistOnAllHosts is renamed to EnsureBackupDirectoriesExistOnAllHosts and now branches on whether a plugin is in effect: - with a plugin, "mkdir -p" the coordinator and segment directories; - without one, keep asserting them with "test -d", because there a missing directory means the backup data really is gone. The segment directories are now handled for every plugin restore rather than only for single-data-file ones. That combination previously skipped the segments altogether, which is harmless for a plain restore (restoreDataFromTimestamp only starts gpbackup_helper when SingleDataFile || resizeCluster, and otherwise the segments read straight from the plugin via COPY FROM PROGRAM) but leaves the staging directory missing for a resize restore, whose helper writes its pipes, oid files and segment TOCs there. Adds unit coverage for all three cases via the cluster test executor.
…s too --plugin-config is registered on four flag sets: gpbackup, gprestore, delete-backup and delete-backups-before. gpbackup and gprestore now reject an explicitly-empty value, but the two delete commands never call DoFlagValidation, so an empty value still reached setupDeletionTargets as "" and meant "no plugin". Lifts the guard out of validateFlagValues into an exported ValidatePluginConfigFlag and calls it from DoDeleteBackup and DoDeleteBackupsBefore, after UseCmdFlags has pointed cmdFlags at the subcommand's own flag set. Reported on the ticket: a silent fallback to local storage is dangerous in both directions -- a "backup" that is not on the appliance can fill the segment hosts, and a delete that ignores the plugin removes only the local half of a backup set.
Bonartze
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the reported DD Boost restore failure, where
gprestorestopped withafter the simulation host loss by deleting
$COORDINATOR_DATA_DIRECTORY/backups/YYYYMMDD(and, separately, a segment's
backups/<date>/<ts>). Both errors point at the local filesystemrather than at the real problem, and the documented workaround was to NFS-mount the Data Domain
storage unit and copy the files back by hand.
Root cause
--plugin-config "$CFG"was passed with$CFGempty.utils.ValidateFullPathshort-circuits onlen(path) > 0(utils/util.go:129), so the empty value passes validation, andrestore/restore.go:95then reads "empty plugin config" as "no plugin at all" and restores from local disk with no warning.
The reporter has since confirmed this on the ticket. The same guard is used by
gpbackup, where theconsequence is worse: the backup silently lands on segment local disk and reports success, which can
fill the segment hosts.
What changed
--plugin-configis now a config error, on every command that accepts theflag:
gpbackup,gprestore,gpbackup delete-backupandgpbackup delete-backups-before. Thetwo delete commands never run
DoFlagValidation, so they call the sharedValidatePluginConfigFlagthemselves afterUseCmdFlags. An empty value there would have deletedonly the local half of a backup set.
ValidateBackupFlagPluginCombinations(renamed from the unexported form) now runs at the top ofBackupConfigurationValidation, ahead of the directory check. A plugin backup restored without--plugin-configreports "Backup was taken with plugin ... The --plugin-config flag must be usedto restore" instead of a missing local directory. That function had one production caller, so this
is a reordering, not new logic.
configured and files are expected on local disk under a named directory.
VerifyBackupDirectoriesExistOnAllHostsbecomesEnsureBackupDirectoriesExistOnAllHosts: with aplugin it
mkdir -ps the coordinator and segment directories, since those only stage filesgprestore downloads from the plugin's storage; without a plugin it keeps asserting them with
test -d, because there a missing directory means the data really is gone. Segments are now coveredfor every plugin restore rather than only single-data-file ones — that combination previously
skipped them entirely, which is harmless for a plain restore but leaves the staging directory
missing for a
--resize-clusterrestore, whose helper writes its pipes, oid files and segment TOCsthere.
Verification
Unit tests for each guard, each negative-checked (disabling the guard fails exactly the matching
spec), plus
EnsureBackupDirectoriesExistOnAllHostsspecs asserting the generated commands via thecluster test executor.
Also run end to end on a live WHPG cluster with gpbackup's example plugin, which stores the backup
set outside the cluster in the same flat, host-independent layout the DD Boost plugin produces —
1.34.0-WHPG binaries against this branch:
gprestore --plugin-config "", coordinator directory deletedopen ..._config.yaml: no such file or directory— the reported error, verbatimThe --plugin-config flag was specified with an empty valuegprestore --plugin-config "", segment directory deletedBackup directories missing or inaccessible on 1 segment— the second reported error, verbatimgprestorewith a valid config, segment directory deletedgpbackup --plugin-config "" --leaf-partition-dataBackup completed successfully, files written to segment local disk, nothing on plugin storageWorth stating plainly for reviewers: with a valid
--plugin-config, 1.34.0 already recovered fromboth deleted directories — the plugin recreates the coordinator files via
MustRestoreFile, and thesegment check is skipped for non-single-data-file backups. So the reported ticket is fixed by the
validation and ordering changes; the directory-creation change is robustness — it closes the
--resize-clusterhole above and removes thetest -dprecondition for a rebuilt host.