curtin

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

  1. trunk.fix-lp1722322
  2. Merge into trunk
Proposed by Ryan Harper on 2017-10-09
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 2017-10-09 Approve on 2017-10-11
Server Team CI bot continuous-integration Approve on 2017-10-11
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.
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.

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
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
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.

Ryan Harper (raharper) :
lp:~raharper/curtin/trunk.fix-lp1722322 updated on 2017-10-10
534. By Ryan Harper on 2017-10-09

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

535. By Ryan Harper on 2017-10-10

Modify UEFI Basic test to trigger LP:1722322

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

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

lp:~raharper/curtin/trunk.fix-lp1722322 updated on 2017-10-10
536. By Ryan Harper on 2017-10-10

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

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
lp:~raharper/curtin/trunk.fix-lp1722322 updated on 2017-10-11
537. By Ryan Harper on 2017-10-11

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

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
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
lp:~raharper/curtin/trunk.fix-lp1722322 updated on 2017-10-11
538. By Ryan Harper on 2017-10-11

Add comment around use of wipe to clear partition tables

539. By Ryan Harper on 2017-10-11

Drop apostrophe

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Scott Moser (smoser) :
review: Approve
Scott Moser (smoser) wrote :

merged. thanks ryan.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'curtin/commands/block_meta.py'
2--- curtin/commands/block_meta.py 2017-05-26 14:07:29 +0000
3+++ curtin/commands/block_meta.py 2017-10-11 16:38:41 +0000
4@@ -384,7 +384,11 @@
5 LOG.info("labeling device: '%s' with '%s' partition table", disk,
6 ptable)
7 if ptable == "gpt":
8- util.subp(["sgdisk", "--clear", disk])
9+ # Wipe both MBR and GPT that may be present on the disk.
10+ # N.B.: wipe_volume wipes 1M at front and end of the disk.
11+ # This could destroy disk data in filesystems that lived
12+ # there.
13+ block.wipe_volume(disk, mode='superblock')
14 elif ptable in _dos_names:
15 util.subp(["parted", disk, "--script", "mklabel", "msdos"])
16 else:
17
18=== modified file 'examples/tests/uefi_basic.yaml'
19--- examples/tests/uefi_basic.yaml 2016-06-29 23:07:19 +0000
20+++ examples/tests/uefi_basic.yaml 2017-10-11 16:38:41 +0000
21@@ -1,4 +1,12 @@
22 showtrace: true
23+
24+early_commands:
25+ # Recreate and test LP:1722322
26+ # Make one disk dirty with an MBR and a storage configuration
27+ # GPT and don't supply wipe: superblock. This will exercise
28+ # curtin use of sgdisk --zap-all instead of --clear (GPT only)
29+ blockmeta: ["parted", /dev/vdc, "--script", "mklabel", "msdos"]
30+
31 storage:
32 config:
33 - id: id_disk0
34@@ -55,4 +63,19 @@
35 id: id_efi_mount
36 path: /boot/efi
37 type: mount
38+ - id: pnum_disk
39+ type: disk
40+ path: /dev/vdc
41+ name: pnum_disk
42+ ptable: gpt
43+ - id: pnum_disk_p1
44+ type: partition
45+ number: 1
46+ size: 1GB
47+ device: pnum_disk
48+ - id: pnum_disk_p2
49+ type: partition
50+ number: 10
51+ size: 1GB
52+ device: pnum_disk
53 version: 1
54
55=== modified file 'tests/vmtests/test_uefi_basic.py'
56--- tests/vmtests/test_uefi_basic.py 2017-08-02 15:46:35 +0000
57+++ tests/vmtests/test_uefi_basic.py 2017-10-11 16:38:41 +0000
58@@ -9,7 +9,7 @@
59 interactive = False
60 arch_skip = ["s390x"]
61 conf_file = "examples/tests/uefi_basic.yaml"
62- extra_disks = []
63+ extra_disks = ['4G']
64 uefi = True
65 disk_to_check = [('main_disk', 1), ('main_disk', 2), ('main_disk', 3)]
66 collect_scripts = [textwrap.dedent("""

Subscribers

People subscribed via source and target branches