Dne 13.1.2015 v 13:01 Petr Vobornik napsal(a):
On 01/12/2015 02:28 PM, Jan Cholasta wrote:
Hi,
the attached patch fixes <https://fedorahosted.org/freeipa/ticket/4797>.
Note that --data with data-only backup and --logs-only with data-only
restore are deliberately ignored and considered no-op.
Honza
1. I'm not sure how relative path to backup dir should work:
I have a backup: /var/lib/ipa/backup/ipa-data-2015-01-13-10-53-06/
Absolute path works great. But:
- ipa-data-2015-01-13-10-53-06
fails with `ipa-restore: error: must provide path to backup directory`
Ugly hybrid (I was in home dir):
../../var/lib/ipa/backup/ipa-data-2015-01-13-10-53-06/
fails with:
/var/lib/ipa/backup/../../var/lib/ipa/backup/ipa-data-2015-01-13-10-53-06/header
I.e., dir name won't pass
if not os.path.isdir(self.args[0]):
and the second concatenation is weird.
man ipa-restore says:
Only the name of the backup needs to be passed in, not the full
path. Backups are stored in a subdirectory in /var/lib/ipa/backup. If a
backup is in another location then the full path must be provided.
This was validated wrong, fixed.
2. Data backup cannot be done because the first 'for' never breaks
+ for instance in instances:
+ for backend in backends:
+ db_dir = (paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
+ (instance, backend))
+ if os.path.exists(db_dir):
+ break
+ else:
+ raise admintool.ScriptError(
+ "Cannot restore a data backup into an empty
system")
Oops, fixed.
3. When #2 fixed, data backup with --no-logs doesn't raise warning as
requested in ticket.
IMO such a warning does not make sense. You request no logs, you get no
logs, I don't see anything worth warning about here.
4. Since backup type is detected automatically(--data doesn't have to be
specified for data restore), I guess that the ticket requirement: `Here
expecting an error message that --online option can only provided along
with --data option. ` is invalid, right?
Man page states that --online requires --data option, which is not true.
Fixed in man page.
5. Nitpick: imho option validation should be done before temp dir creation.
Fixed.
6. pep8, fixing them is up to you:
./ipaserver/install/ipa_restore.py:150:5: E129 visually indented line
with same indent as next logical line
./ipaserver/install/ipa_restore.py:184:13: E128 continuation line
under-indented for visual indent
Fixed.
Updated patch attached.
--
Jan Cholasta
>From 44801a195e293b790fc7ec08bff04e8d0cf29787 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Mon, 12 Jan 2015 12:44:21 +0000
Subject: [PATCH] Fix validation of ipa-restore options
Fix restore mode checks. Do some of the existing checks earlier to make them
effective. Check if --instance and --backend exist both in the filesystem and
in the backup.
Log backup type and restore mode before performing restore.
Update ipa-restore man page.
https://fedorahosted.org/freeipa/ticket/4797
---
install/tools/man/ipa-restore.1 | 8 +-
ipaserver/install/ipa_restore.py | 176 +++++++++++++++++++++++----------------
2 files changed, 108 insertions(+), 76 deletions(-)
diff --git a/install/tools/man/ipa-restore.1 b/install/tools/man/ipa-restore.1
index d758490..5f40181 100644
--- a/install/tools/man/ipa-restore.1
+++ b/install/tools/man/ipa-restore.1
@@ -65,16 +65,16 @@ Restore the data only. The default is to restore everything in the backup.
The full path to a GPG keyring. The keyring consists of two files, a public and a private key (.sec and .pub respectively). Specify the path without an extension.
.TP
\fB\-\-no\-logs\fR
-Exclude the IPA service log files in the backup (if they were backed up). Applicable only with a full backup.
+Exclude the IPA service log files in the backup (if they were backed up).
.TP
\fB\-\-online\fR
-Perform the restore on\-line. Requires the \-\-data option.
+Perform the restore on\-line. Requires data\-only backup or the \-\-data option.
.TP
\fB\-\-instance\fR=\fIINSTANCE\fR
-Restore only the databases in this 389\-ds instance. The default is to restore all found (at most this is the IPA REALM instance and the PKI\-IPA instance).
+Restore only the databases in this 389\-ds instance. The default is to restore all found (at most this is the IPA REALM instance and the PKI\-IPA instance). Requires data\-only backup or the \-\-data option.
.TP
\fB\-\-backend\fR=\fIBACKEND\fR
-The backend to restore within an instance or instances.
+The backend to restore within an instance or instances. Requires data\-only backup or the \-\-data option.
.TP
\fB\-\-v\fR, \fB\-\-verbose\fR
Print debugging information
diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py
index 0977039..dbebd83 100644
--- a/ipaserver/install/ipa_restore.py
+++ b/ipaserver/install/ipa_restore.py
@@ -24,6 +24,7 @@ import tempfile
import time
import pwd
from ConfigParser import SafeConfigParser
+import itertools
from ipalib import api, errors
from ipapython import version, ipautil, certdb, dogtag
@@ -134,32 +135,25 @@ class Restore(admintool.AdminTool):
def validate_options(self):
+ parser = self.option_parser
options = self.options
super(Restore, self).validate_options(needs_root=True)
- if options.data_only:
- installutils.check_server_configuration()
if len(self.args) < 1:
- self.option_parser.error(
- "must provide the backup to restore")
+ parser.error("must provide the backup to restore")
elif len(self.args) > 1:
- self.option_parser.error(
- "must provide exactly one name for the backup")
+ parser.error("must provide exactly one name for the backup")
dirname = self.args[0]
if not os.path.isabs(dirname):
- self.backup_dir = os.path.join(BACKUP_DIR, dirname)
- else:
- self.backup_dir = dirname
-
+ dirname = os.path.join(BACKUP_DIR, dirname)
if not os.path.isdir(dirname):
- raise self.option_parser.error("must provide path to backup directory")
+ parser.error("must provide path to backup directory")
if options.gpg_keyring:
if (not os.path.exists(options.gpg_keyring + '.pub') or
- not os.path.exists(options.gpg_keyring + '.sec')):
- raise admintool.ScriptError('No such key %s' %
- options.gpg_keyring)
+ not os.path.exists(options.gpg_keyring + '.sec')):
+ parser.error("no such key %s" % options.gpg_keyring)
def ask_for_options(self):
@@ -185,38 +179,89 @@ class Restore(admintool.AdminTool):
api.bootstrap(in_server=False, context='restore')
api.finalize()
+ self.backup_dir = self.args[0]
+ if not os.path.isabs(self.backup_dir):
+ self.backup_dir = os.path.join(BACKUP_DIR, self.backup_dir)
+
self.log.info("Preparing restore from %s on %s",
- self.backup_dir, api.env.host)
+ self.backup_dir, api.env.host)
- if options.instance and options.backend:
- database = (options.instance, options.backend)
- if not os.path.exists(paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
- database):
- raise admintool.ScriptError(
- "Instance %s with backend %s does not exist" % database)
+ try:
+ self.read_header()
+ except IOError as e:
+ raise admintool.ScriptError("Cannot read backup metadata: %s" % e)
- databases = [database]
+ if options.data_only:
+ restore_type = 'DATA'
+ else:
+ restore_type = self.backup_type
+
+ instances = [realm_to_serverid(api.env.realm), 'PKI-IPA']
+ backends = ['userRoot', 'ipaca']
+
+ # These checks would normally be in the validate method but
+ # we need to know the type of backup we're dealing with.
+ if restore_type == 'FULL':
+ if options.online:
+ raise admintool.ScriptError(
+ "File restoration cannot be done online")
+ if options.instance or options.backend:
+ raise admintool.ScriptError(
+ "Restore must be in data-only mode when restoring a "
+ "specific instance or backend")
else:
+ installutils.check_server_configuration()
+
if options.instance:
+ instance_dir = (paths.VAR_LIB_SLAPD_INSTANCE_DIR_TEMPLATE %
+ options.instance)
+ if not os.path.exists(instance_dir):
+ raise admintool.ScriptError(
+ "Instance %s does not exist" % options.instance)
+
instances = [options.instance]
- else:
- instances = [realm_to_serverid(api.env.realm), 'PKI-IPA']
if options.backend:
+ for instance in instances:
+ db_dir = (paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
+ (instance, options.backend))
+ if os.path.exists(db_dir):
+ break
+ else:
+ raise admintool.ScriptError(
+ "Backend %s does not exist" % options.backend)
+
backends = [options.backend]
+
+ for instance, backend in itertools.product(instances, backends):
+ db_dir = (paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
+ (instance, backend))
+ if os.path.exists(db_dir):
+ break
else:
- backends = ['userRoot', 'ipaca']
+ raise admintool.ScriptError(
+ "Cannot restore a data backup into an empty system")
- databases = []
- for instance in instances:
- for backend in backends:
- database = (instance, backend)
- if os.path.exists(paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
- database):
- databases.append(database)
+ self.log.info("Performing %s restore from %s backup" %
+ (restore_type, self.backup_type))
+
+ if self.backup_host != api.env.host:
+ self.log.warning(
+ "Host name %s does not match backup name %s" %
+ (api.env.host, self.backup_host))
+ if (not options.unattended and
+ not user_input("Continue to restore?", False)):
+ raise admintool.ScriptError("Aborted")
- if options.data_only and not databases:
- raise admintool.ScriptError('No instances to restore to')
+ if self.backup_ipa_version != str(version.VERSION):
+ self.log.warning(
+ "Restoring data from a different release of IPA.\n"
+ "Data is version %s.\n"
+ "Server is running %s." %
+ (self.backup_ipa_version, str(version.VERSION)))
+ if (not options.unattended and
+ not user_input("Continue to restore?", False)):
+ raise admintool.ScriptError("Aborted")
create_ds_user()
pent = pwd.getpwnam(DS_USER)
@@ -236,42 +281,31 @@ class Restore(admintool.AdminTool):
try:
dirsrv = services.knownservices.dirsrv
- try:
- self.read_header()
- except IOError as e:
- raise admintool.ScriptError('Cannot read backup metadata: %s' % e)
- # These two checks would normally be in the validate method but
- # we need to know the type of backup we're dealing with.
- if (self.backup_type != 'FULL' and not options.data_only and
- not databases):
- raise admintool.ScriptError('Cannot restore a data backup into an empty system')
- if (self.backup_type == 'FULL' and not options.data_only and
- (options.instance or options.backend)):
- raise admintool.ScriptError('Restore must be in data-only mode when restoring a specific instance or backend.')
- if self.backup_host != api.env.host:
- self.log.warning('Host name %s does not match backup name %s' %
- (api.env.host, self.backup_host))
- if (not options.unattended and
- not user_input("Continue to restore?", False)):
- raise admintool.ScriptError("Aborted")
- if self.backup_ipa_version != str(version.VERSION):
- self.log.warning(
- "Restoring data from a different release of IPA.\n"
- "Data is version %s.\n"
- "Server is running %s." %
- (self.backup_ipa_version, str(version.VERSION)))
- if (not options.unattended and
- not user_input("Continue to restore?", False)):
- raise admintool.ScriptError("Aborted")
-
self.extract_backup(options.gpg_keyring)
- for database in databases:
- ldifname = '%s-%s.ldif' % database
- ldiffile = os.path.join(self.dir, ldifname)
- if not os.path.exists(ldiffile):
+ databases = []
+ for instance in instances:
+ for backend in backends:
+ database = (instance, backend)
+ ldiffile = os.path.join(self.dir, '%s-%s.ldif' % database)
+ if os.path.exists(ldiffile):
+ databases.append(database)
+
+ if options.instance:
+ for instance, backend in databases:
+ if instance == options.instance:
+ break
+ else:
+ raise admintool.ScriptError(
+ "Instance %s not found in backup" % options.instance)
+
+ if options.backend:
+ for instance, backend in databases:
+ if backend == options.backend:
+ break
+ else:
raise admintool.ScriptError(
- "Instance %s with backend %s not in backup" % database)
+ "Backend %s not found in backup" % options.backend)
# Big fat warning
if (not options.unattended and
@@ -290,7 +324,7 @@ class Restore(admintool.AdminTool):
self.log.info("Disabling all replication.")
self.disable_agreements()
- if options.data_only:
+ if restore_type != 'FULL':
if not options.online:
self.log.info('Stopping Directory Server')
dirsrv.stop(capture_output=False)
@@ -307,11 +341,9 @@ class Restore(admintool.AdminTool):
# We do either a full file restore or we restore data.
- if self.backup_type == 'FULL' and not options.data_only:
+ if restore_type == 'FULL':
if 'CA' in self.backup_services:
create_ca_user()
- if options.online:
- raise admintool.ScriptError('File restoration cannot be done online.')
self.cert_restore_prepare()
self.file_restore(options.no_logs)
self.cert_restore()
@@ -326,7 +358,7 @@ class Restore(admintool.AdminTool):
for instance, backend in databases:
self.ldif2db(instance, backend, online=options.online)
- if options.data_only:
+ if restore_type != 'FULL':
if not options.online:
self.log.info('Starting Directory Server')
dirsrv.start(capture_output=False)
--
2.1.0
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
