From 62c6377adf5c3400834b29667cbf5e29e0e8bde6 Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Mon, 3 Apr 2017 16:24:46 -0400 Subject: [PATCH] Node should reflect what was saved 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 065326c0f55a73c99706148f7bfbbec87ea863bc 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 --- ironic/conductor/manager.py | 4 +++- ironic/drivers/modules/agent.py | 4 +++- ironic/objects/node.py | 8 +------- ironic/tests/unit/drivers/modules/ilo/test_boot.py | 4 +++- .../tests/unit/drivers/modules/ilo/test_inspect.py | 4 +++- ironic/tests/unit/drivers/modules/test_agent.py | 12 +++++++++--- ironic/tests/unit/drivers/modules/test_ipmitool.py | 8 ++++++-- ironic/tests/unit/objects/test_node.py | 7 ++++++- .../node-save-internal-info-c5cc8f56f1d0dab0.yaml | 5 +++++ 9 files changed, 39 insertions(+), 17 deletions(-) create mode 100644 releasenotes/notes/node-save-internal-info-c5cc8f56f1d0dab0.yaml diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 8b63497594..d5208bea3f 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -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. diff --git a/ironic/drivers/modules/agent.py b/ironic/drivers/modules/agent.py index 1160066971..91f9b578a9 100644 --- a/ironic/drivers/modules/agent.py +++ b/ironic/drivers/modules/agent.py @@ -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) diff --git a/ironic/objects/node.py b/ironic/objects/node.py index 8a690151fb..5a65e80d2f 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -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. diff --git a/ironic/tests/unit/drivers/modules/ilo/test_boot.py b/ironic/tests/unit/drivers/modules/ilo/test_boot.py index 4d3964c61c..4dc4fe9987 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_boot.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_boot.py @@ -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) diff --git a/ironic/tests/unit/drivers/modules/ilo/test_inspect.py b/ironic/tests/unit/drivers/modules/ilo/test_inspect.py index 2bf33b681a..7276b3ddaf 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_inspect.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_inspect.py @@ -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'} diff --git a/ironic/tests/unit/drivers/modules/test_agent.py b/ironic/tests/unit/drivers/modules/test_agent.py index b011ac9e91..77bc57f54c 100644 --- a/ironic/tests/unit/drivers/modules/test_agent.py +++ b/ironic/tests/unit/drivers/modules/test_agent.py @@ -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) diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index 27c8e9a0aa..aff0758b1f 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py @@ -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, diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index b464a180ba..2ec0510546 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/ironic/tests/unit/objects/test_node.py @@ -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): diff --git a/releasenotes/notes/node-save-internal-info-c5cc8f56f1d0dab0.yaml b/releasenotes/notes/node-save-internal-info-c5cc8f56f1d0dab0.yaml new file mode 100644 index 0000000000..acba691c72 --- /dev/null +++ b/releasenotes/notes/node-save-internal-info-c5cc8f56f1d0dab0.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes an issue where some internal information for a node was not being + saved to the database.