diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 237c691296..fae7d56ff4 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -165,6 +165,9 @@ class ConductorManager(base_manager.BaseConductorManager): # interfaces for active nodes in the future. allowed_update_states = [states.ENROLL, states.INSPECTING, states.MANAGEABLE, states.AVAILABLE] + action = _("Node %(node)s can not have %(field)s " + "updated unless it is in one of allowed " + "(%(allowed)s) states or in maintenance mode.") for iface in drivers_base.ALL_INTERFACES: interface_field = '%s_interface' % iface if interface_field not in delta: @@ -172,13 +175,10 @@ class ConductorManager(base_manager.BaseConductorManager): if not (node_obj.provision_state in allowed_update_states or node_obj.maintenance): - action = _("Node %(node)s can not have %(iface)s " - "updated unless it is in one of allowed " - "(%(allowed)s) states or in maintenance mode.") raise exception.InvalidState( action % {'node': node_obj.uuid, 'allowed': ', '.join(allowed_update_states), - 'iface': interface_field}) + 'field': interface_field}) driver_factory.check_and_update_node_interfaces(node_obj) @@ -192,6 +192,17 @@ class ConductorManager(base_manager.BaseConductorManager): raise exception.NodeAssociated( node=node_id, instance=task.node.instance_uuid) + # NOTE(dtantsur): if the resource class is changed for an active + # instance, nova will not update its internal record. That will + # result in the new resource class exposed on the node as available + # for consumption, and nova may try to schedule on this node again. + if ('resource_class' in delta and task.node.resource_class and + task.node.provision_state not in allowed_update_states): + raise exception.InvalidState( + action % {'node': node_obj.uuid, + 'allowed': ', '.join(allowed_update_states), + 'field': 'resource_class'}) + node_obj.save() return node_obj diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 7873b4ff76..2b7913b476 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -655,6 +655,54 @@ class UpdateNodeTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): for iface_name in self.IFACE_UPDATE_DICT: self._test_update_node_interface_invalid(node, iface_name) + def _test_update_node_change_resource_class(self, state, + resource_class=None, + new_resource_class='new', + expect_error=False): + node = obj_utils.create_test_node(self.context, driver='fake', + uuid=uuidutils.generate_uuid(), + provision_state=state, + resource_class=resource_class) + self.addCleanup(node.destroy) + + node.resource_class = new_resource_class + if expect_error: + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.update_node, + self.context, + node) + # Compare true exception hidden by @messaging.expected_exceptions + self.assertEqual(exception.InvalidState, exc.exc_info[0]) + + # verify change did not happen + res = objects.Node.get_by_uuid(self.context, node['uuid']) + self.assertEqual(resource_class, res['resource_class']) + else: + self.service.update_node(self.context, node) + + res = objects.Node.get_by_uuid(self.context, node['uuid']) + self.assertEqual('new', res['resource_class']) + + def test_update_resource_class_allowed_state(self): + for state in [states.ENROLL, states.MANAGEABLE, states.INSPECTING, + states.AVAILABLE]: + self._test_update_node_change_resource_class( + state, resource_class='old', expect_error=False) + + def test_update_resource_class_no_previous_value(self): + for state in [states.ENROLL, states.MANAGEABLE, states.INSPECTING, + states.AVAILABLE, states.ACTIVE]: + self._test_update_node_change_resource_class( + state, resource_class=None, expect_error=False) + + def test_update_resource_class_not_allowed(self): + self._test_update_node_change_resource_class( + states.ACTIVE, resource_class='old', new_resource_class='new', + expect_error=True) + self._test_update_node_change_resource_class( + states.ACTIVE, resource_class='old', new_resource_class=None, + expect_error=True) + @mgr_utils.mock_record_keepalive class VendorPassthruTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): diff --git a/releasenotes/notes/resource-class-change-563797d5a3c35683.yaml b/releasenotes/notes/resource-class-change-563797d5a3c35683.yaml new file mode 100644 index 0000000000..922c0c669e --- /dev/null +++ b/releasenotes/notes/resource-class-change-563797d5a3c35683.yaml @@ -0,0 +1,12 @@ +--- +upgrade: + - | + Changing the ``resource_class`` field of a node in the ``active`` state + or any of the transient states is no longer possible. Please update your + scripts to only set a resource class for nodes that are not deployed to. + Setting a resource class for nodes that do not have it is still possible. +fixes: + - | + No longer allows changing the ``resource_class`` field for ``active`` nodes + if it was already set to a non-empty value. Doing so would break the + Compute scheduler.