Merge ~raharper/curtin:fix/block-meta-get-device-paths-skip-missing into curtin:master

Proposed by Ryan Harper
Status: Merged
Approved by: Ryan Harper
Approved revision: fdf08aa88f6fe1242793e03c43673f3fb34f4a13
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~raharper/curtin:fix/block-meta-get-device-paths-skip-missing
Merge into: curtin:master
Diff against target: 208 lines (+38/-16)
7 files modified
curtin/block/__init__.py (+3/-0)
curtin/block/clear_holders.py (+1/-0)
curtin/block/multipath.py (+14/-9)
curtin/commands/block_meta.py (+10/-5)
examples/tests/multipath-lvm.yaml (+5/-1)
examples/tests/multipath.yaml (+4/-0)
tests/unittests/test_block.py (+1/-1)
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Richard Harding Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+382718@code.launchpad.net

Commit message

block-meta: skip wipe device paths if not present

Curtin extracts the devices from storage config with wipe enabled.
These devices may not exist yet (curtin will be creating them) so
there's no need to clear them, instead drop them from the list of
devices to send to clear-holders.

Other changes:

multipath:
 - Clarify multipath logging on device type checking
block-meta:
 - Stop iterating through disk lookup keys after we find one
 - Use udevadm_settle after parted partition creation before
   calling kpartx to prevent race with kernel partition update
clear-holders:
 - Log the inputs to clear-holders
block:
 - Log a warning when sys_block_path is called on a non-existent
   device path.
 - Log the return value from block.lookup_disk()

LP: #1869075

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Richard Harding (rharding) wrote :

LGTM ty for the updates

review: Approve
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

This all seems fine, thanks for banging it together so quickly! I'm not sure this is something to change so close to release but "get_device_paths_from_storage_config" is perhaps not a great name for what that function does.

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

