Merge ~mwhudson/curtin:feature/grub-resilient-boot into ~raharper/curtin:feature/grub-resilient-boot

Proposed by Michael Hudson-Doyle
Status: Needs review
Proposed branch: ~mwhudson/curtin:feature/grub-resilient-boot
Merge into: ~raharper/curtin:feature/grub-resilient-boot
Diff against target: 459 lines (+257/-34) (has conflicts)
7 files modified
curtin/block/__init__.py (+30/-1)
curtin/commands/block_meta.py (+1/-1)
curtin/commands/curthooks.py (+143/-26)
tests/unittests/test_block.py (+24/-1)
tests/unittests/test_commands_block_meta.py (+1/-1)
tests/unittests/test_curthooks.py (+43/-4)
tests/vmtests/test_mdadm_bcache.py (+15/-0)
Conflict in tests/unittests/test_curthooks.py
Conflict in tests/vmtests/test_mdadm_bcache.py
Reviewer Review Type Date Requested Status
Ryan Harper Pending
Review via email: mp+382556@code.launchpad.net

Commit message

store partuuid in debconf

To post a comment you must log in.
b10e778... by Michael Hudson-Doyle on 2020-04-20

update some docstrings

f932beb... by Michael Hudson-Doyle on 2020-04-20

fix tests to read partuuid from installed system

5708850... by Michael Hudson-Doyle on 2020-04-20

xenial compat

Revision history for this message
Ryan Harper (raharper) wrote :

Neat.

As I understand things, partuuid is *only* useful in the case of something like bios_grub which does not have any filesystem on it in which case a GPT uuid is useful.

ESPs will have GPT uuid; though in generally I think /dev/disk/by-id symlinks are just as good as persistent links to the ESP and provide more information in their naming scheme than a UUID.

Unmerged commits

5708850... by Michael Hudson-Doyle on 2020-04-20

xenial compat

f932beb... by Michael Hudson-Doyle on 2020-04-20

fix tests to read partuuid from installed system

b10e778... by Michael Hudson-Doyle on 2020-04-20

update some docstrings

341de09... by Michael Hudson-Doyle on 2020-04-20

lint, small fixes

b2beb1f... by Michael Hudson-Doyle on 2020-04-20

add block.disk_to_bypartuuid_path and use it in configure_grub_debconf

2280103... by Michael Hudson-Doyle on 2020-04-20

change vmtest to expect partuuid in debconf

bd61089... by Ryan Harper on 2020-04-17

Handle byid returning None; return the original device name in that case

7899bdb... by Ryan Harper on 2020-04-17

Fix typo

22ac60b... by Ryan Harper on 2020-04-17

'path' is required element for type:mount

138bd70... by Ryan Harper on 2020-04-17

