Merge lp:~raharper/curtin/trunk.fix-lp1718699 into lp:~curtin-dev/curtin/trunk

[Ryan Harper]

block: handle wiping bcache parts

In some cases block devices may have bcache parts which are owned by the
block layer but are not assembled into a bcache device. The result is that
curtin fails to get exclusive opens when attempting to wipe the device.
Resolve this by testing if a volume has a 'bcache' directory under sysfs
and issuing the appropriate stop commands before wiping.

It's also possible for old "buried" metadata for bcache or lvm or raid
at particular partition offsets. Once curtin starts partitioning the
device the bcache kernel layer "finds" the parts and claims device
ownership and prevents wiping of the partition. We resolve this issue
by having curtin wipe 1M at the location of the partition before we
actually create it.

Fix 'curtin clear-holders' subcommand.

LP: #1718699

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:538
https://jenkins.ubuntu.com/server/job/curtin-ci/664/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/664/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/664/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/664/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/664/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/664/rebuild

review: Needs Fixing (continuous-integration)

[lp:~raharper/curtin/trunk.fix-lp1718699]

539. By Ryan Harper on 2017-10-23

merge from trunk

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:539
https://jenkins.ubuntu.com/server/job/curtin-ci/666/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/666/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/666/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/666/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/666/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/666/rebuild

review: Needs Fixing (continuous-integration)

[lp:~raharper/curtin/trunk.fix-lp1718699]

540. By Ryan Harper on 2017-10-25

Fix style

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:540
https://jenkins.ubuntu.com/server/job/curtin-ci/668/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/668
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/668
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/668
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/668

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/668/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Please mention the fix in clear_holders_main. suggested:

   Also, fix a bug found in testing 'curtin clear-holders'.

Some tiny things, please fix.

Other than that, approve.

review: Approve

[Ryan Harper (raharper)]

Thanks for the review, will fix up, retest and push to trunk

[lp:~raharper/curtin/trunk.fix-lp1718699]

541. By Ryan Harper on 2017-10-25

Reword vmtest test input

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:541
https://jenkins.ubuntu.com/server/job/curtin-ci/669/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/669
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/669
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/669
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/669

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/669/rebuild

review: Approve (continuous-integration)

=== modified file 'curtin/block/clear_holders.py'
--- curtin/block/clear_holders.py	2017-10-13 17:48:29 +0000
+++ curtin/block/clear_holders.py	2017-10-25 20:32:23 +0000
@@ -258,6 +258,19 @@
         LOG.info("extended partitions do not need wiping, so skipping: '%s'",
                  blockdev)
     else:
+        # some volumes will be claimed by the bcache layer but do not surface
+        # an actual /dev/bcacheN device which owns the parts (backing, cache)
+        # The result is that some volumes cannot be wiped while bcache claims
+        # the device.  Resolve this by stopping bcache layer on those volumes
+        # if present.
+        for bcache_path in ['bcache', 'bcache/set']:
+            stop_path = os.path.join(device, bcache_path)
+            if os.path.exists(stop_path):
+                LOG.debug('Attempting to release bcache layer from device: %s',
+                          device)
+                maybe_stop_bcache_device(stop_path)
+                continue
+
         retries = [1, 3, 5, 7]
         LOG.info('wiping superblock on %s', blockdev)
         for attempt, wait in enumerate(retries):

=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py	2017-10-11 16:38:32 +0000
+++ curtin/commands/block_meta.py	2017-10-25 20:32:23 +0000
@@ -548,6 +548,21 @@
              info.get('id'), device, disk_ptable)
     LOG.debug("partnum: %s offset_sectors: %s length_sectors: %s",
               partnumber, offset_sectors, length_sectors)
+
+    # Wipe the partition if told to do so, do not wipe dos extended partitions
+    # as this may damage the extended partition table
+    if config.value_as_boolean(info.get('wipe')):
+        LOG.info("Preparing partition location on disk %s", disk)
+        if info.get('flag') == "extended":
+            LOG.warn("extended partitions do not need wiping, so skipping: "
+                     "'%s'" % info.get('id'))
+        else:
+            # wipe the start of the new partition first by zeroing 1M at the
+            # length of the previous partition
+            wipe_offset = int(offset_sectors * logical_block_size_bytes)
+            LOG.debug('Wiping 1M on %s at offset %s', disk, wipe_offset)
+            block.zero_file_at_offsets(disk, [wipe_offset])
+
     if disk_ptable == "msdos":
         if flag in ["extended", "logical", "primary"]:
             partition_type = flag
