curtin

Merge ~raharper/curtin:fix/bcache-over-raid5 into curtin:master

  1. Git
  2. lp:~raharper/curtin
  3. fix/bcache-over-raid5
  4. Merge into master
Proposed by Ryan Harper
Status: Merged
Approved by: Scott Moser
Approved revision: 46f8000f783cea49cf12051f741edba05fbc7900
Merged at revision: 46f8000f783cea49cf12051f741edba05fbc7900
Proposed branch: ~raharper/curtin:fix/bcache-over-raid5
Merge into: curtin:master
Diff against target: 381 lines (+120/-58)
7 files modified
curtin/block/__init__.py (+11/-9)
curtin/block/clear_holders.py (+62/-28)
curtin/commands/block_meta.py (+9/-4)
examples/tests/mdadm_bcache.yaml (+22/-5)
tests/unittests/test_block.py (+10/-6)
tests/unittests/test_clear_holders.py (+4/-4)
tests/vmtests/test_mdadm_bcache.py (+2/-2)
Reviewer Review Type Date Requested Status
Scott Moser (community) Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+339415@code.launchpad.net

Description of the change

clear_holders: wipe complex devices before disassembly

The curtin clear-holders code did not wipe the contents of the
assembled devices prior to shutting them down. In cases where
curtin re-assembles the same devices, in a RAID for example, then
when the RAID device comes on-line, it may prevent some tools from
creating new metadata on the target device. In particular, the
bcache tools do not want to overwrite an existing bcache superblock
resulting in a failed deployment when layering a bcache over a RAID
volume.

The fix is in two parts. First, clear-holders will wipe the
superblock of the assembled device before it breaks it apart.
Second, when creating a bcache backing device, check first if the
target device is already claimed by the bcache layer; if so then
stop and wipe that device first and then proceed with formatting the
bcache superblock.

curtin.block
 - expose 'exclusive' boolean and pass it through to context manager

curtin.block.clear_holders
 - Add logic to wipe assembled devices before calling shutdown
   function
 - introduce internal _wipe_superblock which only contains the retry
   logic
 - switch to using dmsetup remove for lvm destruction
 - improve identify_raid, it incorrectly identified partitions on
   RAID0 and RAID1 devices as raid devices, but they are actually
   partitions.

LP: #1750519

To post a comment you must log in.
Revision history for this message
Scott Moser (smoser) wrote :

One question inline.

Revision history for this message
Ryan Harper (raharper) :
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:c2158b3eaa594416b254d85cb170fe89272c52c1
https://jenkins.ubuntu.com/server/job/curtin-ci/850/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/850
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/850
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/850
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/850

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/850/rebuild

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

PASSED: Continuous integration, rev:46f8000f783cea49cf12051f741edba05fbc7900
https://jenkins.ubuntu.com/server/job/curtin-ci/856/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/856
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/856
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/856
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/856

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/856/rebuild

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

thanks.

