Merge lp:~raharper/curtin/trunk.nvme_bcache into lp:~curtin-dev/curtin/trunk
- trunk.nvme_bcache
- Merge into trunk
| Status: | Merged |
|---|---|
| Merged at revision: | 492 |
| Proposed branch: | lp:~raharper/curtin/trunk.nvme_bcache |
| Merge into: | lp:~curtin-dev/curtin/trunk |
| Diff against target: |
684 lines (+559/-22) 6 files modified
curtin/block/clear_holders.py (+81/-14) curtin/util.py (+21/-0) examples/tests/nvme_bcache.yaml (+114/-0) tests/unittests/test_clear_holders.py (+197/-8) tests/unittests/test_util.py (+69/-0) tests/vmtests/test_nvme.py (+77/-0) |
| To merge this branch: | bzr merge lp:~raharper/curtin/trunk.nvme_bcache |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Server Team CI bot | continuous-integration | Approve | |
| Scott Moser (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Description of the change
clear-holders: ensure that bcache handler waits bcache devices to shutdown
Clear-holder's bcache shutdown handler does not wait for devices owned by bcache driver to be released before returning. On sufficiently fast systems (and with multiple cpus) the time for bcache to release the backing devices may occur slower than the time it takes for curtin to
start attempting to wipe partition data on a backing device. This results
in an IO error due to curtin asserting exclusive access to a device for
wiping.
This series introduces some additional logic in the bcache shutdown
handler by testing that the bcache devices are removed before returning
from the handler.
If the underlying devices are not released, the handler raises an
exception.
Fixes: LP:#1680409
| Server Team CI bot (server-team-bot) wrote : | # |
| Scott Moser (smoser) wrote : | # |
Some comments inline.
The unit tests seem really tied to the implementation (mocking os.path.exists which is called by 2 different callers).
I realize this is tricky, but maybe some re-work could make the tests more clear.
A test for get_bcache_sys_path would be good.
| Ryan Harper (raharper) wrote : | # |
On Tue, Apr 11, 2017 at 9:17 AM, Scott Moser <email address hidden> wrote:
> Some comments inline.
> The unit tests seem really tied to the implementation (mocking
> os.path.exists which is called by 2 different callers).
>
> I realize this is tricky, but maybe some re-work could make the tests more
> clear.
>
I'm open to reworking them. Not 100% happy with the current coverage.
>
>
> A test for get_bcache_sys_path would be good.
>
Pretty sure I have that.
>
>
> Diff comments:
>
> > === modified file 'curtin/
> > --- curtin/
> > +++ curtin/
> > @@ -56,32 +56,75 @@
> >
> > def get_bcache_
> > """
> > - Get the /sys/fs/bcache/ path of the bcache volume using specified
> device
> > + Get the /sys/fs/bcache/ path of the bcache cache device bound to
> > + specified device
> > """
> > # FIXME: when block.bcache is written this should be moved there
> > sysfs_path = block.sys_
> > return os.path.
> 'cache'))
> >
> >
> > +def get_bcache_
> > + """
> > + Get the /sys/class/
> > + """
> > + sysfs_path = block.sys_
> > + return os.path.
>
> The comment says "if present", but its not clear what the return would be.
> If the *device* does not exist (get_bcache_
> you'll
> raise an OSError, but if the device exists and the 'bcache' entry does not
> you'll just return it either way.
>
> >>> os.path.
> '/bogus/path/here'
>
Ack, the comment was really from the caller;
I'll rework as I'd like it validate the path before the realpath call as
well.
>
> > +
> > +
> > def shutdown_
> > """
> > Shut down bcache for specified bcache device
> > +
> > + 1. Stop the cacheset that `device` is connected to
> > + 2. Stop the 'device'
> > """
> > + if not device.
>
> we use the string '/sys/class/block' other places, we really should
> SYS_CLASS_BLOCK = '/sys/block'
> not "your fault", but i think having that constant would be good.
>
ACK, though I'd like to punt on the general cleanup into a separate MP.
>
> > + raise ValueError('Invalid Device (%s): '
> > + 'Device path must start with
> /sys/class/block/',
> > + device)
> > +
> > bcache_
> determined '
> > 'that the device has already been shut
> down '
> > 'during handling of another bcache dev. '
> > 'skipping'
> > - if not os.path.
> > - LOG.info(
> > - return
> > -
> > - bcache_sysfs = get_bcache_
> > - if not os.path.
> > - LOG.info(
> > - return
> > -
> ...
- 493. By Ryan Harper
-
Add strict mode for bcache sysfs paths and longer, frequent retries
Feedback from MP suggested we should apply some path checking
when handling sysfs bcache entries; add the strict mode used in
other block methods.When waiting for file removal, we want to avoid blocking for long
periods and also ensure we've waited long enough. A bcache device
may need to flush a cache to the backing device which could take
some time. We also don't want to add latency if it isn't needed by
incurring long sleep intervals. Now we will wait 30 seconds in total
and check every 0.2 seconds if the device has been removed.
| Scott Moser (smoser) : | # |
| Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:493
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
- 494. By Ryan Harper
-
merge from trunk
| Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:494
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
- 495. By Ryan Harper
-
merge from trunk
| Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:495
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
Preview Diff
| 1 | === modified file 'curtin/block/clear_holders.py' |
| 2 | --- curtin/block/clear_holders.py 2017-01-31 22:03:02 +0000 |
| 3 | +++ curtin/block/clear_holders.py 2017-04-24 17:53:29 +0000 |
| 4 | @@ -21,6 +21,7 @@ |
| 5 | having to reboot the system |
| 6 | """ |
| 7 | |
| 8 | +import errno |
| 9 | import os |
| 10 | |
| 11 | from curtin import (block, udev, util) |
| 12 | @@ -54,34 +55,100 @@ |
| 13 | return out.strip() |
| 14 | |
| 15 | |
| 16 | -def get_bcache_using_dev(device): |
| 17 | +def get_bcache_using_dev(device, strict=True): |
| 18 | """ |
| 19 | - Get the /sys/fs/bcache/ path of the bcache volume using specified device |
| 20 | + Get the /sys/fs/bcache/ path of the bcache cache device bound to |
| 21 | + specified device |
| 22 | """ |
| 23 | # FIXME: when block.bcache is written this should be moved there |
| 24 | sysfs_path = block.sys_block_path(device) |
| 25 | - return os.path.realpath(os.path.join(sysfs_path, 'bcache', 'cache')) |
| 26 | + path = os.path.realpath(os.path.join(sysfs_path, 'bcache', 'cache')) |
| 27 | + if strict and not os.path.exists(path): |
| 28 | + err = OSError( |
| 29 | + "device '{}' did not have existing syspath '{}'".format( |
| 30 | + device, path)) |
| 31 | + err.errno = errno.ENOENT |
| 32 | + raise err |
| 33 | + |
| 34 | + return path |
| 35 | + |
| 36 | + |
| 37 | +def get_bcache_sys_path(device, strict=True): |
| 38 | + """ |
| 39 | + Get the /sys/class/block/<device>/bcache path |
| 40 | + """ |
| 41 | + sysfs_path = block.sys_block_path(device) |
| 42 | + path = os.path.join(sysfs_path, 'bcache') |
| 43 | + if strict and not os.path.exists(path): |
| 44 | + err = OSError( |
| 45 | + "device '{}' did not have existing syspath '{}'".format( |
| 46 | + device, path)) |
| 47 | + err.errno = errno.ENOENT |
| 48 | + raise err |
| 49 | + |
| 50 | + return path |
| 51 | |
| 52 | |
| 53 | def shutdown_bcache(device): |
| 54 | """ |
| 55 | Shut down bcache for specified bcache device |
| 56 | + |
| 57 | + 1. Stop the cacheset that `device` is connected to |
| 58 | + 2. Stop the 'device' |
| 59 | """ |
| 60 | + if not device.startswith('/sys/class/block'): |
| 61 | + raise ValueError('Invalid Device (%s): ' |
| 62 | + 'Device path must start with /sys/class/block/', |
| 63 | + device) |
| 64 | + |
| 65 | + # bcache device removal should be fast but in an extreme |
| 66 | + # case, might require the cache device to flush large |
| 67 | + # amounts of data to a backing device. The strategy here |
| 68 | + # is to wait for approximately 30 seconds but to check |
| 69 | + # frequently since curtin cannot proceed until devices |
| 70 | + # cleared. |
| 71 | + removal_retries = [0.2] * 150 # 30 seconds total |
| 72 | bcache_shutdown_message = ('shutdown_bcache running on {} has determined ' |
| 73 | 'that the device has already been shut down ' |
| 74 | 'during handling of another bcache dev. ' |
| 75 | 'skipping'.format(device)) |
| 76 | - if not os.path.exists(device): |
| 77 | - LOG.info(bcache_shutdown_message) |
| 78 | - return |
| 79 | - |
| 80 | - bcache_sysfs = get_bcache_using_dev(device) |
| 81 | - if not os.path.exists(bcache_sysfs): |
| 82 | - LOG.info(bcache_shutdown_message) |
| 83 | - return |
| 84 | - |
| 85 | - LOG.debug('stopping bcache at: %s', bcache_sysfs) |
| 86 | - util.write_file(os.path.join(bcache_sysfs, 'stop'), '1', mode=None) |
| 87 | + |
| 88 | + if not os.path.exists(device): |
| 89 | + LOG.info(bcache_shutdown_message) |
| 90 | + return |
| 91 | + |
| 92 | + # stop cacheset if it exists |
| 93 | + bcache_cache_sysfs = get_bcache_using_dev(device) |
| 94 | + if not os.path.exists(bcache_cache_sysfs): |
| 95 | + LOG.info('bcache cacheset already removed: %s', |
| 96 | + os.path.basename(bcache_cache_sysfs)) |
| 97 | + else: |
| 98 | + LOG.info('stopping bcache cacheset at: %s', bcache_cache_sysfs) |
| 99 | + util.write_file(os.path.join(bcache_cache_sysfs, 'stop'), |
| 100 | + '1', mode=None) |
| 101 | + try: |
| 102 | + util.wait_for_removal(bcache_cache_sysfs, retries=removal_retries) |
| 103 | + except OSError: |
| 104 | + LOG.info('Failed to stop bcache cacheset %s', bcache_cache_sysfs) |
| 105 | + raise |
| 106 | + |
| 107 | + # after stopping cache set, we may need to stop the device |
| 108 | + if not os.path.exists(device): |
| 109 | + LOG.info('bcache backing device already removed: %s', device) |
| 110 | + return |
| 111 | + else: |
| 112 | + bcache_block_sysfs = get_bcache_sys_path(device) |
| 113 | + LOG.info('stopping bcache backing device at: %s', bcache_block_sysfs) |
| 114 | + util.write_file(os.path.join(bcache_block_sysfs, 'stop'), |
| 115 | + '1', mode=None) |
| 116 | + try: |
| 117 | + util.wait_for_removal(device, retries=removal_retries) |
| 118 | + except OSError: |
| 119 | + LOG.info('Failed to stop bcache backing device %s', |
| 120 | + bcache_block_sysfs) |
| 121 | + raise |
| 122 | + |
| 123 | + return |
| 124 | |
| 125 | |
| 126 | def shutdown_lvm(device): |
| 127 | |
| 128 | === modified file 'curtin/util.py' |
| 129 | --- curtin/util.py 2017-04-12 01:38:04 +0000 |
| 130 | +++ curtin/util.py 2017-04-24 17:53:29 +0000 |
| 131 | @@ -194,6 +194,27 @@ |
| 132 | return _subp(*args, **kwargs) |
| 133 | |
| 134 | |
| 135 | +def wait_for_removal(path, retries=[1, 3, 5, 7]): |
| 136 | + if not path: |
| 137 | + raise ValueError('wait_for_removal: missing path parameter') |
| 138 | + |
| 139 | + # Retry with waits between checking for existence |
| 140 | + LOG.debug('waiting for %s to be removed', path) |
| 141 | + for num, wait in enumerate(retries): |
| 142 | + if not os.path.exists(path): |
| 143 | + LOG.debug('%s has been removed', path) |
| 144 | + return |
| 145 | + LOG.debug('sleeping %s', wait) |
| 146 | + time.sleep(wait) |
| 147 | + |
| 148 | + # final check |
| 149 | + if not os.path.exists(path): |
| 150 | + LOG.debug('%s has been removed', path) |
| 151 | + return |
| 152 | + |
| 153 | + raise OSError('Timeout exceeded for removal of %s', path) |
| 154 | + |
| 155 | + |
| 156 | def load_command_environment(env=os.environ, strict=False): |
| 157 | |
| 158 | mapping = {'scratch': 'WORKING_DIR', 'fstab': 'OUTPUT_FSTAB', |
| 159 | |
| 160 | === added file 'examples/tests/nvme_bcache.yaml' |
| 161 | --- examples/tests/nvme_bcache.yaml 1970-01-01 00:00:00 +0000 |
| 162 | +++ examples/tests/nvme_bcache.yaml 2017-04-24 17:53:29 +0000 |
| 163 | @@ -0,0 +1,114 @@ |
| 164 | +showtrace: true |
| 165 | +storage: |
| 166 | + config: |
| 167 | + - grub_device: true |
| 168 | + id: sda |
| 169 | + model: LOGICAL VOLUME |
| 170 | + name: sda |
| 171 | + ptable: gpt |
| 172 | + path: /dev/vdb |
| 173 | + type: disk |
| 174 | + wipe: superblock |
| 175 | + - id: sdb |
| 176 | + model: LOGICAL VOLUME |
| 177 | + name: sdb |
| 178 | + path: /dev/vdc |
| 179 | + type: disk |
| 180 | + wipe: superblock |
| 181 | + - id: nvme0n1 |
| 182 | + model: INTEL SSDPEDME400G4 |
| 183 | + name: nvme0n1 |
| 184 | + ptable: gpt |
| 185 | + path: /dev/nvme0n1 |
| 186 | + type: disk |
| 187 | + wipe: superblock |
| 188 | + - device: sda |
| 189 | + flag: boot |
| 190 | + id: sda-part1 |
| 191 | + name: sda-part1 |
| 192 | + number: 1 |
| 193 | + offset: 4194304B |
| 194 | + size: 536870912B |
| 195 | + type: partition |
| 196 | + uuid: 9f0fda16-c096-4cee-82ac-4f5f294253a2 |
| 197 | + wipe: superblock |
| 198 | + - device: sda |
| 199 | + flag: boot |
| 200 | + id: sda-part2 |
| 201 | + name: sda-part2 |
| 202 | + number: 2 |
| 203 | + size: 1G |
| 204 | + type: partition |
| 205 | + uuid: 7dbb079c-60e5-4900-b3ca-4a92c5cbd6d3 |
| 206 | + wipe: superblock |
| 207 | + - device: sda |
| 208 | + id: sda-part3 |
| 209 | + name: sda-part3 |
| 210 | + number: 3 |
| 211 | + size: 5G |
| 212 | + type: partition |
| 213 | + uuid: 560de16e-6dc1-4e85-8e66-aaf1062dabcd |
| 214 | + wipe: superblock |
| 215 | + - device: nvme0n1 |
| 216 | + id: nvme0n1-part1 |
| 217 | + name: nvme0n1-part1 |
| 218 | + number: 1 |
| 219 | + offset: 4194304B |
| 220 | + size: 5G |
| 221 | + type: partition |
| 222 | + uuid: 9692359a-8a6d-4fa2-bbe6-48b97c4c8832 |
| 223 | + wipe: superblock |
| 224 | + - backing_device: sda-part3 |
| 225 | + cache_device: nvme0n1-part1 |
| 226 | + cache_mode: writeback |
| 227 | + id: bcache1 |
| 228 | + name: bcache1 |
| 229 | + type: bcache |
| 230 | + - backing_device: sdb |
| 231 | + cache_device: nvme0n1-part1 |
| 232 | + cache_mode: writeback |
| 233 | + id: bcache2 |
| 234 | + name: bcache2 |
| 235 | + type: bcache |
| 236 | + - fstype: fat32 |
| 237 | + id: sda-part1_format |
| 238 | + label: efi |
| 239 | + type: format |
| 240 | + uuid: e23b3115-228d-410c-a2b0-5bffeaf1c1d0 |
| 241 | + volume: sda-part1 |
| 242 | + - fstype: ext4 |
| 243 | + id: sda-part2_format |
| 244 | + label: boot |
| 245 | + type: format |
| 246 | + uuid: 8940edad-7882-4978-91a6-5f80f95ea4e8 |
| 247 | + volume: sda-part2 |
| 248 | + - fstype: ext4 |
| 249 | + id: bcache1_format |
| 250 | + label: root |
| 251 | + type: format |
| 252 | + uuid: 627296af-d976-4baf-8abc-b388143d4dd3 |
| 253 | + volume: bcache1 |
| 254 | + - fstype: xfs |
| 255 | + id: bcache2_format |
| 256 | + label: '' |
| 257 | + type: format |
| 258 | + uuid: ada2cfb1-cfad-4d66-99ff-ea3e79790233 |
| 259 | + volume: bcache2 |
| 260 | + - device: bcache1_format |
| 261 | + id: bcache1_mount |
| 262 | + path: / |
| 263 | + type: mount |
| 264 | + - device: sda-part2_format |
| 265 | + id: sda-part2_mount |
| 266 | + path: /boot |
| 267 | + type: mount |
| 268 | + - device: sda-part1_format |
| 269 | + id: sda-part1_mount |
| 270 | + path: /boot/efi |
| 271 | + type: mount |
| 272 | + - device: bcache2_format |
| 273 | + id: bcache2_mount |
| 274 | + options: '' |
| 275 | + path: /srv |
| 276 | + type: mount |
| 277 | + version: 1 |
| 278 | |
| 279 | === modified file 'tests/unittests/test_clear_holders.py' |
| 280 | --- tests/unittests/test_clear_holders.py 2017-01-27 01:00:39 +0000 |
| 281 | +++ tests/unittests/test_clear_holders.py 2017-04-24 17:53:29 +0000 |
| 282 | @@ -9,6 +9,7 @@ |
| 283 | class TestClearHolders(TestCase): |
| 284 | test_blockdev = '/dev/null' |
| 285 | test_syspath = '/sys/class/block/null' |
| 286 | + remove_retries = [0.2] * 150 # clear_holders defaults to 30 seconds |
| 287 | example_holders_trees = [ |
| 288 | [{'device': '/sys/class/block/sda', 'name': 'sda', 'holders': |
| 289 | [{'device': '/sys/class/block/sda/sda1', 'name': 'sda1', |
| 290 | @@ -89,12 +90,25 @@ |
| 291 | def test_get_bcache_using_dev(self, mock_os, mock_block): |
| 292 | """Ensure that get_bcache_using_dev works""" |
| 293 | fake_bcache = '/sys/fs/bcache/fake' |
| 294 | + mock_os.path.join.side_effect = os.path.join |
| 295 | + mock_block.sys_block_path.return_value = self.test_syspath |
| 296 | mock_os.path.realpath.return_value = fake_bcache |
| 297 | - mock_os.path.exists.side_effect = lambda x: x == fake_bcache |
| 298 | - mock_block.sys_block_path.return_value = self.test_blockdev |
| 299 | + |
| 300 | bcache_dir = clear_holders.get_bcache_using_dev(self.test_blockdev) |
| 301 | + mock_os.path.realpath.assert_called_with(self.test_syspath + |
| 302 | + '/bcache/cache') |
| 303 | self.assertEqual(bcache_dir, fake_bcache) |
| 304 | |
| 305 | + @mock.patch('curtin.block.clear_holders.os') |
| 306 | + @mock.patch('curtin.block.clear_holders.block') |
| 307 | + def test_get_bcache_sys_path(self, mock_block, mock_os): |
| 308 | + fake_backing = '/sys/class/block/fake' |
| 309 | + mock_block.sys_block_path.return_value = fake_backing |
| 310 | + mock_os.path.join.side_effect = os.path.join |
| 311 | + mock_os.path.exists.return_value = True |
| 312 | + bcache_dir = clear_holders.get_bcache_sys_path("/dev/fake") |
| 313 | + self.assertEqual(bcache_dir, fake_backing + "/bcache") |
| 314 | + |
| 315 | @mock.patch('curtin.block.clear_holders.get_dmsetup_uuid') |
| 316 | @mock.patch('curtin.block.clear_holders.block') |
| 317 | def test_differentiate_lvm_and_crypt( |
| 318 | @@ -115,22 +129,197 @@ |
| 319 | mock_block.path_to_kname.assert_called_with(self.test_syspath) |
| 320 | mock_get_dmsetup_uuid.assert_called_with(self.test_syspath) |
| 321 | |
| 322 | + @mock.patch('curtin.block.clear_holders.get_bcache_sys_path') |
| 323 | @mock.patch('curtin.block.clear_holders.util') |
| 324 | @mock.patch('curtin.block.clear_holders.os') |
| 325 | @mock.patch('curtin.block.clear_holders.LOG') |
| 326 | @mock.patch('curtin.block.clear_holders.get_bcache_using_dev') |
| 327 | def test_shutdown_bcache(self, mock_get_bcache, mock_log, mock_os, |
| 328 | - mock_util): |
| 329 | + mock_util, mock_get_bcache_block): |
| 330 | """test clear_holders.shutdown_bcache""" |
| 331 | + # |
| 332 | + # pass in a sysfs path to a bcache block device, |
| 333 | + # determine the bcache cset it is part of (or not) |
| 334 | + # 1) stop the cset device (if it's enabled) |
| 335 | + # 2) wait on cset to be removed if it was present |
| 336 | + # 3) stop the block device (if it's still present after stopping cset) |
| 337 | + # 4) wait on bcache block device to be removed |
| 338 | + # |
| 339 | + |
| 340 | + device = self.test_syspath |
| 341 | + bcache_cset_uuid = 'c08ae789-a964-46fb-a66e-650f0ae78f94' |
| 342 | + |
| 343 | mock_os.path.exists.return_value = True |
| 344 | mock_os.path.join.side_effect = os.path.join |
| 345 | - mock_get_bcache.return_value = self.test_blockdev |
| 346 | - clear_holders.shutdown_bcache(self.test_syspath) |
| 347 | - mock_get_bcache.assert_called_with(self.test_syspath) |
| 348 | - self.assertTrue(mock_log.debug.called) |
| 349 | + # os.path.realpath on symlink of /sys/class/block/null/bcache/cache -> |
| 350 | + # to /sys/fs/bcache/cset_UUID |
| 351 | + mock_get_bcache.return_value = '/sys/fs/bcache/' + bcache_cset_uuid |
| 352 | + mock_get_bcache_block.return_value = device + '/bcache' |
| 353 | + |
| 354 | + clear_holders.shutdown_bcache(device) |
| 355 | + |
| 356 | + mock_get_bcache.assert_called_with(device) |
| 357 | + mock_get_bcache_block.assert_called_with(device) |
| 358 | + |
| 359 | + self.assertTrue(mock_log.info.called) |
| 360 | self.assertFalse(mock_log.warn.called) |
| 361 | - mock_util.write_file.assert_called_with(self.test_blockdev + '/stop', |
| 362 | + mock_util.wait_for_removal.assert_has_calls([ |
| 363 | + mock.call('/sys/fs/bcache/' + bcache_cset_uuid, |
| 364 | + retries=self.remove_retries), |
| 365 | + mock.call(device, retries=self.remove_retries)]) |
| 366 | + |
| 367 | + mock_util.write_file.assert_has_calls([ |
| 368 | + mock.call('/sys/fs/bcache/%s/stop' % bcache_cset_uuid, |
| 369 | + '1', mode=None), |
| 370 | + mock.call(device + '/bcache/stop', |
| 371 | + '1', mode=None)]) |
| 372 | + |
| 373 | + @mock.patch('curtin.block.clear_holders.get_bcache_sys_path') |
| 374 | + @mock.patch('curtin.block.clear_holders.util') |
| 375 | + @mock.patch('curtin.block.clear_holders.os') |
| 376 | + @mock.patch('curtin.block.clear_holders.LOG') |
| 377 | + @mock.patch('curtin.block.clear_holders.get_bcache_using_dev') |
| 378 | + def test_shutdown_bcache_non_sysfs_device(self, mock_get_bcache, mock_log, |
| 379 | + mock_os, mock_util, |
| 380 | + mock_get_bcache_block): |
| 381 | + device = "/dev/fakenull" |
| 382 | + with self.assertRaises(ValueError): |
| 383 | + clear_holders.shutdown_bcache(device) |
| 384 | + |
| 385 | + self.assertEqual(0, len(mock_get_bcache.call_args_list)) |
| 386 | + self.assertEqual(0, len(mock_log.call_args_list)) |
| 387 | + self.assertEqual(0, len(mock_os.call_args_list)) |
| 388 | + self.assertEqual(0, len(mock_util.call_args_list)) |
| 389 | + self.assertEqual(0, len(mock_get_bcache_block.call_args_list)) |
| 390 | + |
| 391 | + @mock.patch('curtin.block.clear_holders.get_bcache_sys_path') |
| 392 | + @mock.patch('curtin.block.clear_holders.util') |
| 393 | + @mock.patch('curtin.block.clear_holders.os') |
| 394 | + @mock.patch('curtin.block.clear_holders.LOG') |
| 395 | + @mock.patch('curtin.block.clear_holders.get_bcache_using_dev') |
| 396 | + def test_shutdown_bcache_no_device(self, mock_get_bcache, mock_log, |
| 397 | + mock_os, mock_util, |
| 398 | + mock_get_bcache_block): |
| 399 | + device = "/sys/class/block/null" |
| 400 | + mock_os.path.exists.return_value = False |
| 401 | + |
| 402 | + clear_holders.shutdown_bcache(device) |
| 403 | + |
| 404 | + self.assertEqual(1, len(mock_log.info.call_args_list)) |
| 405 | + self.assertEqual(1, len(mock_os.path.exists.call_args_list)) |
| 406 | + self.assertEqual(0, len(mock_get_bcache.call_args_list)) |
| 407 | + self.assertEqual(0, len(mock_util.call_args_list)) |
| 408 | + self.assertEqual(0, len(mock_get_bcache_block.call_args_list)) |
| 409 | + |
| 410 | + @mock.patch('curtin.block.clear_holders.get_bcache_sys_path') |
| 411 | + @mock.patch('curtin.block.clear_holders.util') |
| 412 | + @mock.patch('curtin.block.clear_holders.os') |
| 413 | + @mock.patch('curtin.block.clear_holders.LOG') |
| 414 | + @mock.patch('curtin.block.clear_holders.get_bcache_using_dev') |
| 415 | + def test_shutdown_bcache_no_cset(self, mock_get_bcache, mock_log, |
| 416 | + mock_os, mock_util, |
| 417 | + mock_get_bcache_block): |
| 418 | + device = "/sys/class/block/null" |
| 419 | + mock_os.path.exists.side_effect = iter([ |
| 420 | + True, # backing device exists |
| 421 | + False, # cset device not present (already removed) |
| 422 | + True, # backing device (still) exists |
| 423 | + ]) |
| 424 | + mock_get_bcache.return_value = '/sys/fs/bcache/fake' |
| 425 | + mock_get_bcache_block.return_value = device + '/bcache' |
| 426 | + mock_os.path.join.side_effect = os.path.join |
| 427 | + |
| 428 | + clear_holders.shutdown_bcache(device) |
| 429 | + |
| 430 | + self.assertEqual(2, len(mock_log.info.call_args_list)) |
| 431 | + self.assertEqual(3, len(mock_os.path.exists.call_args_list)) |
| 432 | + self.assertEqual(1, len(mock_get_bcache.call_args_list)) |
| 433 | + self.assertEqual(1, len(mock_get_bcache_block.call_args_list)) |
| 434 | + self.assertEqual(1, len(mock_util.write_file.call_args_list)) |
| 435 | + self.assertEqual(1, len(mock_util.wait_for_removal.call_args_list)) |
| 436 | + |
| 437 | + mock_get_bcache.assert_called_with(device) |
| 438 | + mock_get_bcache_block.assert_called_with(device) |
| 439 | + mock_util.write_file.assert_called_with(device + '/bcache/stop', |
| 440 | '1', mode=None) |
| 441 | + retries = self.remove_retries |
| 442 | + mock_util.wait_for_removal.assert_called_with(device, retries=retries) |
| 443 | + |
| 444 | + @mock.patch('curtin.block.clear_holders.get_bcache_sys_path') |
| 445 | + @mock.patch('curtin.block.clear_holders.util') |
| 446 | + @mock.patch('curtin.block.clear_holders.os') |
| 447 | + @mock.patch('curtin.block.clear_holders.LOG') |
| 448 | + @mock.patch('curtin.block.clear_holders.get_bcache_using_dev') |
| 449 | + def test_shutdown_bcache_delete_cset_and_backing(self, mock_get_bcache, |
| 450 | + mock_log, mock_os, |
| 451 | + mock_util, |
| 452 | + mock_get_bcache_block): |
| 453 | + device = "/sys/class/block/null" |
| 454 | + mock_os.path.exists.side_effect = iter([ |
| 455 | + True, # backing device exists |
| 456 | + True, # cset device not present (already removed) |
| 457 | + True, # backing device (still) exists |
| 458 | + ]) |
| 459 | + cset = '/sys/fs/bcache/fake' |
| 460 | + mock_get_bcache.return_value = cset |
| 461 | + mock_get_bcache_block.return_value = device + '/bcache' |
| 462 | + mock_os.path.join.side_effect = os.path.join |
| 463 | + |
| 464 | + clear_holders.shutdown_bcache(device) |
| 465 | + |
| 466 | + self.assertEqual(2, len(mock_log.info.call_args_list)) |
| 467 | + self.assertEqual(3, len(mock_os.path.exists.call_args_list)) |
| 468 | + self.assertEqual(1, len(mock_get_bcache.call_args_list)) |
| 469 | + self.assertEqual(1, len(mock_get_bcache_block.call_args_list)) |
| 470 | + self.assertEqual(2, len(mock_util.write_file.call_args_list)) |
| 471 | + self.assertEqual(2, len(mock_util.wait_for_removal.call_args_list)) |
| 472 | + |
| 473 | + mock_get_bcache.assert_called_with(device) |
| 474 | + mock_get_bcache_block.assert_called_with(device) |
| 475 | + mock_util.write_file.assert_has_calls([ |
| 476 | + mock.call(cset + '/stop', '1', mode=None), |
| 477 | + mock.call(device + '/bcache/stop', '1', mode=None)]) |
| 478 | + mock_util.wait_for_removal.assert_has_calls([ |
| 479 | + mock.call(cset, retries=self.remove_retries), |
| 480 | + mock.call(device, retries=self.remove_retries) |
| 481 | + ]) |
| 482 | + |
| 483 | + @mock.patch('curtin.block.clear_holders.get_bcache_sys_path') |
| 484 | + @mock.patch('curtin.block.clear_holders.util') |
| 485 | + @mock.patch('curtin.block.clear_holders.os') |
| 486 | + @mock.patch('curtin.block.clear_holders.LOG') |
| 487 | + @mock.patch('curtin.block.clear_holders.get_bcache_using_dev') |
| 488 | + def test_shutdown_bcache_delete_cset_no_backing(self, mock_get_bcache, |
| 489 | + mock_log, mock_os, |
| 490 | + mock_util, |
| 491 | + mock_get_bcache_block): |
| 492 | + device = "/sys/class/block/null" |
| 493 | + mock_os.path.exists.side_effect = iter([ |
| 494 | + True, # backing device exists |
| 495 | + True, # cset device not present (already removed) |
| 496 | + False, # backing device is removed with cset |
| 497 | + ]) |
| 498 | + cset = '/sys/fs/bcache/fake' |
| 499 | + mock_get_bcache.return_value = cset |
| 500 | + mock_get_bcache_block.return_value = device + '/bcache' |
| 501 | + mock_os.path.join.side_effect = os.path.join |
| 502 | + |
| 503 | + clear_holders.shutdown_bcache(device) |
| 504 | + |
| 505 | + self.assertEqual(2, len(mock_log.info.call_args_list)) |
| 506 | + self.assertEqual(3, len(mock_os.path.exists.call_args_list)) |
| 507 | + self.assertEqual(1, len(mock_get_bcache.call_args_list)) |
| 508 | + self.assertEqual(0, len(mock_get_bcache_block.call_args_list)) |
| 509 | + self.assertEqual(1, len(mock_util.write_file.call_args_list)) |
| 510 | + self.assertEqual(1, len(mock_util.wait_for_removal.call_args_list)) |
| 511 | + |
| 512 | + mock_get_bcache.assert_called_with(device) |
| 513 | + mock_util.write_file.assert_has_calls([ |
| 514 | + mock.call(cset + '/stop', '1', mode=None), |
| 515 | + ]) |
| 516 | + mock_util.wait_for_removal.assert_has_calls([ |
| 517 | + mock.call(cset, retries=self.remove_retries) |
| 518 | + ]) |
| 519 | |
| 520 | @mock.patch('curtin.block.clear_holders.LOG') |
| 521 | @mock.patch('curtin.block.clear_holders.block.sys_block_path') |
| 522 | |
| 523 | === modified file 'tests/unittests/test_util.py' |
| 524 | --- tests/unittests/test_util.py 2017-04-07 20:12:45 +0000 |
| 525 | +++ tests/unittests/test_util.py 2017-04-24 17:53:29 +0000 |
| 526 | @@ -614,4 +614,73 @@ |
| 527 | except KeyError as e: |
| 528 | self.fail("unexpected key error raised: %s" % e) |
| 529 | |
| 530 | + |
| 531 | +class TestWaitForRemoval(TestCase): |
| 532 | + def test_wait_for_removal_missing_path(self): |
| 533 | + with self.assertRaises(ValueError): |
| 534 | + util.wait_for_removal(None) |
| 535 | + |
| 536 | + @mock.patch('curtin.util.time') |
| 537 | + @mock.patch('curtin.util.os') |
| 538 | + def test_wait_for_removal(self, mock_os, mock_time): |
| 539 | + path = "/file/to/remove" |
| 540 | + mock_os.path.exists.side_effect = iter([ |
| 541 | + True, # File is not yet removed |
| 542 | + False, # File has been removed |
| 543 | + ]) |
| 544 | + |
| 545 | + util.wait_for_removal(path) |
| 546 | + |
| 547 | + self.assertEqual(2, len(mock_os.path.exists.call_args_list)) |
| 548 | + self.assertEqual(1, len(mock_time.sleep.call_args_list)) |
| 549 | + mock_os.path.exists.assert_has_calls([ |
| 550 | + mock.call(path), |
| 551 | + mock.call(path), |
| 552 | + ]) |
| 553 | + mock_time.sleep.assert_has_calls([ |
| 554 | + mock.call(1), |
| 555 | + ]) |
| 556 | + |
| 557 | + @mock.patch('curtin.util.time') |
| 558 | + @mock.patch('curtin.util.os') |
| 559 | + def test_wait_for_removal_timesout(self, mock_os, mock_time): |
| 560 | + path = "/file/to/remove" |
| 561 | + mock_os.path.exists.return_value = True |
| 562 | + |
| 563 | + with self.assertRaises(OSError): |
| 564 | + util.wait_for_removal(path) |
| 565 | + |
| 566 | + self.assertEqual(5, len(mock_os.path.exists.call_args_list)) |
| 567 | + self.assertEqual(4, len(mock_time.sleep.call_args_list)) |
| 568 | + mock_os.path.exists.assert_has_calls(5 * [mock.call(path)]) |
| 569 | + mock_time.sleep.assert_has_calls([ |
| 570 | + mock.call(1), |
| 571 | + mock.call(3), |
| 572 | + mock.call(5), |
| 573 | + mock.call(7), |
| 574 | + ]) |
| 575 | + |
| 576 | + @mock.patch('curtin.util.time') |
| 577 | + @mock.patch('curtin.util.os') |
| 578 | + def test_wait_for_removal_custom_retry(self, mock_os, mock_time): |
| 579 | + path = "/file/to/remove" |
| 580 | + timeout = 100 |
| 581 | + mock_os.path.exists.side_effect = iter([ |
| 582 | + True, # File is not yet removed |
| 583 | + False, # File has been removed |
| 584 | + ]) |
| 585 | + |
| 586 | + util.wait_for_removal(path, retries=[timeout]) |
| 587 | + |
| 588 | + self.assertEqual(2, len(mock_os.path.exists.call_args_list)) |
| 589 | + self.assertEqual(1, len(mock_time.sleep.call_args_list)) |
| 590 | + mock_os.path.exists.assert_has_calls([ |
| 591 | + mock.call(path), |
| 592 | + mock.call(path), |
| 593 | + ]) |
| 594 | + mock_time.sleep.assert_has_calls([ |
| 595 | + mock.call(timeout), |
| 596 | + ]) |
| 597 | + |
| 598 | + |
| 599 | # vi: ts=4 expandtab syntax=python |
| 600 | |
| 601 | === modified file 'tests/vmtests/test_nvme.py' |
| 602 | --- tests/vmtests/test_nvme.py 2017-02-22 01:42:16 +0000 |
| 603 | +++ tests/vmtests/test_nvme.py 2017-04-24 17:53:29 +0000 |
| 604 | @@ -99,3 +99,80 @@ |
| 605 | |
| 606 | class ZestyTestNvme(relbase.zesty, TestNvmeAbs): |
| 607 | __test__ = True |
| 608 | + |
| 609 | + |
| 610 | +class TestNvmeBcacheAbs(VMBaseClass): |
| 611 | + arch_skip = [ |
| 612 | + "s390x", # nvme is a pci device, no pci on s390x |
| 613 | + ] |
| 614 | + interactive = False |
| 615 | + conf_file = "examples/tests/nvme_bcache.yaml" |
| 616 | + extra_disks = ['10G'] |
| 617 | + nvme_disks = ['6G'] |
| 618 | + uefi = True |
| 619 | + disk_to_check = [('sda', 1), ('sda', 2), ('sda', 3)] |
| 620 | + |
| 621 | + collect_scripts = [textwrap.dedent(""" |
| 622 | + cd OUTPUT_COLLECT_D |
| 623 | + ls /sys/class/ > sys_class |
| 624 | + ls /sys/class/nvme/ > ls_nvme |
| 625 | + ls /dev/nvme* > ls_dev_nvme |
| 626 | + ls /dev/disk/by-dname/ > ls_dname |
| 627 | + ls -al /dev/bcache/by-uuid/ > ls_bcache_by_uuid |: |
| 628 | + blkid -o export /dev/vda > blkid_output_vda |
| 629 | + blkid -o export /dev/vda1 > blkid_output_vda1 |
| 630 | + blkid -o export /dev/vda2 > blkid_output_vda2 |
| 631 | + bcache-super-show /dev/nvme0n1p1 > bcache_super_nvme0n1p1 |
| 632 | + ls /sys/fs/bcache > bcache_ls |
| 633 | + cat /sys/block/bcache0/bcache/cache_mode > bcache_cache_mode |
| 634 | + cat /proc/partitions > proc_partitions |
| 635 | + ls -al /dev/disk/by-uuid/ > ls_uuid |
| 636 | + cat /etc/fstab > fstab |
| 637 | + mkdir -p /dev/disk/by-dname |
| 638 | + ls /dev/disk/by-dname/ > ls_dname |
| 639 | + find /etc/network/interfaces.d > find_interfacesd |
| 640 | + |
| 641 | + v="" |
| 642 | + out=$(apt-config shell v Acquire::HTTP::Proxy) |
| 643 | + eval "$out" |
| 644 | + echo "$v" > apt-proxy |
| 645 | + """)] |
| 646 | + |
| 647 | + def test_output_files_exist(self): |
| 648 | + self.output_files_exist(["ls_nvme", "ls_dname", "ls_dev_nvme"]) |
| 649 | + |
| 650 | + def test_nvme_device_names(self): |
| 651 | + ls_nvme = os.path.join(self.td.collect, 'ls_nvme') |
| 652 | + # trusty and vivid do not have sys/class/nvme but |
| 653 | + # nvme devices do work |
| 654 | + if os.path.getsize(ls_nvme) > 0: |
| 655 | + self.check_file_strippedline("ls_nvme", "nvme0") |
| 656 | + else: |
| 657 | + self.check_file_strippedline("ls_dev_nvme", "/dev/nvme0") |
| 658 | + self.check_file_strippedline("ls_dev_nvme", "/dev/nvme0n1") |
| 659 | + self.check_file_strippedline("ls_dev_nvme", "/dev/nvme0n1p1") |
| 660 | + |
| 661 | + def test_bcache_output_files_exist(self): |
| 662 | + self.output_files_exist(["bcache_super_nvme0n1p1", "bcache_ls", |
| 663 | + "bcache_cache_mode"]) |
| 664 | + |
| 665 | + def test_bcache_status(self): |
| 666 | + bcache_cset_uuid = None |
| 667 | + bcache_super = self.load_collect_file("bcache_super_nvme0n1p1") |
| 668 | + for line in bcache_super.splitlines(): |
| 669 | + if line != "" and line.split()[0] == "cset.uuid": |
| 670 | + bcache_cset_uuid = line.split()[-1].rstrip() |
| 671 | + self.assertIsNotNone(bcache_cset_uuid) |
| 672 | + self.assertTrue(bcache_cset_uuid in |
| 673 | + self.load_collect_file("bcache_ls").splitlines()) |
| 674 | + |
| 675 | + def test_bcache_cachemode(self): |
| 676 | + self.check_file_regex("bcache_cache_mode", r"\[writeback\]") |
| 677 | + |
| 678 | + |
| 679 | +class XenialTestNvmeBcache(relbase.xenial, TestNvmeBcacheAbs): |
| 680 | + __test__ = True |
| 681 | + |
| 682 | + |
| 683 | +class ZestyTestNvmeBcache(relbase.zesty, TestNvmeBcacheAbs): |
| 684 | + __test__ = True |
PASSED: Continuous integration, rev:492
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/