Merge ~raharper/curtin:fix/eui-is-not-wwn into curtin:master

[Ryan Harper]

block_meta: use lookup for wwn, fix fallback from wwn, serial, path

NVMe devices may provide a ID_WWN value from udev, but the NVMe rules
do not create a by-id/wwn-$ID_WWN symlink which breaks behavior with
other ID_WWN devices resulting in curtin not finding certain NVMe
devices. Resolve this issue buy using block.lookup_disk on WWN values
as well as serial.

Additional fixes to block_meta.get_path_to_storage_volume:
- get_path_to_storage_volume will try wwn, serial, and ultimately
  path values in order to find the specified disk
- Add os.path.exists check to proposed constructed values, if the
  path is not valid, we continue to use different keys if possible.
- Updated the Exception message to indicate the volume wasn't found
  and emit the config it used.

LP: #1849322

fa3dd45... by Ryan Harper on 2019-10-22

Drop vmtest wwn; that does not work the way we need

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

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

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Approve (continuous-integration)

[Dan Watkins (oddbloke)]

A couple of nits (so leaving to you to merge) and some questions for my own edification, but looks good to me. Thanks!

review: Approve

493fa9f... by Ryan Harper on 2019-10-23

Move path check to bottom of loop

92188d8... by Ryan Harper on 2019-10-23

Use assertEqual with expected list of calls

[Dan Watkins (oddbloke)]

On Wed, Oct 23, 2019 at 08:22:56PM -0000, Ryan Harper wrote:
> Diff comments:
>
> > diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
> > index 37b93dd..74faa63 100644
> > --- a/curtin/commands/block_meta.py
> > +++ b/curtin/commands/block_meta.py
> > @@ -416,21 +419,23 @@ def get_path_to_storage_volume(volume, storage_config):
> > # sys/class/block access is valid. ie, there are no
> > # udev generated values in sysfs
> > volume_path = os.path.realpath(vol_value)
> > - break
> > - elif disk_key == 'wwn':
> > - by_wwn = '/dev/disk/by-id/wwn-%s' % vol.get('wwn')
> > - volume_path = os.path.realpath(by_wwn)
> > + if not os.path.exists(volume_path):
>
> Yes, I can move to bottom of loop.

Thanks!

> > + block_meta.get_path_to_storage_volume(volume, s_cfg)
> > + self.m_lookup.assert_called_with(wwn)
>
> OK, I think I can convert to an assertEqual on the call_args_list.

I think assert_called_with more accurately maps to `assertIn` (though
`assertEqual` may be more correct in the context of a given test, which
would be more like `assert_called_once_with`).

`assert_has_calls` can most easily either be `assertEqual([call(...),
call(...)], m.call_args_list)` for strict ordering, or the same but with
the list wrapped in `set()` for non-strict ordering. Strictly speaking,
though, I think assert_has_calls is really doing something more like
`assertTrue(set([call(...), call(..)]).issubset(set(m.call_args_list)))`
but that probably doesn't matter in most contexts.

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

PASSED: Continuous integration, rev:92188d82e389247517ac5b4d580eaffd72e78cf4
https://jenkins.ubuntu.com/server/job/curtin-ci/3766/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3766/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3766/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3766/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3766/

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

I think we might need to fix the two points I mentioned inline. Will look again when you get a chance to review.

review: Needs Fixing

[Chad Smith (chad.smith)]

Approve once we drop the inner try/except and manipulate the outer try/except ValueError to be a 'continue' instead of 'pass'

review: Approve

3841a0d... by Ryan Harper on 2019-10-23

Drop inner try/except and continue on Exception

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

FAILED: Continuous integration, rev:3841a0dca1314b8311456316f7eb30da403d083f
https://jenkins.ubuntu.com/server/job/curtin-ci/3767/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3767/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3767/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3767/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3767/

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

review: Needs Fixing (continuous-integration)

[Ryan Harper (raharper)]

rebuilding, git.lp was unhappy.

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

PASSED: Continuous integration, rev:3841a0dca1314b8311456316f7eb30da403d083f
https://jenkins.ubuntu.com/server/job/curtin-ci/3770/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3770/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3770/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3770/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3770/

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Looks good thanks for the updates.

review: Approve

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 37b93dd..cb2122e 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -404,9 +404,8 @@ def get_path_to_storage_volume(volume, storage_config):
             try:
                 if not vol_value:
                     continue
