[linux-lvm] [PATCH 0/2] yum-utils fs snapshot plugin: fix old LVM snapshots and add thinp support

[linux-lvm] [PATCH 0/2] yum-utils fs snapshot plugin: fix old LVM snapshots and add thinp support

[Mike Snitzer]

Hi,

I'd really appreciate it if someone with commit access to yum-utils
could both review and then commit these changes for me.  I'd like to
see these changes committed in time for the F19 feature freeze this
Tuesday (Mar 12), as this work is a prereq for the following F19
feature:
https://fedoraproject.org/wiki/Features/YumFsSnapshotThinpSupport

Ondrej Kozina may also offer an additional patch to have fs-snapshot
use the snapper utility [1] to create the snapshots (by Tuesday) but
that is still TBD.

While implementing snapshot support for thinly provisioned LVM volumes
I quickly noticed that the older LVM snapshot no longer works.  This
was due to a regression in commandline processing in "dmsetup
splitname".  So the first patch addresses this regression.

The second patch adds support for thinly provisioned LVM volumes.
Please see each patch's headers for more details.

Thanks,
Mike

[1] http://en.opensuse.org/Portal:Snapper

Mike Snitzer (2):
  fs snapshot plugin: fix inspect_volume_lvm to use supported dmsetup splitname options
  fs snapshot plugin: add support for snapshotting thinly provisioned LVM volumes

 docs/yum-fs-snapshot.conf.5        |  3 ++-
 plugins/fs-snapshot/fs-snapshot.py | 34 +++++++++++++++++++++-------------
 2 files changed, 23 insertions(+), 14 deletions(-)

-- 
1.8.1

[linux-lvm] [PATCH 1/2] fs snapshot plugin: fix inspect_volume_lvm to use supported dmsetup splitname options

[Mike Snitzer]

