Merge lp:~raharper/curtin/trunk.fix-bcache-multi-bdev-to-cdev-lp1514094 into lp:~curtin-dev/curtin/trunk

[Ryan Harper]

Allow re-use of bcache cache devices with separate backing devices

Decouple creation of bcache caches and backing devices by dropping
use of make-bcache -B <> -C <>, in favor of single use -B and -C
and combining backing devices with caches via sysfs attach attributes.
Update the mdadm/bcache vmtest to include a multi-bcache backing device cached by single cachedevice test.

While validating results, fix lurking issue with mdadm.conf not getting updated post raid configuration in target triggering https://bugs.launchpad.net/ubuntu/+source/mdadm/+bug/964052

Refactor how we create bcache caches and backing devices; decoupling those (ie, don't use make-bcache -B -C) allow us to discover previously created backing or caching devices and attach them via sysfs interface.

Update the mdadm/bcache vmtest to include a multi-bcache backing device cached by single cachedevice test.

While validating results, fix lurking issue with mdadm.conf not getting updated post raid configuration in target triggering https://bugs.launchpad.net/ubuntu/+source/mdadm/+bug/964052

Latestly, this branch passes make check and make vmtest.

[Scott Moser (smoser)]

this looks good other than the one comment i have inline below.
basically, for speed purposes i'm interested in avoiding calling and recalling update-initramfs mjultiple times.

[Scott Moser (smoser)]

On Tue, 17 Nov 2015, Ryan Harper wrote:

>
>
> Diff comments:
>
> >
> > === modified file 'curtin/commands/curthooks.py'
> > --- curtin/commands/curthooks.py 2015-10-02 19:59:11 +0000
> > +++ curtin/commands/curthooks.py 2015-11-11 14:47:59 +0000
> > @@ -687,6 +687,10 @@
> > "mdadm.conf")
> > if os.path.exists(mdadm_location):
> > copy_mdadm_conf(mdadm_location, target)
> > + # as per https://bugs.launchpad.net/ubuntu/+source/mdadm/+bug/964052
> > + # reconfigure mdadm
> > + util.subp(['chroot', target, 'dpkg-reconfigure',
> > + '--frontend=noninteractive', 'mdadm'], data=None)
>
> Hrm, unclear to me yet if we end up tripping ourselves up with modifications to the target that could potentially stick around.

> Also not clear to me that we won't have issues with configurations to installations of packages that might expect something to be present
> post install that update-initramfs might do to the target filesystem.

update-intiramfs is a focused operation without side effects. If it has
side effects, those are issues with it and we really should raise it.

> I hate to extend the install time, that said, this only hits for mdadm case where it is really needed.

It already runs so much though. And each one is a multi-second operation
with disk io and cpu.

Can we at least try to make this a call to update_initramfs so that we
explicitly "know" that is what we're doing? That'd mean in the future we
could possibly disable update-intiramfs during install and then update
once at the end (just having each update_initramfs call mark that one was
requested and then running at the end if any were).

Scott

[Ryan Harper (raharper)]

On Tue, Nov 17, 2015 at 9:21 AM, Scott Moser <email address hidden> wrote:

> On Tue, 17 Nov 2015, Ryan Harper wrote:
>
> >
> >
> > Diff comments:
> >
> > >
> > > === modified file 'curtin/commands/curthooks.py'
> > > --- curtin/commands/curthooks.py 2015-10-02 19:59:11 +0000
> > > +++ curtin/commands/curthooks.py 2015-11-11 14:47:59 +0000
> > > @@ -687,6 +687,10 @@
> > > "mdadm.conf")
> > > if os.path.exists(mdadm_location):
> > > copy_mdadm_conf(mdadm_location, target)
> > > + # as per
> https://bugs.launchpad.net/ubuntu/+source/mdadm/+bug/964052
> > > + # reconfigure mdadm
> > > + util.subp(['chroot', target, 'dpkg-reconfigure',
> > > + '--frontend=noninteractive', 'mdadm'], data=None)
> >
> > Hrm, unclear to me yet if we end up tripping ourselves up with
> modifications to the target that could potentially stick around.
>
> > Also not clear to me that we won't have issues with configurations to
> installations of packages that might expect something to be present
> > post install that update-initramfs might do to the target filesystem.
>
> update-intiramfs is a focused operation without side effects. If it has
> side effects, those are issues with it and we really should raise it.
>

