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

[Josef Bacik]

On Mon, Dec 14, 2009 at 11:31 AM, Mike Bonnet <mikeb at redhat.com> wrote:
> On 12/14/2009 10:34 AM, Josef Bacik wrote:
>> Hello,
>>
>> This is my second pass at a yum plugin for snapshotting filesystems
>> before running a yum transaction.  I've cleaned the plugin up quite a
>> bit since the original posting, and hopefully I've made it easy to add
>> in other snapshotting methods for things like LVM and NILFS2.  This
>> patch is complete with man pages, but the only real option that I've
>> added is an exclude option to exclude certain mount points from being
>> snapshotted.  I've tested this heavily on my system which is btrfs
>> (the only thing this plugin will snapshot currently) and it works
>> fine.  Thanks,
>
> This looks pretty awesome.  A couple of comments below:
>

<snip>

> +def pretrans_hook(conduit):
> +    """
> +    This runs before the transaction starts.  Try to snapshot anything and
> +    everything that is snapshottable, since we do not know what an RPM will
> +    modify (thank you scriptlets).
> +    """
>
> Should this be using /proc/mounts instead?  Could avoid some cases where /etc/mtab doesn't exist or gets out of sync with the actual mounts.
>

/etc/mtab has the real device that was used for mounting.  This is
important for cases where /proc/mounts says /dev/root was mounted on /
when it was /dev/sda1 or whatever.  This matters because doing things
like blkid /dev/root doesn't spit out the same information as doing a
blkid /dev/sda1 somtimes (this is the case with btrfs) and it would
mean that we may end up not snapshotting the fs.  If /etc/mtab doesn't
exist or it's out of sync the user has bigger issues than us missing
some fs's to snapshot :).

> +    if not os.path.exists("/etc/mtab"):
> +        conduit.info(1, "fs-snapshot: could not open /etc/mtab")
> +        pass
> +

<snip>

> +    snapname = dir + "yum-" + time.strftime("%m-%d-%y-%H:%M")
> +    conduit.info(1, "fs-snapshot: snapshotting " + dir + ": " + snapname)
> +    p = Popen(["btrfsctl", "-c", dir], stdout=PIPE, stderr=PIPE)
> +    err = p.stderr.read()
> +    if err:
> +        return 1
>
> You should use p.wait() to check for a non-zero return code from the subprocess, and use that as a failure indicator, along with the stderr output.
>
> +    p = Popen(["btrfsctl", "-s", snapname, dir], stdout=PIPE, stderr=PIPE)
> +    err = p.stderr.read()
> +    if err:
> +        return 1
>
> Same here.
>

Thank you 1000 times.  I was trying to figure out how to do that and I
somehow read over the .wait() stuff in the documentation.  Thanks for
the review.

Josef