@@ -569,25 +584,6 @@
     else:
         raise ValueError("parent partition has invalid partition table")
 
-    # check if we've triggered hidden metadata like md, lvm or bcache
-    part_kname = get_path_to_storage_volume(info.get('id'), storage_config)
-    holders = clear_holders.get_holders(part_kname)
-    if len(holders) > 0:
-        LOG.debug('Detected block holders on partition %s: %s', part_kname,
-                  holders)
-        clear_holders.clear_holders(part_kname)
-        clear_holders.assert_clear(part_kname)
-
-    # Wipe the partition if told to do so, do not wipe dos extended partitions
-    # as this may damage the extended partition table
-    if config.value_as_boolean(info.get('wipe')):
-        if info.get('flag') == "extended":
-            LOG.warn("extended partitions do not need wiping, so skipping: "
-                     "'%s'" % info.get('id'))
-        else:
-            block.wipe_volume(
-                get_path_to_storage_volume(info.get('id'), storage_config),
-                mode=info.get('wipe'))
     # Make the name if needed
     if storage_config.get(device).get('name') and partition_type != 'extended':
         make_dname(info.get('id'), storage_config)

=== modified file 'curtin/commands/block_wipe.py'
--- curtin/commands/block_wipe.py	2016-08-10 20:18:07 +0000
+++ curtin/commands/block_wipe.py	2017-10-25 20:32:23 +0000
@@ -18,11 +18,15 @@
 import sys
 import curtin.block as block
 from . import populate_one_subcmd
+from .. import log
+
+LOG = log.LOG
 
 
 def wipe_main(args):
     for blockdev in args.devices:
         try:
+            LOG.debug('Wiping volume %s with mode=%s', blockdev, args.mode)
             block.wipe_volume(blockdev, mode=args.mode)
         except Exception as e:
             sys.stderr.write(

=== modified file 'curtin/commands/clear_holders.py'
--- curtin/commands/clear_holders.py	2016-08-11 17:21:40 +0000
+++ curtin/commands/clear_holders.py	2017-10-25 20:32:23 +0000
@@ -28,7 +28,7 @@
         raise ValueError('invalid devices specified')
     block.clear_holders.start_clear_holders_deps()
     block.clear_holders.clear_holders(args.devices, try_preserve=args.preserve)
-    if args.try_preserve:
+    if args.preserve:
         print('ran clear_holders attempting to preserve data. however, '
               'hotplug support for some devices may cause holders to restart ')
     block.clear_holders.assert_clear(args.devices)

=== added file 'examples/tests/bcache-wipe-xfs.yaml'
--- examples/tests/bcache-wipe-xfs.yaml	1970-01-01 00:00:00 +0000
+++ examples/tests/bcache-wipe-xfs.yaml	2017-10-25 20:32:23 +0000
@@ -0,0 +1,74 @@
+showtrace: true
+
+early_commands:
+  # Create a partitioned disk with bcache metadata in one of the partitions.
+  # Then, wipe the partition table.  This leaves "buried" bcache data that
+  # would be seen as soon as the disk was partitioned and cause problems
+  # for curtin's use of the disk.
+  # This config recreates issue LP: #1718699
+  00_blockmeta: [env, -u, OUTPUT_FSTAB, TARGET_MOUNT_POINT=/tmp/my.bdir/target,
+              WORKING_DIR=/tmp/my.bdir/work.d, curtin, --showtrace, -v,
+              block-meta, --umount, custom]
+  01_clear_holders: [curtin, clear-holders, --preserve, /dev/vdb]
+  02_quick_erase: [curtin, block-wipe, --mode, superblock, /dev/vdb]
+
+storage:
+  config:
+  - grub_device: true
+    id: vdb
+    name: vdb
+    path: /dev/vdb
+    ptable: msdos
+    type: disk
+    wipe: superblock
+  - id: vdc
+    name: vdc
+    path: /dev/vdc
+    type: disk
+    wipe: superblock
+  - device: vdb
+    id: vdb-part1
+    name: vdb-part1
+    number: 1
+    offset: 4194304B
+    size: 3997171712B
+    type: partition
+    uuid: 1d112703-1ff7-49fb-9655-741016e216bf
+    wipe: superblock
+  - device: vdb
+    id: vdb-part2
+    name: vdb-part2
+    number: 2
+    size: 3997171712B
+    type: partition
+    uuid: ec219a2e-c4a5-4623-b66a-965da2c6c1f1
+    wipe: superblock
+  - backing_device: vdc
+    cache_device: vdb-part2
+    cache_mode: writeback
+    id: bcache0
+    name: bcache0
+    type: bcache
+  - fstype: ext4
+    id: vdb-part1_format
+    label: ''
+    type: format
+    uuid: 0687dc8f-c089-4f30-8603-0ddf646a5dd7
+    volume: vdb-part1
+  - fstype: xfs
+    id: bcache0_format
+    label: ''
+    type: format
+    uuid: c40a45b2-1f12-454e-a0ec-784eb4ded4e6
+    volume: bcache0
+  - device: vdb-part1_format
+    id: vdb-part1_mount
+    options: ''
+    path: /
+    type: mount
+  - device: bcache0_format
+    id: bcache0_mount
+    options: ''
+    path: /var/lib/ceph-bcache/test-disk
+    type: mount
+  version: 1

=== modified file 'tests/unittests/test_commands_block_meta.py'
--- tests/unittests/test_commands_block_meta.py	2017-08-03 18:14:51 +0000
+++ tests/unittests/test_commands_block_meta.py	2017-10-25 20:32:23 +0000
@@ -130,6 +130,8 @@
                        'mock_clear_holders')
         self.add_patch('curtin.block.clear_holders.assert_clear',
                        'mock_assert_clear')