-                if disk_key == 'serial':
+                if disk_key in ['wwn', 'serial']:
                     volume_path = block.lookup_disk(vol_value)
-                    break
                 elif disk_key == 'path':
                     if vol_value.startswith('iscsi:'):
                         i = iscsi.ensure_disk_connected(vol_value)
@@ -416,21 +415,18 @@ def get_path_to_storage_volume(volume, storage_config):
                         # sys/class/block access is valid.  ie, there are no
                         # udev generated values in sysfs
                         volume_path = os.path.realpath(vol_value)
-                    break
-                elif disk_key == 'wwn':
-                    by_wwn = '/dev/disk/by-id/wwn-%s' % vol.get('wwn')
-                    volume_path = os.path.realpath(by_wwn)
-                    break
                 elif disk_key == 'device_id':
                     dasd_device = dasd.DasdDevice(vol_value)
                     volume_path = dasd_device.devname
-                    break
             except ValueError:
-                pass
+                continue
+            # verify path exists otherwise try the next key
+            if not os.path.exists(volume_path):
+                volume_path = None
 
-        if not volume_path:
-            raise ValueError("serial, wwn or path to block dev must be \
-                specified to identify disk")
+        if volume_path is None:
+            raise ValueError("Failed to find storage volume id='%s' config: %s"
+                             % (vol['id'], vol))
 
     elif vol.get('type') == "lvm_partition":
         # For lvm partitions, a directory in /dev/ should be present with the
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index 167a697..e70503d 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -103,6 +103,32 @@ class TestBlock(CiTestCase):
             mock_os_listdir.return_value = ["other"]
             block.lookup_disk(serial)
 
+    @mock.patch("curtin.block.multipath")
+    @mock.patch("curtin.block.os.path.realpath")
+    @mock.patch("curtin.block.os.path.exists")
+    @mock.patch("curtin.block.os.listdir")
+    def test_lookup_disk_find_wwn(self, mock_os_listdir, mock_os_path_exists,
+                                  mock_os_path_realpath, mock_mpath):
+        wwn = "eui.0025388b710116a1"
+        expected_link = 'nvme-%s' % wwn
+        device = '/wark/nvme0n1'
+        mock_os_listdir.return_value = [
+            "nvme-eui.0025388b710116a1",
+            "nvme-eui.0025388b710116a1-part1",
+            "nvme-eui.0025388b710116a1-part2",
+        ]
+        mock_os_path_exists.return_value = True
+        mock_os_path_realpath.return_value = device
+        mock_mpath.is_mpath_device.return_value = False
+
+        path = block.lookup_disk(wwn)
+
+        mock_os_listdir.assert_called_with("/dev/disk/by-id/")
+        mock_os_path_realpath.assert_called_with("/dev/disk/by-id/" +
+                                                 expected_link)
+        self.assertTrue(mock_os_path_exists.called)
+        self.assertEqual(device, path)
+
     @mock.patch('curtin.block.udevadm_info')
     def test_get_device_mapper_links_returns_first_non_none(self, m_info):
         """ get_device_mapper_links returns first by sort entry in DEVLINKS."""
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index b2e151e..dab133e 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -11,6 +11,96 @@ from curtin import paths, util
 from .helpers import CiTestCase
 
 
