openstack/ironic
Code Issues Proposed changes

Node should reflect what was saved

Browse Source 
After a node is saved to the database, we weren't updating the
Node object to reflect what was saved. This caused a problem
where the node's update_at field was incorrect. It was fixed
in 065326c0f5 by explicitly
setting node.update_at. However, that doesn't address other
node fields that may be out of sync.

The more correct fix would be to do a similar thing that (most
of) the other Objects do, which is for the node to update itself
via ._from_db_object().

Doing this revealed several incorrect tests and code in the conductor
and agent where changes to the node's dictionaries were incorrectly
being set and thus, not being saved. Those are fixed in this patch.

Change-Id: Ia84cd60c1a4eabcc1ad0a756124c338fa9f644c8
Closes-Bug: #1679297
Related-Bug: #1281638
This commit is contained in:
Ruby Loo 2017-04-03 16:24:46 -04:00
parent 9186f7f192
commit 62c6377adf
9 changed files with 39 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
ironic/conductor/manager.py
View File

@ -1381,7 +1381,9 @@ class ConductorManager(base_manager.BaseConductorManager):
# supplied.
iwdi = images.is_whole_disk_image(task.context,
task.node.instance_info)
node.driver_internal_info['is_whole_disk_image'] = iwdi
driver_internal_info = node.driver_internal_info
driver_internal_info['is_whole_disk_image'] = iwdi
node.driver_internal_info = driver_internal_info
# 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.

4
ironic/drivers/modules/agent.py
View File

@ -291,7 +291,9 @@ class AgentDeployMixin(agent_base_vendor.AgentDeployMixin):
'efi_system_partition_uuid'))
else:
efi_sys_uuid = None
task.node.driver_internal_info['root_uuid_or_disk_id'] = root_uuid
driver_internal_info = task.node.driver_internal_info
driver_internal_info['root_uuid_or_disk_id'] = root_uuid
task.node.driver_internal_info = driver_internal_info
task.node.save()
self.prepare_instance_to_boot(task, root_uuid, efi_sys_uuid)
LOG.info(_LI('Image successfully written to node %s'), node.uuid)

8
ironic/objects/node.py
View File

@ -362,13 +362,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat):
self.driver_internal_info = {}
updates = self.obj_get_changes()
db_node = self.dbapi.update_node(self.uuid, updates)
# TODO(galyna): updating specific field not touching others to not
# change default behaviour. Otherwise it will break a bunch of tests
# This can be updated in other way when more fields like `updated_at`
# will appear
self.updated_at = db_node['updated_at']
self.obj_reset_changes()
self._from_db_object(self, db_node)
# NOTE(xek): We don't want to enable RPC on this call just yet. Remotable
# methods can be used in the future to replace current explicit RPC calls.

4
ironic/tests/unit/drivers/modules/ilo/test_boot.py
View File

@ -489,7 +489,9 @@ class IloVirtualMediaBootTestCase(db_base.DbTestCase):
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
task.node.driver_info['ilo_deploy_iso'] = 'deploy-iso'
driver_info = task.node.driver_info
driver_info['ilo_deploy_iso'] = 'deploy-iso'
task.node.driver_info = driver_info
task.driver.boot.prepare_ramdisk(task, ramdisk_params)

4
ironic/tests/unit/drivers/modules/ilo/test_inspect.py
View File

@ -125,7 +125,9 @@ class IloInspectTestCase(db_base.DbTestCase):
power_mock.return_value = states.POWER_ON
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
task.node.properties['local_gb'] = 10
properties = task.node.properties
properties['local_gb'] = 10
task.node.properties = properties
task.node.save()
expected_properties = {'memory_mb': '512', 'local_gb': 10,
'cpus': '1', 'cpu_arch': 'x86_64'}

12
ironic/tests/unit/drivers/modules/test_agent.py
View File

