Petr Viktorin wrote:
On 03/23/2013 05:06 AM, Rob Crittenden wrote:
There are strict limits on what can be restored where. Only exact
matching hostnames and versions are allowed right now. We can
probably
relax the hostname requirement if we're only restoring data, and
the
version perhaps for only the the first two values (so you can
restore a
3.0.0 backup on 3.0.1 but not on 3.1.0).
Do we also need to limit the versions of Dogtag, 389, Kerberos...?
Or is what they put in /var/lib guaranteed portable across versions?
An interesting question. I'd imagine that a major db change would
also
require a major update to IPA, therefore if we restrict by IPA
version
we should be ok. I AM doing an untar of files though so yeah, there
is a
risk here.
That's good to hear!
I think while developing, you should run with -v to get all the
output.
And for production, please have the commands log by default (set
log_file_name).
Yes, I plan on adding that in the future.
ipa-backup does all its binds using ldapi. ipa-restore needs the DM
password because we need to contact remote servers to
enable/disable
replication.
The restore assumes that ipa is already installed, right? I can't
just
run it on a clean machine? Id would be good to check/document this.
It depends.
For a full backup you can actually restore to an uninstalled
server. In
fact, you could restore to a machine w/no IPA bits on it at all in
all
likelihood (which wouldn't be very nice, but not exactly wrong either
IMHO).
I tested this with:
- ipa-server-install
- ipa-backup
- ipa-server-install --uninstall -U
- ipa-restore
- ran the unit tests
I looked in the backup tarball and saw dirsrv PID file and lock
directories. Are these needed?
Probably not. I gathered some of the files to backup based on
watching
what files that changed during an install that are independent of us.
I'll take another pass through it, there may be other oddities too.
The tool runners (install/tools/ipa-backup and
install/tools/ipa-restore) are missing, so IPA doesn't build.
Probably
just a forgotten git add.
Yup.
The patch adds an extra backslash in install/tools/man/Makefile.am;
consider adding $(NULL) at the end.
I'll take a look.
Backup.dirs, Backup.files, Backup.logs:
The code modifies these class-level attributes. This means you can't
run
the command more than once in one Python session, so it can't be
used as
a library (e.g. in a hypothetical test suite).
Please make the class atrributes immutable (tuples), then in
__init__ do
`self.dirs = list(self.dirs)` to to get instance-specific lists.
Ok, fair point.
Code that creates temporary files/directories or does chdir()
should be
next to (or in) a try block whose finally: restores things.
Yes, I mean to add a big try/finally block around everything in run
eventually (or the equivalent anyway).
Instead of star imports (from ipapython.ipa_log_manager import *),
please explicitly import whatever you're using from that package. In
this case, nothing at all!
Yeah, I haven't run this through pylint yet to see all the bad
imports I
cobbled together.
If there's a get_connection() method, it should return the
connection,
and it should always be used to get it. Store the connection in
self._conn, and rather than:
self.get_connection()
self.conn.do_work()
do:
conn = self.get_connection()
conn.do_work()
This makes forgetting to call get_connection() impossible.
My purpose was to avoid creating multiple connections.
If a variable is only used in a single method, like `filename` in
Restore.extract_backup, don't store it in the admintool object.
In general, try to avoid putting data in self if possible. It's
convenient but it allows complex interdependencies.
(Yes, I'm guilty of not following this myself.)
Yes, there is certainly a bit of cleanup to do. I was in a bit of a
rush
to get this WIP out.
In several places, the backup tool logs critical errors and then
just
continues on. Is that by design? I think a nonzero exit code
would be
appropriate.
I'll take a look at them, all I can say at this point is maybe.
In the ipa-restore man page, --backend is not documented.
In db2ldif, db2bak, etc., a more conscise way to get the time
string is
`time.strftime('export_%Y_%m_%d_%H_%M_%S')`.
When validating --gpg-keyring, it would be good to check both files
(.sec and .pub).
Ok, I can do those.
Here:
if (self.backup_type != 'FULL' and not options.data_only and
not instances):
raise admintool.ScriptError('Cannot restore a data backup
into
an empty system')
did you mean `(self.backup_type != 'FULL' or options.data_only) and
not
instances`? (I'd introduce a `data_only` flag to prevent confusion.)
Yeah, looks like that should be an or.
In the ReplicationManager.check_repl_init reporting: use
int(d.total_seconds()) instead of d.seconds, as the latter doesn't
include full days. I don't think anyone would wait long enough for
the
overflow, but better to be correct.
Sure, I doubt anyone would wait 24 hours either but its a
no-brainer to
make it safe.