Thanks. vmtests pass, landing.

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..a7fe22f 100644
3--- a/curtin/block/__init__.py
4+++ b/curtin/block/__init__.py
5@@ -143,6 +143,8 @@ def sys_block_path(devname, add=None, strict=True):
6 toks = ['/sys/class/block']
7 # insert parent dev if devname is partition
8 devname = os.path.normpath(devname)
9+ if devname.startswith('/dev/') and not os.path.exists(devname):
10+ LOG.warning('block.sys_block_path: devname %s does not exist', devname)
11 (parent, partnum) = get_blockdev_for_partition(devname, strict=strict)
12 if partnum:
13 toks.append(path_to_kname(parent))
14@@ -906,6 +908,7 @@ def lookup_disk(serial):
15 if not os.path.exists(path):
16 raise ValueError("path '%s' to block device for disk with serial '%s' \
17 does not exist" % (path, serial_udev))
18+ LOG.debug('block.lookup_disk() returning path %s', path)
19 return path
20
21
22diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
23index 14ad644..279fad8 100644
24--- a/curtin/block/clear_holders.py
25+++ b/curtin/block/clear_holders.py
26@@ -605,6 +605,7 @@ def clear_holders(base_paths, try_preserve=False):
27 # handle single path
28 if not isinstance(base_paths, (list, tuple)):
29 base_paths = [base_paths]
30+ LOG.info('Generating device storage trees for path(s): %s', base_paths)
31
32 # get current holders and plan how to shut them down
33 holder_trees = [gen_holders_tree(path) for path in base_paths]
34diff --git a/curtin/block/multipath.py b/curtin/block/multipath.py
35index a488e85..21fc0bd 100644
36--- a/curtin/block/multipath.py
37+++ b/curtin/block/multipath.py
38@@ -52,35 +52,40 @@ def dmname_to_blkdev_mapping():
39
40 def is_mpath_device(devpath, info=None):
41 """ Check if devpath is a multipath device, returns boolean. """
42+ result = False
43 if not info:
44 info = udev.udevadm_info(devpath)
45 if info.get('DM_UUID', '').startswith('mpath-'):
46- LOG.debug('%s is multipath device', devpath)
47- return True
48+ result = True
49
50- return False
51+ LOG.debug('%s is multipath device? %s', devpath, result)
52+ return result
53
54
55 def is_mpath_member(devpath, info=None):
56 """ Check if a device is a multipath member (a path), returns boolean. """
57+ result = False
58 try:
59 util.subp(['multipath', '-c', devpath], capture=True)
60- LOG.debug('%s is multipath device member', devpath)
61- return True
62+ result = True
63 except util.ProcessExecutionError:
64- return False
65+ pass
66+
67+ LOG.debug('%s is multipath device member? %s', devpath, result)
68+ return result
69
70
71 def is_mpath_partition(devpath, info=None):
72 """ Check if a device is a multipath partition, returns boolean. """
73+ result = False
74 if devpath.startswith('/dev/dm-'):
75 if not info:
76 info = udev.udevadm_info(devpath)
77 if 'DM_PART' in udev.udevadm_info(devpath):
78- LOG.debug("%s is multipath device partition", devpath)
79- return True
80+ result = True
81
82- return False
83+ LOG.debug("%s is multipath device partition? %s", devpath, result)
84+ return result
85
86
87 def mpath_partition_to_mpath_id(devpath):
88diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
89index 36d3146..7fbd89f 100644
90--- a/curtin/commands/block_meta.py
91+++ b/curtin/commands/block_meta.py
92@@ -465,7 +465,9 @@ def get_path_to_storage_volume(volume, storage_config):
93 except ValueError:
94 continue
95 # verify path exists otherwise try the next key
96- if not os.path.exists(volume_path):
97+ if os.path.exists(volume_path):
98+ break
99+ else:
100 volume_path = None
101
102 if volume_path is None:
103@@ -906,8 +908,9 @@ def partition_handler(info, storage_config):
104
105 # ensure partition exists
106 if multipath.is_mpath_device(disk):
107+ udevadm_settle() # allow partition creation to happen
108 # update device mapper table mapping to mpathX-partN
109- util.subp(['kpartx', '-v', '-a', '-p-part', disk])
110+ util.subp(['kpartx', '-v', '-a', '-s', '-p', '-part', disk])
111 part_path = disk + "-part%s" % partnumber
112 else:
113 part_path = block.dev_path(block.partition_kname(disk_kname,
114@@ -1697,7 +1700,7 @@ def zfs_handler(info, storage_config):
115
116 def get_device_paths_from_storage_config(storage_config):
117 """Returns a list of device paths in a storage config which have wipe
118- config enabled.
119+ config enabled filtering out constructed paths that do not exist.
120
121 :param: storage_config: Ordered dict of storage configation
122 """
123@@ -1706,8 +1709,10 @@ def get_device_paths_from_storage_config(storage_config):
124 if v.get('type') in ['disk', 'partition']:
125 if config.value_as_boolean(v.get('wipe')):
126 try:
127- dpaths.append(
128- get_path_to_storage_volume(k, storage_config))
129+ # skip paths that do not exit, nothing to wipe
130+ dpath = get_path_to_storage_volume(k, storage_config)
131+ if os.path.exists(dpath):
132+ dpaths.append(dpath)
133 except Exception:
134 pass
135 return dpaths
136diff --git a/examples/tests/multipath-lvm.yaml b/examples/tests/multipath-lvm.yaml
137index 1215d8b..68c3271 100644
138--- a/examples/tests/multipath-lvm.yaml
139+++ b/examples/tests/multipath-lvm.yaml
140@@ -69,24 +69,28 @@ storage:
141 name: root_disk
142 multipath: mpatha
143 serial: disk-a
144+ path: /dev/sda
145 grub_device: true
146- wipe: superblock-recursive
147+ wipe: superblock
148 - id: boot_bios
149 type: partition
150 size: 1MB
151 number: 1
152 device: main_disk
153 flag: bios_grub
154+ wipe: superblock
155 - id: boot_partition
156 type: partition
157 size: 1GB
158 number: 2
159 device: main_disk
160+ wipe: superblock
161 - id: main_disk_p3
162 type: partition
163 number: 3
164 size: 4GB
165 device: main_disk
166+ wipe: superblock
167 - id: root_vg
168 type: lvm_volgroup
169 name: root_vg
170diff --git a/examples/tests/multipath.yaml b/examples/tests/multipath.yaml
171index 4bafe2d..11838d1 100644
172--- a/examples/tests/multipath.yaml
173+++ b/examples/tests/multipath.yaml
174@@ -11,17 +11,21 @@ storage:
175 name: mpath_a
176 wipe: superblock
177 grub_device: true
178+ multipath: mpatha
179+ path: /dev/sda
180 - id: sda1
181 type: partition
182 number: 1
183 size: 3GB
184 device: sda
185 flag: boot
186+ wipe: superblock
187 - id: sda2
188 type: partition
189 number: 2
190 size: 1GB
191 device: sda
192+ wipe: superblock
193 - id: sda1_root
194 type: format
195 fstype: ext4
196diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
197index dcdcf51..2b0116b 100644
198--- a/tests/unittests/test_block.py
199+++ b/tests/unittests/test_block.py
200@@ -658,7 +658,7 @@ class TestSlaveKnames(CiTestCase):
201 # construct side effects to os.path.exists
202 # and os.listdir based on mapping.
203 dirs = []
204- exists = []
205+ exists = [True] if device.startswith('/dev') else []
206 for (dev, slvs) in cfg.items():
207 # sys_block_dev checks if dev exists
208 exists.append(True)

Subscribers

People subscribed via source and target branches