Merge ~mwhudson/curtin:lp-1868177 into curtin:master

[Michael Hudson-Doyle]

remove 'strict' arguments to block.wipe_volume and block.quick_zero

It was only passed by clear_holders._wipe_superblock and clear_holders
is all best effort anyway.

in particular, this should fix failures seen in subiquity error reports
when wiping very small (<1M) partitions.

LP: #1868177

i haven't tested this beyond running the unittests yet. I guess I should add some more of those too.

[Ryan Harper (raharper)]

Definitely should add a less than 1MB partition in one of the vmtests
Comment below, I suggest that we drop the strict=true in clear_holders.py:_wipe_superblock() instead.

bb8b718... by Michael Hudson-Doyle on 2021-03-23

remove strict argument to block.wipe_volume

It was only passed by clear_holders._wipe_superblock and clear_holders
is all best effort anyway.

in particular, this should fix failures seen in subiquity error reports
when wiping very small (<1M) partitions.

LP: #1868177

[Michael Hudson-Doyle (mwhudson)]

Yes, that seems completely fair. Take another look?

62bf639... by Michael Hudson-Doyle on 2021-03-23

remove strict argument from quick_zero too

c3800f7... by Michael Hudson-Doyle on 2021-03-23

add a tiny partition to a randomly selected vmtest

[Michael Hudson-Doyle (mwhudson)]

