Merge ~raharper/curtin:fix/get_part_table_type into curtin:master

[Ryan Harper]

block: check for fdasd command before using

When probing for vtoc partition table type; this uses fdasd command
which is only present on s390x; return False if we do not have the
command available.

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

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

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Is this general intent to improve execution speed by checking which first before running subp? I don't see how this would emit an unhandled traceback otherwise

[Ryan Harper (raharper)]

Hrm... You're right; I saw fdasd execution in the trace; but I bet you're right that it's not needed.

[Ryan Harper (raharper)]

The other fix which skips checking the part table type if ptable is not present will fix the issue I saw. And on non-s390x, you're right we'll run fdasd, fail, and return False.

Unmerged commits

d452311... by Ryan Harper on 2020-04-16

block: check for fdasd command before using

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index aa5c6f7..2c7cc0e 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -962,6 +962,9 @@ def check_efi_signature(device):
 
 def check_vtoc_signature(device):
     """ check if the specified device has a vtoc partition table. """
+    if not util.which('fdasd'):
+        return False
+
     devname = dev_path(path_to_kname(device))
     try:
         util.subp(['fdasd', '--table', devname])
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index 8ba8333..ab13d65 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -567,17 +567,30 @@ class TestPartTableSignature(CiTestCase):
                 (self.assertTrue if expected else self.assertFalse)(
                     block.check_efi_signature(self.blockdev))
 
+    @mock.patch('curtin.block.util.which')
     @mock.patch('curtin.block.util.subp')
-    def test_check_vtoc_signature_finds_vtoc_returns_true(self, mock_subp):
+    def test_check_vtoc_signature_finds_vtoc_returns_true(self, mock_subp,
+                                                          mock_which):
+        mock_which.return_value = True
         mock_subp.return_value = ("vtoc.....ok", "")
         self.assertTrue(block.check_vtoc_signature(self.blockdev))
 
+    @mock.patch('curtin.block.util.which')
     @mock.patch('curtin.block.util.subp')
-    def test_check_vtoc_signature_returns_false_with_no_sig(self, mock_subp):
+    def test_check_vtoc_signature_returns_false_with_no_sig(self, mock_subp,
+                                                            mock_which):
+        mock_which.return_value = True
         mock_subp.side_effect = [
             util.ProcessExecutionError(stdout="", stderr="", exit_code=1)]
         self.assertFalse(block.check_vtoc_signature(self.blockdev))
 
+    @mock.patch('curtin.block.util.which')
+    @mock.patch('curtin.block.util.subp')
+    def test_check_vtoc_signature_returns_false_with_no_fdasd(self, mock_subp,
+                                                              mock_which):
+        mock_which.return_value = False
+        self.assertFalse(block.check_vtoc_signature(self.blockdev))
+
 
 class TestNonAscii(CiTestCase):
     @mock.patch('curtin.block.util.subp')