curtin

Merge ~raharper/curtin:fix/ptable-label-pmbr into curtin:master

  1. Git
  2. lp:~raharper/curtin
  3. fix/ptable-label-pmbr
  4. Merge into master
Proposed by Ryan Harper on 2019-10-17
Status: Merged
Approved by: Ryan Harper on 2019-10-24
Approved revision: ca77533c21d717e0167043a6ca552e1fa5a28892
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~raharper/curtin:fix/ptable-label-pmbr
Merge into: curtin:master
Diff against target: 109 lines (+27/-6)
4 files modified
curtin/block/schemas.py (+3/-2)
curtin/commands/block_meta.py (+3/-1)
curtin/storage_config.py (+5/-1)
tests/unittests/test_storage_config.py (+16/-2)
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-17 Approve on 2019-10-23
Dimitri John Ledkov (community) Needs Fixing on 2019-10-22
Michael Hudson-Doyle 2019-10-17 Pending
Review via email: mp+374305@code.launchpad.net

Commit message

schema: Add ptable value 'unsupported'

Probert output may include values of partition table 'label'
that curtin does not support. This triggers schema validation
errors for common values, like 'mac' or 'PMBR' and likely
other valid table names but are not something that curtin
can create. Instead of adding more values to the schema, mark
such table types as 'unsupported' allowing the existing table
and partitions to be re-used.

This branch also modifies block_meta disk_handler to not
attempt to validate a preserved partition table if it marked
as 'unsupported'.

LP: #1848535

To post a comment you must log in.
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Dimitri John Ledkov (xnox) wrote :

No, that is not the right thing to do from the point of preservation of partition table.

partition tables that curtin doesn't know how to manipulate (ie. create table, create/change partitions) it should be emitted as unknown or frozen.

Kernel can parse the partitions, thus one can reuse those partitions.

A device without a partition table, is different to a device with an unknown partition table we don't know how to modify correctly.

Thus completely omitting partition table, seems, conceptually, to me like a lie.

review: Needs Fixing
Dimitri John Ledkov (xnox) :
Ryan Harper (raharper) wrote :

We have two paths: one curtin is converting probed data into a storage config; this is where we find "unsupported" partition table types/labels. I generally don't want to encode all of these ever-expanding values into the storage schema. On the other hand, I want subiquity and other users of the schema to be able to verify the config they generated is correct, in which case I do want to encode specific values in the schema that curtin is expected to support.

After discussing this, I'll switch to setting ptable: unsupported for these values that curtin won't create/modify.

~raharper/curtin:fix/ptable-label-pmbr updated on 2019-10-22
8981dde... by Ryan Harper on 2019-10-22

schema: Add ptable value 'unsupported' for types curtin does not handle

Mark unsupported table types as 'unsupported'. This allows the existing
table and partitions to be re-used. Modify block_meta disk_handler to
not attempt to validate a preserved partition table as we now accept
'unsupported' ptable types; we won't be able to validate those entries.

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: 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 (daniel-thewatkins) wrote :

One inline question but this otherwise looks good. Thanks!

review: Needs Information
Ryan Harper (raharper) :
Dan Watkins (daniel-thewatkins) wrote :

Thanks!

review: Approve
Chad Smith (chad.smith) :
review: Needs Information
Ryan Harper (raharper) :
Ryan Harper (raharper) wrote :

