Merge "Prevent changes of a resource class for an active node"

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

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

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.