From a2b28623d42d1979d91bc112a411588d8996a66b Mon Sep 17 00:00:00 2001 From: David Steele Date: Mon, 28 Apr 2014 09:13:25 -0400 Subject: [PATCH] Working on better error handling. --- README.md | 8 ++- pg_backrest.pl | 41 ++++++++-------- pg_backrest_file.pm | 107 +++++++++++++++++++++++++++++++++-------- pg_backrest_utility.pm | 26 +++++++--- 4 files changed, 132 insertions(+), 50 deletions(-) diff --git a/README.md b/README.md index d2881d9ad..226d96c49 100644 --- a/README.md +++ b/README.md @@ -14,11 +14,17 @@ Simple Postgres Backup and Restore ## feature backlog +* Capture SDTERR in file functions - start with file_list_get(). + +* Move backups to be removed to temp before deleting. + +* Async archive-get. + * Database restore. * --version param (with VERSION file written to directory). -* Move backups to be removed to temp before deleting. +* Threading for archive-get and archive-put. ## release notes diff --git a/pg_backrest.pl b/pg_backrest.pl index ff6ee561a..d8afe5fe3 100755 --- a/pg_backrest.pl +++ b/pg_backrest.pl @@ -352,9 +352,9 @@ if ($strOperation eq OP_ARCHIVE_PUSH || $strOperation eq OP_ARCHIVE_PULL) } # Create a lock file to make sure archive-pull does not run more than once - my $strLockFile = "${strArchivePath}/lock/${strStanza}-archive.lock"; + my $strLockPath = "${strArchivePath}/lock/${strStanza}-archive.lock"; - if (!lock_file_create($strLockFile)) + if (!lock_file_create($strLockPath)) { &log(DEBUG, "archive-pull process is already running - exiting"); exit 0 @@ -381,7 +381,8 @@ if ($strOperation eq OP_ARCHIVE_PUSH || $strOperation eq OP_ARCHIVE_PULL) strCommandChecksum => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_CHECKSUM, $bChecksum), strCommandCompress => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_COMPRESS, $bCompress), strCommandDecompress => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_DECOMPRESS, $bCompress), - strCommandManifest => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_MANIFEST) + strCommandManifest => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_MANIFEST), + strLockPath => $strLockPath ); backup_init @@ -484,6 +485,15 @@ elsif ($strType ne "full" && $strType ne "differential" && $strType ne "incremen my $bCompress = config_load(CONFIG_SECTION_BACKUP, CONFIG_KEY_COMPRESS, true, "y") eq "y" ? true : false; my $bChecksum = config_load(CONFIG_SECTION_BACKUP, CONFIG_KEY_CHECKSUM, true, "y") eq "y" ? true : false; +# Set the lock path +my $strLockPath = config_load(CONFIG_SECTION_BACKUP, CONFIG_KEY_PATH, true) . "/lock/${strStanza}-${strOperation}.lock"; + +if (!lock_file_create($strLockPath)) +{ + &log(ERROR, "backup process is already running for stanza ${strStanza} - exiting"); + exit 0 +} + # Run file_init_archive - the rest of the file config required for backup and restore my $oFile = pg_backrest_file->new ( @@ -498,7 +508,8 @@ my $oFile = pg_backrest_file->new strCommandCompress => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_COMPRESS, $bCompress), strCommandDecompress => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_DECOMPRESS, $bCompress), strCommandManifest => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_MANIFEST), - strCommandPsql => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_PSQL) + strCommandPsql => config_load(CONFIG_SECTION_COMMAND, CONFIG_KEY_PSQL), + strLockPath => $strLockPath ); my $oDb = pg_backrest_db->new @@ -527,20 +538,16 @@ backup_init #################################################################################################################################### if ($strOperation eq OP_BACKUP) { - my $strLockFile = $oFile->path_get(PATH_BACKUP, "lock/${strStanza}-backup.lock"); - - if (!lock_file_create($strLockFile)) - { - &log(ERROR, "backup process is already running for stanza ${strStanza} - exiting"); - exit 0 - } - backup(config_load(CONFIG_SECTION_STANZA, CONFIG_KEY_PATH), config_load(CONFIG_SECTION_BACKUP, CONFIG_KEY_START_FAST, true, "n") eq "y" ? true : false); $strOperation = OP_EXPIRE; - lock_file_remove(); +# my %hash = $oFile->manifest_get(PATH_DB_ABSOLUTE, "/Users/dsteele/pg_backrest"); +# print "hash " . %hash . "\n"; + +# lock_file_remove(); +# exit 0; } #################################################################################################################################### @@ -548,14 +555,6 @@ if ($strOperation eq OP_BACKUP) #################################################################################################################################### if ($strOperation eq OP_EXPIRE) { - my $strLockFile = $oFile->path_get(PATH_BACKUP, "lock/${strStanza}-expire.lock"); - - if (!lock_file_create($strLockFile)) - { - &log(ERROR, "expire process is already running for stanza ${strStanza} - exiting"); - exit 0 - } - backup_expire ( $oFile->path_get(PATH_BACKUP_CLUSTER), diff --git a/pg_backrest_file.pm b/pg_backrest_file.pm index 398143148..16db49d90 100644 --- a/pg_backrest_file.pm +++ b/pg_backrest_file.pm @@ -32,6 +32,13 @@ has strCommandDecompress => (is => 'bare'); has strCommandCat => (is => 'bare', default => 'cat %file%'); has strCommandManifest => (is => 'bare'); +# Lock path +has strLockPath => (is => 'bare'); + +# Files to hold stderr +#has strBackupStdErrFile => (is => 'bare'); +#has strDbStdErrFile => (is => 'bare'); + # Module variables has strDbUser => (is => 'bare'); # Database user has strDbHost => (is => 'bare'); # Database host @@ -48,6 +55,21 @@ has bNoCompression => (is => 'bare'); has strStanza => (is => 'bare'); has iThreadIdx => (is => 'bare'); +#################################################################################################################################### +# PATH_GET Constants +#################################################################################################################################### +use constant +{ + PATH_DB => 'db', + PATH_DB_ABSOLUTE => 'db:absolute', + PATH_BACKUP => 'backup', + PATH_BACKUP_ABSOLUTE => 'backup:absolute', + PATH_BACKUP_CLUSTER => 'backup:cluster', + PATH_BACKUP_TMP => 'backup:tmp', + PATH_BACKUP_ARCHIVE => 'backup:archive', + PATH_LOCK_ERR => 'lock:err' +}; + #################################################################################################################################### # CONSTRUCTOR #################################################################################################################################### @@ -79,9 +101,10 @@ sub BUILD if (!defined($self->{oBackupSSH}) && defined($self->{strBackupHost})) { &log(TRACE, "connecting to backup ssh host " . $self->{strBackupHost}); - - # !!! This could be improved by redirecting stderr to a file to get a better error message + $self->{oBackupSSH} = Net::OpenSSH->new($self->{strBackupHost}, timeout => 300, user => $self->{strBackupUser}, +# master_stderr_file => $self->path_get(PATH_LOCK_ERR, "file"), + default_stderr_file => $self->path_get(PATH_LOCK_ERR, "file"), master_opts => [-o => $strOptionSSHCompression, -o => $strOptionSSHRequestTTY]); $self->{oBackupSSH}->error and confess &log(ERROR, "unable to connect to $self->{strBackupHost}: " . $self->{oBackupSSH}->error); } @@ -91,14 +114,26 @@ sub BUILD { &log(TRACE, "connecting to database ssh host $self->{strDbHost}"); - # !!! This could be improved by redirecting stderr to a file to get a better error message $self->{oDbSSH} = Net::OpenSSH->new($self->{strDbHost}, timeout => 300, user => $self->{strDbUser}, +# master_stderr_file => $self->path_get(PATH_LOCK_ERR, "file"), + default_stderr_file => $self->path_get(PATH_LOCK_ERR, "file"), master_opts => [-o => $strOptionSSHCompression, -o => $strOptionSSHRequestTTY]); $self->{oDbSSH}->error and confess &log(ERROR, "unable to connect to $self->{strDbHost}: " . $self->{oDbSSH}->error); } } } +#################################################################################################################################### +# LOCK_PATH_SET +#################################################################################################################################### +#sub lock_path_set +#{ +# my $self = shift; +# my $strLockPathParam = shift; +# +# $self->{strLockPath} = $strLockPathParam; +#} + #################################################################################################################################### # CLONE #################################################################################################################################### @@ -127,24 +162,31 @@ sub clone strBackupClusterPath => $self->{strBackupClusterPath}, bNoCompression => $self->{bNoCompression}, strStanza => $self->{strStanza}, - iThreadIdx => $iThreadIdx + iThreadIdx => $iThreadIdx, + strLockPath => $self->{strLockPath} ); } +#################################################################################################################################### +# ERROR_GET +#################################################################################################################################### +sub error_get +{ + my $self = shift; + + my $strErrorFile = $self->path_get(PATH_LOCK_ERR, "file"); + + open my $hFile, '<', $strErrorFile or return "error opening ${strErrorFile} to read STDERR output"; + + my $strError = do {local $/; <$hFile>}; + close $hFile; + + return trim($strError); +} + #################################################################################################################################### # PATH_GET #################################################################################################################################### -use constant -{ - PATH_DB => 'db', - PATH_DB_ABSOLUTE => 'db:absolute', - PATH_BACKUP => 'backup', - PATH_BACKUP_ABSOLUTE => 'backup:absolute', - PATH_BACKUP_CLUSTER => 'backup:cluster', - PATH_BACKUP_TMP => 'backup:tmp', - PATH_BACKUP_ARCHIVE => 'backup:archive' -}; - sub path_type_get { my $self = shift; @@ -216,6 +258,15 @@ sub path_get } # Get the backup tmp path + if ($strType eq PATH_LOCK_ERR) + { + my $strTempPath = "$self->{strLockPath}"; + + return ${strTempPath} . (defined($strFile) ? "/${strFile}" . + (defined($self->{iThreadIdx}) ? ".$self->{iThreadIdx}" : "") . ".err" : ""); + } + + # Get the backup tmp error path if ($strType eq PATH_BACKUP_TMP) { my $strTempPath = "$self->{strBackupPath}/temp/$self->{strStanza}.tmp"; @@ -774,8 +825,8 @@ sub file_list_get my $strPathList = $self->path_get($strPathType, $strPath); # Builds the file list command -# my $strCommand = "ls ${strPathList} | egrep \"$strExpression\" 2> /dev/null"; - my $strCommand = "ls -1 ${strPathList} 2> /dev/null"; +# my $strCommand = "ls ${strPathList} | egrep \"$strExpression\""; + my $strCommand = "ls -1 ${strPathList}"; # Run the file list command my $strFileList = ""; @@ -787,16 +838,30 @@ sub file_list_get my $oSSH = $self->remote_get($strPathType); $strFileList = $oSSH->capture($strCommand); + + if ($oSSH->error) + { + confess &log(ERROR, "unable to execute file list (${strCommand}): " . $self->error_get()); + } } # Run locally else { &log(TRACE, "file_list_get: local ${strPathType}:${strPathList} ${strCommand}"); - $strFileList = capture($strCommand) or confess("error in ${strCommand}"); + $strFileList = capture($strCommand); } # Split the files into an array - my @stryFileList = grep(/$strExpression/i, split(/\n/, $strFileList)); + my @stryFileList; + + if (defined($strExpression)) + { + @stryFileList = grep(/$strExpression/i, split(/\n/, $strFileList)); + } + else + { + @stryFileList = split(/\n/, $strFileList); + } # Return the array in reverse order if specified if (defined($strSortOrder) && $strSortOrder eq "reverse") @@ -903,7 +968,6 @@ sub manifest_get # Builds the manifest command my $strCommand = $self->{strCommandManifest}; $strCommand =~ s/\%path\%/${strPathManifest}/g; - $strCommand .= " 2> /dev/null"; # Run the manifest command my $strManifest; @@ -914,7 +978,8 @@ sub manifest_get &log(TRACE, "manifest_get: remote ${strPathType}:${strPathManifest}"); my $oSSH = $self->remote_get($strPathType); - $strManifest = $oSSH->capture($strCommand) or confess &log(ERROR, "unable to execute remote command '${strCommand}'"); + $strManifest = $oSSH->capture($strCommand) or + confess &log(ERROR, "unable to execute remote manifest (${strCommand}): " . $self->error_get()); } # Run locally else diff --git a/pg_backrest_utility.pm b/pg_backrest_utility.pm index 430c33708..259a9e5b9 100644 --- a/pg_backrest_utility.pm +++ b/pg_backrest_utility.pm @@ -10,6 +10,7 @@ use warnings; use Carp; use IPC::System::Simple qw(capture); use Fcntl qw(:DEFAULT :flock); +use File::Path qw(remove_tree); use Exporter qw(import); @@ -41,7 +42,7 @@ my $strLogLevelFile = ERROR; my $strLogLevelConsole = ERROR; my %oLogLevelRank; -my $strLockFile; +my $strLockPath; my $hLockFile; $oLogLevelRank{TRACE}{rank} = 6; @@ -57,13 +58,23 @@ $oLogLevelRank{OFF}{rank} = 0; #################################################################################################################################### sub lock_file_create { - my $strLockFileParam = shift; - - $strLockFile = $strLockFileParam; + my $strLockPathParam = shift; + + my $strLockFile = $strLockPathParam . "/process.lock"; if (defined($hLockFile)) { - confess &lock(ASSERT, "${strLockFile} lock is already held, cannot create lock ${strLockFile}"); + confess &lock(ASSERT, "${strLockFile} lock is already held"); + } + + $strLockPath = $strLockPathParam; + + unless (-e $strLockPath) + { + if (system("mkdir -p ${strLockPath}") != 0) + { + confess &log(ERROR, "Unable to create lock path ${strLockPath}"); + } } sysopen($hLockFile, $strLockFile, O_WRONLY | O_CREAT) @@ -86,10 +97,11 @@ sub lock_file_remove if (defined($hLockFile)) { close($hLockFile); - unlink($strLockFile) or confess &log(ERROR, "unable to remove lock file ${strLockFile}"); + + remove_tree($strLockPath) or confess &log(ERROR, "unable to delete lock path ${strLockPath}"); $hLockFile = undef; - $strLockFile = undef; + $strLockPath = undef; } else {