dmsetup splitname -o vg_name,lv_name no longer works (fails with "Option
not recognised").  An additional -c argument is required by newer lvm2;
this is an lvm2 regression but moving forward -c -o vg_name,lv_name will
work as intended.

Signed-off-by: Mike Snitzer <msnitzer@fedoraproject.org>
---
 plugins/fs-snapshot/fs-snapshot.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/fs-snapshot/fs-snapshot.py b/plugins/fs-snapshot/fs-snapshot.py
index ef4afe8..2a7c65d 100644
--- a/plugins/fs-snapshot/fs-snapshot.py
+++ b/plugins/fs-snapshot/fs-snapshot.py
@@ -80,7 +80,7 @@ def inspect_volume_lvm(conduit, volume):
         # convert /dev/mapper name to /dev/vg/lv for use with LVM2 tools
         # - 'dmsetup splitname' will collapse any escaped characters
         p = Popen(["/sbin/dmsetup", "splitname", "--separator", "/",
-                   "--noheadings",
+                   "--noheadings", "-c",
                    "-o", "vg_name,lv_name", device], stdout=PIPE, stderr=PIPE)
         err = p.wait()
         if err:
-- 
1.8.1

[linux-lvm] [PATCH 2/2] fs snapshot plugin: add support for snapshotting thinly provisioned LVM volumes

[Mike Snitzer]

Thinly provisioned LVM volumes offer more scalable snapshots than the
original LVM snapshots.  Thinly provisioned snapshots use a single
shared pool of storage (aka thin-pool) so there is no need to require
the user to have configured "lvcreate_size_args" in the
yum-fs-snapshot.conf for them.

When inspecting an LVM volume we now record the LVM "segtype" in the
volume dict.  is_thin_volume() was introduced to simplify checking
whether an LVM volume is thinly provisioned or not.

Signed-off-by: Mike Snitzer <msnitzer@fedoraproject.org>
---
 docs/yum-fs-snapshot.conf.5        |  3 ++-
 plugins/fs-snapshot/fs-snapshot.py | 32 ++++++++++++++++++++------------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/docs/yum-fs-snapshot.conf.5 b/docs/yum-fs-snapshot.conf.5
index a4b4f02..dc5c0c1 100644
--- a/docs/yum-fs-snapshot.conf.5
+++ b/docs/yum-fs-snapshot.conf.5
@@ -28,7 +28,8 @@ created for filesystems built on LVM logical volumes.
 .IP lvcreate_size_args
 This is the space delimited lvcreate argument list that is used to
 specify the size of the snapshot LV.  Valid lvcreate size options are -l
-or -L.  If not specified then LVM snapshots will not be created.
+or -L.  If not specified then LVM snapshots will not be created for
+volumes that are not thinly provisioned.
 .SH AUTHOR
 .RS
 Josef Bacik <josef@toxicpanda.com>
diff --git a/plugins/fs-snapshot/fs-snapshot.py b/plugins/fs-snapshot/fs-snapshot.py
index 2a7c65d..7483c2d 100644
--- a/plugins/fs-snapshot/fs-snapshot.py
+++ b/plugins/fs-snapshot/fs-snapshot.py
@@ -62,6 +62,9 @@ def kernel_supports_dm_snapshot_merge():
         dm_snapshot_merge_checked = 1
     return dm_snapshot_merge_support
 
+def is_thin_volume(volume):
+    return volume["segtype"] == "thin"
+
 def inspect_volume_lvm(conduit, volume):
     """
     If volume is an LVM logical volume:
@@ -92,12 +95,14 @@ def inspect_volume_lvm(conduit, volume):
     # Check if device is managed by lvm
     # - FIXME filter out snapshot (and other) LVs; for now just rely
     #   on 'lvcreate' to prevent snapshots of unsupported LV types
-    p = Popen(["/sbin/lvs", device], stdout=PIPE, stderr=PIPE)
+    p = Popen(["/sbin/lvs", "--noheadings", "-o", "segtype", device], stdout=PIPE, stderr=PIPE)
     err = p.wait()
     if not err:
+        volume["segtype"] = p.communicate()[0].strip()
+
         # FIXME allow creating snapshot LVs even if kernel doesn't
         # support snapshot-merge based system rollback? make configurable?
-        if not kernel_supports_dm_snapshot_merge():
+        if not is_thin_volume(volume) and not kernel_supports_dm_snapshot_merge():
             conduit.error(1, "fs-snapshot: skipping volume: %s, "
                           "kernel doesn't support snapshot-merge" % device)
             return 0
@@ -223,16 +228,18 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
       has enough free space to accommodate a snapshot LV.
     - Also assumes user has configured 'lvcreate_size_args'.
     """
-    lvcreate_size_args = conduit.confString('lvm', 'lvcreate_size_args',
-                                            default=None)
-    if not lvcreate_size_args:
-        conduit.error(1, "fs-snapshot: 'lvcreate_size_args' was not provided "
-                      "in the '[lvm]' section of the config file")
-        return 1
+    if not is_thin_volume(volume):
+        lvcreate_size_args = conduit.confString('lvm', 'lvcreate_size_args',
+                                                default=None)
 
-    if not lvcreate_size_args.startswith("-L") and not lvcreate_size_args.startswith("-l"):
-        conduit.error(1, "fs-snapshot: 'lvcreate_size_args' did not use -L or -l")
-        return 1
+        if not lvcreate_size_args:
+            conduit.error(1, "fs-snapshot: 'lvcreate_size_args' was not provided "
+                          "in the '[lvm]' section of the config file")
+            return 1
+
+        if not lvcreate_size_args.startswith("-L") and not lvcreate_size_args.startswith("-l"):
+            conduit.error(1, "fs-snapshot: 'lvcreate_size_args' did not use -L or -l")
+            return 1
 
     device = volume["device"]
     if device.count('/') != 3:
@@ -259,7 +266,8 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
                  (mntpnt, device, snap_lvname))
     # Create snapshot LV
     lvcreate_cmd = ["/sbin/lvcreate", "-s", "-n", snap_lvname]
-    lvcreate_cmd.extend(lvcreate_size_args.split())
+    if not is_thin_volume(volume):
+        lvcreate_cmd.extend(lvcreate_size_args.split())
     lvcreate_cmd.append(device)
     p = Popen(lvcreate_cmd, stdout=PIPE, stderr=PIPE)
     err = p.wait()
-- 
1.8.1

[linux-lvm] [PATCH 3/2] fs snapshot plugin: add ability to create snapshots during post transaction

[Mike Snitzer]

It can be useful to create a post transaction snapshot to allow tools
(e.g. snapper) to analyze the differences introduced by the yum
transaction.

Add 'create_snapshots_in_post' to main section of fs-snapshot.conf.

Signed-off-by: Mike Snitzer <msnitzer@fedoraproject.org>
---
 docs/yum-fs-snapshot.conf.5          |  4 ++++
 plugins/fs-snapshot/fs-snapshot.conf |  1 +
 plugins/fs-snapshot/fs-snapshot.py   | 10 +++++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/docs/yum-fs-snapshot.conf.5 b/docs/yum-fs-snapshot.conf.5
index dc5c0c1..4ac623a 100644
--- a/docs/yum-fs-snapshot.conf.5
+++ b/docs/yum-fs-snapshot.conf.5
@@ -20,6 +20,10 @@ utilizes configuration options in the form of
 .IP exclude
 This is a space delimited list of the mount points you do not wish to have
 snapshotted by this plugin.
+.SH OPTION
+.IP create_snapshots_in_post
+This is a boolean value used to control whether snapshots are also created
+after running the yum transaction.
 .SH OPTION - [lvm] section
 .IP enabled
 This is a boolean value used to control whether LVM snapshots will be