+class TestGetPathToStorageVolume(CiTestCase):
+
+    def setUp(self):
+        super(TestGetPathToStorageVolume, self).setUp()
+        basepath = 'curtin.commands.block_meta.'
+        self.add_patch(basepath + 'os.path.exists', 'm_exists')
+        self.add_patch(basepath + 'block.lookup_disk', 'm_lookup')
+        self.add_patch(basepath + 'devsync', 'm_devsync')
+
+    def test_block_lookup_called_with_disk_wwn(self):
+        volume = 'mydisk'
+        wwn = self.random_string()
+        cfg = {'id': volume, 'type': 'disk', 'wwn': wwn}
+        s_cfg = OrderedDict({volume: cfg})
+        block_meta.get_path_to_storage_volume(volume, s_cfg)
+        expected_calls = [call(wwn)]
+        self.assertEqual(expected_calls, self.m_lookup.call_args_list)
+
+    def test_block_lookup_called_with_disk_serial(self):
+        volume = 'mydisk'
+        serial = self.random_string()
+        cfg = {'id': volume, 'type': 'disk', 'serial': serial}
+        s_cfg = OrderedDict({volume: cfg})
+        block_meta.get_path_to_storage_volume(volume, s_cfg)
+        expected_calls = [call(serial)]
+        self.assertEqual(expected_calls, self.m_lookup.call_args_list)
+
+    def test_block_lookup_called_with_disk_wwn_fallback_to_serial(self):
+        volume = 'mydisk'
+        wwn = self.random_string()
+        serial = self.random_string()
+        cfg = {'id': volume, 'type': 'disk', 'wwn': wwn, 'serial': serial}
+        s_cfg = OrderedDict({volume: cfg})
+
+        # doesn't find wwn, returns path on serial
+        self.m_lookup.side_effect = iter([ValueError('Error'), 'foo'])
+
+        block_meta.get_path_to_storage_volume(volume, s_cfg)
+        expected_calls = [call(wwn), call(serial)]
+        self.assertEqual(expected_calls, self.m_lookup.call_args_list)
+
+    def test_fallback_to_path_when_lookup_wwn_serial_fail(self):
+        volume = 'mydisk'
+        wwn = self.random_string()
+        serial = self.random_string()
+        path = "/%s/%s" % (self.random_string(), self.random_string())
+        cfg = {'id': volume, 'type': 'disk',
+               'path': path, 'wwn': wwn, 'serial': serial}
+        s_cfg = OrderedDict({volume: cfg})
+
+        # lookups fail
+        self.m_lookup.side_effect = iter([
+            ValueError('Error'), ValueError('Error')])
+
+        result = block_meta.get_path_to_storage_volume(volume, s_cfg)
+        expected_calls = [call(wwn), call(serial)]
+        self.assertEqual(expected_calls, self.m_lookup.call_args_list)
+        self.assertEqual(path, result)
+
+    def test_block_lookup_not_called_with_wwn_or_serial_keys(self):
+        volume = 'mydisk'
+        path = "/%s/%s" % (self.random_string(), self.random_string())
+        cfg = {'id': volume, 'type': 'disk', 'path': path}
+        s_cfg = OrderedDict({volume: cfg})
+        result = block_meta.get_path_to_storage_volume(volume, s_cfg)
+        self.assertEqual(0, self.m_lookup.call_count)
+        self.assertEqual(path, result)
+
+    def test_exception_raise_if_disk_not_found(self):
+        volume = 'mydisk'
+        wwn = self.random_string()
+        serial = self.random_string()
+        path = "/%s/%s" % (self.random_string(), self.random_string())
+        cfg = {'id': volume, 'type': 'disk',
+               'path': path, 'wwn': wwn, 'serial': serial}
+        s_cfg = OrderedDict({volume: cfg})
+
+        # lookups fail
+        self.m_lookup.side_effect = iter([
+            ValueError('Error'), ValueError('Error')])
+        # no path
+        self.m_exists.return_value = False
+
+        with self.assertRaises(ValueError):
+            block_meta.get_path_to_storage_volume(volume, s_cfg)
+        expected_calls = [call(wwn), call(serial)]
+        self.assertEqual(expected_calls, self.m_lookup.call_args_list)
+        self.m_exists.assert_has_calls([call(path)])
+
+
 class TestBlockMetaSimple(CiTestCase):
     def setUp(self):
         super(TestBlockMetaSimple, self).setUp()
@@ -1086,5 +1176,4 @@ class TestDmCryptHandler(CiTestCase):
         self.m_subp.assert_has_calls(expected_calls)
         self.assertEqual(len(util.load_file(self.crypttab).splitlines()), 1)
 
-
 # vi: ts=4 expandtab syntax=python
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index 613f068..95276f7 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -353,7 +353,8 @@ class TestSetupGrub(CiTestCase):
                 self.target, '/dev/vdb'],),
             self.mock_subp.call_args_list[0][0])
 
-    def test_uses_grub_install_on_storage_config(self):
+    @patch('curtin.commands.curthooks.os.path.exists')
+    def test_uses_grub_install_on_storage_config(self, m_exists):
         cfg = {
             'storage': {
                 'version': 1,
@@ -368,6 +369,7 @@ class TestSetupGrub(CiTestCase):
             },
         }
         self.subp_output.append(('', ''))
+        m_exists.return_value = True
         curthooks.setup_grub(cfg, self.target, osfamily=self.distro_family)
         self.assertEquals(
             ([