Merge "Retry NBD device allocation."

2 files changed, 101 insertions, 36 deletions

diff --git a/nova/tests/virt/disk/test_nbd.py b/nova/tests/virt/disk/test_nbd.py
index 5e08215a5..17e149cbc 100644
--- a/nova/tests/virt/disk/test_nbd.py
+++ b/nova/tests/virt/disk/test_nbd.py
@@ -139,13 +139,13 @@ class NbdTestCase(test.TestCase):
         # re-added. I will fix this in a later patch.
         self.assertEquals('/dev/nbd1', n._allocate_nbd())
 
-    def test_get_dev_no_devices(self):
+    def test_inner_get_dev_no_devices(self):
         tempdir = self.useFixture(fixtures.TempDir()).path
         n = nbd.NbdMount(None, tempdir)
         n.detect_nbd_device = _fake_detect_nbd_devices
-        self.assertFalse(n.get_dev())
+        self.assertFalse(n._inner_get_dev())
 
-    def test_get_dev_qemu_fails(self):
+    def test_inner_get_dev_qemu_fails(self):
         tempdir = self.useFixture(fixtures.TempDir()).path
         n = nbd.NbdMount(None, tempdir)
         self.useFixture(fixtures.MonkeyPatch('os.path.exists',
@@ -157,10 +157,10 @@ class NbdTestCase(test.TestCase):
         self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', fake_trycmd))
 
         # Error logged, no device consumed
-        self.assertFalse(n.get_dev())
+        self.assertFalse(n._inner_get_dev())
         self.assertTrue(n.error.startswith('qemu-nbd error'))
 
-    def test_get_dev_qemu_timeout(self):
+    def test_inner_get_dev_qemu_timeout(self):
         tempdir = self.useFixture(fixtures.TempDir()).path
         n = nbd.NbdMount(None, tempdir)
         n.detect_nbd_device = _fake_detect_nbd_devices
@@ -174,46 +174,46 @@ class NbdTestCase(test.TestCase):
         self.useFixture(fixtures.MonkeyPatch('time.sleep', _fake_noop))
 
         # Error logged, no device consumed
-        self.assertFalse(n.get_dev())
+        self.assertFalse(n._inner_get_dev())
         self.assertTrue(n.error.endswith('did not show up'))
 
-    def test_get_dev_works(self):
-        tempdir = self.useFixture(fixtures.TempDir()).path
-        n = nbd.NbdMount(None, tempdir)
-        n.detect_nbd_device = _fake_detect_nbd_devices
-        self.useFixture(fixtures.MonkeyPatch('random.shuffle', _fake_noop))
-        self.useFixture(fixtures.MonkeyPatch('nova.utils.execute', _fake_noop))
-
+    def fake_exists_one(self, path):
         # We need the pid file for the device which is allocated to exist, but
         # only once it is allocated to us
-        def fake_exists_one(path):
+        if path.startswith('/sys/block/nbd'):
+            if path == '/sys/block/nbd1/pid':
+                return False
+            if path.endswith('pid'):
+                return False
+            return True
+        return ORIG_EXISTS(path)
+
+    def fake_trycmd_creates_pid(self, *args, **kwargs):
+        def fake_exists_two(path):
             if path.startswith('/sys/block/nbd'):
-                if path == '/sys/block/nbd1/pid':
-                    return False
+                if path == '/sys/block/nbd0/pid':
+                    return True
                 if path.endswith('pid'):
                     return False
                 return True
             return ORIG_EXISTS(path)
         self.useFixture(fixtures.MonkeyPatch('os.path.exists',
-                                             fake_exists_one))
+                                            fake_exists_two))
+        return '', ''
 
-        # We have a trycmd that always passed
-        def fake_trycmd(*args, **kwargs):
-            def fake_exists_two(path):
-                if path.startswith('/sys/block/nbd'):
-                    if path == '/sys/block/nbd0/pid':
-                        return True
-                    if path.endswith('pid'):
-                        return False
-                    return True
-                return ORIG_EXISTS(path)
-            self.useFixture(fixtures.MonkeyPatch('os.path.exists',
-                                                 fake_exists_two))
-            return '', ''
-        self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', fake_trycmd))
+    def test_inner_get_dev_works(self):
+        tempdir = self.useFixture(fixtures.TempDir()).path
+        n = nbd.NbdMount(None, tempdir)
+        n.detect_nbd_device = _fake_detect_nbd_devices
+        self.useFixture(fixtures.MonkeyPatch('random.shuffle', _fake_noop))
+        self.useFixture(fixtures.MonkeyPatch('os.path.exists',
+                                             self.fake_exists_one))
+        self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd',
+                                             self.fake_trycmd_creates_pid))
+        self.useFixture(fixtures.MonkeyPatch('nova.utils.execute', _fake_noop))
 
         # No error logged, device consumed