@ -664,7 +664,9 @@ class TestAgentDeploy(db_base.DbTestCase):
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
get_power_state_mock.return_value = states.POWER_OFF
task.node.driver_internal_info['is_whole_disk_image'] = False
driver_internal_info = task.node.driver_internal_info
driver_internal_info['is_whole_disk_image'] = False
task.node.driver_internal_info = driver_internal_info
task.driver.deploy.reboot_to_instance(task)
@ -709,7 +711,9 @@ class TestAgentDeploy(db_base.DbTestCase):
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
get_power_state_mock.return_value = states.POWER_OFF
task.node.driver_internal_info['is_whole_disk_image'] = True
driver_internal_info = task.node.driver_internal_info
driver_internal_info['is_whole_disk_image'] = True
task.node.driver_internal_info = driver_internal_info
task.driver.boot = None
task.driver.deploy.reboot_to_instance(task)
@ -797,7 +801,9 @@ class TestAgentDeploy(db_base.DbTestCase):
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
get_power_state_mock.return_value = states.POWER_OFF
task.node.driver_internal_info['is_whole_disk_image'] = False
driver_internal_info = task.node.driver_internal_info
driver_internal_info['is_whole_disk_image'] = False
task.node.driver_internal_info = driver_internal_info
boot_option = {'capabilities': '{"boot_option": "local"}'}
task.node.instance_info = boot_option
task.driver.deploy.reboot_to_instance(task)

8
ironic/tests/unit/drivers/modules/test_ipmitool.py
View File

@ -2014,7 +2014,9 @@ class IPMIToolDriverTestCase(db_base.DbTestCase):
mock_exec.return_value = [None, None]
with task_manager.acquire(self.context, self.node.uuid) as task:
task.node.driver_info['ipmi_force_boot_device'] = True
driver_info = task.node.driver_info
driver_info['ipmi_force_boot_device'] = True
task.node.driver_info = driver_info
self.info['force_boot_device'] = True
self.driver.management.set_boot_device(task, boot_devices.PXE)
task.node.refresh()
@ -2032,7 +2034,9 @@ class IPMIToolDriverTestCase(db_base.DbTestCase):
mock_exec.return_value = [None, None]
with task_manager.acquire(self.context, self.node.uuid) as task:
task.node.driver_info['ipmi_force_boot_device'] = True
driver_info = task.node.driver_info
driver_info['ipmi_force_boot_device'] = True
task.node.driver_info = driver_info
self.info['force_boot_device'] = True
self.driver.management.set_boot_device(task,
boot_devices.PXE,

7
ironic/tests/unit/objects/test_node.py
View File

@ -73,12 +73,15 @@ class TestNodeObject(base.DbTestCase, obj_utils.SchemasTestMixIn):
def test_save(self):
uuid = self.fake_node['uuid']
test_time = datetime.datetime(2000, 1, 1, 0, 0)
with mock.patch.object(self.dbapi, 'get_node_by_uuid',
autospec=True) as mock_get_node:
mock_get_node.return_value = self.fake_node
with mock.patch.object(self.dbapi, 'update_node',
autospec=True) as mock_update_node:
mock_update_node.return_value = utils.get_test_node()
mock_update_node.return_value = utils.get_test_node(
properties={"fake": "property"}, driver='fake-driver',
driver_internal_info={}, updated_at=test_time)
n = objects.Node.get(self.context, uuid)
self.assertEqual({"private_state": "secret value"},
n.driver_internal_info)
@ -92,6 +95,8 @@ class TestNodeObject(base.DbTestCase, obj_utils.SchemasTestMixIn):
'driver': 'fake-driver',
'driver_internal_info': {}})
self.assertEqual(self.context, n._context)
res_updated_at = (n.updated_at).replace(tzinfo=None)
self.assertEqual(test_time, res_updated_at)
self.assertEqual({}, n.driver_internal_info)
def test_save_updated_at_field(self):

5
releasenotes/notes/node-save-internal-info-c5cc8f56f1d0dab0.yaml Normal file
View File

@ -0,0 +1,5 @@
---
fixes:
- |
Fixes an issue where some internal information for a node was not being
saved to the database.