curtin

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

  1. Git
  2. lp:~raharper/curtin
  3. fix/eui-is-not-wwn
  4. Merge into master
Proposed by Ryan Harper on 2019-10-22
Status: Merged
Approved by: Ryan Harper on 2019-10-24
Approved revision: 3841a0dca1314b8311456316f7eb30da403d083f
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~raharper/curtin:fix/eui-is-not-wwn
Merge into: curtin:master
Diff against target: 210 lines (+127/-14)
4 files modified
curtin/commands/block_meta.py (+8/-12)
tests/unittests/test_block.py (+26/-0)
tests/unittests/test_commands_block_meta.py (+90/-1)
tests/unittests/test_curthooks.py (+3/-1)
Reviewer Review Type Date Requested Status
Chad Smith Approve on 2019-10-24
Server Team CI bot continuous-integration Approve on 2019-10-24
Dan Watkins 2019-10-22 Approve on 2019-10-23
Review via email: mp+374566@code.launchpad.net

Commit message

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

To post a comment you must log in.
~raharper/curtin:fix/eui-is-not-wwn updated on 2019-10-22
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) wrote :
review: Needs Fixing (continuous-integration)
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Dan Watkins (oddbloke) wrote :

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
Ryan Harper (raharper) :
~raharper/curtin:fix/eui-is-not-wwn updated on 2019-10-23
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) wrote :

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) wrote :
review: Approve (continuous-integration)
Chad Smith (chad.smith) wrote :

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
Ryan Harper (raharper) :
Chad Smith (chad.smith) wrote :

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

review: Approve
~raharper/curtin:fix/eui-is-not-wwn updated on 2019-10-23
3841a0d... by Ryan Harper on 2019-10-23

Drop inner try/except and continue on Exception

Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Ryan Harper (raharper) wrote :

rebuilding, git.lp was unhappy.

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Chad Smith (chad.smith) wrote :

