openstack/ironic
Code Issues Proposed changes

Refactor driver_internal_info updates to methods

Browse Source 
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
This commit is contained in:
Steve Baker
2021-11-19 09:52:53 +13:00
parent 3197301dba
commit d5eb6ee567
8 changed files with 141 additions and 146 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
ironic/conductor/cleaning.py
View File

@@ -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

24
ironic/conductor/deployments.py
View File

@@ -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()

23
ironic/conductor/manager.py
View File

@@ -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',

31
ironic/conductor/steps.py
View File

@@ -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()

113
ironic/conductor/utils.py
View File

@@ -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):

38
ironic/objects/node.py
View File

@@ -15,6 +15,7 @@
from oslo_config import cfg
from oslo_log import log
from oslo_utils import strutils
from oslo_utils import timeutils
from oslo_utils import uuidutils
from oslo_utils import versionutils
from oslo_versionedobjects import base as object_base
@@ -691,6 +692,43 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat):
self._convert_network_data_field(target_version,
remove_unavailable_fields)
def set_driver_internal_info(self, key, value):
"""Set a `driver_internal_info` value.
Setting a `driver_internal_info` dict value via this method
ensures that this field will be flagged for saving.
:param key: Key of item to set
:param value: Value of item to set
"""
self.driver_internal_info[key] = value
self._changed_fields.add('driver_internal_info')
def del_driver_internal_info(self, key, default_value=None):
"""Pop a value from the driver_internal_info.
Removing a `driver_internal_info` dict value via this method
ensures that this field will be flagged for saving.
:param key: Key of item to pop off the `driver_internal_info` dict
:param default_value: Value to return if the key doesn't exist
:returns: The removed value, or `default_value`
"""
if key in self.driver_internal_info:
self._changed_fields.add('driver_internal_info')
return self.driver_internal_info.pop(key, default_value)
return default_value
def timestamp_driver_internal_info(self, key):
"""Set a `driver_internal_info` value with the current timestamp.
Setting a `driver_internal_info` timestamp value via this method
ensures that this field will be flagged for saving.
:param key: Key of item to set the timestamp on
"""
self.set_driver_internal_info(key, timeutils.utcnow().isoformat())
@base.IronicObjectRegistry.register
class NodePayload(notification.NotificationPayloadBase):

12
ironic/tests/unit/conductor/test_manager.py
View File

@@ -3059,8 +3059,12 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin,
err_handler=conductor_utils.spawn_rescue_error_handler)
self.assertIn('rescue_password', task.node.instance_info)
self.assertIn('hashed_rescue_password', task.node.instance_info)
self.assertEqual({'other field': 'value'},
task.node.driver_internal_info)
task.node.del_driver_internal_info.assert_has_calls([
mock.call('agent_secret_token'),
mock.call('agent_secret_token_pregenerated'),
mock.call('agent_url'),
mock.call('agent_verify_ca')
])
def test_do_node_rescue_invalid_state(self):
self._start_service()
@@ -3212,7 +3216,9 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin,
mock_acquire.side_effect = self._get_acquire_side_effect(task)
self.service.do_node_unrescue(self.context, task.node.uuid)
task.node.refresh()
self.assertNotIn('agent_url', task.node.driver_internal_info)
task.node.del_driver_internal_info.assert_has_calls([
mock.call('agent_url')
])
task.process_event.assert_called_once_with(
'unrescue',
callback=self.service._spawn_worker,

28
ironic/tests/unit/conductor/test_utils.py
View File

@@ -991,12 +991,11 @@ class DeployingErrorHandlerTestCase(db_base.DbTestCase):
self.task)
def test_deploying_error_handler(self):
info = self.node.driver_internal_info
info['deploy_step_index'] = 2
info['deployment_reboot'] = True
info['deployment_polling'] = True
info['skip_current_deploy_step'] = True
info['agent_url'] = 'url'
self.node.set_driver_internal_info('deploy_step_index', 2)
self.node.set_driver_internal_info('deployment_reboot', True)
self.node.set_driver_internal_info('deployment_polling', True)
self.node.set_driver_internal_info('skip_current_deploy_step', True)
self.node.set_driver_internal_info('agent_url', 'url')
conductor_utils.deploying_error_handler(self.task, self.logmsg,
self.errmsg)
@@ -1128,9 +1127,8 @@ class ErrorHandlersTestCase(db_base.DbTestCase):
clean_error = ("Timeout reached while cleaning the node. Please "
"check if the ramdisk responsible for the cleaning is "
"running on the node. Failed on step {'key': 'val'}.")
self.node.driver_internal_info = {
'cleaning_reboot': True,
'clean_step_index': 0}
self.node.set_driver_internal_info('cleaning_reboot', True)
self.node.set_driver_internal_info('clean_step_index', 0)
conductor_utils.cleanup_cleanwait_timeout(self.task)
self.assertEqual({}, self.node.clean_step)
self.assertNotIn('clean_step_index', self.node.driver_internal_info)
@@ -1146,12 +1144,12 @@ class ErrorHandlersTestCase(db_base.DbTestCase):
target = 'baz'
self.node.target_provision_state = target
self.node.clean_step = {'key': 'val'}
self.node.driver_internal_info = {
'cleaning_reboot': True,
'cleaning_polling': True,
'skip_current_clean_step': True,
'clean_step_index': 0,
'agent_url': 'url'}
self.node.set_driver_internal_info('cleaning_reboot', True)
self.node.set_driver_internal_info('cleaning_polling', True)
self.node.set_driver_internal_info('skip_current_clean_step', True)
self.node.set_driver_internal_info('clean_step_index', 0)
self.node.set_driver_internal_info('agent_url', 'url')
msg = 'error bar'
last_error = "last error"
conductor_utils.cleaning_error_handler(self.task, msg,