That's fair; I suppose the package would fail to install if it was checking
for post update-initramfs state since we don't run that at install time.

>
> > I hate to extend the install time, that said, this only hits for mdadm
> case where it is really needed.
>
> It already runs so much though. And each one is a multi-second operation
> with disk io and cpu.
>
> Can we at least try to make this a call to update_initramfs so that we
> explicitly "know" that is what we're doing? That'd mean in the future we
> could possibly disable update-intiramfs during install and then update
> once at the end (just having each update_initramfs call mark that one was
> requested and then running at the end if any were).
>

I'm fine with that, however; the documentation surrounding this currently
says
that mdadm needs to be reconfigured; If we're fine in saying that the only
thing
that matters is getting mdadm.conf into the initramfs for
post-install-first-boot
to ensure that the device we created at install time (say md0 and md1)
actually
end up as md0 and md1 on first boot then OK.

I'm open to alternatives but I don't think we need to exhaustively search
for that
while holding up a critical release issue.

> Scott
>
> --
>
> https://code.launchpad.net/~raharper/curtin/trunk.fix-bcache-multi-bdev-to-cdev-lp1514094/+merge/277255
> You are the owner of
> lp:~raharper/curtin/trunk.fix-bcache-multi-bdev-to-cdev-lp1514094.
>

=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py	2015-10-30 13:00:28 +0000
+++ curtin/commands/block_meta.py	2015-11-11 14:47:59 +0000
@@ -31,6 +31,7 @@
 import sys
 import tempfile
 import time
+import re
 
 SIMPLE = 'simple'
 SIMPLE_BOOT = 'simple-boot'
@@ -155,20 +156,43 @@
         util.subp(cmd, rcs=[0, 1, 2, 5], capture=True)
 
 
-def get_holders(devname):
+def block_find_sysfs_path(devname):
+    # Look up any block device holders.  Handle devices and partitions
+    # as devnames (vdb, md0, vdb7)
     if not devname:
         return []
 