Confirmed that the vmtest change makes a vmtest fail without the fix that passes with it

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

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

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Needs Fixing (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

arm64 failure is that odd pylint race

[Ryan Harper (raharper)]

I think this is fine. I _was_ thinking to only remove the strict=True default in _wipe_superblock in clear-holders. However no where else is code calling with strict=true; if *something* external were using wipe_superblock and strict=true, so this is the minimal change I think:

(here's me wanting github style markdown in comments)
```
(crispyboi) curtin % git diff
diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
index c182d91a..ba026574 100644
--- a/curtin/block/clear_holders.py
+++ b/curtin/block/clear_holders.py
@@ -332,7 +332,7 @@ def wipe_superblock(device):
             time.sleep(wait)

-def _wipe_superblock(blockdev, exclusive=True, strict=True):
+def _wipe_superblock(blockdev, exclusive=True):
     """ No checks, just call wipe_volume """

     retries = [1, 3, 5, 7]
@@ -341,8 +341,7 @@ def _wipe_superblock(blockdev, exclusive=True, strict=True):
         LOG.debug('wiping %s attempt %s/%s',
                   blockdev, attempt + 1, len(retries))
         try:
- block.wipe_volume(blockdev, mode='superblock',
- exclusive=exclusive, strict=strict)
+ block.wipe_volume(blockdev, mode='superblock', exclusive=exclusive)
             LOG.debug('successfully wiped device %s on attempt %s/%s',
                       blockdev, attempt + 1, len(retries))
             return
```

So, I'll approve and leave it up to you if you want to do the minimal or the full removal that you have.

[Michael Hudson-Doyle (mwhudson)]

Are there any users of curtin-as-a-library beyond maas and subiquity? I'm not aware of any and if this change shakes any out of the tree I'd be interested to talk to them :)

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

review: Approve (continuous-integration)

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index 1b33002..2580a0d 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -1203,7 +1203,7 @@ def wipe_file(path, reader=None, buflen=4 * 1024 * 1024, exclusive=True):
                 fp.write(pbuf)
 
 
-def quick_zero(path, partitions=True, exclusive=True, strict=False):
+def quick_zero(path, partitions=True, exclusive=True):
     """
     zero 1M at front, 1M at end, and 1M at front
     if this is a block device and partitions is true, then
@@ -1227,11 +1227,11 @@ def quick_zero(path, partitions=True, exclusive=True, strict=False):
     for (pt, kname, ptnum) in pt_names:
         LOG.debug('Wiping path: dev:%s kname:%s partnum:%s',
                   pt, kname, ptnum)
-        quick_zero(pt, partitions=False, strict=strict)
+        quick_zero(pt, partitions=False)
 
     LOG.debug("wiping 1M on %s at offsets %s", path, offsets)
     return zero_file_at_offsets(path, offsets, buflen=buflen, count=count,
-                                exclusive=exclusive, strict=strict)
+                                exclusive=exclusive)
 
 
 def zero_file_at_offsets(path, offsets, buflen=1024, count=1024, strict=False,
@@ -1286,7 +1286,7 @@ def zero_file_at_offsets(path, offsets, buflen=1024, count=1024, strict=False,
                     fp.write(buf)
 
 
-def wipe_volume(path, mode="superblock", exclusive=True, strict=False):
+def wipe_volume(path, mode="superblock", exclusive=True):
     """wipe a volume/block device
 
     :param path: a path to a block device
@@ -1299,7 +1299,6 @@ def wipe_volume(path, mode="superblock", exclusive=True, strict=False):
                     volume and beginning and end of any partitions that are
                     known to be on this device.
     :param exclusive: boolean to control how path is opened
-    :param strict: boolean to control when to raise errors on write failures
     """
     if mode == "pvremove":
         # We need to use --force --force in case it's already in a volgroup and
@@ -1317,9 +1316,9 @@ def wipe_volume(path, mode="superblock", exclusive=True, strict=False):
         with open("/dev/urandom", "rb") as reader:
             wipe_file(path, reader=reader.read, exclusive=exclusive)
     elif mode == "superblock":
-        quick_zero(path, partitions=False, exclusive=exclusive, strict=strict)
+        quick_zero(path, partitions=False, exclusive=exclusive)
     elif mode == "superblock-recursive":
-        quick_zero(path, partitions=True, exclusive=exclusive, strict=strict)
+        quick_zero(path, partitions=True, exclusive=exclusive)
     else:
         raise ValueError("wipe mode %s not supported" % mode)
 
diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
index c182d91..ba02657 100644
--- a/curtin/block/clear_holders.py
+++ b/curtin/block/clear_holders.py
@@ -332,7 +332,7 @@ def wipe_superblock(device):
             time.sleep(wait)
 
 
-def _wipe_superblock(blockdev, exclusive=True, strict=True):
+def _wipe_superblock(blockdev, exclusive=True):
     """ No checks, just call wipe_volume """
 
     retries = [1, 3, 5, 7]
@@ -341,8 +341,7 @@ def _wipe_superblock(blockdev, exclusive=True, strict=True):
         LOG.debug('wiping %s attempt %s/%s',
                   blockdev, attempt + 1, len(retries))
         try:
-            block.wipe_volume(blockdev, mode='superblock',
-                              exclusive=exclusive, strict=strict)
+            block.wipe_volume(blockdev, mode='superblock', exclusive=exclusive)
             LOG.debug('successfully wiped device %s on attempt %s/%s',
                       blockdev, attempt + 1, len(retries))
             return
diff --git a/examples/tests/uefi_basic.yaml b/examples/tests/uefi_basic.yaml
index 1f49fa9..91a72ae 100644
--- a/examples/tests/uefi_basic.yaml
+++ b/examples/tests/uefi_basic.yaml
@@ -1,6 +1,10 @@
 showtrace: true
 
 early_commands:
+  # Create a small (512KiB) partition to test the fix for LP: #1868177.
+  tinypartition: [
+    "parted", /dev/disk/by-id/virtio-disk-a, "--script", "mklabel", "gpt",
+    "mkpart", "primary", "4096s", "4096s", "5120s"]
   # Recreate and test LP:1722322
   # Make one disk dirty with an MBR and a storage configuration
   # GPT and don't supply wipe: superblock.  This will exercise
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index d96a4a8..519628a 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -409,11 +409,11 @@ class TestWipeVolume(CiTestCase):
     def test_wipe_superblock(self, mock_quick_zero):
         block.wipe_volume(self.dev, mode='superblock')
         mock_quick_zero.assert_called_with(self.dev, exclusive=True,
-                                           partitions=False, strict=False)
+                                           partitions=False)
         block.wipe_volume(self.dev, exclusive=True,
                           mode='superblock-recursive')
         mock_quick_zero.assert_called_with(self.dev, exclusive=True,
-                                           partitions=True, strict=False)
+                                           partitions=True)
 
     @mock.patch('curtin.block.wipe_file')
     def test_wipe_zero(self, mock_wipe_file):
diff --git a/tests/unittests/test_clear_holders.py b/tests/unittests/test_clear_holders.py
index d1c2590..48697b5 100644
--- a/tests/unittests/test_clear_holders.py
+++ b/tests/unittests/test_clear_holders.py
@@ -142,8 +142,7 @@ class TestClearHolders(CiTestCase):
         # 1. wipe the bcache device contents
         m_block.wipe_volume.assert_called_with(self.test_blockdev,
                                                mode='superblock',
-                                               exclusive=False,
-                                               strict=True)
+                                               exclusive=False)
         # 2. extract the backing device
         m_bcache.get_backing_device.assert_called_with(self.test_blockdev)
         m_bcache.sysfs_path.assert_has_calls([
@@ -243,7 +242,7 @@ class TestClearHolders(CiTestCase):
         clear_holders.shutdown_mdadm(self.test_syspath)
 
         mock_wipe.assert_called_with(self.test_blockdev, exclusive=False,
-                                     mode='superblock', strict=True)
+                                     mode='superblock')
         mock_mdadm.set_sync_action.assert_has_calls([
                 mock.call(self.test_blockdev, action="idle"),
                 mock.call(self.test_blockdev, action="frozen")])
@@ -281,7 +280,7 @@ class TestClearHolders(CiTestCase):
         clear_holders.shutdown_mdadm(self.test_syspath)
 
         mock_wipe.assert_called_with(self.test_blockdev, exclusive=False,
-                                     mode='superblock', strict=True)
+                                     mode='superblock')
         mock_mdadm.set_sync_action.assert_has_calls([
                 mock.call(self.test_blockdev, action="idle"),
                 mock.call(self.test_blockdev, action="frozen")])
@@ -363,7 +362,7 @@ class TestClearHolders(CiTestCase):
         clear_holders.wipe_superblock(self.test_syspath)
         mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
         mock_block.wipe_volume.assert_called_with(
-            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+            self.test_blockdev, exclusive=True, mode='superblock')
 
     @mock.patch('curtin.block.clear_holders.multipath')
     @mock.patch('curtin.block.clear_holders.is_swap_device')
@@ -391,7 +390,7 @@ class TestClearHolders(CiTestCase):
         mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
         mock_zfs.zpool_export.assert_called_with('fake_pool')
         mock_block.wipe_volume.assert_called_with(
-            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+            self.test_blockdev, exclusive=True, mode='superblock')
 
     @mock.patch('curtin.block.clear_holders.multipath')
     @mock.patch('curtin.block.clear_holders.is_swap_device')
@@ -420,7 +419,7 @@ class TestClearHolders(CiTestCase):
         self.assertEqual(0, mock_zfs.device_to_poolname.call_count)
         self.assertEqual(0, mock_zfs.zpool_list.call_count)
         mock_block.wipe_volume.assert_called_with(
-            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+            self.test_blockdev, exclusive=True, mode='superblock')
 
     @mock.patch('curtin.block.clear_holders.udev')
     @mock.patch('curtin.block.clear_holders.multipath')
@@ -454,7 +453,7 @@ class TestClearHolders(CiTestCase):
         clear_holders.wipe_superblock(self.test_syspath)
         mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
         mock_block.wipe_volume.assert_called_with(
-            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+            self.test_blockdev, exclusive=True, mode='superblock')
 
     @mock.patch('curtin.block.clear_holders.multipath')
     @mock.patch('curtin.block.clear_holders.is_swap_device')
@@ -482,7 +481,7 @@ class TestClearHolders(CiTestCase):
         clear_holders.wipe_superblock(self.test_syspath)
         mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
         mock_block.wipe_volume.assert_called_with(
-            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+            self.test_blockdev, exclusive=True, mode='superblock')
         mock_block.get_sysfs_partitions.assert_has_calls(
             [mock.call(self.test_syspath)] * 3)
         mock_block.rescan_block_devices.assert_has_calls(
@@ -515,7 +514,7 @@ class TestClearHolders(CiTestCase):
         clear_holders.wipe_superblock(self.test_syspath)
         mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
         mock_block.wipe_volume.assert_called_with(
-            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+            self.test_blockdev, exclusive=True, mode='superblock')
         mock_block.get_sysfs_partitions.assert_has_calls(
             [mock.call(self.test_syspath)] * 3)
         mock_block.rescan_block_devices.assert_has_calls(
@@ -577,7 +576,7 @@ class TestClearHolders(CiTestCase):
         clear_holders.wipe_superblock(self.test_syspath)
         mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
         mock_block.wipe_volume.assert_called_with(
-            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+            self.test_blockdev, exclusive=True, mode='superblock')
 
     @mock.patch('curtin.block.clear_holders.zfs')
     @mock.patch('curtin.block.clear_holders.get_holders')
@@ -614,7 +613,7 @@ class TestClearHolders(CiTestCase):
         mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
         mock_mpath.remove_partition.assert_called_with('mpatha-part1')
         mock_block.wipe_volume.assert_called_with(
-            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+            self.test_blockdev, exclusive=True, mode='superblock')
 
     @mock.patch('curtin.block.clear_holders.LOG')
     @mock.patch('curtin.block.clear_holders.block')