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

Wed Apr 10 18:27:21 UTC 2013

Petr Viktorin wrote:
> On 04/09/2013 11:21 PM, Rob Crittenden wrote:
>> Petr Viktorin wrote:
>>> On 04/05/2013 10:54 PM, Rob Crittenden wrote:
>>>> Petr Viktorin wrote:
>>>>> 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.
>>>>
>>>> I went ahead and made this change, it wasn't a lot of work.
>>>>
>>>>>
>>>>>
>>>>>
>>>>> The spec changelog needs a small rebase.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> 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).
>>>>>
>>>>
>>>> rob
>>>
>>> I found a bug with the following topology:
>>>
>>> [IPA 2.2] <-> [IPA 3.2 upgraded from 2.2] <-> [IPA 3.2]
>>>
>>> Running ipa-restore on the 3.2 instance tries to disable the CA
>>> replication agreement between the other two.
>>> However, deep in ReplicationManager.agreement_dn it uses its own DS
>>> instance name instead of the Dogtag-9-DS one, so the agreement isn't
>>> found and the restore fails.
>>>
>>>
>>
>> Yeah, I'm not 100% sure how to address that either. Is it safe to assume
>> that if the port is 7389 then the instance is pki-ca?
>
> The port was hardcoded so it is.

Ok, just thanks for the other set of eyes. Updated patch attached.

rob

>
>> Or should we test
>> for the existence of the instance name like we do master/clone?
>>
>> I've also figured out why Update in Progress loops forever. It is
>> because the older ipa-replica-manage do not re-enable the replication
>> agreement, so replication can't continue. I'm not entirely sure how we
>> will need to go about fixing this, except through documentation.
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1093-4-backup.patch
Type: text/x-patch
Size: 78709 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130410/51f1502f/attachment.bin>