Merge "Try to unlock failed device before proceeding"

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,

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,

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 <https://storyboard.openstack.org/#!/story/2002546>`_.