Merge lp:~raharper/curtin/trunk.fix-lp1722322 into lp:~curtin-dev/curtin/trunk

Proposed by Ryan Harper
Status: Merged
Merged at revision: 533
Proposed branch: lp:~raharper/curtin/trunk.fix-lp1722322
Merge into: lp:~curtin-dev/curtin/trunk
Diff against target: 66 lines (+29/-2)
3 files modified
curtin/commands/block_meta.py (+5/-1)
examples/tests/uefi_basic.yaml (+23/-0)
tests/vmtests/test_uefi_basic.py (+1/-1)
To merge this branch: bzr merge lp:~raharper/curtin/trunk.fix-lp1722322
Reviewer Review Type Date Requested Status
Scott Moser (community) Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+332026@code.launchpad.net

Description of the change

block_meta: use block.wipe_volume(mode=superblock) to clear MBR/GPT tables

In the case curtin is not directed to wipe the partition table via the
wipe: configuration, a disk may contain an MBR and sgdisk --clear does
not successfully wipe MBR tables. Replace the call to sgdisk with curtin
block.wipe_volume(mode=superblock) to clear both MBR and GPT

LP: 1722322

To post a comment you must log in.
Revision history for this message
Ryan Harper (raharper) wrote :

I would generally like to recreate this issue by pre-populating an image with an MBR such that the current sgdisk --clear blows up like it does in the bug.

While we do have the dirty-disks mode, that currently runs the same curtin storage config twice; but we really need to apply a separate storage config during the early stage, and then a different one at the normal install stage.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

The dirty disks mode we have just lets the existing config mode pass through.
We can just as easily call curtin-blockmeta with an arbitrary config.

It was just easiest (and immediately effective) to inherit the config from the install environment.

it'd be easy enough to make the 'Injecting early_command to dirty storage devices' code that is in vmtests/__init__.py accept 'cls.dirty_disks' be a config itself or a path to one (rather than just boolean). Then generate a portion of the config that would write the different config.

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

> The dirty disks mode we have just lets the existing config mode pass through.
> We can just as easily call curtin-blockmeta with an arbitrary config.
>
> It was just easiest (and immediately effective) to inherit the config from the
> install environment.

yeah, I ended up just using a different early command to lay down an MBR

>
> it'd be easy enough to make the 'Injecting early_command to dirty storage
> devices' code that is in vmtests/__init__.py accept 'cls.dirty_disks' be a
> config itself or a path to one (rather than just boolean). Then generate a
> portion of the config that would write the different config.

Yes, that's also reasonable.

Revision history for this message
Ryan Harper (raharper) :
534. By Ryan Harper

modify the basic vmtest to trigger the MBR / sgdisk --clear bug

535. By Ryan Harper

Modify UEFI Basic test to trigger LP:1722322

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

I think we should use the existing curtin 'wipe_volume' in superblock mode.

536. By Ryan Harper

disk_handler: use block.wipe_volume superblock to clear out mbr/gpt tables

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
537. By Ryan Harper

Drop changes to basic.py, test case is in uefi_basic.py

Revision history for this message
Scott Moser (smoser) wrote :

Can you just add a comment that 'wipe_volume' is potentially destructive to partitions that start at very beginnning of disk or end at very end.

something like:
  # wipe_volume wipes 1M at front and end of the disk. That could
  # destroy partition's filesystem data that lived there.

other than that, i approve.

Unless you wanted to make a generic helper for prepping disks with an arbitrary storage config.

review: Approve
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
538. By Ryan Harper

Add comment around use of wipe to clear partition tables

539. By Ryan Harper

Drop apostrophe

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) :
review: Approve
Revision history for this message
Scott Moser (smoser) wrote :

merged. thanks ryan.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py 2017-05-26 14:07:29 +0000
+++ curtin/commands/block_meta.py 2017-10-11 16:38:41 +0000
@@ -384,7 +384,11 @@
384 LOG.info("labeling device: '%s' with '%s' partition table", disk,384 LOG.info("labeling device: '%s' with '%s' partition table", disk,
385 ptable)385 ptable)
386 if ptable == "gpt":386 if ptable == "gpt":
387 util.subp(["sgdisk", "--clear", disk])387 # Wipe both MBR and GPT that may be present on the disk.
388 # N.B.: wipe_volume wipes 1M at front and end of the disk.
389 # This could destroy disk data in filesystems that lived
390 # there.
391 block.wipe_volume(disk, mode='superblock')
388 elif ptable in _dos_names:392 elif ptable in _dos_names:
389 util.subp(["parted", disk, "--script", "mklabel", "msdos"])393 util.subp(["parted", disk, "--script", "mklabel", "msdos"])
390 else:394 else:
391395
=== modified file 'examples/tests/uefi_basic.yaml'
--- examples/tests/uefi_basic.yaml 2016-06-29 23:07:19 +0000
+++ examples/tests/uefi_basic.yaml 2017-10-11 16:38:41 +0000
@@ -1,4 +1,12 @@
1showtrace: true1showtrace: true
2
3early_commands:
4 # Recreate and test LP:1722322
5 # Make one disk dirty with an MBR and a storage configuration
6 # GPT and don't supply wipe: superblock. This will exercise
7 # curtin use of sgdisk --zap-all instead of --clear (GPT only)
8 blockmeta: ["parted", /dev/vdc, "--script", "mklabel", "msdos"]
9
2storage:10storage:
3 config:11 config:
4 - id: id_disk012 - id: id_disk0
@@ -55,4 +63,19 @@
55 id: id_efi_mount63 id: id_efi_mount
56 path: /boot/efi64 path: /boot/efi
57 type: mount65 type: mount
66 - id: pnum_disk
67 type: disk
68 path: /dev/vdc
69 name: pnum_disk
70 ptable: gpt
71 - id: pnum_disk_p1
72 type: partition
73 number: 1
74 size: 1GB
75 device: pnum_disk
76 - id: pnum_disk_p2
77 type: partition
78 number: 10
79 size: 1GB
80 device: pnum_disk
58 version: 181 version: 1
5982
=== modified file 'tests/vmtests/test_uefi_basic.py'
--- tests/vmtests/test_uefi_basic.py 2017-08-02 15:46:35 +0000
+++ tests/vmtests/test_uefi_basic.py 2017-10-11 16:38:41 +0000
@@ -9,7 +9,7 @@
9 interactive = False9 interactive = False
10 arch_skip = ["s390x"]10 arch_skip = ["s390x"]
11 conf_file = "examples/tests/uefi_basic.yaml"11 conf_file = "examples/tests/uefi_basic.yaml"
12 extra_disks = []12 extra_disks = ['4G']
13 uefi = True13 uefi = True
14 disk_to_check = [('main_disk', 1), ('main_disk', 2), ('main_disk', 3)]14 disk_to_check = [('main_disk', 1), ('main_disk', 2), ('main_disk', 3)]
15 collect_scripts = [textwrap.dedent("""15 collect_scripts = [textwrap.dedent("""

Subscribers

People subscribed via source and target branches