-    devname_sysfs = \
-        '/sys/class/block/{}/holders'.format(os.path.basename(devname))
-    if not os.path.exists(devname_sysfs):
-        err = ('No sysfs path to device holders:'
+    sys_class_block = '/sys/class/block/'
+    basename = os.path.basename(devname)
+    # try without parent blockdevice, then prepend parent
+    paths = [
+        os.path.join(sys_class_block, basename),
+        os.path.join(sys_class_block,
+                     re.split('[\d+]', basename)[0], basename),
+    ]
+
+    # find path to devname directory in sysfs
+    devname_sysfs = None
+    for path in paths:
+        if os.path.exists(path):
+            devname_sysfs = path
+
+    if devname_sysfs is None:
+        err = ('No sysfs path to device:'
                ' {}'.format(devname_sysfs))
         LOG.error(err)
         raise ValueError(err)
 
-    LOG.debug('Getting blockdev holders: {}'.format(devname_sysfs))
-    return os.listdir(devname_sysfs)
+    return devname_sysfs
+
+
+def get_holders(devname):
+    devname_sysfs = block_find_sysfs_path(devname)
+    if devname_sysfs:
+        LOG.debug('Getting blockdev holders: {}'.format(devname_sysfs))
+        return os.listdir(os.path.join(devname_sysfs, 'holders'))
+
+    return []
 
 
 def clear_holders(sys_block_path):
@@ -901,17 +925,37 @@
     cache_mode = info.get('cache_mode', None)
 
     if not backing_device or not cache_device:
-        raise ValueError("backing device and cache device for bcache must be \
-                specified")
+        raise ValueError("backing device and cache device for bcache"
+                         " must be specified")
 
     # The bcache module is not loaded when bcache is installed by apt-get, so
     # we will load it now
     util.subp(["modprobe", "bcache"])
 
-    # If both the backing device and cache device are specified at the same
-    # time than it is not necessary to attach the cache device manually, as
-    # bcache will do this automatically.
-    util.subp(["make-bcache", "-B", backing_device, "-C", cache_device])
+    if cache_device:
+        # /sys/class/block/XXX/YYY/
+        cache_device_sysfs = block_find_sysfs_path(cache_device)
+
+        if os.path.exists(os.path.join(cache_device_sysfs, "bcache")):
+            # read in cset uuid from cache device
+            (out, err) = util.subp(["bcache-super-show", cache_device],
+                                   capture=True)
+            LOG.debug('out=[{}]'.format(out))
+            [cset_uuid] = [line.split()[-1] for line in out.split("\n")
+                           if line.startswith('cset.uuid')]
+
+        else:
+            # make the cache device, extracting cacheset uuid
+            (out, err) = util.subp(["make-bcache", "-C", cache_device],
+                                   capture=True)
+            LOG.debug('out=[{}]'.format(out))
+            [cset_uuid] = [line.split()[-1] for line in out.split("\n")
+                           if line.startswith('Set UUID:')]
+
+    if backing_device:
+        backing_device_sysfs = block_find_sysfs_path(backing_device)
+        if not os.path.exists(os.path.join(backing_device_sysfs, "bcache")):
+            util.subp(["make-bcache", "-B", backing_device])
 
     # Some versions of bcache-tools will register the bcache device as soon as
     # we run make-bcache using udev rules, so wait for udev to settle, then try
@@ -927,6 +971,22 @@
             fp.write(path)
             fp.close()
 
+    # if we specify both then we need to attach backing to cache
+    if cache_device and backing_device:
+        if cset_uuid:
+            LOG.info("Attaching backing device to cacheset: "
+                     "{} -> {} cset.uuid: {}".format(backing_device,
+                                                     cache_device,
+                                                     cset_uuid))
+            attach = os.path.join(backing_device_sysfs,
+                                  "bcache",
+                                  "attach")
+            with open(attach, "w") as fp:
+                fp.write(cset_uuid)
+        else:
+            LOG.error("Invalid cset_uuid: {}".format(cset_uuid))
+            raise
+
     if cache_mode:
         # find the actual bcache device name via sysfs using the
         # backing device's holders directory.

=== modified file 'curtin/commands/curthooks.py'
--- curtin/commands/curthooks.py	2015-10-02 19:59:11 +0000
+++ curtin/commands/curthooks.py	2015-11-11 14:47:59 +0000
@@ -687,6 +687,10 @@
                                   "mdadm.conf")
     if os.path.exists(mdadm_location):
         copy_mdadm_conf(mdadm_location, target)
+        # as per https://bugs.launchpad.net/ubuntu/+source/mdadm/+bug/964052
+        # reconfigure mdadm
+        util.subp(['chroot', target, 'dpkg-reconfigure',
+                   '--frontend=noninteractive', 'mdadm'], data=None)
 
     # If udev dname rules were created, copy them to target
     udev_rules_d = os.path.join(state['scratch'], "rules.d")

=== modified file 'examples/tests/mdadm_bcache.yaml'
--- examples/tests/mdadm_bcache.yaml	2015-10-29 19:13:25 +0000
+++ examples/tests/mdadm_bcache.yaml	2015-11-11 14:47:59 +0000
@@ -32,6 +32,10 @@
        type: partition
        size: 1GB
        device: sda
+     - id: sda6
+       type: partition
+       size: 1GB
+       device: sda
      - id: mddevice
        name: md0
        type: raid
@@ -47,6 +51,12 @@
        backing_device: mddevice
        cache_device: sda5
        cache_mode: writeback
+     - id: bcache1
+       type: bcache
+       name: cached_array_2
+       backing_device: sda6
+       cache_device: sda5
+       cache_mode: writeback
      - id: sda1_root
        type: format
        fstype: ext4
