From 7b43fb4ebda7eb1dac74d1aaac1d347a3b611a96 Mon Sep 17 00:00:00 2001 From: Michael Still Date: Tue, 13 Mar 2018 06:54:41 +1100 Subject: [PATCH] Move xenapi disk resizing to privsep. The same pattern as the rest of the changes. This means that privsep now needs to let you pass flags to e2fsck, which I don't love and will remove in a later patch. Change-Id: I6c695c04ae586fec6adc354257638116277dda88 blueprint: hurrah-for-privsep --- nova/privsep/fs.py | 32 ++++++++------- nova/tests/unit/virt/disk/test_api.py | 5 ++- nova/tests/unit/virt/xenapi/test_vm_utils.py | 40 +++++++++---------- nova/virt/disk/api.py | 19 +++++++-- nova/virt/xenapi/vm_utils.py | 14 ++----- ...-rocky-rootwrap-adds-644c43fbd86f9f8a.yaml | 2 +- 6 files changed, 61 insertions(+), 51 deletions(-) diff --git a/nova/privsep/fs.py b/nova/privsep/fs.py index 2ee52429c17e..638fd50ad48f 100644 --- a/nova/privsep/fs.py +++ b/nova/privsep/fs.py @@ -138,24 +138,28 @@ def get_filesystem_type(device): @nova.privsep.sys_admin_pctxt.entrypoint -def resize2fs(image, check_exit_code): - unprivileged_resize2fs(image, check_exit_code) +def e2fsck(image, flags='-fp'): + unprivileged_e2fsck(image, flags=flags) # NOTE(mikal): this method is deliberately not wrapped in a privsep entrypoint -def unprivileged_resize2fs(image, check_exit_code): - try: - processutils.execute('e2fsck', - '-fp', - image, - check_exit_code=[0, 1, 2]) - except processutils.ProcessExecutionError as exc: - LOG.debug("Checking the file system with e2fsck has failed, " - "the resize will be aborted. (%s)", exc) +def unprivileged_e2fsck(image, flags='-fp'): + processutils.execute('e2fsck', flags, image, check_exit_code=[0, 1, 2]) + + +@nova.privsep.sys_admin_pctxt.entrypoint +def resize2fs(image, check_exit_code, size=None): + unprivileged_resize2fs(image, check_exit_code=check_exit_code, size=size) + + +# NOTE(mikal): this method is deliberately not wrapped in a privsep entrypoint +def unprivileged_resize2fs(image, check_exit_code, size=None): + if size: + cmd = ['resize2fs', image, size] else: - processutils.execute('resize2fs', - image, - check_exit_code=check_exit_code) + cmd = ['resize2fs', image] + + processutils.execute(*cmd, check_exit_code=check_exit_code) @nova.privsep.sys_admin_pctxt.entrypoint diff --git a/nova/tests/unit/virt/disk/test_api.py b/nova/tests/unit/virt/disk/test_api.py index bd5e506a6cb6..fd0157181471 100644 --- a/nova/tests/unit/virt/disk/test_api.py +++ b/nova/tests/unit/virt/disk/test_api.py @@ -77,7 +77,9 @@ class APITestCase(test.NoDBTestCase): @mock.patch('oslo_concurrency.processutils.execute') @mock.patch('nova.privsep.fs.resize2fs') - def test_resize2fs_success_as_root(self, mock_resize, mock_exec): + @mock.patch('nova.privsep.fs.e2fsck') + def test_resize2fs_success_as_root(self, mock_fsck, mock_resize, + mock_exec): imgfile = tempfile.NamedTemporaryFile() self.addCleanup(imgfile.close) @@ -85,6 +87,7 @@ class APITestCase(test.NoDBTestCase): mock_exec.assert_not_called() mock_resize.assert_called() + mock_fsck.assert_called() @mock.patch('oslo_concurrency.processutils.execute', autospec=True, side_effect=processutils.ProcessExecutionError("fs error")) diff --git a/nova/tests/unit/virt/xenapi/test_vm_utils.py b/nova/tests/unit/virt/xenapi/test_vm_utils.py index f92f95d4b002..fb2b82cd4f38 100644 --- a/nova/tests/unit/virt/xenapi/test_vm_utils.py +++ b/nova/tests/unit/virt/xenapi/test_vm_utils.py @@ -39,7 +39,6 @@ from nova.tests.unit import fake_instance from nova.tests.unit.objects import test_flavor from nova.tests.unit.virt.xenapi import stubs from nova.tests import uuidsentinel as uuids -from nova import utils from nova.virt import hardware from nova.virt.xenapi import driver as xenapi_conn from nova.virt.xenapi import fake @@ -341,27 +340,22 @@ class ResizeHelpersTestCase(VMUtilsTestBase): super(ResizeHelpersTestCase, self).setUp() self.context = context.RequestContext('user', 'project') - @mock.patch.object(utils, 'execute', return_value=("size is: 42", "")) - def test_repair_filesystem(self, mock_execute): - vm_utils._repair_filesystem("fakepath") - mock_execute.assert_called_once_with( - 'e2fsck', '-f', "-y", "fakepath", run_as_root=True, - check_exit_code=[0, 1, 2]) - @mock.patch('nova.privsep.fs.ext_journal_disable') @mock.patch('nova.privsep.fs.ext_journal_enable') @mock.patch('nova.privsep.fs.resize_partition') - @mock.patch.object(vm_utils, '_repair_filesystem') - @mock.patch.object(utils, 'execute') + @mock.patch('nova.privsep.fs.resize2fs') + @mock.patch('nova.privsep.fs.e2fsck') def test_resize_part_and_fs_down_succeeds( - self, mock_execute, mock_repair, mock_resize, + self, mock_fsck, mock_resize2fs, mock_resize, mock_disable_journal, mock_enable_journal): dev_path = '/dev/fake' partition_path = '%s1' % dev_path vm_utils._resize_part_and_fs('fake', 0, 20, 10, 'boot') - mock_execute.assert_has_calls([ - mock.call('resize2fs', partition_path, '10s', run_as_root=True)]) + mock_fsck.assert_has_calls([ + mock.call(partition_path, flags='-fy')]) + mock_resize2fs.assert_has_calls([ + mock.call(partition_path, None, size='10s')]) mock_resize.assert_has_calls([ mock.call(dev_path, 0, 9, True)]) mock_disable_journal.assert_has_calls([ @@ -391,29 +385,33 @@ class ResizeHelpersTestCase(VMUtilsTestBase): mock_debug.assert_not_called() @mock.patch('nova.privsep.fs.ext_journal_disable') - @mock.patch.object(vm_utils, '_repair_filesystem') - @mock.patch.object(utils, 'execute', + @mock.patch('nova.privsep.fs.resize2fs', side_effect=processutils.ProcessExecutionError) + @mock.patch('nova.privsep.fs.e2fsck') def test_resize_part_and_fs_down_fails_disk_too_big( - self, mock_execute, mock_repair, mock_disable_journal): + self, mock_fsck, mock_resize2fs, mock_disable_journal): self.assertRaises(exception.ResizeError, vm_utils._resize_part_and_fs, "fake", 0, 20, 10, "boot") + mock_fsck.assert_has_calls([ + mock.call('/dev/fake1', flags='-fy')]) @mock.patch('nova.privsep.fs.ext_journal_disable') @mock.patch('nova.privsep.fs.ext_journal_enable') @mock.patch('nova.privsep.fs.resize_partition') - @mock.patch.object(vm_utils, '_repair_filesystem') - @mock.patch.object(utils, 'execute') + @mock.patch('nova.privsep.fs.resize2fs') + @mock.patch('nova.privsep.fs.e2fsck') def test_resize_part_and_fs_up_succeeds( - self, mock_execute, mock_repair, mock_resize, + self, mock_fsck, mock_resize2fs, mock_resize, mock_disable_journal, mock_enable_journal): dev_path = '/dev/fake' partition_path = '%s1' % dev_path vm_utils._resize_part_and_fs('fake', 0, 20, 30, '') - mock_execute.assert_has_calls([ - mock.call('resize2fs', partition_path, run_as_root=True)]) + mock_fsck.assert_has_calls([ + mock.call(partition_path, flags='-fy')]) + mock_resize2fs.assert_has_calls([ + mock.call(partition_path, None)]) mock_resize.assert_has_calls([ mock.call(dev_path, 0, 29, False)]) mock_disable_journal.assert_has_calls([ diff --git a/nova/virt/disk/api.py b/nova/virt/disk/api.py index c13fd39a84bc..694e95275dd5 100644 --- a/nova/virt/disk/api.py +++ b/nova/virt/disk/api.py @@ -70,10 +70,23 @@ def mkfs(os_type, fs_label, target, run_as_root=True, specified_fs=None): def resize2fs(image, check_exit_code=False, run_as_root=False): - if run_as_root: - nova.privsep.fs.resize2fs(image, check_exit_code) + # NOTE(mikal): note that the check_exit_code kwarg here only refers to + # resize2fs, not the precursor e2fsck. Yes, I agree it's confusing. + try: + if run_as_root: + nova.privsep.fs.e2fsck(image) + else: + nova.privsep.fs.unprivileged_e2fsck(image) + + except processutils.ProcessExecutionError as exc: + LOG.debug("Checking the file system with e2fsck has failed, " + "the resize will be aborted. (%s)", exc) + else: - nova.privsep.fs.unprivileged_resize2fs(image, check_exit_code) + if run_as_root: + nova.privsep.fs.resize2fs(image, check_exit_code) + else: + nova.privsep.fs.unprivileged_resize2fs(image, check_exit_code) def get_disk_size(path): diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py index 19787bc12ee0..e01c7b13f6ef 100644 --- a/nova/virt/xenapi/vm_utils.py +++ b/nova/virt/xenapi/vm_utils.py @@ -2283,13 +2283,6 @@ def _write_partition(session, virtual_size, dev): LOG.debug('Writing partition table %s done.', dev_path) -def _repair_filesystem(partition_path): - # Exit Code 1 = File system errors corrected - # 2 = File system errors corrected, system needs a reboot - utils.execute('e2fsck', '-f', '-y', partition_path, run_as_root=True, - check_exit_code=[0, 1, 2]) - - def _resize_part_and_fs(dev, start, old_sectors, new_sectors, flags): """Resize partition and fileystem. @@ -2303,7 +2296,7 @@ def _resize_part_and_fs(dev, start, old_sectors, new_sectors, flags): partition_path = utils.make_dev_path(dev, partition=1) # Replay journal if FS wasn't cleanly unmounted - _repair_filesystem(partition_path) + nova.privsep.fs.e2fsck(partition_path, flags='-fy') # Remove ext3 journal (making it ext2) nova.privsep.fs.ext_journal_disable(partition_path) @@ -2311,8 +2304,7 @@ def _resize_part_and_fs(dev, start, old_sectors, new_sectors, flags): if new_sectors < old_sectors: # Resizing down, resize filesystem before partition resize try: - utils.execute('resize2fs', partition_path, '%ds' % size, - run_as_root=True) + nova.privsep.fs.resize2fs(partition_path, None, size='%ds' % size) except processutils.ProcessExecutionError as exc: LOG.error(six.text_type(exc)) reason = _("Shrinking the filesystem down with resize2fs " @@ -2325,7 +2317,7 @@ def _resize_part_and_fs(dev, start, old_sectors, new_sectors, flags): if new_sectors > old_sectors: # Resizing up, resize filesystem after partition resize - utils.execute('resize2fs', partition_path, run_as_root=True) + nova.privsep.fs.resize2fs(partition_path, None) # Add back journal nova.privsep.fs.ext_journal_enable(partition_path) diff --git a/releasenotes/notes/privsep-rocky-rootwrap-adds-644c43fbd86f9f8a.yaml b/releasenotes/notes/privsep-rocky-rootwrap-adds-644c43fbd86f9f8a.yaml index ce265c1f0f24..cdd0b744da7b 100644 --- a/releasenotes/notes/privsep-rocky-rootwrap-adds-644c43fbd86f9f8a.yaml +++ b/releasenotes/notes/privsep-rocky-rootwrap-adds-644c43fbd86f9f8a.yaml @@ -2,4 +2,4 @@ upgrade: - | The following commands are no longer required to be listed in your rootwrap - configuration: mkfs; tune2fs; xenstore_read. + configuration: e2fsck; mkfs; tune2fs; xenstore_read.