[Yum-devel] [PATCH] add the btrfs-snapshot plugin to yum-utils

[Josef Bacik]

On Thu, Dec 10, 2009 at 12:37 AM, James Antill <james at fedoraproject.org> wrote:
> On Wed, 2009-12-09 at 17:14 -0500, Josef Bacik wrote:
>> Hello,
>>
>> This is my first yum plugin and maybe my second time writing python,
>> so any comments would be greatly appreciated.  This plugin simply
>> takes any directory that a package is going to mess with that belongs
>> to a btrfs filesystem while updating/removing the package and takes a
>> snapshot of that btrfs filesystem.  The idea here is that if something
>> catastrophic goes wrong, we can always boot into an older snapshot and
>> try and pick up the pieces.  I've tested this plugin on my box with a
>> simple package removal and the resulting snapshot is clean, the rpmdb
>> is good and everything is kosher.  Thanks,
>
>  Mostly cosmetic stuff:
>
>
> 1. Don't do init stuff in init_hook :). Because your plugin only ever
> needs to do anything when we run a transaction, delay everything until
> at least pretrans_hook time.
>
> 2. In python:
>
>            dirlist = dirlist + [mntpnt]
>
> ...is done as:
>
>            dirlist.append(mntpnt)
>
> 3. Why do you only care about update/erase packages? Install packages
> will run scriplets etc. ... and TS_OBSOLETING is a synonym for TS_UPDATE
> (and most everything else :) dito. TS_OBSOLETED vs. TS_ERASE.
>  Also in _probably rare_ cases TS_UPDATED packages could have files in
> very different places than their TS_UPDATING counterparts.
>

The idea was that installing a new application isn't going to cause
anything else to regress, but I forgot about scriptlets, I will change
this.

> 4. Accessing ts.po.dirlist for a "remote" package means the user needs
> to download the filelist MD. This isn't easy to fix :(.
>

I guess the same thing goes for po.filelist and such right?

> 5. You don't take bind() mounts into account
>

Somebody always has to bring up bind() mounts ;).  I will see what can
be done about this.

> 6. Opt: Presort dirlist by len(), then you can break on the first match.
>
> 7. snaplist.index(); del == snaplist.remove() (or even better, use a
> set() and do snaplist.del()).
>
> 8. I can see why you're doing the scan, and only snapshoting "what will
> change", but I think this can only lead to madness:
>
>        i. Lots of other stuff goes on, Eg. yum history, yumdb, rpmdb.
>        ii. Even if you account for all of that ... scriplets can (and
>        do) do anything, anywhere.
>
> ...you want this to be a non-default option, I think, and just snapshot
> all of snaplist by default. But that's IMHO.
>

So the idea here is if the user setup a different subvolume for /home
that they don't necessarily want /home snapshotted every time they do
an update.  Packages shouldn't be touching users home directory right?
 I guess it wouldn't be terrible to do everything, and it would
certainly make things simpler, I'll have to think about it.

> 9. Does it output anything on the error path?
>

Hmm no, I wanted to be as not verbose as possible so users wouldn't
freak out if they saw errors right before their transaction ran, but I
guess we should indicate if there was a failure.

> 10. Call it something generic like "fs-snapshot" or something, even if
> it only works for btrfs atm.
>

Agreed.

>
> 99. Man pages FTW ;) -- esp. if you create a config. option.
>

Sounds good.  Thanks for the feedback.  Unfortunately my testbox is
also my main workstation, which is having other issues at the moment.
I will hopefully have this all fixed up and re-sent tomorrow.  Thank
you,

Josef