@@ -55,6 +65,10 @@
        type: format
        fstype: ext4
        volume: bcache0
+     - id: bcache_storage
+       type: format
+       fstype: ext4
+       volume: bcache1
      - id: sda1_mount
        type: mount
        path: /
@@ -63,3 +77,7 @@
        type: mount
        path: /media/data
        device: raid_storage
+     - id: bcache1_mount
+       type: mount
+       path: /media/bcache1
+       device: bcache_storage

=== modified file 'tests/vmtests/test_mdadm_bcache.py'
--- tests/vmtests/test_mdadm_bcache.py	2015-10-30 13:38:36 +0000
+++ tests/vmtests/test_mdadm_bcache.py	2015-11-11 14:47:59 +0000
@@ -44,31 +44,60 @@
                      'main_disk': 5,
                      'main_disk': 6,
                      'md0': 0,
-                     'cached_array': 0}
+                     'cached_array': 0,
+                     'cached_array_2': 0}
 
     collect_scripts = TestMdadmAbs.collect_scripts + [textwrap.dedent("""
         cd OUTPUT_COLLECT_D
         bcache-super-show /dev/vda6 > bcache_super_vda6
+        bcache-super-show /dev/vda7 > bcache_super_vda7
+        bcache-super-show /dev/md0 > bcache_super_md0
         ls /sys/fs/bcache > bcache_ls
         cat /sys/block/bcache0/bcache/cache_mode > bcache_cache_mode
+        cat /proc/mounts > proc_mounts
         """)]
     fstab_expected = {
-        '/dev/bcache0': '/media/data'
+        '/dev/bcache0': '/media/data',
+        '/dev/bcache1': '/media/bcache1',
     }
 
     def test_bcache_output_files_exist(self):
-        self.output_files_exist(["bcache_super_vda6", "bcache_ls",
+        self.output_files_exist(["bcache_super_vda6",
+                                 "bcache_super_vda7",
+                                 "bcache_super_md0",
+                                 "bcache_ls",
                                  "bcache_cache_mode"])
 
     def test_bcache_status(self):
+        bcache_supers = [
+            "bcache_super_vda6",
+            "bcache_super_vda7",
+            "bcache_super_md0",
+        ]
         bcache_cset_uuid = None
-        with open(os.path.join(self.td.mnt, "bcache_super_vda6"), "r") as fp:
-            for line in fp.read().splitlines():
-                if line != "" and line.split()[0] == "cset.uuid":
-                    bcache_cset_uuid = line.split()[-1].rstrip()
-        self.assertIsNotNone(bcache_cset_uuid)
-        with open(os.path.join(self.td.mnt, "bcache_ls"), "r") as fp:
-            self.assertTrue(bcache_cset_uuid in fp.read().splitlines())
+        found = {}
+        for bcache_super in bcache_supers:
+            with open(os.path.join(self.td.mnt, bcache_super), "r") as fp:
+                for line in fp.read().splitlines():
+                    if line != "" and line.split()[0] == "cset.uuid":
+                        bcache_cset_uuid = line.split()[-1].rstrip()
+                        if bcache_cset_uuid in found:
+                            found[bcache_cset_uuid].append(bcache_super)
+                        else:
+                            found[bcache_cset_uuid] = [bcache_super]
+            self.assertIsNotNone(bcache_cset_uuid)
+            with open(os.path.join(self.td.mnt, "bcache_ls"), "r") as fp:
+                self.assertTrue(bcache_cset_uuid in fp.read().splitlines())
+
+        # one cset.uuid for all devices
+        self.assertEqual(len(found), 1)
+
+        # three devices with same cset.uuid
+        self.assertEqual(len(found[bcache_cset_uuid]), 3)
+
+        # check the cset.uuid in the dict
+        self.assertEqual(list(found.keys()).pop(),
+                         bcache_cset_uuid)
 
     def test_bcache_cachemode(self):
         self.check_file_regex("bcache_cache_mode", r"\[writeback\]")