From fd4b2aa4cbff797ed7904c0c3b7ebb72e8d6da47 Mon Sep 17 00:00:00 2001 From: Michael Still Date: Wed, 27 Sep 2017 06:57:28 +1000 Subject: [PATCH] Move loopback setup and removal to privsep. Once more, again. Change-Id: I602582927c30f2929722474f68601ce47b4e98f6 blueprint: hurrah-for-privsep --- etc/nova/rootwrap.d/compute.filters | 4 -- nova/privsep/fs.py | 10 +++++ nova/tests/unit/virt/disk/mount/test_loop.py | 30 +++++--------- nova/tests/unit/virt/test_virt.py | 39 ++++++++++--------- nova/virt/disk/api.py | 3 +- nova/virt/disk/mount/loop.py | 9 ++--- ...queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml | 6 +-- 7 files changed, 48 insertions(+), 53 deletions(-) diff --git a/etc/nova/rootwrap.d/compute.filters b/etc/nova/rootwrap.d/compute.filters index 77026083c4d4..2ed7bcf37d39 100644 --- a/etc/nova/rootwrap.d/compute.filters +++ b/etc/nova/rootwrap.d/compute.filters @@ -14,10 +14,6 @@ tune2fs: CommandFilter, tune2fs, root # nova/virt/disk/mount/nbd.py: 'qemu-nbd', '-d', device qemu-nbd: CommandFilter, qemu-nbd, root -# nova/virt/disk/mount/loop.py: 'losetup', '--find', '--show', image -# nova/virt/disk/mount/loop.py: 'losetup', '--detach', device -losetup: CommandFilter, losetup, root - # nova/virt/disk/vfs/localfs.py: 'blkid', '-o', 'value', '-s', 'TYPE', device blkid: CommandFilter, blkid, root diff --git a/nova/privsep/fs.py b/nova/privsep/fs.py index 8137cc9d3cc8..8b1fb707ebc3 100644 --- a/nova/privsep/fs.py +++ b/nova/privsep/fs.py @@ -87,3 +87,13 @@ def clear(path, volume_size, shred=False): cmd.extend(['-n0', '-z']) cmd.extend(['-s%d' % volume_size, path]) processutils.execute(*cmd) + + +@nova.privsep.sys_admin_pctxt.entrypoint +def loopsetup(path): + return processutils.execute('losetup', '--find', '--show', path) + + +@nova.privsep.sys_admin_pctxt.entrypoint +def loopremove(device): + return processutils.execute('losetup', '--detach', device, attempts=3) diff --git a/nova/tests/unit/virt/disk/mount/test_loop.py b/nova/tests/unit/virt/disk/mount/test_loop.py index e317d8c39ca2..1cd207abf018 100644 --- a/nova/tests/unit/virt/disk/mount/test_loop.py +++ b/nova/tests/unit/virt/disk/mount/test_loop.py @@ -15,6 +15,7 @@ import fixtures +import mock from nova import test from nova.virt.disk.mount import loop @@ -25,14 +26,6 @@ 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.NoDBTestCase): def setUp(self): super(LoopTestCase, self).setUp() @@ -40,11 +33,11 @@ class LoopTestCase(test.NoDBTestCase): self.file = imgmodel.LocalFileImage("/some/file.qcow2", imgmodel.FORMAT_QCOW2) - def test_get_dev(self): + @mock.patch('nova.privsep.fs.loopsetup', return_value=('/dev/loop0', '')) + @mock.patch('nova.privsep.fs.loopremove') + def test_get_dev(self, mock_loopremove, mock_loopsetup): tempdir = self.useFixture(fixtures.TempDir()).path l = loop.LoopMount(self.file, tempdir) - self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', - _fake_trycmd_losetup_works)) self.useFixture(fixtures.MonkeyPatch('nova.utils.execute', _fake_noop)) @@ -60,11 +53,10 @@ class LoopTestCase(test.NoDBTestCase): self.assertEqual('', l.error) self.assertIsNone(l.device) - def test_inner_get_dev_fails(self): + @mock.patch('nova.privsep.fs.loopsetup', return_value=('', 'doh')) + def test_inner_get_dev_fails(self, mock_loopsetup): tempdir = self.useFixture(fixtures.TempDir()).path l = loop.LoopMount(self.file, tempdir) - self.useFixture(fixtures.MonkeyPatch('nova.utils.trycmd', - _fake_trycmd_losetup_fails)) # No error logged, device consumed self.assertFalse(l._inner_get_dev()) @@ -77,12 +69,11 @@ class LoopTestCase(test.NoDBTestCase): self.assertFalse(l.linked) self.assertIsNone(l.device) - def test_get_dev_timeout(self): + @mock.patch('nova.privsep.fs.loopsetup', return_value=('', 'doh')) + def test_get_dev_timeout(self, mock_loopsetup): tempdir = self.useFixture(fixtures.TempDir()).path l = loop.LoopMount(self.file, 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)) @@ -94,11 +85,10 @@ class LoopTestCase(test.NoDBTestCase): # Fail to get a device self.assertFalse(l.get_dev()) - def test_unget_dev(self): + @mock.patch('nova.privsep.fs.loopremove') + def test_unget_dev(self, mock_loopremove): tempdir = self.useFixture(fixtures.TempDir()).path l = loop.LoopMount(self.file, 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 diff --git a/nova/tests/unit/virt/test_virt.py b/nova/tests/unit/virt/test_virt.py index 7e729fc4ec77..f54e217a5348 100644 --- a/nova/tests/unit/virt/test_virt.py +++ b/nova/tests/unit/virt/test_virt.py @@ -182,6 +182,8 @@ class TestDiskImage(test.NoDBTestCase): class TestVirtDisk(test.NoDBTestCase): def setUp(self): super(TestVirtDisk, self).setUp() + + # TODO(mikal): this can probably be removed post privsep cleanup. self.executes = [] def fake_execute(*cmd, **kwargs): @@ -209,8 +211,11 @@ class TestVirtDisk(test.NoDBTestCase): self.assertEqual(disk_api.setup_container(image, container_dir), '/dev/fake') + @mock.patch('os.path.exists', return_value=True) + @mock.patch('nova.privsep.fs.loopremove') @mock.patch('nova.privsep.fs.umount') - def test_lxc_teardown_container(self, mock_umount): + def test_lxc_teardown_container( + self, mock_umount, mock_loopremove, mock_exist): def proc_mounts(mount_point): mount_points = { @@ -221,21 +226,22 @@ class TestVirtDisk(test.NoDBTestCase): } return mount_points[mount_point] - self.stub_out('os.path.exists', lambda _: True) self.stub_out('nova.virt.disk.api._DiskImage._device_for_path', proc_mounts) expected_commands = [] disk_api.teardown_container('/mnt/loop/nopart') - expected_commands += [('losetup', '--detach', '/dev/loop0')] + mock_loopremove.assert_has_calls([mock.call('/dev/loop0')]) + mock_loopremove.reset_mock() mock_umount.assert_has_calls([mock.call('/dev/loop0')]) mock_umount.reset_mock() disk_api.teardown_container('/mnt/loop/part') expected_commands += [ - ('kpartx', '-d', '/dev/loop0'), - ('losetup', '--detach', '/dev/loop0'), + ('kpartx', '-d', '/dev/loop0') ] + mock_loopremove.assert_has_calls([mock.call('/dev/loop0')]) + mock_loopremove.reset_mock() mock_umount.assert_has_calls([mock.call('/dev/mapper/loop0p1')]) mock_umount.reset_mock() @@ -264,25 +270,22 @@ class TestVirtDisk(test.NoDBTestCase): self.assertEqual(self.executes, expected_commands) - def test_lxc_teardown_container_with_namespace_cleaned(self): + @mock.patch('os.path.exists', return_value=True) + @mock.patch('nova.virt.disk.api._DiskImage._device_for_path', + return_value=None) + @mock.patch('nova.privsep.fs.loopremove') + def test_lxc_teardown_container_with_namespace_cleaned( + self, mock_loopremove, mock_device_for_path, mock_exists): - def proc_mounts(mount_point): - return None - - self.stub_out('os.path.exists', lambda _: True) - self.stub_out('nova.virt.disk.api._DiskImage._device_for_path', - proc_mounts) expected_commands = [] disk_api.teardown_container('/mnt/loop/nopart', '/dev/loop0') - expected_commands += [ - ('losetup', '--detach', '/dev/loop0'), - ] + mock_loopremove.assert_has_calls([mock.call('/dev/loop0')]) + mock_loopremove.reset_mock() disk_api.teardown_container('/mnt/loop/part', '/dev/loop0') - expected_commands += [ - ('losetup', '--detach', '/dev/loop0'), - ] + mock_loopremove.assert_has_calls([mock.call('/dev/loop0')]) + mock_loopremove.reset_mock() disk_api.teardown_container('/mnt/nbd/nopart', '/dev/nbd15') expected_commands += [ diff --git a/nova/virt/disk/api.py b/nova/virt/disk/api.py index 956ea46243e8..2d45d86fecf5 100644 --- a/nova/virt/disk/api.py +++ b/nova/virt/disk/api.py @@ -447,8 +447,7 @@ def teardown_container(container_dir, container_root_device=None): if container_root_device: if 'loop' in container_root_device: LOG.debug("Release loop device %s", container_root_device) - utils.execute('losetup', '--detach', container_root_device, - run_as_root=True, attempts=3) + 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, diff --git a/nova/virt/disk/mount/loop.py b/nova/virt/disk/mount/loop.py index 95aa79e5acf0..738c1a01e1fb 100644 --- a/nova/virt/disk/mount/loop.py +++ b/nova/virt/disk/mount/loop.py @@ -16,7 +16,7 @@ from oslo_log import log as logging from nova.i18n import _ -from nova import utils +import nova.privsep.fs from nova.virt.disk.mount import api LOG = logging.getLogger(__name__) @@ -27,9 +27,7 @@ class LoopMount(api.Mount): mode = 'loop' def _inner_get_dev(self): - out, err = utils.trycmd('losetup', '--find', '--show', - self.image.path, - run_as_root=True) + out, err = nova.privsep.fs.loopsetup(self.image.path) if err: self.error = _('Could not attach image to loopback: %s') % err LOG.info('Loop mount error: %s', self.error) @@ -57,7 +55,6 @@ class LoopMount(api.Mount): # 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, - attempts=3) + nova.privsep.fs.loopremove(self.device) self.linked = False self.device = None diff --git a/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml b/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml index d0f527702dc9..d72b220ca63b 100644 --- a/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml +++ b/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml @@ -10,6 +10,6 @@ upgrade: internal functionality using privsep. - | The following commands are no longer required to be listed in your rootwrap - configuration: cat; chown; cryptsetup; dd; lvcreate; lvremove; lvs; mkdir; - mount; nova-idmapshift; ploop; prl_disk_tool; readlink; shred; tee; touch; - umount; vgs; and xend. + configuration: cat; chown; cryptsetup; dd; losetup; lvcreate; lvremove; + lvs; mkdir; mount; nova-idmapshift; ploop; prl_disk_tool; readlink; shred; + tee; touch; umount; vgs; and xend.