Move nbd commands to privsep.

The same pattern as previous patches. Some of these unit tests
are starting to be a bit simpler as we finish the transition.

Change-Id: If0e1fe4c0466f2f88525dc575af2ef366d4bb59d
blueprint: hurrah-for-privsep

etc/nova/rootwrap.d/compute.filters

@@ -10,10 +10,6 @@ kpartx: CommandFilter, kpartx, root
 # nova/virt/xenapi/vm_utils.py: tune2fs, -j, partition_path
 tune2fs: CommandFilter, tune2fs, root
 
-# nova/virt/disk/mount/nbd.py: 'qemu-nbd', '-c', device, image
-# nova/virt/disk/mount/nbd.py: 'qemu-nbd', '-d', device
-qemu-nbd: CommandFilter, qemu-nbd, root
-
 # nova/virt/disk/vfs/localfs.py: 'blkid', '-o', 'value', '-s', 'TYPE', device
 blkid: CommandFilter, blkid, root
 

nova/privsep/fs.py

@@ -97,3 +97,13 @@ def loopsetup(path):
 @nova.privsep.sys_admin_pctxt.entrypoint
 def loopremove(device):
     return processutils.execute('losetup', '--detach', device, attempts=3)
+
+
+@nova.privsep.sys_admin_pctxt.entrypoint
+def nbd_connect(device, image):
+    return processutils.execute('qemu-nbd', '-c', device, image)
+
+
+@nova.privsep.sys_admin_pctxt.entrypoint
+def nbd_disconnect(device):
+    return processutils.execute('qemu-nbd', '-d', device)

nova/tests/unit/virt/disk/mount/test_nbd.py

@@ -143,70 +143,45 @@ class NbdTestCase(test.NoDBTestCase):
         n = nbd.NbdMount(self.file, tempdir)
         self.assertFalse(n._inner_get_dev())
 
-    def test_inner_get_dev_qemu_fails(self):
+    @mock.patch('nova.privsep.fs.nbd_connect', return_value=('', 'broken'))
+    def test_inner_get_dev_qemu_fails(self, mock_nbd_connect):
         tempdir = self.useFixture(fixtures.TempDir()).path
         n = nbd.NbdMount(self.file, tempdir)
         self.useFixture(fixtures.MonkeyPatch('os.path.exists',
                                              _fake_exists_no_users))
 
-        # We have a trycmd that always fails
-        def fake_trycmd(*args, **kwargs):
-            return '', 'broken'
-        self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', fake_trycmd))
-
         # Error logged, no device consumed
         self.assertFalse(n._inner_get_dev())
         self.assertTrue(n.error.startswith('qemu-nbd error'))
 
-    def test_inner_get_dev_qemu_timeout(self):
+    @mock.patch('random.shuffle')
+    @mock.patch('os.path.exists', side_effect=[True, False, False, False,
+                                               False, False, False, False])
+    @mock.patch('os.listdir', return_value=['nbd0', 'nbd1', 'loop0'])
+    @mock.patch('nova.privsep.fs.nbd_connect', return_value=('', ''))
+    @mock.patch('nova.privsep.fs.nbd_disconnect', return_value=('', ''))
+    @mock.patch('time.sleep')
+    def test_inner_get_dev_qemu_timeout(self, mock_sleep, mock_nbd_disconnct,
+                                        mock_nbd_connect, mock_exists,
+                                        mock_listdir, mock_shuffle):
+        self.flags(timeout_nbd=3)
         tempdir = self.useFixture(fixtures.TempDir()).path
         n = nbd.NbdMount(self.file, tempdir)
-        self.useFixture(fixtures.MonkeyPatch('os.path.exists',
-                                             _fake_exists_no_users))
-
-        # We have a trycmd that always passed
-        def fake_trycmd(*args, **kwargs):
-            return '', ''
-        self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', fake_trycmd))
-        self.useFixture(fixtures.MonkeyPatch('time.sleep', _fake_noop))
 
         # Error logged, no device consumed
         self.assertFalse(n._inner_get_dev())
         self.assertTrue(n.error.endswith('did not show up'))
 
