diff --git a/ironic_python_agent/disk_utils.py b/ironic_python_agent/disk_utils.py index dfabc3fc4..4195b3ca8 100644 --- a/ironic_python_agent/disk_utils.py +++ b/ironic_python_agent/disk_utils.py @@ -49,8 +49,6 @@ _PARTED_TABLE_TYPE_RE = re.compile(r'^.*partition\s+table\s*:\s*(gpt|msdos)', CONFIGDRIVE_LABEL = "config-2" MAX_CONFIG_DRIVE_SIZE_MB = 64 -GPT_SIZE_SECTORS = 33 - # Maximum disk size supported by MBR is 2TB (2 * 1024 * 1024 MB) MAX_DISK_SIZE_MB_SUPPORTED_BY_MBR = 2097152 @@ -428,10 +426,16 @@ def get_image_mb(image_path, virtual_size=True): return image_mb -def get_dev_block_size(dev): - """Get the device size in 512 byte sectors.""" - block_sz, cmderr = utils.execute('blockdev', '--getsz', dev) - return int(block_sz) +def get_dev_byte_size(dev): + """Get the device size in bytes.""" + byte_sz, cmderr = utils.execute('blockdev', '--getsize64', dev) + 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): @@ -465,21 +469,32 @@ def destroy_disk_metadata(dev, node_uuid): # This is the same bug as # 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) + dd_bs = 'bs=%s' % sector_size dd_device = 'of=%s' % dev - dd_count = 'count=%s' % GPT_SIZE_SECTORS - dev_size = get_dev_block_size(dev) - if dev_size < GPT_SIZE_SECTORS: - dd_count = 'count=%s' % dev_size - utils.execute('dd', 'bs=512', 'if=/dev/zero', dd_device, dd_count, + dd_count = 'count=%s' % gpt_sectors + dev_size = get_dev_byte_size(dev) + if dev_size < gpt_sectors * sector_size: + dd_count = 'count=%s' % int(dev_size / sector_size) + utils.execute('dd', dd_bs, 'if=/dev/zero', dd_device, dd_count, 'oflag=direct', use_standard_locale=True) # Overwrite the Secondary GPT, do this only if there could be one - if dev_size > GPT_SIZE_SECTORS: - gpt_backup = dev_size - GPT_SIZE_SECTORS + if dev_size > gpt_sectors * sector_size: + gpt_backup = int(dev_size / sector_size - gpt_sectors) dd_seek = 'seek=%i' % gpt_backup - dd_count = 'count=%s' % GPT_SIZE_SECTORS - utils.execute('dd', 'bs=512', 'if=/dev/zero', dd_device, dd_count, + dd_count = 'count=%s' % gpt_sectors + utils.execute('dd', dd_bs, 'if=/dev/zero', dd_device, dd_count, 'oflag=direct', dd_seek, use_standard_locale=True) # Go ahead and let sgdisk run as well. diff --git a/ironic_python_agent/tests/unit/test_disk_utils.py b/ironic_python_agent/tests/unit/test_disk_utils.py index 4c0275df2..fe7772360 100644 --- a/ironic_python_agent/tests/unit/test_disk_utils.py +++ b/ironic_python_agent/tests/unit/test_disk_utils.py @@ -312,13 +312,11 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): self.dev = 'fake-dev' 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. + def test_destroy_disk_metadata_4096(self, mock_exec): mock_exec.side_effect = iter([ (None, None), - ('1024\n', None), + ('4096\n', None), + ('524288\n', None), (None, None), (None, None), (None, None), @@ -326,7 +324,37 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev', 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', 'of=fake-dev', 'count=33', 'oflag=direct', use_standard_locale=True), @@ -353,7 +381,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): def test_destroy_disk_metadata_sgdisk_fail(self, mock_exec): expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev', 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', 'of=fake-dev', 'count=33', 'oflag=direct', use_standard_locale=True), @@ -364,7 +393,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): use_standard_locale=True)] mock_exec.side_effect = iter([ (None, None), - ('1024\n', None), + ('512\n', None), + ('524288\n', None), (None, None), (None, None), processutils.ProcessExecutionError()]) @@ -378,7 +408,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): mock_exec.side_effect = iter([ processutils.ProcessExecutionError(description='--force'), (None, None), - ('1024\n', None), + ('512\n', None), + ('524288\n', None), (None, None), (None, None), (None, None), @@ -394,7 +425,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): def test_destroy_disk_metadata_ebr(self, mock_exec): expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev', 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', 'of=fake-dev', 'count=2', 'oflag=direct', use_standard_locale=True), @@ -402,7 +434,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): use_standard_locale=True)] mock_exec.side_effect = iter([ (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), @@ -413,7 +446,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): def test_destroy_disk_metadata_tiny_partition(self, mock_exec): expected_calls = [mock.call('wipefs', '--force', '--all', 'fake-dev', 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', 'of=fake-dev', 'count=33', 'oflag=direct', use_standard_locale=True), @@ -424,7 +458,8 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): use_standard_locale=True)] mock_exec.side_effect = iter([ (None, None), - ('42\n', None), + ('512\n', None), + ('21504\n', None), (None, None), (None, None), (None, None), @@ -434,17 +469,17 @@ class DestroyMetaDataTestCase(base.IronicLibTestCase): @mock.patch.object(utils, 'execute', autospec=True) -class GetDeviceBlockSizeTestCase(base.IronicLibTestCase): +class GetDeviceByteSizeTestCase(base.IronicLibTestCase): def setUp(self): - super(GetDeviceBlockSizeTestCase, self).setUp() + super(GetDeviceByteSizeTestCase, self).setUp() self.dev = 'fake-dev' 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", "") - expected_call = [mock.call('blockdev', '--getsz', self.dev)] - disk_utils.get_dev_block_size(self.dev) + expected_call = [mock.call('blockdev', '--getsize64', self.dev)] + disk_utils.get_dev_byte_size(self.dev) mock_exec.assert_has_calls(expected_call) diff --git a/releasenotes/notes/support-4096-sector-size-490adc8ed256092d.yaml b/releasenotes/notes/support-4096-sector-size-490adc8ed256092d.yaml new file mode 100644 index 000000000..be4eb3984 --- /dev/null +++ b/releasenotes/notes/support-4096-sector-size-490adc8ed256092d.yaml @@ -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.