openstack/ironic-lib
Code Issues Proposed changes

Erase expected GPT locations in metadata wipe

Browse Source 
While `sgdisk -Z` is useful to wipe disk and partition
metadata, it will prevent execution and not do anything
if a CRC error is detected with the data already on disk.

We had previously run into this with IPA, however we never
addressed this with-in ironic-lib[0].

[0]: https://storyboard.openstack.org/#!/story/1737556
Story: 1737556
Task: 36305

Change-Id: I8fb390cb1daa481583ebcba6a010e9be3255b373
This commit is contained in:
Julia Kreger 2019-08-19 13:23:05 -04:00
parent cd51319528
commit 28a2ed1a90
3 changed files with 69 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
ironic_lib/disk_utils.py
View File

@ -451,7 +451,20 @@ def destroy_disk_metadata(dev, node_uuid):
utils.execute('wipefs', '--all', dev,
run_as_root=True,
use_standard_locale=True)
# NOTE(TheJulia): sgdisk attempts to load and make sense of the
# partition tables in advance of wiping the partition data.
# This means when a CRC error is found, sgdisk fails before
# erasing partition data.
# This is the same bug as
# https://bugs.launchpad.net/ironic-python-agent/+bug/1737556
dd_device = 'of=%s' % dev
gpt_backup = get_dev_block_size(dev) - 33
dd_seek = 'seek=%i' % gpt_backup
utils.execute('dd', 'bs=512', 'if=/dev/zero', dd_device, 'count=33',
run_as_root=True, use_standard_locale=True)
utils.execute('dd', 'bs=512', 'if=/dev/zero', dd_device, 'count=33',
dd_seek, run_as_root=True, use_standard_locale=True)
# Go ahead and let sgdisk run as well.
utils.execute('sgdisk', '-Z', dev, run_as_root=True,
use_standard_locale=True)

56
ironic_lib/tests/test_disk_utils.py
View File

@ -683,7 +683,7 @@ class MakePartitionsTestCase(base.IronicLibTestCase):
self.assertEqual(expected_result, result)
@mock.patch.object(utils, 'execute', autospec=True, return_value=('', ''))
@mock.patch.object(utils, 'execute', autospec=True)
class DestroyMetaDataTestCase(base.IronicLibTestCase):
def setUp(self):
@ -692,9 +692,31 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
self.node_uuid = "12345678-1234-1234-1234-1234567890abcxyz"
def test_destroy_disk_metadata(self, mock_exec):
# Note(TheJulia): This list will get-reused, but only the second
# execution returning a string is needed for the test as otherwise
# command output is not used.
mock_exec.side_effect = iter([
(None, None),
('1024\n', None),
(None, None),
(None, None),
(None, None),
(None, None)])
expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev',
run_as_root=True,
use_standard_locale=True),
mock.call('blockdev', '--getsz', 'fake-dev',
check_exit_code=[0],
run_as_root=True),
mock.call('dd', 'bs=512', 'if=/dev/zero',
'of=fake-dev', 'count=33',
run_as_root=True,
use_standard_locale=True),
mock.call('dd', 'bs=512', 'if=/dev/zero',
'of=fake-dev', 'count=33', 'seek=991',
run_as_root=True,
use_standard_locale=True),
mock.call('sgdisk', '-Z', 'fake-dev',
run_as_root=True,
use_standard_locale=True),
@ -720,11 +742,26 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev',
run_as_root=True,
use_standard_locale=True),
mock.call('blockdev', '--getsz', 'fake-dev',
check_exit_code=[0],
run_as_root=True),
mock.call('dd', 'bs=512', 'if=/dev/zero',
'of=fake-dev', 'count=33',
run_as_root=True,
use_standard_locale=True),
mock.call('dd', 'bs=512', 'if=/dev/zero',
'of=fake-dev', 'count=33', 'seek=991',
run_as_root=True,
use_standard_locale=True),
mock.call('sgdisk', '-Z', 'fake-dev',
run_as_root=True,
use_standard_locale=True)]
mock_exec.side_effect = [(None, None),
processutils.ProcessExecutionError()]
mock_exec.side_effect = iter([
(None, None),
('1024\n', None),
(None, None),
(None, None),
processutils.ProcessExecutionError()])
self.assertRaises(processutils.ProcessExecutionError,
disk_utils.destroy_disk_metadata,
self.dev,
@ -732,11 +769,14 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
mock_exec.assert_has_calls(expected_calls)
def test_destroy_disk_metadata_wipefs_not_support_force(self, mock_exec):
mock_exec.side_effect = iter(
[processutils.ProcessExecutionError(description='--force'),
(None, None),
(None, None),
('', '')])
mock_exec.side_effect = iter([
processutils.ProcessExecutionError(description='--force'),
(None, None),
('1024\n', None),
(None, None),
(None, None),
(None, None),
(None, None)])
expected_call = [mock.call('wipefs', '--force', '--all', 'fake-dev',
run_as_root=True,

7
releasenotes/notes/wipe-gpt-on-metadata-wipe-ac0a93b16e00893f.yaml Normal file
View File

@ -0,0 +1,7 @@
---
fixes:
- |
Fixes an issue where CRC errors in the GPT partition information would
cause cleaning to fail. A similar issue was previously encountered with
ironic-python-agent. See `Story 1737556 <https://storyboard.openstack.org/#!/story/1737556>`_
for details.