-    def fake_exists_one(self, path):
+    @mock.patch('random.shuffle')
-        # We need the pid file for the device which is allocated to exist, but
+    @mock.patch('os.path.exists', side_effect=[True, False, False, False,
-        # only once it is allocated to us
+                                               False, True])
-        if path.startswith('/sys/block/nbd'):
+    @mock.patch('os.listdir', return_value=['nbd0', 'nbd1', 'loop0'])
-            if path == '/sys/block/nbd1/pid':
+    @mock.patch('nova.privsep.fs.nbd_connect', return_value=('', ''))
-                return False
+    @mock.patch('nova.privsep.fs.nbd_disconnect')
-            if path.endswith('pid'):
+    def test_inner_get_dev_works(self, mock_nbd_disconnect, mock_nbd_connect,
-                return False
+                                 mock_exists, mock_listdir, mock_shuffle):
-            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/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 '', ''
-
-    def test_inner_get_dev_works(self):
         tempdir = self.useFixture(fixtures.TempDir()).path
         n = nbd.NbdMount(self.file, tempdir)
-        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._inner_get_dev())
@@ -228,15 +203,16 @@ class NbdTestCase(test.NoDBTestCase):
         self.useFixture(fixtures.MonkeyPatch('nova.utils.execute', _fake_noop))
         n.unget_dev()
 
-    def test_get_dev(self):
+    @mock.patch('random.shuffle')
+    @mock.patch('os.path.exists', side_effect=[True, False, False, False,
+                                               False, True])
+    @mock.patch('os.listdir', return_value=['nbd0', 'nbd1', 'loop0'])
+    @mock.patch('nova.privsep.fs.nbd_connect', return_value=('', ''))
+    @mock.patch('nova.privsep.fs.nbd_disconnect')
+    def test_get_dev(self, mock_nbd_disconnect, mock_nbd_connect,
+                     mock_exists, mock_listdir, mock_shuffle):
         tempdir = self.useFixture(fixtures.TempDir()).path
         n = nbd.NbdMount(self.file, tempdir)
-        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())
@@ -250,22 +226,18 @@ class NbdTestCase(test.NoDBTestCase):
         self.assertEqual('', n.error)
         self.assertIsNone(n.device)
 
-    def test_get_dev_timeout(self):
+    @mock.patch('random.shuffle')
-        # Always fail to get a device
+    @mock.patch('time.sleep')
-        def fake_get_dev_fails(self):
+    @mock.patch('nova.privsep.fs.nbd_connect')
-            return False
+    @mock.patch('nova.privsep.fs.nbd_disconnect')
-        self.stub_out('nova.virt.disk.mount.nbd.NbdMount._inner_get_dev',
+    @mock.patch('os.path.exists', return_value=True)
-                      fake_get_dev_fails)
+    @mock.patch('nova.virt.disk.mount.nbd.NbdMount._inner_get_dev',
-
+                return_value=False)
+    def test_get_dev_timeout(self, mock_get_dev, mock_exists,
+                             mock_nbd_disconnect, mock_nbd_connect,
+                             mock_sleep, mock_shuffle):
         tempdir = self.useFixture(fixtures.TempDir()).path
         n = nbd.NbdMount(self.file, tempdir)
-        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.api.'
                                               'MAX_DEVICE_WAIT'), -10))
 
@@ -288,7 +260,11 @@ class NbdTestCase(test.NoDBTestCase):
 
         self.assertFalse(mount.do_mount())
 