-        self.assertTrue(n.get_dev())
+        self.assertTrue(n._inner_get_dev())
         self.assertTrue(n.linked)
         self.assertEquals('', n.error)
         self.assertEquals('/dev/nbd0', n.device)
@@ -229,4 +229,50 @@ class NbdTestCase(test.TestCase):
         # something we don't have
         tempdir = self.useFixture(fixtures.TempDir()).path
         n = nbd.NbdMount(None, tempdir)
+        self.useFixture(fixtures.MonkeyPatch('nova.utils.execute', _fake_noop))
         n.unget_dev()
+
+    def test_get_dev(self):
+        tempdir = self.useFixture(fixtures.TempDir()).path
+        n = nbd.NbdMount(None, tempdir)
+        n.detect_nbd_device = _fake_detect_nbd_devices
+        self.useFixture(fixtures.MonkeyPatch('random.shuffle', _fake_noop))
+        self.useFixture(fixtures.MonkeyPatch('nova.utils.execute', _fake_noop))
+        self.useFixture(fixtures.MonkeyPatch('os.path.exists',
+                                             self.fake_exists_one))
+        self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd',
+                                             self.fake_trycmd_creates_pid))
+
+        # No error logged, device consumed
+        self.assertTrue(n.get_dev())
+        self.assertTrue(n.linked)
+        self.assertEquals('', n.error)
+        self.assertEquals('/dev/nbd0', n.device)
+
+        # Free
+        n.unget_dev()
+        self.assertFalse(n.linked)
+        self.assertEquals('', n.error)
+        self.assertEquals(None, n.device)
+
+    def test_get_dev_timeout(self):
+        tempdir = self.useFixture(fixtures.TempDir()).path
+        n = nbd.NbdMount(None, tempdir)
+        n.detect_nbd_device = _fake_detect_nbd_devices
+        self.useFixture(fixtures.MonkeyPatch('random.shuffle', _fake_noop))
+        self.useFixture(fixtures.MonkeyPatch('time.sleep', _fake_noop))
+        self.useFixture(fixtures.MonkeyPatch('nova.utils.execute', _fake_noop))
+        self.useFixture(fixtures.MonkeyPatch('os.path.exists',
+                                             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))
+
+        # Always fail to get a device
+        def fake_get_dev_fails():
+            return False
+        n._inner_get_dev = fake_get_dev_fails
+
+        # No error logged, device consumed
+        self.assertFalse(n.get_dev())
diff --git a/nova/virt/disk/mount/nbd.py b/nova/virt/disk/mount/nbd.py
index 90d858a4b..1b6cc0778 100644
--- a/nova/virt/disk/mount/nbd.py
+++ b/nova/virt/disk/mount/nbd.py
@@ -37,15 +37,13 @@ CONF = cfg.CONF
 CONF.register_opts(nbd_opts)
 
 NBD_DEVICE_RE = re.compile('nbd[0-9]+')
+MAX_NBD_WAIT = 30
 
 
 class NbdMount(api.Mount):
     """qemu-nbd support disk images."""
     mode = 'nbd'
 
-    # NOTE(padraig): The remaining issue with this code is that multiple
-    # workers on a system can race against each other.
-
     def _detect_nbd_devices(self):
         """Detect nbd device files."""
         return filter(NBD_DEVICE_RE.match, os.listdir('/sys/block/'))
@@ -78,7 +76,7 @@ class NbdMount(api.Mount):
             pid = int(f.readline())
         return pid
 
-    def get_dev(self):
+    def _inner_get_dev(self):
         device = self._allocate_nbd()
         if not device:
             return False
@@ -102,12 +100,33 @@ class NbdMount(api.Mount):
                 break
             time.sleep(1)
         else:
+            _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 = ''
         self.linked = True
         return True
 
+    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
+
     def unget_dev(self):
         if not self.linked:
             return