Refactor driver_internal_info updates to methods

Making updates to driver_internal_info can result in hard to read code
due the requirement to assign the whole driver_internal_info back to
the node to trigger the expected update operation. This change
replaces driver_internal_info update operations with a new
methods:
- set_driver_internal_info
- del_driver_internal_info
- timestamp_driver_internal_info

This change defines the functions and moves core conductor logic to
use them. Subsequent changes in this series will move drivers to use
the new functions.

Change-Id: Ib8917c3c674e77cd3aba6a1e73c65162e3ee1141

5 files changed, 81 insertions, 128 deletions

diff --git a/ironic/conductor/cleaning.py b/ironic/conductor/cleaning.py
index 1ea35bfcf..3b2f7db04 100644
--- a/ironic/conductor/cleaning.py
+++ b/ironic/conductor/cleaning.py
@@ -75,10 +75,9 @@ def do_node_clean(task, clean_steps=None, disable_ramdisk=False):
 
     utils.wipe_cleaning_internal_info(task)
     if manual_clean:
-        info = node.driver_internal_info
-        info['clean_steps'] = clean_steps
-        info['cleaning_disable_ramdisk'] = disable_ramdisk
-        node.driver_internal_info = info
+        node.set_driver_internal_info('clean_steps', clean_steps)
+        node.set_driver_internal_info('cleaning_disable_ramdisk',
+                                      disable_ramdisk)
     task.node.save()
 
     # Retrieve BIOS config settings for this node
@@ -163,9 +162,7 @@ def do_next_clean_step(task, step_index, disable_ramdisk=None):
         # Save which step we're about to start so we can restart
         # if necessary
         node.clean_step = step
-        driver_internal_info = node.driver_internal_info
-        driver_internal_info['clean_step_index'] = step_index + ind
-        node.driver_internal_info = driver_internal_info
+        node.set_driver_internal_info('clean_step_index', step_index + ind)
         node.save()
         interface = getattr(task.driver, step.get('interface'))
         LOG.info('Executing %(step)s on node %(node)s',
@@ -179,8 +176,8 @@ def do_next_clean_step(task, step_index, disable_ramdisk=None):
                              'after cleaning reboot, waiting for agent to '
                              'come up to run next clean step %(step)s.',
                              {'node': node.uuid, 'step': step})
-                    driver_internal_info['skip_current_clean_step'] = False
-                    node.driver_internal_info = driver_internal_info
+                    node.set_driver_internal_info('skip_current_clean_step',
+                                                  False)
                     target_state = (states.MANAGEABLE if manual_clean
                                     else None)
                     task.process_event('wait', target_state=target_state)
@@ -191,8 +188,7 @@ def do_next_clean_step(task, step_index, disable_ramdisk=None):
                          'executing a command. Error: %(error)s',
                          {'node': task.node.uuid,
                           'error': e})
-                driver_internal_info['skip_current_clean_step'] = False
-                node.driver_internal_info = driver_internal_info
+                node.set_driver_internal_info('skip_current_clean_step', False)
                 target_state = states.MANAGEABLE if manual_clean else None
                 task.process_event('wait', target_state=target_state)
                 return
