[Freeipa-devel] [PATCH] WIP backup and restore

[Rob Crittenden]

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.

rob