From 042aa9ab5a27e251c8fb2f1855695cf5e791ecf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A9ri=20Le=20Bouder?= Date: Fri, 26 Feb 2016 11:27:54 -0500 Subject: [PATCH] use wipefs to erase FS meta information destroy_disk_metadata should destroy a bit more data than expected if we want to be sure grub won't conflict with remaining bits of filesystem. grub probes the hard drive to find remaining partition or filesystem. If it find something, it will refuse to install itself on the disk. By using wipefs instead of dd, we not only erase the MBR and partition table but also all trace of filesystem, raid or partition-table signatures (magic strings). This without extra Python code. wipefs is part of util-linux which is available on all the Linux distribution. References: https://review.openstack.org/#/c/284347/ https://bugzilla.redhat.com/show_bug.cgi?id=1310883 Partial-Bug: 1550604 Change-Id: I39637e22c344703ad48fc271f6f866aa018bbdd1 --- etc/rootwrap.d/ironic-lib.filters | 5 +++ ironic_lib/disk_utils.py | 47 ++++++----------------------- ironic_lib/tests/test_disk_utils.py | 32 +++----------------- 3 files changed, 19 insertions(+), 65 deletions(-) diff --git a/etc/rootwrap.d/ironic-lib.filters b/etc/rootwrap.d/ironic-lib.filters index acce7a7b..0de94962 100644 --- a/etc/rootwrap.d/ironic-lib.filters +++ b/etc/rootwrap.d/ironic-lib.filters @@ -2,12 +2,17 @@ # The following commands should be used in filters for disk manipulation. # This file should be owned by (and only-writeable by) the root user. +# NOTE: +# if you update this file, you will also need to adjust the +# ironic-lib.filters from the ironic module. + [Filters] # ironic_lib/disk_utils.py blkid: CommandFilter, blkid, root blockdev: CommandFilter, blockdev, root hexdump: CommandFilter, hexdump, root qemu-img: CommandFilter, qemu-img, root +wipefs: CommandFilter, wipefs, root # ironic_lib/utils.py mkswap: CommandFilter, mkswap, root diff --git a/ironic_lib/disk_utils.py b/ironic_lib/disk_utils.py index 367dbdb2..031a5945 100644 --- a/ironic_lib/disk_utils.py +++ b/ironic_lib/disk_utils.py @@ -284,49 +284,20 @@ def get_dev_block_size(dev): def destroy_disk_metadata(dev, node_uuid): """Destroy metadata structures on node's disk. - Ensure that node's disk appears to be blank without zeroing the entire - drive. To do this we will zero the first 18KiB to clear MBR / GPT data - and the last 18KiB to clear GPT and other metadata like LVM, veritas, - MDADM, DMRAID, etc. + Ensure that node's disk magic strings are wiped without zeroing the + entire drive. To do this we use the wipefs tool from util-linux. + + :param dev: Path for the device to work on. + :param node_uuid: Node's uuid. Used for logging. """ # NOTE(NobodyCam): This is needed to work around bug: # https://bugs.launchpad.net/ironic/+bug/1317647 LOG.debug("Start destroy disk metadata for node %(node)s.", {'node': node_uuid}) - try: - utils.dd('/dev/zero', dev, 'bs=512', 'count=36') - except processutils.ProcessExecutionError as err: - with excutils.save_and_reraise_exception(): - LOG.error(_LE("Failed to erase beginning of disk for node " - "%(node)s. Command: %(command)s. Error: %(error)s."), - {'node': node_uuid, - 'command': err.cmd, - 'error': err.stderr}) - - # now wipe the end of the disk. - # get end of disk seek value - try: - block_sz = get_dev_block_size(dev) - except processutils.ProcessExecutionError as err: - with excutils.save_and_reraise_exception(): - LOG.error(_LE("Failed to get disk block count for node %(node)s. " - "Command: %(command)s. Error: %(error)s."), - {'node': node_uuid, - 'command': err.cmd, - 'error': err.stderr}) - else: - seek_value = block_sz - 36 - try: - utils.dd('/dev/zero', dev, 'bs=512', 'count=36', - 'seek=%d' % seek_value) - except processutils.ProcessExecutionError as err: - with excutils.save_and_reraise_exception(): - LOG.error(_LE("Failed to erase the end of the disk on node " - "%(node)s. Command: %(command)s. " - "Error: %(error)s."), - {'node': node_uuid, - 'command': err.cmd, - 'error': err.stderr}) + utils.execute('wipefs', '--all', dev, + run_as_root=True, + check_exit_code=[0], + use_standard_locale=True) LOG.info(_LI("Disk metadata on %(dev)s successfully destroyed for node " "%(node)s"), {'dev': dev, 'node': node_uuid}) diff --git a/ironic_lib/tests/test_disk_utils.py b/ironic_lib/tests/test_disk_utils.py index 9e102f45..9df097c3 100644 --- a/ironic_lib/tests/test_disk_utils.py +++ b/ironic_lib/tests/test_disk_utils.py @@ -276,7 +276,6 @@ class MakePartitionsTestCase(test_base.BaseTestCase): self.assertEqual(expected_result, result) -@mock.patch.object(disk_utils, 'get_dev_block_size') @mock.patch.object(utils, 'execute') class DestroyMetaDataTestCase(test_base.BaseTestCase): @@ -285,39 +284,19 @@ class DestroyMetaDataTestCase(test_base.BaseTestCase): self.dev = 'fake-dev' self.node_uuid = "12345678-1234-1234-1234-1234567890abcxyz" - def test_destroy_disk_metadata(self, mock_exec, mock_gz): - mock_gz.return_value = 64 - expected_calls = [mock.call('dd', 'if=/dev/zero', 'of=fake-dev', - 'bs=512', 'count=36', run_as_root=True, - check_exit_code=[0], - use_standard_locale=True), - mock.call('dd', 'if=/dev/zero', 'of=fake-dev', - 'bs=512', 'count=36', 'seek=28', + def test_destroy_disk_metadata(self, mock_exec): + expected_calls = [mock.call('wipefs', '--all', 'fake-dev', run_as_root=True, check_exit_code=[0], use_standard_locale=True)] disk_utils.destroy_disk_metadata(self.dev, self.node_uuid) mock_exec.assert_has_calls(expected_calls) - self.assertTrue(mock_gz.called) - def test_destroy_disk_metadata_get_dev_size_fail(self, mock_exec, mock_gz): - mock_gz.side_effect = processutils.ProcessExecutionError - - expected_call = [mock.call('dd', 'if=/dev/zero', 'of=fake-dev', - 'bs=512', 'count=36', run_as_root=True, - check_exit_code=[0], - use_standard_locale=True)] - self.assertRaises(processutils.ProcessExecutionError, - disk_utils.destroy_disk_metadata, - self.dev, - self.node_uuid) - mock_exec.assert_has_calls(expected_call) - - def test_destroy_disk_metadata_dd_fail(self, mock_exec, mock_gz): + def test_destroy_disk_metadata_wipefs_fail(self, mock_exec): mock_exec.side_effect = processutils.ProcessExecutionError - expected_call = [mock.call('dd', 'if=/dev/zero', 'of=fake-dev', - 'bs=512', 'count=36', run_as_root=True, + expected_call = [mock.call('wipefs', '--all', 'fake-dev', + run_as_root=True, check_exit_code=[0], use_standard_locale=True)] self.assertRaises(processutils.ProcessExecutionError, @@ -325,7 +304,6 @@ class DestroyMetaDataTestCase(test_base.BaseTestCase): self.dev, self.node_uuid) mock_exec.assert_has_calls(expected_call) - self.assertFalse(mock_gz.called) @mock.patch.object(utils, 'execute')