From 0f7b5a0896cf1b3080b7679278ff97a8dff0be80 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Wed, 13 Jun 2018 11:58:27 -0700 Subject: [PATCH] Try to unlock failed device before proceeding When a hard error has occured with secure erase, we should attempt an unlock of the device becuase the current mode can prevent disk IO. This may upset some things like raid controllers even if they are in a pass-through mode. Change-Id: I32e1d962fbbb4a305d5dbebea92ac48ebd9b67ca Story: #2002546 Task: #22107 --- ironic_python_agent/hardware.py | 51 +++++++++++-------- .../tests/unit/test_hardware.py | 8 ++- ...mpts-ata-disk-unlock-897d76c494ec2976.yaml | 16 ++++++ 3 files changed, 53 insertions(+), 22 deletions(-) create mode 100644 releasenotes/notes/attempts-ata-disk-unlock-897d76c494ec2976.yaml diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 2a4f8eef1..55753740d 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -889,6 +889,31 @@ class GenericHardwareManager(HardwareManager): return security_lines def _ata_erase(self, block_device): + + def __attempt_unlock_drive(block_device, security_lines=None): + # Attempt to unlock the drive in the event it has already been + # locked by a previous failed attempt. We try the empty string as + # versions of hdparm < 9.51, interpreted NULL as the literal + # string, "NULL", as opposed to the empty string. + if not security_lines: + security_lines = self._get_ata_security_lines(block_device) + unlock_passwords = ['NULL', ''] + for password in unlock_passwords: + if 'not locked' in security_lines: + break + try: + utils.execute('hdparm', '--user-master', 'u', + '--security-unlock', password, + block_device.name) + except processutils.ProcessExecutionError as e: + LOG.info('Security unlock failed for device ' + '%(name)s using password "%(password)s": %(err)s', + {'name': block_device.name, + 'password': password, + 'err': e}) + security_lines = self._get_ata_security_lines(block_device) + return security_lines + security_lines = self._get_ata_security_lines(block_device) # If secure erase isn't supported return False so erase_block_device @@ -907,26 +932,8 @@ class GenericHardwareManager(HardwareManager): ).format(block_device.name)) # At this point, we could be in SEC1,4,5 - - # Attempt to unlock the drive in the event it has already been - # locked by a previous failed attempt. We try the empty string as - # versions of hdparm < 9.51, interpreted NULL as the literal string, - # "NULL", as opposed to the empty string. - unlock_passwords = ['NULL', ''] - for password in unlock_passwords: - if 'not locked' in security_lines: - break - try: - utils.execute('hdparm', '--user-master', 'u', - '--security-unlock', password, - block_device.name) - except processutils.ProcessExecutionError as e: - LOG.info('Security unlock failed for device ' - '%(name)s using password "%(password)s": %(err)s', - {'name': block_device.name, - 'password': password, - 'err': e}) - security_lines = self._get_ata_security_lines(block_device) + # Attempt to unlock the drive if it has failed in a prior attempt. + security_lines = __attempt_unlock_drive(block_device, security_lines) # If the unlock failed we will still be in SEC4, otherwise, we will be # in SEC1 or SEC5 @@ -959,6 +966,10 @@ class GenericHardwareManager(HardwareManager): utils.execute('hdparm', '--user-master', 'u', erase_option, 'NULL', block_device.name) except processutils.ProcessExecutionError as e: + # NOTE(TheJulia): Attempt unlock to allow fallback to shred + # to occur, otherwise shred will fail as well, as the security + # mode will prevent IO operations to the disk. + __attempt_unlock_drive(block_device) raise errors.BlockDeviceEraseError('Erase failed for device ' '%(name)s: %(err)s' % {'name': block_device.name, diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 555487456..2f0a8354a 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -1576,11 +1576,15 @@ class TestGenericHardwareManager(base.IronicAgentTest): # Exception on security erase hdparm_output = create_hdparm_info( supported=True, enabled=False, frozen=False, enhanced_erase=False) - + hdparm_unlocked_output = create_hdparm_info( + supported=True, locked=True, frozen=False, enhanced_erase=False) mocked_execute.side_effect = [ (hdparm_output, '', '-1'), '', # security-set-pass - processutils.ProcessExecutionError() # security-erase + processutils.ProcessExecutionError(), # security-erase + (hdparm_unlocked_output, '', '-1'), + '', # attempt security unlock + (hdparm_output, '', '-1') ] block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, diff --git a/releasenotes/notes/attempts-ata-disk-unlock-897d76c494ec2976.yaml b/releasenotes/notes/attempts-ata-disk-unlock-897d76c494ec2976.yaml new file mode 100644 index 000000000..74402d574 --- /dev/null +++ b/releasenotes/notes/attempts-ata-disk-unlock-897d76c494ec2976.yaml @@ -0,0 +1,16 @@ +--- +fixes: + - | + Fixes the ATA Secure Erase logic to attempt an immediate unlock in the + event of a failed attempt to Secure Erase. This is required to permit + fallback to make use of the ``shred`` disk utility. + + In the event that an ATA Secure Erase operation fails during cleaning, + the disk will be write locked. In this case, the disk must be explicitly + unlocked. + + This should also prevent failures where an ATA Secure Erase operation + fails with a pass-through disk controller, which may prevent the disk + from being available after a reboot operation. For additional information, + please see + `story 2002546 `_.