-    def test_device_creation_race(self):
+    @mock.patch('nova.privsep.fs.nbd_connect')
+    @mock.patch('nova.privsep.fs.nbd_disconnect')
+    @mock.patch('os.path.exists')
+    def test_device_creation_race(self, mock_exists, mock_nbd_disconnect,
+                                  mock_nbd_connect):
         # Make sure that even if two threads create instances at the same time
         # they cannot choose the same nbd number (see bug 1207422)
 
@@ -316,10 +292,8 @@ class NbdTestCase(test.NoDBTestCase):
 
         self.stub_out('nova.virt.disk.mount.nbd.NbdMount._allocate_nbd',
                       fake_find_unused)
-        self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd',
+        mock_nbd_connect.side_effect = delay_and_remove_device
-                                             delay_and_remove_device))
+        mock_exists.side_effect = pid_exists
-        self.useFixture(fixtures.MonkeyPatch('os.path.exists',
-                                             pid_exists))
 
         def get_a_device():
             n = nbd.NbdMount(self.file, tempdir)

nova/tests/unit/virt/test_virt.py

@@ -214,8 +214,10 @@ class TestVirtDisk(test.NoDBTestCase):
     @mock.patch('os.path.exists', return_value=True)
     @mock.patch('nova.privsep.fs.loopremove')
     @mock.patch('nova.privsep.fs.umount')
+    @mock.patch('nova.privsep.fs.nbd_disconnect')
     def test_lxc_teardown_container(
-            self, mock_umount, mock_loopremove, mock_exist):
+            self, mock_nbd_disconnect, mock_umount, mock_loopremove,
+            mock_exist):
 
         def proc_mounts(mount_point):
             mount_points = {
@@ -248,18 +250,20 @@ class TestVirtDisk(test.NoDBTestCase):
         disk_api.teardown_container('/mnt/nbd/nopart')
         expected_commands += [
                               ('blockdev', '--flushbufs', '/dev/nbd15'),
-                              ('qemu-nbd', '-d', '/dev/nbd15'),
                              ]
+        mock_nbd_disconnect.assert_has_calls([mock.call('/dev/nbd15')])
         mock_umount.assert_has_calls([mock.call('/dev/nbd15')])
+        mock_nbd_disconnect.reset_mock()
         mock_umount.reset_mock()
 
         disk_api.teardown_container('/mnt/nbd/part')
         expected_commands += [
                               ('blockdev', '--flushbufs', '/dev/nbd15'),
                               ('kpartx', '-d', '/dev/nbd15'),
-                              ('qemu-nbd', '-d', '/dev/nbd15'),
                              ]
+        mock_nbd_disconnect.assert_has_calls([mock.call('/dev/nbd15')])
         mock_umount.assert_has_calls([mock.call('/dev/mapper/nbd15p1')])
+        mock_nbd_disconnect.reset_mock()
         mock_umount.reset_mock()
 
         # NOTE(thomasem): Not adding any commands in this case, because we're
@@ -274,10 +278,10 @@ class TestVirtDisk(test.NoDBTestCase):
     @mock.patch('nova.virt.disk.api._DiskImage._device_for_path',
                 return_value=None)
     @mock.patch('nova.privsep.fs.loopremove')
+    @mock.patch('nova.privsep.fs.nbd_disconnect')
     def test_lxc_teardown_container_with_namespace_cleaned(
-            self, mock_loopremove, mock_device_for_path, mock_exists):
+            self, mock_nbd_disconnect, mock_loopremove, mock_device_for_path,
-
+            mock_exists):
-        expected_commands = []
 
         disk_api.teardown_container('/mnt/loop/nopart', '/dev/loop0')
         mock_loopremove.assert_has_calls([mock.call('/dev/loop0')])
@@ -288,13 +292,9 @@ class TestVirtDisk(test.NoDBTestCase):
         mock_loopremove.reset_mock()
 
         disk_api.teardown_container('/mnt/nbd/nopart', '/dev/nbd15')
