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

[Petr Viktorin]

On 04/04/2013 03:04 PM, Rob Crittenden wrote:
> Rob Crittenden wrote:
>> 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.
>
> I think I've addressed just about everything.
>
> The reason that /var/run/dirsrv and var/lock/dirsrv are included in the
> backup is for the case of a full restore. These directories are not
> created/provided by the package itself, but by installing the first
> instance, so we need the ownership/SELinux context preserved.
>
> One thing I need tested is restoring over top of new data with multiple
> replicas. So basically install, do a backup, add a bunch of data,
> restore, re-initialize all the other masters and then confirm that ALL
> the new data is gone. I think I've got the sequence right but this is
> critical.
>
> This should work on fresh installs and upgrades from 3.0 (e.g. dogtag
> 9/multi-instance DS configurations).
>
> One thing I tested was:
>
> - ipa-server-install
> - ipa-backup
> - ipa-server-install --uninstall -U
> - ipa-restore ipa-full-...
>
> This will actually get your server back EXCEPT that dogtag won't start.
> This is because the log directories are created by the instance
> creation. There are two solutions: backup with --logs or manually
> re-create the log directories (there are several, depending on dogtag
> version).

Could backup without --logs save which directories are there, and 
restore create empty directories if they don't exist? To me 
re-initializing a completely wiped machine looks like a big gain for the 
extra effort.



The spec changelog needs a small rebase.

I've tested some scenarios and didn't find any other issues so far.

I still want to test some more with upgraded masters; installing them 
takes some time.
Also I still need to test CA replicas (with the DNAME patches removed).

-- 
Petr³