For the record, simply emitting ptable: unsupported out of the discover command is sufficient to resolve the bug (curtin didn't like the PMBR value). Subiquity will still need a change to replace the ptable value with 'gpt'. And the preserve: true use-case is waiting on further work on the other branch.

~raharper/curtin:fix/ptable-label-pmbr updated on 2019-10-23
be9e2d3... by Ryan Harper on 2019-10-23

schema: add _ptable_unsupported variable

48fd93a... by Ryan Harper on 2019-10-23

Restore ptable preserve check, except when 'unsupported'

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

Thanks for this minor reference request for schemas._ptable_unsupported until a followup branch renames public schema '_*' variables.

review: Approve
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
~raharper/curtin:fix/ptable-label-pmbr updated on 2019-10-24
ca77533... by Ryan Harper on 2019-10-24

Replace use of 'unsupported' with schemas._ptable_unsupported

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

LGTM! Thanks rharper.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/curtin/block/schemas.py b/curtin/block/schemas.py
2index aa45d61..fc7e522 100644
3--- a/curtin/block/schemas.py
4+++ b/curtin/block/schemas.py
5@@ -7,6 +7,8 @@ _path_nondev = r'(^/$|^(/[^/]+)+$)'
6 _fstypes = ['btrfs', 'ext2', 'ext3', 'ext4', 'fat', 'fat12', 'fat16', 'fat32',
7 'iso9660', 'vfat', 'jfs', 'ntfs', 'reiserfs', 'swap', 'xfs',
8 'zfsroot']
9+_ptables = ['dos', 'gpt', 'msdos', 'vtoc']
10+_ptable_unsupported = 'unsupported'
11
12 definitions = {
13 'id': {'type': 'string'},
14@@ -14,8 +16,7 @@ definitions = {
15 'devices': {'type': 'array', 'items': {'$ref': '#/definitions/ref_id'}},
16 'name': {'type': 'string'},
17 'preserve': {'type': 'boolean'},
18- 'ptable': {'type': 'string', 'enum': ['dos', 'gpt', 'mac', 'msdos',
19- 'vtoc']},
20+ 'ptable': {'type': 'string', 'enum': _ptables + [_ptable_unsupported]},
21 'size': {'type': ['string', 'number'],
22 'minimum': 1,
23 'pattern': r'^([1-9]\d*(.\d+)?|\d+.\d+)(K|M|G|T)?B?'},
24diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
25index 37b93dd..6afb048 100644
26--- a/curtin/commands/block_meta.py
27+++ b/curtin/commands/block_meta.py
28@@ -2,6 +2,7 @@
29
30 from collections import OrderedDict, namedtuple
31 from curtin import (block, config, paths, util)
32+from curtin.block import schemas
33 from curtin.block import (bcache, clear_holders, dasd, iscsi, lvm, mdadm, mkfs,
34 zfs)
35 from curtin import distro
36@@ -32,6 +33,7 @@ SIMPLE = 'simple'
37 SIMPLE_BOOT = 'simple-boot'
38 CUSTOM = 'custom'
39 BCACHE_REGISTRATION_RETRY = [0.2] * 60
40+PTABLE_UNSUPPORTED = schemas._ptable_unsupported
41
42 CMD_ARGUMENTS = (
43 ((('-D', '--devices'),
44@@ -536,7 +538,7 @@ def disk_handler(info, storage_config):
45
46 if config.value_as_boolean(info.get('preserve')):
47 # Handle preserve flag, verifying if ptable specified in config
48- if config.value_as_boolean(ptable):
49+ if config.value_as_boolean(ptable) and ptable != PTABLE_UNSUPPORTED:
50 current_ptable = block.get_part_table_type(disk)
51 if not ((ptable in _dos_names and current_ptable in _dos_names) or
52 (ptable == 'gpt' and current_ptable == 'gpt')):
53diff --git a/curtin/storage_config.py b/curtin/storage_config.py
54index 3bfa466..03cfb4e 100644
55--- a/curtin/storage_config.py
56+++ b/curtin/storage_config.py
57@@ -743,7 +743,11 @@ class BlockdevParser(ProbertParser):
58 entry.update(uniq_ids)
59
60 if 'ID_PART_TABLE_TYPE' in blockdev_data:
61- entry['ptable'] = blockdev_data['ID_PART_TABLE_TYPE']
62+ ptype = blockdev_data['ID_PART_TABLE_TYPE']
63+ if ptype in schemas._ptables:
64+ entry['ptable'] = ptype
65+ else:
66+ entry['ptable'] = schemas._ptable_unsupported
67 return entry
68
69 if entry['type'] == 'partition':
70diff --git a/tests/unittests/test_storage_config.py b/tests/unittests/test_storage_config.py
71index 154a8b2..d4113ea 100644
72--- a/tests/unittests/test_storage_config.py
73+++ b/tests/unittests/test_storage_config.py
74@@ -43,12 +43,11 @@ class TestStorageConfigSchema(CiTestCase):
75 storage_config.validate_config(config)
76
77 @skipUnlessJsonSchema()
78- def test_disk_schema_accepts_mac_partition_table(self):
79+ def test_disk_schema_accepts_missing_ptable(self):
80 disk = {
81 "id": "disk-vdc",
82 "path": "/dev/vdc",
83 "type": "disk",
84- "ptable": "mac",
85 }
86 config = {'config': [disk], 'version': 1}
87 storage_config.validate_config(config)
88@@ -385,6 +384,21 @@ class TestBlockdevParser(CiTestCase):
89 self.assertDictEqual(expected_dict,
90 self.bdevp.asdict(blockdev))
91
92+ def test_blockdev_asdict_disk_marks_unknown_ptable_as_unspported(self):
93+ blockdev = self.bdevp.blockdev_data['/dev/sda']
94+ expected_dict = {
95+ 'id': 'disk-sda',
96+ 'type': 'disk',
97+ 'wwn': '0x3001438034e549a0',
98+ 'serial': '33001438034e549a0',
99+ 'ptable': 'unsupported',
100+ 'path': '/dev/sda',
101+ }
102+ for invalid in ['mac', 'PMBR']:
103+ blockdev['ID_PART_TABLE_TYPE'] = invalid
104+ self.assertDictEqual(expected_dict,
105+ self.bdevp.asdict(blockdev))
106+
107 def test_blockdev_detects_multipath(self):
108 self.probe_data = _get_data('probert_storage_multipath.json')
109 self.bdevp = BlockdevParser(self.probe_data)

Subscribers

People subscribed via source and target branches