From a246c5576d726c7bc385b49bc7b626eb6edcd137 Mon Sep 17 00:00:00 2001 From: Michael Still Date: Sat, 15 Dec 2012 18:32:20 +1100 Subject: Make NBD retry logic more generic, add retry to loop. Other device implementations require retry logic as well. This change pushes the retry logic up one layer so that its easy to implement for the other drivers that will benefit from it, and then adds retry to the loop driver. This change also adds some unit test coverage for the loop driver. Change-Id: Iab0d42d5075e9d50b4e7eb8c7fcef12cae281b40 --- nova/tests/virt/disk/test_loop.py | 105 ++++++++++++++++++++++++++++++++++++++ nova/tests/virt/disk/test_nbd.py | 4 +- nova/virt/disk/mount/api.py | 23 +++++++++ nova/virt/disk/mount/loop.py | 20 +++++++- nova/virt/disk/mount/nbd.py | 20 +++----- 5 files changed, 154 insertions(+), 18 deletions(-) create mode 100644 nova/tests/virt/disk/test_loop.py diff --git a/nova/tests/virt/disk/test_loop.py b/nova/tests/virt/disk/test_loop.py new file mode 100644 index 000000000..47f45d565 --- /dev/null +++ b/nova/tests/virt/disk/test_loop.py @@ -0,0 +1,105 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# Copyright 2012 Michael Still +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +import os + +import fixtures +from testtools.matchers import Equals +from testtools.matchers import MatchesListwise + +from nova import test +from nova import utils +from nova.virt.disk.mount import loop + + +def _fake_noop(*args, **kwargs): + return + + +def _fake_trycmd_losetup_works(*args, **kwargs): + return '/dev/loop0', '' + + +def _fake_trycmd_losetup_fails(*args, **kwards): + return '', 'doh' + + +class LoopTestCase(test.TestCase): + def test_get_dev(self): + tempdir = self.useFixture(fixtures.TempDir()).path + l = loop.LoopMount(None, tempdir) + self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', + _fake_trycmd_losetup_works)) + self.useFixture(fixtures.MonkeyPatch('nova.utils.execute', + _fake_noop)) + + # No error logged, device consumed + self.assertTrue(l.get_dev()) + self.assertTrue(l.linked) + self.assertEquals('', l.error) + self.assertEquals('/dev/loop0', l.device) + + # Free + l.unget_dev() + self.assertFalse(l.linked) + self.assertEquals('', l.error) + self.assertEquals(None, l.device) + + def test_inner_get_dev_fails(self): + tempdir = self.useFixture(fixtures.TempDir()).path + l = loop.LoopMount(None, tempdir) + self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', + _fake_trycmd_losetup_fails)) + + # No error logged, device consumed + self.assertFalse(l._inner_get_dev()) + self.assertFalse(l.linked) + self.assertNotEquals('', l.error) + self.assertEquals(None, l.device) + + # Free + l.unget_dev() + self.assertFalse(l.linked) + self.assertEquals(None, l.device) + + def test_get_dev_timeout(self): + tempdir = self.useFixture(fixtures.TempDir()).path + l = loop.LoopMount(None, tempdir) + self.useFixture(fixtures.MonkeyPatch('time.sleep', _fake_noop)) + self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', + _fake_trycmd_losetup_fails)) + self.useFixture(fixtures.MonkeyPatch(('nova.virt.disk.mount.api.' + 'MAX_DEVICE_WAIT'), -10)) + + # Always fail to get a device + def fake_get_dev_fails(): + return False + l._inner_get_dev = fake_get_dev_fails + + # Fail to get a device + self.assertFalse(l.get_dev()) + + def test_unget_dev(self): + tempdir = self.useFixture(fixtures.TempDir()).path + l = loop.LoopMount(None, tempdir) + self.useFixture(fixtures.MonkeyPatch('nova.utils.execute', + _fake_noop)) + + # This just checks that a free of something we don't have doesn't + # throw an exception + l.unget_dev() diff --git a/nova/tests/virt/disk/test_nbd.py b/nova/tests/virt/disk/test_nbd.py index 4b067d405..a73df8a84 100644 --- a/nova/tests/virt/disk/test_nbd.py +++ b/nova/tests/virt/disk/test_nbd.py @@ -268,8 +268,8 @@ class NbdTestCase(test.TestCase): self.fake_exists_one)) self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', self.fake_trycmd_creates_pid)) - self.useFixture(fixtures.MonkeyPatch(('nova.virt.disk.mount.nbd.' - 'MAX_NBD_WAIT'), -10)) + self.useFixture(fixtures.MonkeyPatch(('nova.virt.disk.mount.api.' + 'MAX_DEVICE_WAIT'), -10)) # No error logged, device consumed self.assertFalse(n.get_dev()) diff --git a/nova/virt/disk/mount/api.py b/nova/virt/disk/mount/api.py index 5354c8553..ac396ff80 100644 --- a/nova/virt/disk/mount/api.py +++ b/nova/virt/disk/mount/api.py @@ -16,6 +16,7 @@ """Support for mounting virtual image files""" import os +import time from nova.openstack.common import importutils from nova.openstack.common import log as logging @@ -23,6 +24,8 @@ from nova import utils LOG = logging.getLogger(__name__) +MAX_DEVICE_WAIT = 30 + class Mount(object): """Standard mounting operations, that can be overridden by subclasses. @@ -102,6 +105,26 @@ class Mount(object): self.linked = True return True + def _get_dev_retry_helper(self): + """Some implementations need to retry their get_dev.""" + # NOTE(mikal): This method helps implement retries. The implementation + # simply calls _get_dev_retry_helper from their get_dev, and implements + # _inner_get_dev with their device acquistion logic. The NBD + # implementation has an example. + start_time = time.time() + device = self._inner_get_dev() + while not device: + LOG.info(_('Device allocation failed. Will retry in 2 seconds.')) + time.sleep(2) + if time.time() - start_time > MAX_DEVICE_WAIT: + LOG.warn(_('Device allocation failed after repeated retries.')) + return False + device = self._inner_get_dev() + return True + + def _inner_get_dev(self): + raise NotImplementedError() + def unget_dev(self): """Release the block device from the file system namespace.""" self.linked = False diff --git a/nova/virt/disk/mount/loop.py b/nova/virt/disk/mount/loop.py index 180e4d796..667ecee14 100644 --- a/nova/virt/disk/mount/loop.py +++ b/nova/virt/disk/mount/loop.py @@ -26,11 +26,14 @@ class LoopMount(api.Mount): """loop back support for raw images.""" mode = 'loop' - def get_dev(self): + def _inner_get_dev(self): out, err = utils.trycmd('losetup', '--find', '--show', self.image, run_as_root=True) if err: self.error = _('Could not attach image to loopback: %s') % err + LOG.info(_('Loop mount error: %s'), self.error) + self.linked = False + self.device = None return False self.device = out.strip() @@ -38,9 +41,22 @@ class LoopMount(api.Mount): self.linked = True return True + def get_dev(self): + # NOTE(mikal): the retry is required here in case we are low on loop + # devices. Note however that modern kernels will use more loop devices + # if they exist. If you're seeing lots of retries, consider adding + # more devices. + return self._get_dev_retry_helper() + def unget_dev(self): if not self.linked: return + + # NOTE(mikal): On some kernels, losetup -d will intermittently fail, + # thus leaking a loop device unless the losetup --detach is retried: + # https://lkml.org/lkml/2012/9/28/62 LOG.debug(_("Release loop device %s"), self.device) - utils.execute('losetup', '--detach', self.device, run_as_root=True) + utils.execute('losetup', '--detach', self.device, run_as_root=True, + attempts=3) self.linked = False + self.device = None diff --git a/nova/virt/disk/mount/nbd.py b/nova/virt/disk/mount/nbd.py index 1b6cc0778..ec9bde73c 100644 --- a/nova/virt/disk/mount/nbd.py +++ b/nova/virt/disk/mount/nbd.py @@ -37,7 +37,6 @@ CONF = cfg.CONF CONF.register_opts(nbd_opts) NBD_DEVICE_RE = re.compile('nbd[0-9]+') -MAX_NBD_WAIT = 30 class NbdMount(api.Mount): @@ -89,6 +88,7 @@ class NbdMount(api.Mount): run_as_root=True) if err: self.error = _('qemu-nbd error: %s') % err + LOG.info(_('NBD mount error: %s'), self.error) return False # NOTE(vish): this forks into another process, so give it a chance @@ -100,12 +100,15 @@ class NbdMount(api.Mount): break time.sleep(1) else: + self.error = _('nbd device %s did not show up') % device + LOG.info(_('NBD mount error: %s'), self.error) + + # Cleanup _out, err = utils.trycmd('qemu-nbd', '-d', device, run_as_root=True) if err: LOG.warn(_('Detaching from erroneous nbd device returned ' 'error: %s'), err) - self.error = _('nbd device %s did not show up') % device return False self.error = '' @@ -114,18 +117,7 @@ class NbdMount(api.Mount): def get_dev(self): """Retry requests for NBD devices.""" - start_time = time.time() - device = self._inner_get_dev() - while not device: - LOG.info(_('nbd device allocation failed. Will retry in 2 ' - 'seconds.')) - time.sleep(2) - if time.time() - start_time > MAX_NBD_WAIT: - LOG.warn(_('nbd device allocation failed after repeated ' - 'retries.')) - return False - device = self._inner_get_dev() - return True + return self._get_dev_retry_helper() def unget_dev(self): if not self.linked: -- cgit