-        expected_commands += [
+        mock_nbd_disconnect.assert_has_calls([mock.call('/dev/nbd15')])
-                              ('qemu-nbd', '-d', '/dev/nbd15'),
+        mock_nbd_disconnect.reset_mock()
-                             ]
 
         disk_api.teardown_container('/mnt/nbd/part', '/dev/nbd15')
-        expected_commands += [
+        mock_nbd_disconnect.assert_has_calls([mock.call('/dev/nbd15')])
-                              ('qemu-nbd', '-d', '/dev/nbd15'),
+        mock_nbd_disconnect.reset_mock()
-                             ]
-
-        self.assertEqual(self.executes, expected_commands)

nova/virt/disk/api.py

@@ -37,6 +37,7 @@ from oslo_serialization import jsonutils
 import nova.conf
 from nova import exception
 from nova.i18n import _
+import nova.privsep.fs
 import nova.privsep.libvirt
 from nova import utils
 from nova.virt.disk.mount import api as mount
@@ -450,8 +451,7 @@ def teardown_container(container_dir, container_root_device=None):
                 nova.privsep.fs.loopremove(container_root_device)
             elif 'nbd' in container_root_device:
                 LOG.debug('Release nbd device %s', container_root_device)
-                utils.execute('qemu-nbd', '-d', container_root_device,
+                nova.privsep.fs.nbd_disconnect(container_root_device)
-                              run_as_root=True)
             else:
                 LOG.debug('No release necessary for block device %s',
                           container_root_device)

nova/virt/disk/mount/nbd.py

@@ -18,10 +18,13 @@ import random
 import re
 import time
 
+from oslo_concurrency import processutils
 from oslo_log import log as logging
+import six
 
 import nova.conf
 from nova.i18n import _
+import nova.privsep.fs
 from nova import utils
 from nova.virt.disk.mount import api
 
@@ -76,9 +79,11 @@ class NbdMount(api.Mount):
         # already in use.
         LOG.debug('Get nbd device %(dev)s for %(imgfile)s',
                   {'dev': device, 'imgfile': self.image.path})
-        _out, err = utils.trycmd('qemu-nbd', '-c', device,
+        try:
-                                 self.image.path,
+            _out, err = nova.privsep.fs.nbd_connect(device, self.image.path)
-                                 run_as_root=True)
+        except processutils.ProcessExecutionError as exc:
+            err = six.text_type(exc)
+
         if err:
             self.error = _('qemu-nbd error: %s') % err
             LOG.info('NBD mount error: %s', self.error)
@@ -97,8 +102,11 @@ class NbdMount(api.Mount):
             LOG.info('NBD mount error: %s', self.error)
 
             # Cleanup
-            _out, err = utils.trycmd('qemu-nbd', '-d', device,
+            try:
-                                     run_as_root=True)
+                _out, err = nova.privsep.fs.nbd_disconnect(device)
+            except processutils.ProcessExecutionError as exc:
+                err = six.text_type(exc)
+
             if err:
                 LOG.warning('Detaching from erroneous nbd device returned '
                             'error: %s', err)
@@ -116,7 +124,7 @@ class NbdMount(api.Mount):
         if not self.linked:
             return
         LOG.debug('Release nbd device %s', self.device)
-        utils.execute('qemu-nbd', '-d', self.device, run_as_root=True)
+        nova.privsep.fs.nbd_disconnect(self.device)
         self.linked = False
         self.device = None
 

releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml

@@ -11,5 +11,5 @@ upgrade:
   - |
     The following commands are no longer required to be listed in your rootwrap
     configuration: cat; chown; cryptsetup; dd; losetup; lvcreate; lvremove;
-    lvs; mkdir; mount; nova-idmapshift; ploop; prl_disk_tool; readlink; shred;
+    lvs; mkdir; mount; nova-idmapshift; ploop; prl_disk_tool; qemu-nbd;
-    tee; touch; umount; vgs; and xend.
+    readlink; shred; tee; touch; umount; vgs; and xend.