Ensure we're looking at a type:mount when checking 'path'

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
2index 54ee263..f43cbbb 100644
3--- a/curtin/block/__init__.py
4+++ b/curtin/block/__init__.py
5@@ -819,13 +819,42 @@ def _get_dev_disk_by_prefix(prefix):
6 '/dev/sda1': '/dev/disk/<prefix>/virtio-aaaa-part1',
7 }
8 """
9+ try:
10+ listing = os.listdir(prefix)
11+ except OSError as e:
12+ if e.errno == errno.ENOENT:
13+ return {}
14+ raise
15 return {
16 os.path.realpath(bypfx): bypfx
17 for bypfx in [os.path.join(prefix, path)
18- for path in os.listdir(prefix)]
19+ for path in listing]
20 }
21
22
23+def get_dev_disk_bypartuuid():
24+ """
25+ Construct a dictionary mapping devname to disk/by-partuuid paths
26+
27+ :returns: Dictionary populated by examining /dev/disk/by-partuuid/*
28+
29+ {
30+ '/dev/sda': '/dev/disk/by-partuuid/virtio-aaaa',
31+ '/dev/sda1': '/dev/disk/by-partuuid/virtio-aaaa-part1',
32+ }
33+ """
34+ return _get_dev_disk_by_prefix('/dev/disk/by-partuuid')
35+
36+
37+def disk_to_bypartuuid_path(kname):
38+ """"
39+ Return a /dev/disk/by-partuuid path to kname if present.
40+ """
41+
42+ mapping = get_dev_disk_bypartuuid()
43+ return mapping.get(dev_path(kname))
44+
45+
46 def get_dev_disk_byid():
47 """
48 Construct a dictionary mapping devname to disk/by-id paths
49diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
50index a7332fe..e3c4ce1 100644
51--- a/curtin/commands/block_meta.py
52+++ b/curtin/commands/block_meta.py
53@@ -577,7 +577,7 @@ def disk_handler(info, storage_config):
54 disk = get_path_to_storage_volume(info.get('id'), storage_config)
55 if config.value_as_boolean(info.get('preserve')):
56 # Handle preserve flag, verifying if ptable specified in config
57- if ptable != PTABLE_UNSUPPORTED:
58+ if ptable and ptable != PTABLE_UNSUPPORTED:
59 current_ptable = block.get_part_table_type(disk)
60 LOG.debug('disk: current ptable type: %s', current_ptable)
61 if current_ptable not in PTABLES_SUPPORTED:
62diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
63index 2d3d5c0..323c311 100644
64--- a/curtin/commands/curthooks.py
65+++ b/curtin/commands/curthooks.py
66@@ -444,21 +444,21 @@ def uefi_reorder_loaders(grubcfg, target):
67 front of the BootOrder.
68 """
69 if grubcfg.get('reorder_uefi', True):
70- with util.ChrootableTarget(target):
71- efi_output = util.get_efibootmgr(target=target)
72- currently_booted = efi_output.get('current', None)
73- boot_order = efi_output.get('order', [])
74- if currently_booted:
75- if currently_booted in boot_order:
76- boot_order.remove(currently_booted)
77- boot_order = [currently_booted] + boot_order
78- new_boot_order = ','.join(boot_order)
79- LOG.debug(
80- "Setting currently booted %s as the first "
81- "UEFI loader.", currently_booted)
82- LOG.debug(
83- "New UEFI boot order: %s", new_boot_order)
84- util.subp(['efibootmgr', '-o', new_boot_order])
85+ efi_output = util.get_efibootmgr(target=target)
86+ currently_booted = efi_output.get('current', None)
87+ boot_order = efi_output.get('order', [])
88+ if currently_booted:
89+ if currently_booted in boot_order:
90+ boot_order.remove(currently_booted)
91+ boot_order = [currently_booted] + boot_order
92+ new_boot_order = ','.join(boot_order)
93+ LOG.debug(
94+ "Setting currently booted %s as the first "
95+ "UEFI loader.", currently_booted)
96+ LOG.debug(
97+ "New UEFI boot order: %s", new_boot_order)
98+ with util.ChrootableTarget(target) as in_chroot:
99+ in_chroot.subp(['efibootmgr', '-o', new_boot_order])
100 else:
101 LOG.debug("Skipped reordering of UEFI boot methods.")
102 LOG.debug("Currently booted UEFI loader might no longer boot.")
103@@ -466,19 +466,136 @@ def uefi_reorder_loaders(grubcfg, target):
104
105 def uefi_remove_duplicate_entries(grubcfg, target):
106 seen = set()
107- with util.ChrootableTarget(target):
108- efi_output = util.get_efibootmgr(target=target)
109- entries = efi_output.get('entries', {})
110- for bootnum in sorted(entries):
111- entry = entries[bootnum]
112- t = tuple(entry.items())
113- if t not in seen:
114- seen.add(t)
115- else:
116+ to_remove = []
117+ efi_output = util.get_efibootmgr(target=target)
118+ entries = efi_output.get('entries', {})
119+ for bootnum in sorted(entries):
120+ entry = entries[bootnum]
121+ t = tuple(entry.items())
122+ if t not in seen:
123+ seen.add(t)
124+ else:
125+ to_remove.append((bootnum, entry))
126+ if to_remove:
127+ with util.ChrootableTarget(target) as in_chroot:
128+ for bootnum, entry in to_remove:
129 LOG.debug('Removing duplicate EFI entry (%s, %s)',
130 bootnum, entry)
131- util.subp(['efibootmgr', '--bootnum=%s' % bootnum,
132- '--delete-bootnum'])
133+ in_chroot.subp(['efibootmgr', '--bootnum=%s' % bootnum,
134+ '--delete-bootnum'])
135+
136+
137+def _debconf_multiselect(package, variable, choices):
138+ return "{package} {variable} multiselect {choices}".format(
139+ package=package, variable=variable, choices=", ".join(choices))
140+
141+
142+def configure_grub_debconf(boot_devices, target, uefi):
143+ """Configure grub debconf variables in target.
144+
145+ Non-UEFI:
146+ grub-pc grub-pc/install_devices multiselect d1, d2, d3
147+
148+ UEFI:
149+ grub-pc grub-efi/install_devices multiselect d1
150+
151+ """
152+ LOG.debug('Generating grub debconf_selections for devices=%s uefi=%s',
153+ boot_devices, uefi)
154+
155+ links = []
156+ for dev in boot_devices:
157+ link = block.disk_to_bypartuuid_path(dev)
158+ if link is not None:
159+ links.append(link)
160+ continue
161+ link = block.disk_to_byid_path(dev)
162+ if link is not None:
163+ links.append(link)
164+ continue
165+ links.append(dev)
166+
167+ selections = []
168+ if uefi:
169+ selections.append(_debconf_multiselect(
170+ 'grub-pc', 'grub-efi/install_devices', links))
171+ else:
172+ selections.append(_debconf_multiselect(
173+ 'grub-pc', 'grub-pc/install_devices', links))
174+
175+ cfg = {'debconf_selections': {'grub': "\n".join(selections)}}
176+ LOG.info('Applying grub debconf_selections config:\n%s', cfg)
177+ apt_config.apply_debconf_selections(cfg, target)
178+ return
179+
180+
181+def uefi_find_grub_device_ids(sconfig):
182+ """ Scan the provided storage config for device_ids on which we
183+ will install grub. An order of precendence is required due to
184+ legacy configurations which set grub_device on the disk but not
185+ on the ESP config itself. We prefer the latter as this allows
186+ a disk to contain more than on ESP and choose to install grub
187+ to a subset. We always look for the 'primary' ESP which is
188+ signified by being mounted at /boot/efi (only one can be mounted).
189+
190+ 1. ESPs with grub_device: true are the preferred way to find
191+ the specific set of devices on which to install grub
192+ 2. ESPs whose parent disk has grub_device: true
193+
194+ The primary ESP is the first element of the result if any
195+ devices are found.
196+
197+ returns a list of storage-config ids on which grub will be installed.
198+ """
199+ # Only one EFI system partition can be mounted, but backup EFI
200+ # partitions may exist. Find all EFI partitions and determine
201+ # the primary.
202+ grub_device_ids = []
203+ primary_esp = None
204+ grub_partitions = []
205+ esp_partitions = []
206+ for item_id, item in sconfig.items():
207+ if item['type'] == 'partition':
208+ if item.get('grub_device'):
209+ grub_partitions.append(item_id)
210+ continue
211+ elif item.get('flag') == 'boot':
212+ esp_partitions.append(item_id)
213+ continue
214+
215+ if item['type'] == 'mount' and item['path'] == '/boot/efi':
216+ if primary_esp:
217+ LOG.debug('Ignoring duplicate mounted primary ESP: %s',
218+ item_id)
219+ continue
220+ primary_esp = sconfig[item['device']]['volume']
221+ if sconfig[primary_esp]['type'] == 'partition':
222+ LOG.debug("Found primary UEFI ESP: %s", primary_esp)
223+ else:
224+ LOG.warn('Found primary ESP not on a partition: %s', item)
225+
226+ if primary_esp is None:
227+ raise RuntimeError('Failed to find primary ESP mounted at /boot/efi')
228+
229+ # prefer grub_device: true partitions
230+ if len(grub_partitions):
231+ if primary_esp in grub_partitions:
232+ grub_partitions.remove(primary_esp)
233+ # insert the primary esp as first element
234+ grub_device_ids = [primary_esp] + grub_partitions
235+
236+ # look at all esp entries, check if parent disk is grub_device: true
237+ elif len(esp_partitions):
238+ if primary_esp in esp_partitions:
239+ esp_partitions.remove(primary_esp)
240+ grub_device_ids = [primary_esp]
241+ for esp_id in esp_partitions:
242+ esp_disk = sconfig[sconfig[esp_id]['device']]
243+ if esp_disk.get('grub_device'):
244+ grub_device_ids.append(esp_id)
245+
246+ LOG.debug('Found UEFI ESP(s) for grub install: %s', grub_device_ids)
247+ return grub_device_ids
248
249
250 def _debconf_multiselect(package, variable, choices):
251diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
252index 707386f..5fdcc2b 100644
253--- a/tests/unittests/test_block.py
254+++ b/tests/unittests/test_block.py
255@@ -4,6 +4,7 @@ import functools
256 import os
257 import mock
258 import sys
259+import uuid
260
261 from collections import OrderedDict
262
263@@ -157,7 +158,7 @@ class TestBlock(CiTestCase):
264
265 @mock.patch("curtin.block.get_dev_disk_byid")
266 def test_disk_to_byid_path(self, mock_byid):
267- """ disk_to_byid path returns a /dev/disk/by-id path """
268+ """ disk_to_byid_path returns a /dev/disk/by-id path """
269 mapping = {
270 '/dev/sda': '/dev/disk/by-id/scsi-abcdef',
271 }
272@@ -177,6 +178,28 @@ class TestBlock(CiTestCase):
273 byid_path = block.disk_to_byid_path('/dev/sdb')
274 self.assertEqual(mapping.get('/dev/sdb'), byid_path)
275
276+ @mock.patch("curtin.block.get_dev_disk_bypartuuid")
277+ def test_disk_to_bypartuuid_path(self, mock_bypartuuid):
278+ """ disk_to_bypartuuid_path returns a /dev/disk/by-partuuid path """
279+ mapping = {
280+ '/dev/sda': '/dev/disk/by-uuid/' + str(uuid.uuid4()),
281+ }
282+ mock_bypartuuid.return_value = mapping
283+
284+ byid_path = block.disk_to_bypartuuid_path('/dev/sda')
285+ self.assertEqual(mapping['/dev/sda'], byid_path)
286+
287+ @mock.patch("curtin.block.get_dev_disk_bypartuuid")
288+ def test_disk_to_bypartuuid_path_notfound(self, mock_bypartuuid):
289+ """ disk_to_bypartuuid_path returns a /dev/disk/by-partuuid path """
290+ mapping = {
291+ '/dev/sda': '/dev/disk/by-partuuid/' + str(uuid.uuid4()),
292+ }
293+ mock_bypartuuid.return_value = mapping
294+
295+ byid_path = block.disk_to_bypartuuid_path('/dev/sdb')
296+ self.assertEqual(None, byid_path)
297+
298
299 class TestSysBlockPath(CiTestCase):
300 @mock.patch("curtin.block.get_blockdev_for_partition")
301diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
302index 2ed3cb3..aac1a9e 100644
303--- a/tests/unittests/test_commands_block_meta.py
304+++ b/tests/unittests/test_commands_block_meta.py
305@@ -1269,7 +1269,7 @@ class TestDiskHandler(CiTestCase):
306 m_getpath.return_value = disk_path
307 block_meta.disk_handler(info, storage_config)
308 m_getpath.assert_called_with(info['id'], storage_config)
309- self.assertEqual(1, m_block.get_part_table_type.call_count)
310+ self.assertEqual(0, m_block.get_part_table_type.call_count)
311
312 @patch('curtin.commands.block_meta.block')
313 @patch('curtin.commands.block_meta.util')
314diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
315index ac72dfa..c7a6736 100644
316--- a/tests/unittests/test_curthooks.py
317+++ b/tests/unittests/test_curthooks.py
318@@ -793,12 +793,12 @@ class TestSetupGrub(CiTestCase):
319 },
320 }
321 }
322- self.subp_output.append(('', ''))
323+ self.in_chroot_subp_output.append(('', ''))
324 self.mock_haspkg.return_value = False
325 curthooks.setup_grub(cfg, self.target, osfamily=self.distro_family)
326 self.assertEquals(
327 (['efibootmgr', '-o', '0001,0000'],),
328- self.mock_subp.call_args_list[1][0])
329+ self.mock_in_chroot_subp.call_args_list[0][0])
330
331
332 class TestUefiRemoveDuplicateEntries(CiTestCase):
333@@ -845,8 +845,10 @@ class TestUefiRemoveDuplicateEntries(CiTestCase):
334
335 curthooks.uefi_remove_duplicate_entries(cfg, self.target)
336 self.assertEquals([
337- call(['efibootmgr', '--bootnum=0001', '--delete-bootnum']),
338- call(['efibootmgr', '--bootnum=0003', '--delete-bootnum'])],
339+ call(['efibootmgr', '--bootnum=0001', '--delete-bootnum'],
340+ target=self.target),
341+ call(['efibootmgr', '--bootnum=0003', '--delete-bootnum'],
342+ target=self.target)],
343 self.m_subp.call_args_list)
344
345 @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
346@@ -1650,6 +1652,10 @@ class TestCurthooksGrubDebconf(CiTestCase):
347 self.add_patch(
348 base + 'apt_config.apply_debconf_selections', 'm_debconf')
349 self.add_patch(base + 'block.disk_to_byid_path', 'm_byid')
350+<<<<<<< tests/unittests/test_curthooks.py
351+=======
352+ self.add_patch(base + 'block.disk_to_bypartuuid_path', 'm_bypartuuid')
353+>>>>>>> tests/unittests/test_curthooks.py
354
355 def test_debconf_multiselect(self):
356 package = self.random_string()
357@@ -1667,6 +1673,10 @@ class TestCurthooksGrubDebconf(CiTestCase):
358 byid_boot_devs = ["/dev/disk/by-id/" + dev for dev in boot_devs]
359 uefi = False
360 self.m_byid.side_effect = (lambda x: '/dev/disk/by-id/' + x)
361+<<<<<<< tests/unittests/test_curthooks.py
362+=======
363+ self.m_bypartuuid.side_effect = (lambda x: None)
364+>>>>>>> tests/unittests/test_curthooks.py
365 curthooks.configure_grub_debconf(boot_devs, target, uefi)
366 expected_selection = [
367 ('grub-pc grub-pc/install_devices '
368@@ -1679,6 +1689,7 @@ class TestCurthooksGrubDebconf(CiTestCase):
369 def test_configure_grub_debconf_uefi_enabled(self):
370 target = self.random_string()
371 boot_devs = [self.random_string()]
372+<<<<<<< tests/unittests/test_curthooks.py
373 byid_boot_devs = ["/dev/disk/by-id/" + dev for dev in boot_devs]
374 uefi = True
375 self.m_byid.side_effect = (lambda x: '/dev/disk/by-id/' + x)
376@@ -1686,6 +1697,18 @@ class TestCurthooksGrubDebconf(CiTestCase):
377 expected_selection = [
378 ('grub-pc grub-efi/install_devices '
379 'multiselect %s' % ", ".join(byid_boot_devs))
380+=======
381+ bypartuuid_boot_devs = [
382+ "/dev/disk/by-partuuid/" + dev for dev in boot_devs]
383+ uefi = True
384+ self.m_byid.side_effect = (lambda x: 1/0)
385+ self.m_bypartuuid.side_effect = (
386+ lambda x: "/dev/disk/by-partuuid/" + x)
387+ curthooks.configure_grub_debconf(boot_devs, target, uefi)
388+ expected_selection = [
389+ ('grub-pc grub-efi/install_devices '
390+ 'multiselect %s' % ", ".join(bypartuuid_boot_devs))
391+>>>>>>> tests/unittests/test_curthooks.py
392 ]
393 expectedcfg = {
394 'debconf_selections': {'grub': "\n".join(expected_selection)}}
395@@ -1693,6 +1716,7 @@ class TestCurthooksGrubDebconf(CiTestCase):
396
397 def test_configure_grub_debconf_handle_no_byid_result(self):
398 target = self.random_string()
399+<<<<<<< tests/unittests/test_curthooks.py
400 boot_devs = ['aaaaa', 'bbbbb']
401 uefi = True
402 self.m_byid.side_effect = (
403@@ -1701,6 +1725,21 @@ class TestCurthooksGrubDebconf(CiTestCase):
404 expected_selection = [
405 ('grub-pc grub-efi/install_devices '
406 'multiselect /dev/disk/by-id/aaaaa, bbbbb')
407+=======
408+ boot_devs = ['aaaaa', 'bbbbb', 'ccccc']
409+ uefi = True
410+ self.m_byid.side_effect = (
411+ lambda x: (
412+ '/dev/disk/by-id/' + x if 'a' in x or 'b' in x else None))
413+ self.m_bypartuuid.side_effect = (
414+ lambda x: (
415+ '/dev/disk/by-partuuid/' + x if 'a' in x else None))
416+ curthooks.configure_grub_debconf(boot_devs, target, uefi)
417+ expected_selection = [
418+ ('grub-pc grub-efi/install_devices '
419+ 'multiselect /dev/disk/by-partuuid/aaaaa, '
420+ '/dev/disk/by-id/bbbbb, ccccc')
421+>>>>>>> tests/unittests/test_curthooks.py
422 ]
423 expectedcfg = {
424 'debconf_selections': {'grub': "\n".join(expected_selection)}}
425diff --git a/tests/vmtests/test_mdadm_bcache.py b/tests/vmtests/test_mdadm_bcache.py
426index 865a36e..c569334 100644
427--- a/tests/vmtests/test_mdadm_bcache.py
428+++ b/tests/vmtests/test_mdadm_bcache.py
429@@ -282,6 +282,11 @@ class TestMirrorbootPartitionsUEFIAbs(TestMdadmAbs):
430 (cd /boot/efi && find .) | sort > diska-part1-efi.out
431 mount /dev/disk/by-id/virtio-disk-b-part1 /mnt
432 (cd /mnt && find .) | sort > diskb-part1-efi.out
433+<<<<<<< tests/vmtests/test_mdadm_bcache.py
434+=======
435+ lsblk -n -o PARTUUID /dev/disk/by-id/virtio-disk-a-part1 > disk-a-ptuuid.out
436+ lsblk -n -o PARTUUID /dev/disk/by-id/virtio-disk-b-part1 > disk-b-ptuuid.out
437+>>>>>>> tests/vmtests/test_mdadm_bcache.py
438 umount /mnt
439 exit 0
440 """)]
441@@ -298,9 +303,19 @@ class TestMirrorbootPartitionsUEFIAbs(TestMdadmAbs):
442 """Verify we have grub2/efi_install_devices set correctly."""
443 selections = self.load_collect_file("debconf_selections.txt")
444 found_selections = re.findall(self.GRUB_RE, selections, re.MULTILINE)
445+<<<<<<< tests/vmtests/test_mdadm_bcache.py
446 disks_byid = ['/dev/disk/by-id/virtio-disk-a-part1',
447 '/dev/disk/by-id/virtio-disk-b-part1']
448 choice = ", ".join(disks_byid)
449+=======
450+ disk_a_ptuuid = self.load_collect_file("disk-a-ptuuid.out").strip()
451+ disk_b_ptuuid = self.load_collect_file("disk-b-ptuuid.out").strip()
452+ disks_bypartuuid = [
453+ '/dev/disk/by-partuuid/' + disk_a_ptuuid,
454+ '/dev/disk/by-partuuid/' + disk_b_ptuuid,
455+ ]
456+ choice = ", ".join(disks_bypartuuid)
457+>>>>>>> tests/vmtests/test_mdadm_bcache.py
458 self.assertIn(
459 ('grub-pc', 'grub-efi/install_devices', choice), found_selections)
460

Subscribers

People subscribed via source and target branches