Looks good thanks for the updates.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
2index 37b93dd..cb2122e 100644
3--- a/curtin/commands/block_meta.py
4+++ b/curtin/commands/block_meta.py
5@@ -404,9 +404,8 @@ def get_path_to_storage_volume(volume, storage_config):
6 try:
7 if not vol_value:
8 continue
9- if disk_key == 'serial':
10+ if disk_key in ['wwn', 'serial']:
11 volume_path = block.lookup_disk(vol_value)
12- break
13 elif disk_key == 'path':
14 if vol_value.startswith('iscsi:'):
15 i = iscsi.ensure_disk_connected(vol_value)
16@@ -416,21 +415,18 @@ def get_path_to_storage_volume(volume, storage_config):
17 # sys/class/block access is valid. ie, there are no
18 # udev generated values in sysfs
19 volume_path = os.path.realpath(vol_value)
20- break
21- elif disk_key == 'wwn':
22- by_wwn = '/dev/disk/by-id/wwn-%s' % vol.get('wwn')
23- volume_path = os.path.realpath(by_wwn)
24- break
25 elif disk_key == 'device_id':
26 dasd_device = dasd.DasdDevice(vol_value)
27 volume_path = dasd_device.devname
28- break
29 except ValueError:
30- pass
31+ continue
32+ # verify path exists otherwise try the next key
33+ if not os.path.exists(volume_path):
34+ volume_path = None
35
36- if not volume_path:
37- raise ValueError("serial, wwn or path to block dev must be \
38- specified to identify disk")
39+ if volume_path is None:
40+ raise ValueError("Failed to find storage volume id='%s' config: %s"
41+ % (vol['id'], vol))
42
43 elif vol.get('type') == "lvm_partition":
44 # For lvm partitions, a directory in /dev/ should be present with the
45diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
46index 167a697..e70503d 100644
47--- a/tests/unittests/test_block.py
48+++ b/tests/unittests/test_block.py
49@@ -103,6 +103,32 @@ class TestBlock(CiTestCase):
50 mock_os_listdir.return_value = ["other"]
51 block.lookup_disk(serial)
52
53+ @mock.patch("curtin.block.multipath")
54+ @mock.patch("curtin.block.os.path.realpath")
55+ @mock.patch("curtin.block.os.path.exists")
56+ @mock.patch("curtin.block.os.listdir")
57+ def test_lookup_disk_find_wwn(self, mock_os_listdir, mock_os_path_exists,
58+ mock_os_path_realpath, mock_mpath):
59+ wwn = "eui.0025388b710116a1"
60+ expected_link = 'nvme-%s' % wwn
61+ device = '/wark/nvme0n1'
62+ mock_os_listdir.return_value = [
63+ "nvme-eui.0025388b710116a1",
64+ "nvme-eui.0025388b710116a1-part1",
65+ "nvme-eui.0025388b710116a1-part2",
66+ ]
67+ mock_os_path_exists.return_value = True
68+ mock_os_path_realpath.return_value = device
69+ mock_mpath.is_mpath_device.return_value = False
70+
71+ path = block.lookup_disk(wwn)
72+
73+ mock_os_listdir.assert_called_with("/dev/disk/by-id/")
74+ mock_os_path_realpath.assert_called_with("/dev/disk/by-id/" +
75+ expected_link)
76+ self.assertTrue(mock_os_path_exists.called)
77+ self.assertEqual(device, path)
78+
79 @mock.patch('curtin.block.udevadm_info')
80 def test_get_device_mapper_links_returns_first_non_none(self, m_info):
81 """ get_device_mapper_links returns first by sort entry in DEVLINKS."""
82diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
83index b2e151e..dab133e 100644
84--- a/tests/unittests/test_commands_block_meta.py
85+++ b/tests/unittests/test_commands_block_meta.py
86@@ -11,6 +11,96 @@ from curtin import paths, util
87 from .helpers import CiTestCase
88
89
90+class TestGetPathToStorageVolume(CiTestCase):
91+
92+ def setUp(self):
93+ super(TestGetPathToStorageVolume, self).setUp()
94+ basepath = 'curtin.commands.block_meta.'
95+ self.add_patch(basepath + 'os.path.exists', 'm_exists')
96+ self.add_patch(basepath + 'block.lookup_disk', 'm_lookup')
97+ self.add_patch(basepath + 'devsync', 'm_devsync')
98+
99+ def test_block_lookup_called_with_disk_wwn(self):
100+ volume = 'mydisk'
101+ wwn = self.random_string()
102+ cfg = {'id': volume, 'type': 'disk', 'wwn': wwn}
103+ s_cfg = OrderedDict({volume: cfg})
104+ block_meta.get_path_to_storage_volume(volume, s_cfg)
105+ expected_calls = [call(wwn)]
106+ self.assertEqual(expected_calls, self.m_lookup.call_args_list)
107+
108+ def test_block_lookup_called_with_disk_serial(self):
109+ volume = 'mydisk'
110+ serial = self.random_string()
111+ cfg = {'id': volume, 'type': 'disk', 'serial': serial}
112+ s_cfg = OrderedDict({volume: cfg})
113+ block_meta.get_path_to_storage_volume(volume, s_cfg)
114+ expected_calls = [call(serial)]
115+ self.assertEqual(expected_calls, self.m_lookup.call_args_list)
116+
117+ def test_block_lookup_called_with_disk_wwn_fallback_to_serial(self):
118+ volume = 'mydisk'
119+ wwn = self.random_string()
120+ serial = self.random_string()
121+ cfg = {'id': volume, 'type': 'disk', 'wwn': wwn, 'serial': serial}
122+ s_cfg = OrderedDict({volume: cfg})
123+
124+ # doesn't find wwn, returns path on serial
125+ self.m_lookup.side_effect = iter([ValueError('Error'), 'foo'])
126+
127+ block_meta.get_path_to_storage_volume(volume, s_cfg)
128+ expected_calls = [call(wwn), call(serial)]
129+ self.assertEqual(expected_calls, self.m_lookup.call_args_list)
130+
131+ def test_fallback_to_path_when_lookup_wwn_serial_fail(self):
132+ volume = 'mydisk'
133+ wwn = self.random_string()
134+ serial = self.random_string()
135+ path = "/%s/%s" % (self.random_string(), self.random_string())
136+ cfg = {'id': volume, 'type': 'disk',
137+ 'path': path, 'wwn': wwn, 'serial': serial}
138+ s_cfg = OrderedDict({volume: cfg})
139+
140+ # lookups fail
141+ self.m_lookup.side_effect = iter([
142+ ValueError('Error'), ValueError('Error')])
143+
144+ result = block_meta.get_path_to_storage_volume(volume, s_cfg)
145+ expected_calls = [call(wwn), call(serial)]
146+ self.assertEqual(expected_calls, self.m_lookup.call_args_list)
147+ self.assertEqual(path, result)
148+
149+ def test_block_lookup_not_called_with_wwn_or_serial_keys(self):
150+ volume = 'mydisk'
151+ path = "/%s/%s" % (self.random_string(), self.random_string())
152+ cfg = {'id': volume, 'type': 'disk', 'path': path}
153+ s_cfg = OrderedDict({volume: cfg})
154+ result = block_meta.get_path_to_storage_volume(volume, s_cfg)
155+ self.assertEqual(0, self.m_lookup.call_count)
156+ self.assertEqual(path, result)
157+
158+ def test_exception_raise_if_disk_not_found(self):
159+ volume = 'mydisk'
160+ wwn = self.random_string()
161+ serial = self.random_string()
162+ path = "/%s/%s" % (self.random_string(), self.random_string())
163+ cfg = {'id': volume, 'type': 'disk',
164+ 'path': path, 'wwn': wwn, 'serial': serial}
165+ s_cfg = OrderedDict({volume: cfg})
166+
167+ # lookups fail
168+ self.m_lookup.side_effect = iter([
169+ ValueError('Error'), ValueError('Error')])
170+ # no path
171+ self.m_exists.return_value = False
172+
173+ with self.assertRaises(ValueError):
174+ block_meta.get_path_to_storage_volume(volume, s_cfg)
175+ expected_calls = [call(wwn), call(serial)]
176+ self.assertEqual(expected_calls, self.m_lookup.call_args_list)
177+ self.m_exists.assert_has_calls([call(path)])
178+
179+
180 class TestBlockMetaSimple(CiTestCase):
181 def setUp(self):
182 super(TestBlockMetaSimple, self).setUp()
183@@ -1086,5 +1176,4 @@ class TestDmCryptHandler(CiTestCase):
184 self.m_subp.assert_has_calls(expected_calls)
185 self.assertEqual(len(util.load_file(self.crypttab).splitlines()), 1)
186
187-
188 # vi: ts=4 expandtab syntax=python
189diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
190index 613f068..95276f7 100644
191--- a/tests/unittests/test_curthooks.py
192+++ b/tests/unittests/test_curthooks.py
193@@ -353,7 +353,8 @@ class TestSetupGrub(CiTestCase):
194 self.target, '/dev/vdb'],),
195 self.mock_subp.call_args_list[0][0])
196
197- def test_uses_grub_install_on_storage_config(self):
198+ @patch('curtin.commands.curthooks.os.path.exists')
199+ def test_uses_grub_install_on_storage_config(self, m_exists):
200 cfg = {
201 'storage': {
202 'version': 1,
203@@ -368,6 +369,7 @@ class TestSetupGrub(CiTestCase):
204 },
205 }
206 self.subp_output.append(('', ''))
207+ m_exists.return_value = True
208 curthooks.setup_grub(cfg, self.target, osfamily=self.distro_family)
209 self.assertEquals(
210 ([

Subscribers

People subscribed via source and target branches