diff --git a/plugins/fs-snapshot/fs-snapshot.conf b/plugins/fs-snapshot/fs-snapshot.conf
index 3a67c69..9ada717 100644
--- a/plugins/fs-snapshot/fs-snapshot.conf
+++ b/plugins/fs-snapshot/fs-snapshot.conf
@@ -1,5 +1,6 @@
 [main]
 enabled = 1
+create_snapshots_in_post = 0
 
 [lvm]
 enabled = 0
diff --git a/plugins/fs-snapshot/fs-snapshot.py b/plugins/fs-snapshot/fs-snapshot.py
index 7483c2d..32a1f17 100644
--- a/plugins/fs-snapshot/fs-snapshot.py
+++ b/plugins/fs-snapshot/fs-snapshot.py
@@ -287,7 +287,7 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
         return 1
     return 2
 
-def pretrans_hook(conduit):
+def create_snapshots(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
@@ -304,3 +304,11 @@ def pretrans_hook(conduit):
         elif rc == 2 and hasattr(conduit, 'registerPackageName'):
             # A snapshot was successfully created
             conduit.registerPackageName("yum-plugin-fs-snapshot")
+
+def pretrans_hook(conduit):
+    create_snapshots(conduit)
+
+def posttrans_hook(conduit):
+    create_snapshots_in_post = conduit.confBool('main', 'create_snapshots_in_post', default=0)
+    if create_snapshots_in_post:
+        create_snapshots(conduit)
-- 
1.8.1

[linux-lvm] [PATCH 4/2] fs snapshot plugin: add LVM tag to allow tools to link pre and post snapshots

[Mike Snitzer]

Add a "yum_fs_snapshot_<trans_id>_<origin_volume>" LVM tag to LVM-based
snapshots (old or thinp).  These tags will allow tools (e.g snapper) to
link the pre and post snapshot volumes together.

yum_fs_snapshot_trans_id() uses the first 16 digits of the hash() of the
rpm TransactionSet instance to establish a unique id that is common to
both the pretrans_hook() and posttrans_hook() -- this is quite the hack;
I'm open to using other (more future-proof) methods.

Factored add_lvm_tag_to_snapshot() out to allow for reuse.

Signed-off-by: Mike Snitzer <msnitzer@fedoraproject.org>
---
 plugins/fs-snapshot/fs-snapshot.py | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/plugins/fs-snapshot/fs-snapshot.py b/plugins/fs-snapshot/fs-snapshot.py
index 32a1f17..22582aa 100644
--- a/plugins/fs-snapshot/fs-snapshot.py
+++ b/plugins/fs-snapshot/fs-snapshot.py
@@ -43,6 +43,12 @@ lvm_key = "create_lvm_snapshot"
 dm_snapshot_merge_checked = 0
 dm_snapshot_merge_support = 0
 
+def yum_fs_snapshot_trans_id(ts):
+    # return pseudo yum transaction id string
+    # this string is identical for both {pre,post}trans_hook
+    yum_ts_hash = "%d" % abs(hash(ts.ts))
+    return "yum_fs_snapshot_" + yum_ts_hash[:16]
+
 def _fail(msg):
     raise PluginYumExit(msg)
 
@@ -221,6 +227,14 @@ def _create_btrfs_snapshot(conduit, snapshot_tag, volume):
         return 1
     return 2
 
+def add_lvm_tag_to_snapshot(conduit, tag, snap_volume):
+    p = Popen(["/sbin/lvchange", "--addtag", tag, snap_volume],
+              stdout=PIPE, stderr=PIPE)
+    err = p.wait()
+    if err:
+        conduit.error(1, "fs-snapshot: couldn't add tag to snapshot: %s" %
+                      snap_volume)
+
 def _create_lvm_snapshot(conduit, snapshot_tag, volume):
     """
     Create LVM snapshot LV and tag it with $snapshot_tag.
@@ -248,6 +262,8 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
     mntpnt = volume["mntpnt"]
     kern_inst = True # Default to saying it might be.
     ts = conduit._base.rpmdb.readOnlyTS()
+    # save pseudo yum transaction id
+    yum_trans_id = yum_fs_snapshot_trans_id(ts)
     kern_pkgtup = yum.misc.get_running_kernel_pkgtup(ts)
     del ts
     if kern_pkgtup is not None:
@@ -260,6 +276,7 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
                         "                      being altered /boot may need to be manually restored\n"
                         "                      in the event that a system rollback proves necessary.\n")
 
+    orig_lvname = device.split('/')[3]
     snap_device = device + "_" + snapshot_tag
     snap_lvname = snap_device.split('/')[3]
     conduit.info(1, "fs-snapshot: snapshotting %s (%s): %s" %
@@ -278,12 +295,11 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
     # Add tag ($snapshot_tag) to snapshot LV
     # - should help facilitate merge of all snapshot LVs created
     #   by a yum transaction, e.g.: lvconvert --merge @snapshot_tag
-    p = Popen(["/sbin/lvchange", "--addtag", snapshot_tag, snap_device],
-              stdout=PIPE, stderr=PIPE)
-    err = p.wait()
-    if err:
-        conduit.error(1, "fs-snapshot: couldn't add tag to snapshot: %s" %
-                      snap_device)
+    if add_lvm_tag_to_snapshot(conduit, snapshot_tag, snap_device):
+        return 1
+    # Add tag to allow other tools (e.g. snapper) to link pre
+    # and post snapshot LVs together
+    if add_lvm_tag_to_snapshot(conduit, yum_trans_id + "_" + orig_lvname, snap_device):
         return 1
     return 2
 

Re: [linux-lvm] [Yum-devel] [PATCH 4/2] fs snapshot plugin: add LVM tag to allow tools to link pre and post snapshots

[James Antill]

On Mon, 2013-03-11 at 17:25 -0400, Mike Snitzer wrote:
> Add a "yum_fs_snapshot_<trans_id>_<origin_volume>" LVM tag to LVM-based
> snapshots (old or thinp).  These tags will allow tools (e.g snapper) to
> link the pre and post snapshot volumes together.
> 
> yum_fs_snapshot_trans_id() uses the first 16 digits of the hash() of the
> rpm TransactionSet instance to establish a unique id that is common to
> both the pretrans_hook() and posttrans_hook() -- this is quite the hack;
> I'm open to using other (more future-proof) methods.

 Esp. as hash(ts.ts) is just the address of the pointer for that object
in C.
 It depends what you want, I guess.
 My first guess is that you'd want roughly what the rest of yum uses,
so:

rpmdbv  = self.rpmdb.simpleVersion(main_only=True)[0]
frpmdbv = self.tsInfo.futureRpmDBVersion()

...except things like yum history also get the real "future" rpmdbv
after the transaction happens, which this can't (unless we can add the
tags in post_trans? -- and then there are problems about what happens if
we don't get there).

 This should identify the state of the system, and what will happen well
enough ... but that means if the user does:

yum blah
yum history undo last
yum history undo last
yum history undo last

...you'll have multiple snapshots with the same tag (as the system will
be in the same states from the packaging POV -- is this what you want?).
 The other alternative is to just use a stored time.time(), of the first
snapshot (or maybe that and the start rpmdbv?).

Re: [linux-lvm] [PATCH 4/2] fs snapshot plugin: add LVM tag to allow tools to link pre and post snapshots

[Mike Snitzer]

On Tue, Mar 12 2013 at  9:46am -0400,
James Antill <james@fedoraproject.org> wrote:

> On Mon, 2013-03-11 at 17:25 -0400, Mike Snitzer wrote:
> > Add a "yum_fs_snapshot_<trans_id>_<origin_volume>" LVM tag to LVM-based
> > snapshots (old or thinp).  These tags will allow tools (e.g snapper) to
> > link the pre and post snapshot volumes together.
> > 
> > yum_fs_snapshot_trans_id() uses the first 16 digits of the hash() of the
> > rpm TransactionSet instance to establish a unique id that is common to
> > both the pretrans_hook() and posttrans_hook() -- this is quite the hack;
> > I'm open to using other (more future-proof) methods.
> 
>  Esp. as hash(ts.ts) is just the address of the pointer for that object
> in C.
>  It depends what you want, I guess.

I couldn't figure out how to store anything in pretrans_hook() and
retrieve it in posttrans_hook().  Use of a global caused the plugin to
fail silently.

All I was going for was a unique number that was the same in both
pretrans_hook() and posttrans_hook().

>  My first guess is that you'd want roughly what the rest of yum uses,
> so:
> 
> rpmdbv  = self.rpmdb.simpleVersion(main_only=True)[0]
> frpmdbv = self.tsInfo.futureRpmDBVersion()
> 
> ...except things like yum history also get the real "future" rpmdbv
> after the transaction happens, which this can't (unless we can add the
> tags in post_trans? -- and then there are problems about what happens if
> we don't get there).
> 
>  This should identify the state of the system, and what will happen well
> enough ... but that means if the user does:
> 
> yum blah
> yum history undo last
> yum history undo last
> yum history undo last
> 
> ...you'll have multiple snapshots with the same tag (as the system will
> be in the same states from the packaging POV -- is this what you want?).
>  The other alternative is to just use a stored time.time(), of the first
> snapshot (or maybe that and the start rpmdbv?).

I like the idea of using the actual future rpmDB version; but as you
note it won't be unique on its own if you undo a transaction.  SO this
is the incremental change I came up with.  I'll post v2 of the 4/2 patch
with these chnages folded in:

diff --git a/plugins/fs-snapshot/fs-snapshot.py b/plugins/fs-snapshot/fs-snapshot.py
index 22582aa..1625564 100644
--- a/plugins/fs-snapshot/fs-snapshot.py
+++ b/plugins/fs-snapshot/fs-snapshot.py
@@ -43,11 +43,14 @@ lvm_key = "create_lvm_snapshot"
 dm_snapshot_merge_checked = 0
 dm_snapshot_merge_support = 0
 
-def yum_fs_snapshot_trans_id(ts):
+def yum_fs_snapshot_trans_id(conduit):
     # return pseudo yum transaction id string
     # this string is identical for both {pre,post}trans_hook
-    yum_ts_hash = "%d" % abs(hash(ts.ts))
-    return "yum_fs_snapshot_" + yum_ts_hash[:16]
+    tsInfo = conduit.getTsInfo()
+    # using hash of tsInfo purely to get unique number that isn't tied to rpmDB
+    tsInfo_hash = "%d" % (abs(hash(tsInfo)))
+    frpmdbv = tsInfo.futureRpmDBVersion()
+    return "yum_fs_snapshot_%s_%s" % (tsInfo_hash[:8], frpmdbv)
 
 def _fail(msg):
     raise PluginYumExit(msg)
@@ -262,8 +265,6 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
     mntpnt = volume["mntpnt"]
     kern_inst = True # Default to saying it might be.
     ts = conduit._base.rpmdb.readOnlyTS()
-    # save pseudo yum transaction id
-    yum_trans_id = yum_fs_snapshot_trans_id(ts)
     kern_pkgtup = yum.misc.get_running_kernel_pkgtup(ts)
     del ts
     if kern_pkgtup is not None:
@@ -299,6 +300,7 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
         return 1
     # Add tag to allow other tools (e.g. snapper) to link pre
     # and post snapshot LVs together
+    yum_trans_id = yum_fs_snapshot_trans_id(conduit)
     if add_lvm_tag_to_snapshot(conduit, yum_trans_id + "_" + orig_lvname, snap_device):
         return 1
     return 2

[linux-lvm] [PATCH 4/2 v2] fs snapshot plugin: add LVM tag to allow tools to link pre and post snapshots

[Mike Snitzer]

Add a "yum_fs_snapshot_<trans_id>_<origin_volume>" LVM tag to LVM-based
snapshots (old or thinp).  These tags will allow tools (e.g snapper) to
link the pre and post snapshot volumes together.

yum_fs_snapshot_trans_id() uses the first 8 digits of the hash() of the
tsInfo instance to establish a unique id that is common to both the
pretrans_hook() and posttrans_hook() -- this is done because the
futureRpmDBVersion(), also included in the trans_id, will not be unique
over time if transactions are rolled back (e.g.: yum history undo last)

Factored out add_lvm_tag_to_snapshot() to allow for reuse.

Signed-off-by: Mike Snitzer <msnitzer@fedoraproject.org>
---
 plugins/fs-snapshot/fs-snapshot.py | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

v2: include the futureRpmDBVersion() in the yum_fs_snapshot_trans_id

diff --git a/plugins/fs-snapshot/fs-snapshot.py b/plugins/fs-snapshot/fs-snapshot.py
index 32a1f17..1625564 100644
--- a/plugins/fs-snapshot/fs-snapshot.py
+++ b/plugins/fs-snapshot/fs-snapshot.py
@@ -43,6 +43,15 @@ lvm_key = "create_lvm_snapshot"
 dm_snapshot_merge_checked = 0
 dm_snapshot_merge_support = 0
 
+def yum_fs_snapshot_trans_id(conduit):
+    # return pseudo yum transaction id string
+    # this string is identical for both {pre,post}trans_hook
+    tsInfo = conduit.getTsInfo()
+    # using hash of tsInfo purely to get unique number that isn't tied to rpmDB
+    tsInfo_hash = "%d" % (abs(hash(tsInfo)))
+    frpmdbv = tsInfo.futureRpmDBVersion()
+    return "yum_fs_snapshot_%s_%s" % (tsInfo_hash[:8], frpmdbv)
+
 def _fail(msg):
     raise PluginYumExit(msg)
 
@@ -221,6 +230,14 @@ def _create_btrfs_snapshot(conduit, snapshot_tag, volume):
         return 1
     return 2
 
+def add_lvm_tag_to_snapshot(conduit, tag, snap_volume):
+    p = Popen(["/sbin/lvchange", "--addtag", tag, snap_volume],
+              stdout=PIPE, stderr=PIPE)
+    err = p.wait()
+    if err:
+        conduit.error(1, "fs-snapshot: couldn't add tag to snapshot: %s" %
+                      snap_volume)
+
 def _create_lvm_snapshot(conduit, snapshot_tag, volume):
     """
     Create LVM snapshot LV and tag it with $snapshot_tag.
@@ -260,6 +277,7 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
                         "                      being altered /boot may need to be manually restored\n"
                         "                      in the event that a system rollback proves necessary.\n")
 