review: Approve

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 349ea43..a82ed57 100644
3--- a/curtin/block/__init__.py
4+++ b/curtin/block/__init__.py
5@@ -840,7 +840,7 @@ def exclusive_open(path, exclusive=True):
6 raise
7
8
9-def wipe_file(path, reader=None, buflen=4 * 1024 * 1024):
10+def wipe_file(path, reader=None, buflen=4 * 1024 * 1024, exclusive=True):
11 """
12 wipe the existing file at path.
13 if reader is provided, it will be called as a 'reader(buflen)'
14@@ -859,7 +859,7 @@ def wipe_file(path, reader=None, buflen=4 * 1024 * 1024):
15 LOG.debug("%s is %s bytes. wiping with buflen=%s",
16 path, size, buflen)
17
18- with exclusive_open(path) as fp:
19+ with exclusive_open(path, exclusive=exclusive) as fp:
20 while True:
21 pbuf = readfunc(buflen)
22 pos = fp.tell()
23@@ -875,7 +875,7 @@ def wipe_file(path, reader=None, buflen=4 * 1024 * 1024):
24 fp.write(pbuf)
25
26
27-def quick_zero(path, partitions=True):
28+def quick_zero(path, partitions=True, exclusive=True):
29 """
30 zero 1M at front, 1M at end, and 1M at front
31 if this is a block device and partitions is true, then
32@@ -902,7 +902,8 @@ def quick_zero(path, partitions=True):
33 quick_zero(pt, partitions=False)
34
35 LOG.debug("wiping 1M on %s at offsets %s", path, offsets)
36- return zero_file_at_offsets(path, offsets, buflen=buflen, count=count)
37+ return zero_file_at_offsets(path, offsets, buflen=buflen, count=count,
38+ exclusive=exclusive)
39
40
41 def zero_file_at_offsets(path, offsets, buflen=1024, count=1024, strict=False,
42@@ -957,7 +958,7 @@ def zero_file_at_offsets(path, offsets, buflen=1024, count=1024, strict=False,
43 fp.write(buf)
44
45
46-def wipe_volume(path, mode="superblock"):
47+def wipe_volume(path, mode="superblock", exclusive=True):
48 """wipe a volume/block device
49
50 :param path: a path to a block device
51@@ -969,6 +970,7 @@ def wipe_volume(path, mode="superblock"):
52 superblock-recursive: zero the beginning of the volume, the end of the
53 volume and beginning and end of any partitions that are
54 known to be on this device.
55+ :param exclusive: boolean to control how path is opened
56 """
57 if mode == "pvremove":
58 # We need to use --force --force in case it's already in a volgroup and
59@@ -981,14 +983,14 @@ def wipe_volume(path, mode="superblock"):
60 rcs=[0, 5], capture=True)
61 lvm.lvm_scan()
62 elif mode == "zero":
63- wipe_file(path)
64+ wipe_file(path, exclusive=exclusive)
65 elif mode == "random":
66 with open("/dev/urandom", "rb") as reader:
67- wipe_file(path, reader=reader.read)
68+ wipe_file(path, reader=reader.read, exclusive=exclusive)
69 elif mode == "superblock":
70- quick_zero(path, partitions=False)
71+ quick_zero(path, partitions=False, exclusive=exclusive)
72 elif mode == "superblock-recursive":
73- quick_zero(path, partitions=True)
74+ quick_zero(path, partitions=True, exclusive=exclusive)
75 else:
76 raise ValueError("wipe mode %s not supported" % mode)
77
78diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
79index 88907db..4b3feeb 100644
80--- a/curtin/block/clear_holders.py
81+++ b/curtin/block/clear_holders.py
82@@ -188,11 +188,11 @@ def shutdown_lvm(device):
83 # '{volume group}-{logical volume}'. The volume can be freed using lvremove
84 name_file = os.path.join(device, 'dm', 'name')
85 (vg_name, lv_name) = lvm.split_lvm_name(util.load_file(name_file))
86- # use two --force flags here in case the volume group that this lv is
87- # attached two has been damaged
88- LOG.debug('running lvremove on %s/%s', vg_name, lv_name)
89- util.subp(['lvremove', '--force', '--force',
90- '{}/{}'.format(vg_name, lv_name)], rcs=[0, 5])
91+
92+ # use dmsetup as lvm commands require valid /etc/lvm/* metadata
93+ LOG.debug('using "dmsetup remove" on %s-%s', vg_name, lv_name)
94+ util.subp(['dmsetup', 'remove', '{}-{}'.format(vg_name, lv_name)])
95+
96 # if that was the last lvol in the volgroup, get rid of volgroup
97 if len(lvm.get_lvols_in_volgroup(vg_name)) == 0:
98 util.subp(['vgremove', '--force', '--force', vg_name], rcs=[0, 5])
99@@ -268,24 +268,30 @@ def wipe_superblock(device):
100 maybe_stop_bcache_device(stop_path)
101 continue
102
103- retries = [1, 3, 5, 7]
104- LOG.info('wiping superblock on %s', blockdev)
105- for attempt, wait in enumerate(retries):
106- LOG.debug('wiping %s attempt %s/%s',
107+ _wipe_superblock(blockdev)
108+
109+
110+def _wipe_superblock(blockdev, exclusive=True):
111+ """ No checks, just call wipe_volume """
112+
113+ retries = [1, 3, 5, 7]
114+ LOG.info('wiping superblock on %s', blockdev)
115+ for attempt, wait in enumerate(retries):
116+ LOG.debug('wiping %s attempt %s/%s',
117+ blockdev, attempt + 1, len(retries))
118+ try:
119+ block.wipe_volume(blockdev, mode='superblock', exclusive=exclusive)
120+ LOG.debug('successfully wiped device %s on attempt %s/%s',
121 blockdev, attempt + 1, len(retries))
122- try:
123- block.wipe_volume(blockdev, mode='superblock')
124- LOG.debug('successfully wiped device %s on attempt %s/%s',
125- blockdev, attempt + 1, len(retries))
126- return
127- except OSError:
128- if attempt + 1 >= len(retries):
129- raise
130- else:
131- LOG.debug("wiping device '%s' failed on attempt"
132- " %s/%s. sleeping %ss before retry",
133- blockdev, attempt + 1, len(retries), wait)
134- time.sleep(wait)
135+ return
136+ except OSError:
137+ if attempt + 1 >= len(retries):
138+ raise
139+ else:
140+ LOG.debug("wiping device '%s' failed on attempt"
141+ " %s/%s. sleeping %ss before retry",
142+ blockdev, attempt + 1, len(retries), wait)
143+ time.sleep(wait)
144
145
146 def identify_lvm(device):
147@@ -308,7 +314,10 @@ def identify_mdadm(device):
148 """
149 determine if specified device is a mdadm device
150 """
151- return block.path_to_kname(device).startswith('md')
152+ # RAID0 and 1 devices can be partitioned and the partitions are *not*
153+ # raid devices with a sysfs 'md' subdirectory
154+ partition = identify_partition(device)
155+ return block.path_to_kname(device).startswith('md') and not partition
156
157
158 def identify_bcache(device):
159@@ -501,21 +510,46 @@ def clear_holders(base_paths, try_preserve=False):
160 '\n'.join(format_holders_tree(tree) for tree in holder_trees))
161 ordered_devs = plan_shutdown_holder_trees(holder_trees)
162
163+ # run wipe-superblock on layered devices
164+ for dev_info in ordered_devs:
165+ dev_type = DEV_TYPES.get(dev_info['dev_type'])
166+ shutdown_function = dev_type.get('shutdown')
167+ if not shutdown_function:
168+ continue
169+
170+ if try_preserve and shutdown_function in DATA_DESTROYING_HANDLERS:
171+ LOG.info('shutdown function for holder type: %s is destructive. '
172+ 'attempting to preserve data, so skipping' %
173+ dev_info['dev_type'])
174+ continue
175+
176+ # for layered block devices, wipe first, then shutdown
177+ if dev_info['dev_type'] in ['bcache', 'raid']:
178+ LOG.info("Wiping superblock on layered device type: "
179+ "'%s' syspath: '%s'", dev_info['dev_type'],
180+ dev_info['device'])
181+ # we just want to wipe data, we don't care about exclusive
182+ _wipe_superblock(block.sysfs_to_devpath(dev_info['device']),
183+ exclusive=False)
184+
185 # run shutdown functions
186 for dev_info in ordered_devs:
187 dev_type = DEV_TYPES.get(dev_info['dev_type'])
188 shutdown_function = dev_type.get('shutdown')
189 if not shutdown_function:
190 continue
191+
192 if try_preserve and shutdown_function in DATA_DESTROYING_HANDLERS:
193 LOG.info('shutdown function for holder type: %s is destructive. '
194- 'attempting to preserve data, so not skipping' %
195+ 'attempting to preserve data, so skipping' %
196 dev_info['dev_type'])
197 continue
198- LOG.info("shutdown running on holder type: '%s' syspath: '%s'",
199- dev_info['dev_type'], dev_info['device'])
200- shutdown_function(dev_info['device'])
201- udev.udevadm_settle()
202+
203+ if os.path.exists(dev_info['device']):
204+ LOG.info("shutdown running on holder type: '%s' syspath: '%s'",
205+ dev_info['dev_type'], dev_info['device'])
206+ shutdown_function(dev_info['device'])
207+ udev.udevadm_settle()
208
209
210 def start_clear_holders_deps():
211diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
212index 8a1dea8..504a16b 100644
213--- a/curtin/commands/block_meta.py
214+++ b/curtin/commands/block_meta.py
215@@ -596,7 +596,6 @@ def partition_handler(info, storage_config):
216
217 def format_handler(info, storage_config):
218 volume = info.get('volume')
219- LOG.debug('WARK: format: info: %s', info)
220 if not volume:
221 raise ValueError("volume must be specified for partition '%s'" %
222 info.get('id'))
223@@ -1114,9 +1113,15 @@ def bcache_handler(info, storage_config):
224 if backing_device:
225 backing_device_sysfs = block.sys_block_path(backing_device)
226 target_sysfs_path = os.path.join(backing_device_sysfs, "bcache")
227- if not os.path.exists(os.path.join(backing_device_sysfs, "bcache")):
228- LOG.debug('Creating a backing device on %s', backing_device)
229- util.subp(["make-bcache", "-B", backing_device])
230+
231+ # there should not be any pre-existing bcache device
232+ bdir = os.path.join(backing_device_sysfs, "bcache")
233+ if os.path.exists(bdir):
234+ raise RuntimeError(
235+ 'Unexpected old bcache device: %s', backing_device)
236+
237+ LOG.debug('Creating a backing device on %s', backing_device)
238+ util.subp(["make-bcache", "-B", backing_device])
239 ensure_bcache_is_registered(backing_device, target_sysfs_path)
240
241 # via the holders we can identify which bcache device we just created
242diff --git a/examples/tests/mdadm_bcache.yaml b/examples/tests/mdadm_bcache.yaml
243index 7c123c3..4025bb3 100644
244--- a/examples/tests/mdadm_bcache.yaml
245+++ b/examples/tests/mdadm_bcache.yaml
246@@ -65,6 +65,24 @@ storage:
247 model: QEMU HARDDISK
248 serial: disk-c
249 name: third_disk
250+ - id: sdd
251+ type: disk
252+ wipe: superblock
253+ model: QEMU HARDDISK
254+ serial: disk-d
255+ name: raid5_disk_1
256+ - id: sde
257+ type: disk
258+ wipe: superblock
259+ model: QEMU HARDDISK
260+ serial: disk-e
261+ name: raid5_disk_2
262+ - id: sdf
263+ type: disk
264+ wipe: superblock
265+ model: QEMU HARDDISK
266+ serial: disk-f
267+ name: raid5_disk_3
268 - id: sdc1
269 type: partition
270 size: 3GB
271@@ -73,12 +91,11 @@ storage:
272 - id: mddevice
273 name: md0
274 type: raid
275- raidlevel: 1
276+ raidlevel: 5
277 devices:
278- - sda2
279- - sda3
280- spare_devices:
281- - sda4
282+ - sdd
283+ - sde
284+ - sdf
285 - id: bcache1_raid
286 type: bcache
287 name: cached_array
288diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
289index cb2efcf..d9b19a4 100644
290--- a/tests/unittests/test_block.py
291+++ b/tests/unittests/test_block.py
292@@ -358,15 +358,18 @@ class TestWipeVolume(CiTestCase):
293 @mock.patch('curtin.block.quick_zero')
294 def test_wipe_superblock(self, mock_quick_zero):
295 block.wipe_volume(self.dev, mode='superblock')
296- mock_quick_zero.assert_called_with(self.dev, partitions=False)
297- block.wipe_volume(self.dev, mode='superblock-recursive')
298- mock_quick_zero.assert_called_with(self.dev, partitions=True)
299+ mock_quick_zero.assert_called_with(self.dev, exclusive=True,
300+ partitions=False)
301+ block.wipe_volume(self.dev, exclusive=True,
302+ mode='superblock-recursive')
303+ mock_quick_zero.assert_called_with(self.dev, exclusive=True,
304+ partitions=True)
305
306 @mock.patch('curtin.block.wipe_file')
307 def test_wipe_zero(self, mock_wipe_file):
308 with simple_mocked_open():
309- block.wipe_volume(self.dev, mode='zero')
310- mock_wipe_file.assert_called_with(self.dev)
311+ block.wipe_volume(self.dev, exclusive=True, mode='zero')
312+ mock_wipe_file.assert_called_with(self.dev, exclusive=True)
313
314 @mock.patch('curtin.block.wipe_file')
315 def test_wipe_random(self, mock_wipe_file):
316@@ -374,7 +377,8 @@ class TestWipeVolume(CiTestCase):
317 block.wipe_volume(self.dev, mode='random')
318 mock_open.assert_called_with('/dev/urandom', 'rb')
319 mock_wipe_file.assert_called_with(
320- self.dev, reader=mock_open.return_value.__enter__().read)
321+ self.dev, exclusive=True,
322+ reader=mock_open.return_value.__enter__().read)
323
324 def test_bad_input(self):
325 with self.assertRaises(ValueError):
326diff --git a/tests/unittests/test_clear_holders.py b/tests/unittests/test_clear_holders.py
327index f2acf21..4c07a9c 100644
328--- a/tests/unittests/test_clear_holders.py
329+++ b/tests/unittests/test_clear_holders.py
330@@ -397,8 +397,8 @@ class TestClearHolders(CiTestCase):
331 '-'.join((vg_name, lv_name)))
332 self.assertTrue(mock_log.debug.called)
333 mock_util.subp.assert_called_with(
334- ['lvremove', '--force', '--force', '/'.join((vg_name, lv_name))],
335- rcs=[0, 5])
336+ ['dmsetup', 'remove', '-'.join((vg_name, lv_name))])
337+
338 mock_lvm.get_lvols_in_volgroup.assert_called_with(vg_name)
339 self.assertEqual(len(mock_util.subp.call_args_list), 1)
340 self.assertTrue(mock_lvm.lvm_scan.called)
341@@ -495,7 +495,7 @@ class TestClearHolders(CiTestCase):
342 clear_holders.wipe_superblock(self.test_syspath)
343 mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
344 mock_block.wipe_volume.assert_called_with(
345- self.test_blockdev, mode='superblock')
346+ self.test_blockdev, exclusive=True, mode='superblock')
347
348 @mock.patch('curtin.block.clear_holders.zfs')
349 @mock.patch('curtin.block.clear_holders.LOG')
350@@ -514,7 +514,7 @@ class TestClearHolders(CiTestCase):
351 mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
352 mock_zfs.zpool_export.assert_called_with('fake_pool')
353 mock_block.wipe_volume.assert_called_with(
354- self.test_blockdev, mode='superblock')
355+ self.test_blockdev, exclusive=True, mode='superblock')
356
357 @mock.patch('curtin.block.clear_holders.LOG')
358 @mock.patch('curtin.block.clear_holders.block')
359diff --git a/tests/vmtests/test_mdadm_bcache.py b/tests/vmtests/test_mdadm_bcache.py
360index 2b12245..e34d84b 100644
361--- a/tests/vmtests/test_mdadm_bcache.py
362+++ b/tests/vmtests/test_mdadm_bcache.py
363@@ -8,8 +8,8 @@ import textwrap
364
365 class TestMdadmAbs(VMBaseClass):
366 interactive = False
367- extra_disks = []
368 active_mdadm = "1"
369+ dirty_disks = True
370 collect_scripts = VMBaseClass.collect_scripts + [textwrap.dedent("""
371 cd OUTPUT_COLLECT_D
372 cat /etc/fstab > fstab
373@@ -50,7 +50,7 @@ class TestMdadmBcacheAbs(TestMdadmAbs):
374 ('md0', 0),
375 ('cached_array', 0),
376 ('cached_array_2', 0)]
377- extra_disks = ['4G', '4G']
378+ extra_disks = ['4G', '4G', '4G', '4G', '4G']
379 collect_scripts = TestMdadmAbs.collect_scripts + [textwrap.dedent("""
380 cd OUTPUT_COLLECT_D
381 bcache-super-show /dev/vda6 > bcache_super_vda6

Subscribers

People subscribed via source and target branches