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.

Yeah, it is right on the edge of scope for what I was doing. It would definitely be nice for users. I think the trick would be to be able to discover what kind of install we have, which I guess we can assume based on the existence of the PKI-IPA instance.

I should also note that on uninstall I don't remove any backups. I'm generally pretty cautious when removing user data. Now that I think about it, should we warn users when backups are left over?

The big concern is you install IPA for a test, do a bunch of stuff including a backup, uninstall, install for real, backup, then accidentally restore the wrong one out of confusion. I suppose you could restore the right one later, but this could get wierd.


The spec changelog needs a small rebase.

Yeah, a couple of commits right after mine were done. I'll fix it up for the next candidate patch.

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).


thanks

rob

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to