+    orig_lvname = device.split('/')[3]
     snap_device = device + "_" + snapshot_tag
     snap_lvname = snap_device.split('/')[3]
     conduit.info(1, "fs-snapshot: snapshotting %s (%s): %s" %
@@ -278,12 +296,12 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
     # Add tag ($snapshot_tag) to snapshot LV
     # - should help facilitate merge of all snapshot LVs created
     #   by a yum transaction, e.g.: lvconvert --merge @snapshot_tag
-    p = Popen(["/sbin/lvchange", "--addtag", snapshot_tag, snap_device],
-              stdout=PIPE, stderr=PIPE)
-    err = p.wait()
-    if err:
-        conduit.error(1, "fs-snapshot: couldn't add tag to snapshot: %s" %
-                      snap_device)
+    if add_lvm_tag_to_snapshot(conduit, snapshot_tag, snap_device):
+        return 1
+    # Add tag to allow other tools (e.g. snapper) to link pre
+    # and post snapshot LVs together
+    yum_trans_id = yum_fs_snapshot_trans_id(conduit)
+    if add_lvm_tag_to_snapshot(conduit, yum_trans_id + "_" + orig_lvname, snap_device):
         return 1
     return 2
 

Re: [linux-lvm] [PATCH 4/2] fs snapshot plugin: add LVM tag to allow tools to link pre and post snapshots

[James Antill]

On Tue, 2013-03-12 at 10:41 -0400, Mike Snitzer wrote:
> On Tue, Mar 12 2013 at  9:46am -0400,
> James Antill <james@fedoraproject.org> wrote:
> 
> > On Mon, 2013-03-11 at 17:25 -0400, Mike Snitzer wrote:
> > > Add a "yum_fs_snapshot_<trans_id>_<origin_volume>" LVM tag to LVM-based
> > > snapshots (old or thinp).  These tags will allow tools (e.g snapper) to
> > > link the pre and post snapshot volumes together.
> > > 
> > > yum_fs_snapshot_trans_id() uses the first 16 digits of the hash() of the
> > > rpm TransactionSet instance to establish a unique id that is common to
> > > both the pretrans_hook() and posttrans_hook() -- this is quite the hack;
> > > I'm open to using other (more future-proof) methods.
> > 
> >  Esp. as hash(ts.ts) is just the address of the pointer for that object
> > in C.
> >  It depends what you want, I guess.
> 
> I couldn't figure out how to store anything in pretrans_hook() and
> retrieve it in posttrans_hook().  Use of a global caused the plugin to
> fail silently.

 Using globals should work, the only real problem is if some python API
creates multiple YumBase() instances (but although we've worried about
that, nothing has ever really done it AFAIK).
 The common way is to just stuff something unique in the yum base object
(base.__plugin_fssnap_whatever = blah).

[...]

> I like the idea of using the actual future rpmDB version; but as you
> note it won't be unique on its own if you undo a transaction.  SO this
> is the incremental change I came up with.  I'll post v2 of the 4/2 patch
> with these chnages folded in:
[...] 
> -def yum_fs_snapshot_trans_id(ts):
> +def yum_fs_snapshot_trans_id(conduit):
>      # return pseudo yum transaction id string
>      # this string is identical for both {pre,post}trans_hook
> -    yum_ts_hash = "%d" % abs(hash(ts.ts))
> -    return "yum_fs_snapshot_" + yum_ts_hash[:16]
> +    tsInfo = conduit.getTsInfo()
> +    # using hash of tsInfo purely to get unique number that isn't tied to rpmDB
> +    tsInfo_hash = "%d" % (abs(hash(tsInfo)))
> +    frpmdbv = tsInfo.futureRpmDBVersion()
> +    return "yum_fs_snapshot_%s_%s" % (tsInfo_hash[:8], frpmdbv)

 I still worry about how unique "abs(hash(ts.ts))" will be over multiple
runs ... is the reason for not using a timestamp just that you don't
know where to store it?

Re: [linux-lvm] [PATCH 4/2] fs snapshot plugin: add LVM tag to allow tools to link pre and post snapshots

[Mike Snitzer]

On Tue, Mar 12 2013 at 12:41pm -0400,
James Antill <james@fedoraproject.org> wrote:

> On Tue, 2013-03-12 at 10:41 -0400, Mike Snitzer wrote:
> > On Tue, Mar 12 2013 at  9:46am -0400,
> > James Antill <james@fedoraproject.org> wrote:
> > 
> > > On Mon, 2013-03-11 at 17:25 -0400, Mike Snitzer wrote:
> > > > Add a "yum_fs_snapshot_<trans_id>_<origin_volume>" LVM tag to LVM-based
> > > > snapshots (old or thinp).  These tags will allow tools (e.g snapper) to
> > > > link the pre and post snapshot volumes together.
> > > > 
> > > > yum_fs_snapshot_trans_id() uses the first 16 digits of the hash() of the
> > > > rpm TransactionSet instance to establish a unique id that is common to
> > > > both the pretrans_hook() and posttrans_hook() -- this is quite the hack;
> > > > I'm open to using other (more future-proof) methods.
> > > 
> > >  Esp. as hash(ts.ts) is just the address of the pointer for that object
> > > in C.
> > >  It depends what you want, I guess.
> > 
> > I couldn't figure out how to store anything in pretrans_hook() and
> > retrieve it in posttrans_hook().  Use of a global caused the plugin to
> > fail silently.
> 
>  Using globals should work, the only real problem is if some python API
> creates multiple YumBase() instances (but although we've worried about
> that, nothing has ever really done it AFAIK).

Just adding a single global caused the plugin to fail: global foo = None

>  The common way is to just stuff something unique in the yum base object
> (base.__plugin_fssnap_whatever = blah).

OK.

> > I like the idea of using the actual future rpmDB version; but as you
> > note it won't be unique on its own if you undo a transaction.  SO this
> > is the incremental change I came up with.  I'll post v2 of the 4/2 patch
> > with these chnages folded in:
> [...] 
> > -def yum_fs_snapshot_trans_id(ts):
> > +def yum_fs_snapshot_trans_id(conduit):
> >      # return pseudo yum transaction id string
> >      # this string is identical for both {pre,post}trans_hook
> > -    yum_ts_hash = "%d" % abs(hash(ts.ts))
> > -    return "yum_fs_snapshot_" + yum_ts_hash[:16]
> > +    tsInfo = conduit.getTsInfo()
> > +    # using hash of tsInfo purely to get unique number that isn't tied to rpmDB
> > +    tsInfo_hash = "%d" % (abs(hash(tsInfo)))
> > +    frpmdbv = tsInfo.futureRpmDBVersion()
> > +    return "yum_fs_snapshot_%s_%s" % (tsInfo_hash[:8], frpmdbv)
> 
>  I still worry about how unique "abs(hash(ts.ts))" will be over multiple
> runs ... is the reason for not using a timestamp just that you don't
> know where to store it?

abs(hash(tsInfo)) should be sufficiently unique when coupled with
futureRpmDBVersion().  But yeah, I didn't know how to store the timestamp.

I'll send a simpler v3 now.. thanks for your help!

[linux-lvm] [PATCH 4/2 v3] fs snapshot plugin: add LVM tag to allow tools to link pre and post snapshots

[Mike Snitzer]

Add a "yum_fs_snapshot_pre_lv_name=<pre_snapshot_lv>" LVM tag to the
post transaction LVM-based snapshot (old or thinp).  This tag will
allow tools (e.g snapper) to link the pre and post snapshot volumes
together.

Factored out add_lvm_tag_to_snapshot() to allow for reuse.

Signed-off-by: Mike Snitzer <msnitzer@fedoraproject.org>
---
 plugins/fs-snapshot/fs-snapshot.py | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/plugins/fs-snapshot/fs-snapshot.py b/plugins/fs-snapshot/fs-snapshot.py
index 32a1f17..38496c5 100644
--- a/plugins/fs-snapshot/fs-snapshot.py
+++ b/plugins/fs-snapshot/fs-snapshot.py
@@ -221,6 +221,14 @@ def _create_btrfs_snapshot(conduit, snapshot_tag, volume):
         return 1
     return 2
 
+def add_lvm_tag_to_snapshot(conduit, tag, snap_volume):
+    p = Popen(["/sbin/lvchange", "--addtag", tag, snap_volume],
+              stdout=PIPE, stderr=PIPE)
+    err = p.wait()
+    if err:
+        conduit.error(1, "fs-snapshot: couldn't add tag to snapshot: %s" %
+                      snap_volume)
+
 def _create_lvm_snapshot(conduit, snapshot_tag, volume):
     """
     Create LVM snapshot LV and tag it with $snapshot_tag.
@@ -278,13 +287,15 @@ def _create_lvm_snapshot(conduit, snapshot_tag, volume):
     # Add tag ($snapshot_tag) to snapshot LV
     # - should help facilitate merge of all snapshot LVs created
     #   by a yum transaction, e.g.: lvconvert --merge @snapshot_tag
-    p = Popen(["/sbin/lvchange", "--addtag", snapshot_tag, snap_device],
-              stdout=PIPE, stderr=PIPE)
-    err = p.wait()
-    if err:
-        conduit.error(1, "fs-snapshot: couldn't add tag to snapshot: %s" %
-                      snap_device)
+    if add_lvm_tag_to_snapshot(conduit, snapshot_tag, snap_device):
         return 1
+    if conduit._base.__plugin_fs_snapshot_post_snapshot_tag == snapshot_tag:
+        # Add tag to allow other tools (e.g. snapper) to link pre
+        # and post snapshot LVs together
+        pre_snap_lv_name = "%s_%s" % (device, conduit._base.__plugin_fs_snapshot_pre_snapshot_tag)
+        pre_snapshot_tag = "yum_fs_snapshot_pre_lv_name=" + pre_snap_lv_name
+        if add_lvm_tag_to_snapshot(conduit, pre_snapshot_tag, snap_device):
+            return 1
     return 2
 
 def create_snapshots(conduit):
@@ -295,6 +306,10 @@ def create_snapshots(conduit):
     """
     # common snapshot tag format: yum_${year}${month}${day}${hour}${minute}${sec}
     snapshot_tag = "yum_" + time.strftime("%Y%m%d%H%M%S")
+    if not conduit._base.__plugin_fs_snapshot_pre_snapshot_tag:
+        conduit._base.__plugin_fs_snapshot_pre_snapshot_tag = snapshot_tag
+    else:
+        conduit._base.__plugin_fs_snapshot_post_snapshot_tag = snapshot_tag
 
     volumes = get_volumes(conduit)
     for volume in volumes:
@@ -306,6 +321,8 @@ def create_snapshots(conduit):
             conduit.registerPackageName("yum-plugin-fs-snapshot")
 
 def pretrans_hook(conduit):
+    conduit._base.__plugin_fs_snapshot_pre_snapshot_tag = None
+    conduit._base.__plugin_fs_snapshot_post_snapshot_tag = None
     create_snapshots(conduit)
 
 def posttrans_hook(conduit):

Re: [linux-lvm] [Yum-devel] [PATCH 4/2] fs snapshot plugin: add LVM tag to allow tools to link pre and post snapshots

[James Antill]

On Tue, 2013-03-12 at 14:09 -0400, Mike Snitzer wrote:
> On Tue, Mar 12 2013 at 12:41pm -0400,
> James Antill <james@fedoraproject.org> wrote:
> >  Using globals should work, the only real problem is if some python API
> > creates multiple YumBase() instances (but although we've worried about
> > that, nothing has ever really done it AFAIK).
> 
> Just adding a single global caused the plugin to fail: global foo = None

 That's a syntax error, python is much more annoying about globals than
perl (or pretty much any other language :).
 Also note that you can pass "-d 9" (maybe even just -v) to yum get the
traceback from plugins, we suppress them on load, by default, as that's
much better for users.

> abs(hash(tsInfo)) should be sufficiently unique when coupled with
> futureRpmDBVersion().

 Probably, I was just worried that in some cases it might have similar
addresses (given it's doing mostly the same work each time, without some
kind of memory randomization it might well be at an identical memory
address).

>   But yeah, I didn't know how to store the timestamp.
> 
> I'll send a simpler v3 now.. thanks for your help!

 Looks fine, I've pushed it and built it in rawhide (on F19 branch too).

Re: [linux-lvm] [PATCH 4/2] fs snapshot plugin: add LVM tag to allow tools to link pre and post snapshots

[Mike Snitzer]

On Tue, Mar 12 2013 at  3:52pm -0400,
James Antill <james@fedoraproject.org> wrote:

> On Tue, 2013-03-12 at 14:09 -0400, Mike Snitzer wrote:
> > On Tue, Mar 12 2013 at 12:41pm -0400,
> > James Antill <james@fedoraproject.org> wrote:
> > >  Using globals should work, the only real problem is if some python API
> > > creates multiple YumBase() instances (but although we've worried about
> > > that, nothing has ever really done it AFAIK).
> > 
> > Just adding a single global caused the plugin to fail: global foo = None
> 
>  That's a syntax error, python is much more annoying about globals than
> perl (or pretty much any other language :).
>  Also note that you can pass "-d 9" (maybe even just -v) to yum get the
> traceback from plugins, we suppress them on load, by default, as that's
> much better for users.

OK, thanks.
 
> >   But yeah, I didn't know how to store the timestamp.
> > 
> > I'll send a simpler v3 now.. thanks for your help!
> 
>  Looks fine, I've pushed it and built it in rawhide (on F19 branch too).

Much appreciated!