+        self.add_patch('curtin.block.zero_file_at_offsets',
+                       'mock_block_zero_file_at_offsets')
 
         self.target = "my_target"
         self.config = {
@@ -177,32 +179,26 @@
         self.mock_clear_holders.assert_called_with(disk)
         self.mock_assert_clear.assert_called_with(disk)
 
-    def test_partition_handler_calls_clear_holder(self):
+    def test_partition_handler_wipes_at_partition_offset(self):
+        """ Test wiping partition at offset prior to creating partition"""
         disk_info = self.storage_config.get('sda')
         part_info = self.storage_config.get('sda-part1')
         disk_kname = disk_info.get('path')
         part_kname = disk_kname + '1'
         self.mock_getpath.side_effect = iter([
-            disk_info.get('id'),
-            part_kname,
-            part_kname,
+            disk_kname,
             part_kname,
         ])
-
         self.mock_block_get_part_table_type.return_value = 'dos'
         kname = 'xxx'
         self.mock_block_path_to_kname.return_value = kname
         self.mock_block_sys_block_path.return_value = '/sys/class/block/xxx'
-        self.mock_subp.side_effect = iter([
-            ("", 0),  # parted mkpart
-            ("", 0),  # ??
-        ])
-        holders = ['md1']
-        self.mock_get_holders.return_value = holders
 
         block_meta.partition_handler(part_info, self.storage_config)
 
-        print("clear_holders: %s" % self.mock_clear_holders.call_args_list)
-        print("assert_clear: %s" % self.mock_assert_clear.call_args_list)
-        self.mock_clear_holders.assert_called_with(part_kname)
-        self.mock_assert_clear.assert_called_with(part_kname)
+        part_offset = 2048 * 512
+        self.mock_block_zero_file_at_offsets.assert_called_with(disk_kname,
+                                                                [part_offset])
+        self.mock_subp.assert_called_with(['parted', disk_kname, '--script',
+                                           'mkpart', 'primary', '2048s',
+                                           '1001471s'], capture=True)

=== added file 'tests/vmtests/test_bcache_bug1718699.py'
--- tests/vmtests/test_bcache_bug1718699.py	1970-01-01 00:00:00 +0000
+++ tests/vmtests/test_bcache_bug1718699.py	2017-10-25 20:32:23 +0000
@@ -0,0 +1,25 @@
+from .releases import base_vm_classes as relbase
+from .test_bcache_basic import TestBcacheBasic
+
+
+class TestBcacheBug1718699(TestBcacheBasic):
+    conf_file = "examples/tests/bcache-wipe-xfs.yaml"
+    dirty_disks = False
+    nr_cpus = 2
+    extra_disks = ['10G']
+
+
+class PreciseTestBcacheBug1718699(relbase.precise_hwe_t, TestBcacheBug1718699):
+    __test__ = True
+
+
+class XenialTestBcacheBug1718699(relbase.xenial, TestBcacheBug1718699):
+    __test__ = True
+
+
+class ZestyTestBcacheBug1718699(relbase.zesty, TestBcacheBug1718699):
+    __test__ = True
+
+
+class ArtfulTestBcacheBug1718699(relbase.artful, TestBcacheBug1718699):
+    __test__ = True