diff --git a/ironic/conductor/deployments.py b/ironic/conductor/deployments.py
index 6799f059a..30f24e404 100644
--- a/ironic/conductor/deployments.py
+++ b/ironic/conductor/deployments.py
@@ -91,9 +91,7 @@ def start_deploy(task, manager, configdrive=None, event='deploy',
     # NOTE(sirushtim): The iwdi variable can be None. It's up to
     # the deploy driver to validate this.
     iwdi = images.is_whole_disk_image(task.context, node.instance_info)
-    driver_internal_info = node.driver_internal_info
-    driver_internal_info['is_whole_disk_image'] = iwdi
-    node.driver_internal_info = driver_internal_info
+    node.set_driver_internal_info('is_whole_disk_image', iwdi)
     node.save()
 
     try:
@@ -187,9 +185,7 @@ def do_node_deploy(task, conductor_id=None, configdrive=None,
         # validated & processed later together with driver and deploy template
         # steps.
         if deploy_steps:
-            info = node.driver_internal_info
-            info['user_deploy_steps'] = deploy_steps
-            node.driver_internal_info = info
+            node.set_driver_internal_info('user_deploy_steps', deploy_steps)
             node.save()
         # This gets the deploy steps (if any) from driver, deploy template and
         # deploy_steps argument and updates them in the node's
@@ -256,9 +252,7 @@ def do_next_deploy_step(task, step_index):
         # Save which step we're about to start so we can restart
         # if necessary
         node.deploy_step = step
-        driver_internal_info = node.driver_internal_info
-        driver_internal_info['deploy_step_index'] = idx
-        node.driver_internal_info = driver_internal_info
+        node.set_driver_internal_info('deploy_step_index', idx)
         node.save()
         interface = getattr(task.driver, step.get('interface'))
         LOG.info('Executing %(step)s on node %(node)s',
@@ -271,8 +265,8 @@ def do_next_deploy_step(task, step_index):
                      'executing a command. Error: %(error)s',
                      {'node': task.node.uuid,
                       'error': e})
-            driver_internal_info['skip_current_deploy_step'] = False
-            node.driver_internal_info = driver_internal_info
+            node.set_driver_internal_info('skip_current_deploy_step',
+                                          False)
             task.process_event('wait')
             return
         except exception.IronicException as e:
@@ -282,8 +276,8 @@ def do_next_deploy_step(task, step_index):
                              'deployment reboot, waiting for agent to come up '
                              'to run next deploy step %(step)s.',
                              {'node': node.uuid, 'step': step})
-                    driver_internal_info['skip_current_deploy_step'] = False
-                    node.driver_internal_info = driver_internal_info
+                    node.set_driver_internal_info('skip_current_deploy_step',
+                                                  False)
                     task.process_event('wait')
                     return
 
@@ -364,9 +358,7 @@ def validate_deploy_steps(task):
     conductor_steps.set_node_deployment_steps(
         task, reset_current=False)
 
-    info = task.node.driver_internal_info
-    info['steps_validated'] = True
-    task.node.driver_internal_info = info
+    task.node.set_driver_internal_info('steps_validated', True)
     task.node.save()
 
 
diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py
index 7b9199617..ebbe86770 100644
--- a/ironic/conductor/manager.py
+++ b/ironic/conductor/manager.py
@@ -1095,12 +1095,10 @@ class ConductorManager(base_manager.BaseConductorManager):
             node.instance_info = {}
             node.instance_uuid = None
             utils.wipe_deploy_internal_info(task)
-            driver_internal_info = node.driver_internal_info
-            driver_internal_info.pop('instance', None)
-            driver_internal_info.pop('root_uuid_or_disk_id', None)
-            driver_internal_info.pop('is_whole_disk_image', None)
-            driver_internal_info.pop('deploy_boot_mode', None)
-            node.driver_internal_info = driver_internal_info
+            node.del_driver_internal_info('instance')
+            node.del_driver_internal_info('root_uuid_or_disk_id')
+            node.del_driver_internal_info('is_whole_disk_image')
+            node.del_driver_internal_info('deploy_boot_mode')
             network.remove_vifs_from_node(task)
             node.save()
             if node.allocation_id:
@@ -1720,9 +1718,7 @@ class ConductorManager(base_manager.BaseConductorManager):
             # supplied.
             iwdi = images.is_whole_disk_image(task.context,
                                               task.node.instance_info)
-            driver_internal_info = node.driver_internal_info
-            driver_internal_info['is_whole_disk_image'] = iwdi
-            node.driver_internal_info = driver_internal_info
+            node.set_driver_internal_info('is_whole_disk_image', iwdi)
             # Calling boot validate to ensure that sufficient information
             # is supplied to allow the node to be able to boot if takeover
             # writes items such as kernel/ramdisk data to disk.
@@ -1771,10 +1767,7 @@ class ConductorManager(base_manager.BaseConductorManager):
             # NOTE(kaifeng) Clear allocated_ipmi_terminal_port if exists,
             # so current conductor can allocate a new free port from local
             # resources.
-            internal_info = task.node.driver_internal_info
-            if 'allocated_ipmi_terminal_port' in internal_info:
-                internal_info.pop('allocated_ipmi_terminal_port')
-                task.node.driver_internal_info = internal_info
+            task.node.del_driver_internal_info('allocated_ipmi_terminal_port')
             try:
                 task.driver.console.start_console(task)
             except Exception as err:
@@ -1906,7 +1899,7 @@ class ConductorManager(base_manager.BaseConductorManager):
             # node instance for the current validations.
             iwdi = images.is_whole_disk_image(context,
                                               task.node.instance_info)
-            task.node.driver_internal_info['is_whole_disk_image'] = iwdi
+            task.node.set_driver_internal_info('is_whole_disk_image', iwdi)
             for iface_name in task.driver.non_vendor_interfaces:
                 iface = getattr(task.driver, iface_name)
                 result = reason = None
@@ -3500,7 +3493,7 @@ class ConductorManager(base_manager.BaseConductorManager):
                 # unusable value that can't be verified against.
                 # This is important if the agent lookup has occured with
                 # pre-generation of tokens with virtual media usage.
-                node.driver_internal_info['agent_secret_token'] = "******"
+                node.set_driver_internal_info('agent_secret_token', "******")
                 return node
             task.upgrade_lock()
             LOG.debug('Generating agent token for node %(node)s',
diff --git a/ironic/conductor/steps.py b/ironic/conductor/steps.py
index 05e07ced0..252b094a9 100644
--- a/ironic/conductor/steps.py
+++ b/ironic/conductor/steps.py
@@ -268,7 +268,6 @@ def set_node_cleaning_steps(task, disable_ramdisk=False):
              clean steps.
     """
     node = task.node
-    driver_internal_info = node.driver_internal_info
 
     # For manual cleaning, the target provision state is MANAGEABLE, whereas
     # for automated cleaning, it is AVAILABLE.
@@ -276,25 +275,24 @@ def set_node_cleaning_steps(task, disable_ramdisk=False):
 
     if not manual_clean:
         # Get the prioritized steps for automated cleaning
-        driver_internal_info['clean_steps'] = _get_cleaning_steps(task,
-                                                                  enabled=True)
+        steps = _get_cleaning_steps(task, enabled=True)
     else:
         # For manual cleaning, the list of cleaning steps was specified by the
         # user and already saved in node.driver_internal_info['clean_steps'].
         # Now that we know what the driver's available clean steps are, we can
         # do further checks to validate the user's clean steps.
-        steps = node.driver_internal_info['clean_steps']
-        driver_internal_info['clean_steps'] = _validate_user_clean_steps(
-            task, steps, disable_ramdisk=disable_ramdisk)
+        steps = _validate_user_clean_steps(
+            task, node.driver_internal_info['clean_steps'],
+            disable_ramdisk=disable_ramdisk)
 
     LOG.debug('List of the steps for %(type)s cleaning of node %(node)s: '
               '%(steps)s', {'type': 'manual' if manual_clean else 'automated',
                             'node': node.uuid,
-                            'steps': driver_internal_info['clean_steps']})
+                            'steps': steps})
 
     node.clean_step = {}
-    driver_internal_info['clean_step_index'] = None
-    node.driver_internal_info = driver_internal_info
+    node.set_driver_internal_info('clean_steps', steps)
+    node.set_driver_internal_info('clean_step_index', None)
     node.save()
 
 
@@ -426,17 +424,16 @@ def set_node_deployment_steps(task, reset_current=True, skip_missing=False):
              deployment steps.
     """
     node = task.node
-    driver_internal_info = node.driver_internal_info
-    driver_internal_info['deploy_steps'] = _get_all_deployment_steps(
-        task, skip_missing=skip_missing)
+    node.set_driver_internal_info('deploy_steps', _get_all_deployment_steps(
+        task, skip_missing=skip_missing))
 
-    LOG.debug('List of the deploy steps for node %(node)s: '
-              '%(steps)s', {'node': node.uuid,
-                            'steps': driver_internal_info['deploy_steps']})
+    LOG.debug('List of the deploy steps for node %(node)s: %(steps)s', {
+        'node': node.uuid,
+        'steps': node.driver_internal_info['deploy_steps']
+    })
     if reset_current:
         node.deploy_step = {}
-        driver_internal_info['deploy_step_index'] = None
-    node.driver_internal_info = driver_internal_info
+        node.set_driver_internal_info('deploy_step_index', None)
     node.save()
 
 
diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py
index 59e5a668c..898c7e6d0 100644
--- a/ironic/conductor/utils.py
+++ b/ironic/conductor/utils.py
@@ -303,10 +303,7 @@ def node_power_action(task, new_state, timeout=None):
     # and clients that work is in progress.
     node['target_power_state'] = target_state
     node['last_error'] = None
-    driver_internal_info = node.driver_internal_info
-    driver_internal_info['last_power_state_change'] = str(
-        timeutils.utcnow().isoformat())
-    node.driver_internal_info = driver_internal_info
+    node.timestamp_driver_internal_info('last_power_state_change')
     # NOTE(dtantsur): wipe token on shutting down, otherwise a reboot in
     # fast-track (or an accidentally booted agent) will cause subsequent
     # actions to fail.
@@ -475,16 +472,14 @@ def cleaning_error_handler(task, logmsg, errmsg=None, traceback=False,
             states.CLEANFAIL):
         # Clear clean step, msg should already include current step
         node.clean_step = {}
-        info = node.driver_internal_info
         # Clear any leftover metadata about cleaning
-        info.pop('clean_step_index', None)
-        info.pop('cleaning_reboot', None)
-        info.pop('cleaning_polling', None)
-        info.pop('skip_current_clean_step', None)
+        node.del_driver_internal_info('clean_step_index')
+        node.del_driver_internal_info('cleaning_reboot')
+        node.del_driver_internal_info('cleaning_polling')
+        node.del_driver_internal_info('skip_current_clean_step')
         # We don't need to keep the old agent URL
         # as it should change upon the next cleaning attempt.
-        info.pop('agent_url', None)
-        node.driver_internal_info = info
+        node.del_driver_internal_info('agent_url')
     # For manual cleaning, the target provision state is MANAGEABLE, whereas
     # for automated cleaning, it is AVAILABLE.
     manual_clean = node.target_provision_state == states.MANAGEABLE
@@ -502,32 +497,29 @@ def cleaning_error_handler(task, logmsg, errmsg=None, traceback=False,
 
 def wipe_internal_info_on_power_off(node):
     """Wipe information that should not survive reboot/power off."""
-    driver_internal_info = node.driver_internal_info
     # DHCP may result in a new IP next time.
-    driver_internal_info.pop('agent_url', None)
+    node.del_driver_internal_info('agent_url')
     if not is_agent_token_pregenerated(node):
         # Wipe the token if it's not pre-generated, otherwise we'll refuse to
         # generate it again for the newly booted agent.
-        driver_internal_info.pop('agent_secret_token', False)
+        node.del_driver_internal_info('agent_secret_token')
     # Wipe cached steps since they may change after reboot.
-    driver_internal_info.pop('agent_cached_deploy_steps', None)
-    driver_internal_info.pop('agent_cached_clean_steps', None)
+    node.del_driver_internal_info('agent_cached_deploy_steps')
+    node.del_driver_internal_info('agent_cached_clean_steps')
     # Remove TLS certificate since it's regenerated on each run.
-    driver_internal_info.pop('agent_verify_ca', None)
-    node.driver_internal_info = driver_internal_info
+    node.del_driver_internal_info('agent_verify_ca')
 
 
 def wipe_token_and_url(task):
     """Remove agent URL and token from the task."""
-    info = task.node.driver_internal_info
-    info.pop('agent_secret_token', None)
-    info.pop('agent_secret_token_pregenerated', None)
+    node = task.node
+    node.del_driver_internal_info('agent_secret_token')
+    node.del_driver_internal_info('agent_secret_token_pregenerated')
     # Remove agent_url since it will be re-asserted
     # upon the next deployment attempt.
-    info.pop('agent_url', None)
+    node.del_driver_internal_info('agent_url')
     # Remove TLS certificate since it's regenerated on each run.
-    info.pop('agent_verify_ca', None)
-    task.node.driver_internal_info = info
+    node.del_driver_internal_info('agent_verify_ca')
 
 
 def wipe_deploy_internal_info(task):
@@ -535,32 +527,30 @@ def wipe_deploy_internal_info(task):
     if not fast_track_able(task):
         wipe_token_and_url(task)
     # Clear any leftover metadata about deployment.
-    info = task.node.driver_internal_info
-    info['deploy_steps'] = None
-    info.pop('user_deploy_steps', None)
-    info.pop('agent_cached_deploy_steps', None)
-    info.pop('deploy_step_index', None)
-    info.pop('deployment_reboot', None)
-    info.pop('deployment_polling', None)
-    info.pop('skip_current_deploy_step', None)
-    info.pop('steps_validated', None)
-    task.node.driver_internal_info = info
+    node = task.node
+    node.set_driver_internal_info('deploy_steps', None)
+    node.del_driver_internal_info('user_deploy_steps')
+    node.del_driver_internal_info('agent_cached_deploy_steps')
+    node.del_driver_internal_info('deploy_step_index')
+    node.del_driver_internal_info('deployment_reboot')
+    node.del_driver_internal_info('deployment_polling')
+    node.del_driver_internal_info('skip_current_deploy_step')
+    node.del_driver_internal_info('steps_validated')
 
 
 def wipe_cleaning_internal_info(task):
     """Remove temporary cleaning fields from driver_internal_info."""
     if not fast_track_able(task):
         wipe_token_and_url(task)
-    info = task.node.driver_internal_info
-    info['clean_steps'] = None
-    info.pop('agent_cached_clean_steps', None)
-    info.pop('clean_step_index', None)
-    info.pop('cleaning_reboot', None)
-    info.pop('cleaning_polling', None)
-    info.pop('cleaning_disable_ramdisk', None)
-    info.pop('skip_current_clean_step', None)
-    info.pop('steps_validated', None)
-    task.node.driver_internal_info = info
+    node = task.node
+    node.set_driver_internal_info('clean_steps', None)
+    node.del_driver_internal_info('agent_cached_clean_steps')
+    node.del_driver_internal_info('clean_step_index')
+    node.del_driver_internal_info('cleaning_reboot')
+    node.del_driver_internal_info('cleaning_polling')
+    node.del_driver_internal_info('cleaning_disable_ramdisk')
+    node.del_driver_internal_info('skip_current_clean_step')
+    node.del_driver_internal_info('steps_validated')
 
 
 def deploying_error_handler(task, logmsg, errmsg=None, traceback=False,
@@ -1172,9 +1162,7 @@ def is_fast_track(task):
 
 def remove_agent_url(node):
     """Helper to remove the agent_url record."""
-    info = node.driver_internal_info
-    info.pop('agent_url', None)
-    node.driver_internal_info = info
+    node.del_driver_internal_info('agent_url')
 
 
 def _get_node_next_steps(task, step_type, skip_current_step=True):
@@ -1246,24 +1234,13 @@ def update_next_step_index(task, step_type):
     :param step_type: The type of steps to process: 'clean' or 'deploy'.
     :returns: Index of the next step.
     """
-    info = task.node.driver_internal_info
-    save_required = False
-
-    try:
-        skip_current_step = info.pop('skip_current_%s_step' % step_type)
-    except KeyError:
-        skip_current_step = True
+    skip_current_step = task.node.del_driver_internal_info(
+        'skip_current_%s_step' % step_type, True)
+    if step_type == 'clean':
+        task.node.del_driver_internal_info('cleaning_polling')
     else:
-        save_required = True
-
-    field = ('cleaning_polling' if step_type == 'clean'
-             else 'deployment_polling')
-    if info.pop(field, None) is not None:
-        save_required = True
-
-    if save_required:
-        task.node.driver_internal_info = info
-        task.node.save()
+        task.node.del_driver_internal_info('deployment_polling')
+    task.node.save()
 
     return _get_node_next_steps(task, step_type,
                                 skip_current_step=skip_current_step)
@@ -1279,13 +1256,11 @@ def add_secret_token(node, pregenerated=False):
                          the token is embedded into the configuration.
     """
     token = secrets.token_urlsafe()
-    i_info = node.driver_internal_info
-    i_info['agent_secret_token'] = token
+    node.set_driver_internal_info('agent_secret_token', token)
     if pregenerated:
-        i_info['agent_secret_token_pregenerated'] = True
+        node.set_driver_internal_info('agent_secret_token_pregenerated', True)
     else:
-        i_info.pop('agent_secret_token_pregenerated', None)
-    node.driver_internal_info = i_info
+        node.del_driver_internal_info('agent_secret_token_pregenerated')
 
 
 def is_agent_token_present(node):