openstack/ironic-python-agent
Code Issues Proposed changes

destroy_disk_metadata: support 4096 sector size

Browse Source 
A sector size of 512 was assumed and hardcoded, causing dd to fail when
it tried to write in chunks smaller than the sector size for disks with
4096 bytes sectors. The size of GPT in sectors also depends on sector size.

Change-Id: Ide5318eb503d728cff3221c26bebbd1c214f6995
This commit is contained in:
Tudor Domnescu 2024-04-16 14:01:02 +02:00 committed by Dmitry Tantsur
parent cdd0a83448
commit ceec5a7367
3 changed files with 88 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
ironic_python_agent/disk_utils.py
View File

@ -49,8 +49,6 @@ _PARTED_TABLE_TYPE_RE = re.compile(r'^.*partition\s+table\s*:\s*(gpt|msdos)',
CONFIGDRIVE_LABEL = "config-2" CONFIGDRIVE_LABEL = "config-2"
MAX_CONFIG_DRIVE_SIZE_MB = 64 MAX_CONFIG_DRIVE_SIZE_MB = 64
GPT_SIZE_SECTORS = 33
# Maximum disk size supported by MBR is 2TB (2 * 1024 * 1024 MB) # Maximum disk size supported by MBR is 2TB (2 * 1024 * 1024 MB)
MAX_DISK_SIZE_MB_SUPPORTED_BY_MBR = 2097152 MAX_DISK_SIZE_MB_SUPPORTED_BY_MBR = 2097152
@ -428,10 +426,16 @@ def get_image_mb(image_path, virtual_size=True):
return image_mb return image_mb
def get_dev_block_size(dev): def get_dev_byte_size(dev):
"""Get the device size in 512 byte sectors.""" """Get the device size in bytes."""
block_sz, cmderr = utils.execute('blockdev', '--getsz', dev) byte_sz, cmderr = utils.execute('blockdev', '--getsize64', dev)
return int(block_sz) return int(byte_sz)
def get_dev_sector_size(dev):
"""Get the device logical sector size in bytes."""
sect_sz, cmderr = utils.execute('blockdev', '--getss', dev)
return int(sect_sz)
def destroy_disk_metadata(dev, node_uuid): def destroy_disk_metadata(dev, node_uuid):
@ -465,21 +469,32 @@ def destroy_disk_metadata(dev, node_uuid):
# This is the same bug as # This is the same bug as
# https://bugs.launchpad.net/ironic-python-agent/+bug/1737556 # https://bugs.launchpad.net/ironic-python-agent/+bug/1737556
sector_size = get_dev_sector_size(dev)
# https://uefi.org/specs/UEFI/2.10/05_GUID_Partition_Table_Format.html If
# the block size is 512, the First Usable LBA must be greater than or equal
# to 34 [...] if the logical block size is 4096, the First Usable LBA must
# be greater than or equal to 6
if sector_size == 512:
gpt_sectors = 33
elif sector_size == 4096:
gpt_sectors = 5
# Overwrite the Primary GPT, catch very small partitions (like EBRs) # Overwrite the Primary GPT, catch very small partitions (like EBRs)
dd_bs = 'bs=%s' % sector_size
dd_device = 'of=%s' % dev dd_device = 'of=%s' % dev
dd_count = 'count=%s' % GPT_SIZE_SECTORS dd_count = 'count=%s' % gpt_sectors
dev_size = get_dev_block_size(dev) dev_size = get_dev_byte_size(dev)
if dev_size < GPT_SIZE_SECTORS: if dev_size < gpt_sectors * sector_size:
dd_count = 'count=%s' % dev_size dd_count = 'count=%s' % int(dev_size / sector_size)
utils.execute('dd', 'bs=512', 'if=/dev/zero', dd_device, dd_count, utils.execute('dd', dd_bs, 'if=/dev/zero', dd_device, dd_count,
'oflag=direct', use_standard_locale=True) 'oflag=direct', use_standard_locale=True)
# Overwrite the Secondary GPT, do this only if there could be one # Overwrite the Secondary GPT, do this only if there could be one
if dev_size > GPT_SIZE_SECTORS: if dev_size > gpt_sectors * sector_size:
gpt_backup = dev_size - GPT_SIZE_SECTORS gpt_backup = int(dev_size / sector_size - gpt_sectors)
dd_seek = 'seek=%i' % gpt_backup dd_seek = 'seek=%i' % gpt_backup
dd_count = 'count=%s' % GPT_SIZE_SECTORS dd_count = 'count=%s' % gpt_sectors
utils.execute('dd', 'bs=512', 'if=/dev/zero', dd_device, dd_count, utils.execute('dd', dd_bs, 'if=/dev/zero', dd_device, dd_count,
'oflag=direct', dd_seek, use_standard_locale=True) 'oflag=direct', dd_seek, use_standard_locale=True)
# Go ahead and let sgdisk run as well. # Go ahead and let sgdisk run as well.

71
ironic_python_agent/tests/unit/test_disk_utils.py
View File

@ -312,13 +312,11 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
self.dev = 'fake-dev' self.dev = 'fake-dev'
self.node_uuid = "12345678-1234-1234-1234-1234567890abcxyz" self.node_uuid = "12345678-1234-1234-1234-1234567890abcxyz"
def test_destroy_disk_metadata(self, mock_exec): def test_destroy_disk_metadata_4096(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([ mock_exec.side_effect = iter([
(None, None), (None, None),
('1024\n', None), ('4096\n', None),
('524288\n', None),
(None, None), (None, None),
(None, None), (None, None),
(None, None), (None, None),
@ -326,7 +324,37 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev', expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev',
use_standard_locale=True), use_standard_locale=True),
mock.call('blockdev', '--getsz', 'fake-dev'), mock.call('blockdev', '--getss', 'fake-dev'),
mock.call('blockdev', '--getsize64', 'fake-dev'),
mock.call('dd', 'bs=4096', 'if=/dev/zero',
'of=fake-dev', 'count=5', 'oflag=direct',
use_standard_locale=True),
mock.call('dd', 'bs=4096', 'if=/dev/zero',
'of=fake-dev', 'count=5', 'oflag=direct',
'seek=123', use_standard_locale=True),
mock.call('sgdisk', '-Z', 'fake-dev',
use_standard_locale=True),
mock.call('fuser', self.dev, check_exit_code=[0, 1])]
disk_utils.destroy_disk_metadata(self.dev, self.node_uuid)
mock_exec.assert_has_calls(expected_calls)
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),
('512\n', None),
('524288\n', None),
(None, None),
(None, None),
(None, None),
(None, None)])
expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev',
use_standard_locale=True),
mock.call('blockdev', '--getss', 'fake-dev'),
mock.call('blockdev', '--getsize64', 'fake-dev'),
mock.call('dd', 'bs=512', 'if=/dev/zero', mock.call('dd', 'bs=512', 'if=/dev/zero',
'of=fake-dev', 'count=33', 'oflag=direct', 'of=fake-dev', 'count=33', 'oflag=direct',
use_standard_locale=True), use_standard_locale=True),
@ -353,7 +381,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
def test_destroy_disk_metadata_sgdisk_fail(self, mock_exec): def test_destroy_disk_metadata_sgdisk_fail(self, mock_exec):
expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev', expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev',
use_standard_locale=True), use_standard_locale=True),
mock.call('blockdev', '--getsz', 'fake-dev'), mock.call('blockdev', '--getss', 'fake-dev'),
mock.call('blockdev', '--getsize64', 'fake-dev'),
mock.call('dd', 'bs=512', 'if=/dev/zero', mock.call('dd', 'bs=512', 'if=/dev/zero',
'of=fake-dev', 'count=33', 'oflag=direct', 'of=fake-dev', 'count=33', 'oflag=direct',
use_standard_locale=True), use_standard_locale=True),
@ -364,7 +393,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
use_standard_locale=True)] use_standard_locale=True)]
mock_exec.side_effect = iter([ mock_exec.side_effect = iter([
(None, None), (None, None),
('1024\n', None), ('512\n', None),
('524288\n', None),
(None, None), (None, None),
(None, None), (None, None),
processutils.ProcessExecutionError()]) processutils.ProcessExecutionError()])
@ -378,7 +408,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
mock_exec.side_effect = iter([ mock_exec.side_effect = iter([
processutils.ProcessExecutionError(description='--force'), processutils.ProcessExecutionError(description='--force'),
(None, None), (None, None),
('1024\n', None), ('512\n', None),
('524288\n', None),
(None, None), (None, None),
(None, None), (None, None),
(None, None), (None, None),
@ -394,7 +425,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
def test_destroy_disk_metadata_ebr(self, mock_exec): def test_destroy_disk_metadata_ebr(self, mock_exec):
expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev', expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev',
use_standard_locale=True), use_standard_locale=True),
mock.call('blockdev', '--getsz', 'fake-dev'), mock.call('blockdev', '--getss', 'fake-dev'),
mock.call('blockdev', '--getsize64', 'fake-dev'),
mock.call('dd', 'bs=512', 'if=/dev/zero', mock.call('dd', 'bs=512', 'if=/dev/zero',
'of=fake-dev', 'count=2', 'oflag=direct', 'of=fake-dev', 'count=2', 'oflag=direct',
use_standard_locale=True), use_standard_locale=True),
@ -402,7 +434,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
use_standard_locale=True)] use_standard_locale=True)]
mock_exec.side_effect = iter([ mock_exec.side_effect = iter([
(None, None), (None, None),
('2\n', None), # an EBR is 2 sectors ('512\n', None),
('1024\n', None), # an EBR is 2 sectors
(None, None), (None, None),
(None, None), (None, None),
(None, None), (None, None),
@ -413,7 +446,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
def test_destroy_disk_metadata_tiny_partition(self, mock_exec): def test_destroy_disk_metadata_tiny_partition(self, mock_exec):
expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev', expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev',
use_standard_locale=True), use_standard_locale=True),
mock.call('blockdev', '--getsz', 'fake-dev'), mock.call('blockdev', '--getss', 'fake-dev'),
mock.call('blockdev', '--getsize64', 'fake-dev'),
mock.call('dd', 'bs=512', 'if=/dev/zero', mock.call('dd', 'bs=512', 'if=/dev/zero',
'of=fake-dev', 'count=33', 'oflag=direct', 'of=fake-dev', 'count=33', 'oflag=direct',
use_standard_locale=True), use_standard_locale=True),
@ -424,7 +458,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
use_standard_locale=True)] use_standard_locale=True)]
mock_exec.side_effect = iter([ mock_exec.side_effect = iter([
(None, None), (None, None),
('42\n', None), ('512\n', None),
('21504\n', None),
(None, None), (None, None),
(None, None), (None, None),
(None, None), (None, None),
@ -434,17 +469,17 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase):
@mock.patch.object(utils, 'execute', autospec=True) @mock.patch.object(utils, 'execute', autospec=True)
class GetDeviceBlockSizeTestCase(base.IronicLibTestCase): class GetDeviceByteSizeTestCase(base.IronicLibTestCase):
def setUp(self): def setUp(self):
super(GetDeviceBlockSizeTestCase, self).setUp() super(GetDeviceByteSizeTestCase, self).setUp()
self.dev = 'fake-dev' self.dev = 'fake-dev'
self.node_uuid = "12345678-1234-1234-1234-1234567890abcxyz" self.node_uuid = "12345678-1234-1234-1234-1234567890abcxyz"
def test_get_dev_block_size(self, mock_exec): def test_get_dev_byte_size(self, mock_exec):
mock_exec.return_value = ("64", "") mock_exec.return_value = ("64", "")
expected_call = [mock.call('blockdev', '--getsz', self.dev)] expected_call = [mock.call('blockdev', '--getsize64', self.dev)]
disk_utils.get_dev_block_size(self.dev) disk_utils.get_dev_byte_size(self.dev)
mock_exec.assert_has_calls(expected_call) mock_exec.assert_has_calls(expected_call)

5
releasenotes/notes/support-4096-sector-size-490adc8ed256092d.yaml Normal file
View File

@ -0,0 +1,5 @@
---
fixes:
- |
Adds support for disks with 4096 sector size when cleaning disk metadata.
Previously, only 512 sector size disks were supported.