Merge "baremetal: implement the correct update of the maintenance_reason field"

doc/source/user/proxies/baremetal.rst

@@ -27,6 +27,8 @@ Node Operations
    .. automethod:: openstack.baremetal.v1._proxy.Proxy.wait_for_nodes_provision_state
    .. automethod:: openstack.baremetal.v1._proxy.Proxy.wait_for_node_reservation
    .. automethod:: openstack.baremetal.v1._proxy.Proxy.validate_node
+   .. automethod:: openstack.baremetal.v1._proxy.Proxy.set_node_maintenance
+   .. automethod:: openstack.baremetal.v1._proxy.Proxy.unset_node_maintenance
 
 Port Operations
 ^^^^^^^^^^^^^^^

openstack/baremetal/v1/_proxy.py

@@ -399,6 +399,27 @@ class Proxy(proxy.Proxy):
         res = self._get_resource(_node.Node, node)
         return res.validate(self, required=required)
 
+    def set_node_maintenance(self, node, reason=None):
+        """Enable maintenance mode on the node.
+
+        :param node: The value can be either the name or ID of a node or
+            a :class:`~openstack.baremetal.v1.node.Node` instance.
+        :param reason: Optional reason for maintenance.
+        :return: This :class:`Node` instance.
+        """
+        res = self._get_resource(_node.Node, node)
+        return res.set_maintenance(self, reason)
+
+    def unset_node_maintenance(self, node):
+        """Disable maintenance mode on the node.
+
+        :param node: The value can be either the name or ID of a node or
+            a :class:`~openstack.baremetal.v1.node.Node` instance.
+        :return: This :class:`Node` instance.
+        """
+        res = self._get_resource(_node.Node, node)
+        return res.unset_maintenance(self)
+
     def delete_node(self, node, ignore_missing=True):
         """Delete a node.
 

openstack/baremetal/v1/node.py

@@ -250,7 +250,7 @@ class Node(_common.ListMixin, resource.Resource):
                 "state %s" % expected_provision_state)
 
         # Ironic cannot set provision_state itself, so marking it as unchanged
-        self._body.clean(only={'provision_state'})
+        self._clean_body_attrs({'provision_state'})
         super(Node, self).create(session, *args, **kwargs)
 
         if (self.provision_state == 'enroll'
@@ -267,6 +267,39 @@ class Node(_common.ListMixin, resource.Resource):
 
         return self
 
+    def commit(self, session, *args, **kwargs):
+        """Commit the state of the instance to the remote resource.
+
+        :param session: The session to use for making this request.
+        :type session: :class:`~keystoneauth1.adapter.Adapter`
+
+        :return: This :class:`Node` instance.
+        """
+        # These fields have to be set through separate API.
+        if ('maintenance_reason' in self._body.dirty
+                or 'maintenance' in self._body.dirty):
+            if not self.is_maintenance and self.maintenance_reason:
+                if 'maintenance' in self._body.dirty:
+                    self.maintenance_reason = None
+                else:
+                    raise ValueError('Maintenance reason cannot be set when '
+                                     'maintenance is False')
+            if self.is_maintenance:
+                self._do_maintenance_action(
+                    session, 'put', {'reason': self.maintenance_reason})
+            else:
+                # This corresponds to setting maintenance=False and
+                # maintenance_reason=None in the same request.
+                self._do_maintenance_action(session, 'delete')
+
+            self._clean_body_attrs({'maintenance', 'maintenance_reason'})
+            if not self.requires_commit:
+                # Other fields are not updated, re-fetch the node to reflect
+                # the new status.
+                return self.fetch(session)
+
+        return super(Node, self).commit(session, *args, **kwargs)
+
     def set_provision_state(self, session, target, config_drive=None,
                             clean_steps=None, rescue_password=None,
                             wait=False, timeout=None):
@@ -647,5 +680,38 @@ class Node(_common.ListMixin, resource.Resource):
         return {key: ValidationResult(value.get('result'), value.get('reason'))
                 for key, value in result.items()}
 
+    def set_maintenance(self, session, reason=None):
+        """Enable maintenance mode on the node.
+
+        :param session: The session to use for making this request.
+        :type session: :class:`~keystoneauth1.adapter.Adapter`
+        :param reason: Optional reason for maintenance.
+        :return: This :class:`Node` instance.
+        """
+        self._do_maintenance_action(session, 'put', {'reason': reason})
+        return self.fetch(session)
+
+    def unset_maintenance(self, session):
+        """Disable maintenance mode on the node.
+
+        :param session: The session to use for making this request.
+        :type session: :class:`~keystoneauth1.adapter.Adapter`
+        :return: This :class:`Node` instance.
+        """
+        self._do_maintenance_action(session, 'delete')
+        return self.fetch(session)
+
+    def _do_maintenance_action(self, session, verb, body=None):
+        session = self._get_session(session)
+        version = self._get_microversion_for(session, 'commit')
+        request = self._prepare_request(requires_id=True)
+        request.url = utils.urljoin(request.url, 'maintenance')
+        response = getattr(session, verb)(
+            request.url, json=body,
+            headers=request.headers, microversion=version)
+        msg = ("Failed to change maintenance mode for node {node}"
+               .format(node=self.id))
+        exceptions.raise_from_response(response, error_message=msg)
+
 
 NodeDetail = Node

openstack/cloud/openstackcloud.py

@@ -9528,17 +9528,10 @@ class _OpenStackCloudMixin(_normalize.Normalizer):
 
         :returns: None
         """
-        msg = ("Error setting machine maintenance state to {state} on node "
-               "{node}").format(state=state, node=name_or_id)
-        url = '/nodes/{name_or_id}/maintenance'.format(name_or_id=name_or_id)
         if state:
-            payload = {'reason': reason}
-            self._baremetal_client.put(url,
-                                       json=payload,
-                                       error_message=msg)
+            self.baremetal.set_node_maintenance(name_or_id, reason)
         else:
-            self._baremetal_client.delete(url, error_message=msg)
-        return None
+            self.baremetal.unset_node_maintenance(name_or_id)
 
     def remove_machine_from_maintenance(self, name_or_id):
         """Remove Baremetal Machine from Maintenance State
@@ -9555,7 +9548,7 @@ class _OpenStackCloudMixin(_normalize.Normalizer):
 
         :returns: None
         """
-        self.set_machine_maintenance_state(name_or_id, False)
+        self.baremetal.unset_node_maintenance(name_or_id)
 
     def set_machine_power_on(self, name_or_id):
         """Activate baremetal machine power

openstack/resource.py

@@ -695,6 +695,14 @@ class Resource(dict):
 
         return relevant_attrs
 
+    def _clean_body_attrs(self, attrs):
+        """Mark the attributes as up-to-date."""
+        self._body.clean(only=attrs)
+        if self.commit_jsonpatch:
+            for attr in attrs:
+                if attr in self._body:
+                    self._original_body[attr] = self._body[attr]
+
     @classmethod
     def _get_mapping(cls, component):
         """Return a dict of attributes of a given component on the class"""
@@ -1181,6 +1189,12 @@ class Resource(dict):
         self._translate_response(response, has_body=False)
         return self
 
+    @property
+    def requires_commit(self):
+        """Whether the next commit() call will do anything."""
+        return (self._body.dirty or self._header.dirty
+                or self.allow_empty_commit)
+
     def commit(self, session, prepend_key=True, has_body=True,
                retry_on_conflict=None, base_path=None):
         """Commit the state of the instance to the remote resource.
@@ -1205,11 +1219,7 @@ class Resource(dict):
         self._body._dirty.discard("id")
 
         # Only try to update if we actually have anything to commit.
-        if not any([
-            self._body.dirty,
-            self._header.dirty,
-            self.allow_empty_commit,
-        ]):
+        if not self.requires_commit:
             return self
 
         if not self.allow_commit:

openstack/tests/functional/baremetal/test_baremetal_node.py

@@ -154,6 +154,84 @@ class TestBareMetalNode(base.BaseBaremetalTest):
         self.assertIsNone(self.conn.baremetal.find_node(uuid))
         self.assertIsNone(self.conn.baremetal.delete_node(uuid))
 
+    def test_maintenance(self):
+        reason = "Prepating for taking over the world"
+
+        node = self.create_node()
+        self.assertFalse(node.is_maintenance)
+        self.assertIsNone(node.maintenance_reason)
+
+        # Initial setting without the reason
+        node = self.conn.baremetal.set_node_maintenance(node)
+        self.assertTrue(node.is_maintenance)
+        self.assertIsNone(node.maintenance_reason)
+
+        # Updating the reason later
+        node = self.conn.baremetal.set_node_maintenance(node, reason)
+        self.assertTrue(node.is_maintenance)
+        self.assertEqual(reason, node.maintenance_reason)
+
+        # Removing the reason later
+        node = self.conn.baremetal.set_node_maintenance(node)
+        self.assertTrue(node.is_maintenance)
+        self.assertIsNone(node.maintenance_reason)
+
+        # Unsetting maintenance
+        node = self.conn.baremetal.unset_node_maintenance(node)
+        self.assertFalse(node.is_maintenance)
+        self.assertIsNone(node.maintenance_reason)
+
+        # Initial setting with the reason
+        node = self.conn.baremetal.set_node_maintenance(node, reason)
+        self.assertTrue(node.is_maintenance)
+        self.assertEqual(reason, node.maintenance_reason)
+
+    def test_maintenance_via_update(self):
+        reason = "Prepating for taking over the world"
+
+        node = self.create_node()
+
+        # Initial setting without the reason
+        node = self.conn.baremetal.update_node(node, is_maintenance=True)
+        self.assertTrue(node.is_maintenance)
+        self.assertIsNone(node.maintenance_reason)
+
+        # Make sure the change has effect on the remote side.
+        node = self.conn.baremetal.get_node(node.id)
+        self.assertTrue(node.is_maintenance)
+        self.assertIsNone(node.maintenance_reason)
+
+        # Updating the reason later
+        node = self.conn.baremetal.update_node(node, maintenance_reason=reason)
+        self.assertTrue(node.is_maintenance)
+        self.assertEqual(reason, node.maintenance_reason)
+
+        # Make sure the change has effect on the remote side.
+        node = self.conn.baremetal.get_node(node.id)
+        self.assertTrue(node.is_maintenance)
+        self.assertEqual(reason, node.maintenance_reason)
+
+        # Unsetting maintenance
+        node = self.conn.baremetal.update_node(node, is_maintenance=False)
+        self.assertFalse(node.is_maintenance)
+        self.assertIsNone(node.maintenance_reason)
+
+        # Make sure the change has effect on the remote side.
+        node = self.conn.baremetal.get_node(node.id)
+        self.assertFalse(node.is_maintenance)
+        self.assertIsNone(node.maintenance_reason)
+
+        # Initial setting with the reason
+        node = self.conn.baremetal.update_node(node, is_maintenance=True,
+                                               maintenance_reason=reason)
+        self.assertTrue(node.is_maintenance)
+        self.assertEqual(reason, node.maintenance_reason)
+
+        # Make sure the change has effect on the remote side.
+        node = self.conn.baremetal.get_node(node.id)
+        self.assertTrue(node.is_maintenance)
+        self.assertEqual(reason, node.maintenance_reason)
+
 
 class TestBareMetalNodeFields(base.BaseBaremetalTest):
 

openstack/tests/unit/baremetal/v1/test_node.py

@@ -562,3 +562,122 @@ class TestNodeSetPowerState(base.TestCase):
             headers=mock.ANY,
             microversion='1.27',
             retriable_status_codes=_common.RETRIABLE_STATUS_CODES)
+
+
+@mock.patch.object(exceptions, 'raise_from_response', mock.Mock())
+@mock.patch.object(node.Node, '_translate_response', mock.Mock())
+@mock.patch.object(node.Node, '_get_session', lambda self, x: x)
+class TestNodeMaintenance(base.TestCase):
+
+    def setUp(self):
+        super(TestNodeMaintenance, self).setUp()
+        self.node = node.Node.existing(**FAKE)
+        self.session = mock.Mock(spec=adapter.Adapter,
+                                 default_microversion='1.1',
+                                 retriable_status_codes=None)
+
+    def test_set(self):
+        self.node.set_maintenance(self.session)
+        self.session.put.assert_called_once_with(
+            'nodes/%s/maintenance' % self.node.id,
+            json={'reason': None},
+            headers=mock.ANY,
+            microversion=mock.ANY)
+
+    def test_set_with_reason(self):
+        self.node.set_maintenance(self.session, 'No work on Monday')
+        self.session.put.assert_called_once_with(
+            'nodes/%s/maintenance' % self.node.id,
+            json={'reason': 'No work on Monday'},
+            headers=mock.ANY,
+            microversion=mock.ANY)
+
+    def test_unset(self):
+        self.node.unset_maintenance(self.session)
+        self.session.delete.assert_called_once_with(
+            'nodes/%s/maintenance' % self.node.id,
+            json=None,
+            headers=mock.ANY,
+            microversion=mock.ANY)
+
+    def test_set_via_update(self):
+        self.node.is_maintenance = True
+        self.node.commit(self.session)
+        self.session.put.assert_called_once_with(
+            'nodes/%s/maintenance' % self.node.id,
+            json={'reason': None},
+            headers=mock.ANY,
+            microversion=mock.ANY)
+
+        self.assertFalse(self.session.patch.called)
+
+    def test_set_with_reason_via_update(self):
+        self.node.is_maintenance = True
+        self.node.maintenance_reason = 'No work on Monday'
+        self.node.commit(self.session)
+        self.session.put.assert_called_once_with(
+            'nodes/%s/maintenance' % self.node.id,
+            json={'reason': 'No work on Monday'},
+            headers=mock.ANY,
+            microversion=mock.ANY)
+        self.assertFalse(self.session.patch.called)
+
+    def test_set_with_other_fields(self):
+        self.node.is_maintenance = True
+        self.node.name = 'lazy-3000'
+        self.node.commit(self.session)
+        self.session.put.assert_called_once_with(
+            'nodes/%s/maintenance' % self.node.id,
+            json={'reason': None},
+            headers=mock.ANY,
+            microversion=mock.ANY)
+
+        self.session.patch.assert_called_once_with(
+            'nodes/%s' % self.node.id,
+            json=[{'path': '/name', 'op': 'replace', 'value': 'lazy-3000'}],
+            headers=mock.ANY,
+            microversion=mock.ANY)
+
+    def test_set_with_reason_and_other_fields(self):
+        self.node.is_maintenance = True
+        self.node.maintenance_reason = 'No work on Monday'
+        self.node.name = 'lazy-3000'
+        self.node.commit(self.session)
+        self.session.put.assert_called_once_with(
+            'nodes/%s/maintenance' % self.node.id,
+            json={'reason': 'No work on Monday'},
+            headers=mock.ANY,
+            microversion=mock.ANY)
+
+        self.session.patch.assert_called_once_with(
+            'nodes/%s' % self.node.id,
+            json=[{'path': '/name', 'op': 'replace', 'value': 'lazy-3000'}],
+            headers=mock.ANY,
+            microversion=mock.ANY)
+
+    def test_no_reason_without_maintenance(self):
+        self.node.maintenance_reason = 'Can I?'
+        self.assertRaises(ValueError, self.node.commit, self.session)
+        self.assertFalse(self.session.put.called)
+        self.assertFalse(self.session.patch.called)
+
+    def test_set_unset_maintenance(self):
+        self.node.is_maintenance = True
+        self.node.maintenance_reason = 'No work on Monday'
+        self.node.commit(self.session)
+
+        self.session.put.assert_called_once_with(
+            'nodes/%s/maintenance' % self.node.id,
+            json={'reason': 'No work on Monday'},
+            headers=mock.ANY,
+            microversion=mock.ANY)
+
+        self.node.is_maintenance = False
+        self.node.commit(self.session)
+        self.assertIsNone(self.node.maintenance_reason)
+
+        self.session.delete.assert_called_once_with(
+            'nodes/%s/maintenance' % self.node.id,
+            json=None,
+            headers=mock.ANY,
+            microversion=mock.ANY)

openstack/tests/unit/cloud/test_baremetal_node.py

@@ -592,6 +592,12 @@ class TestBaremetalNode(base.IronicTestCase):
                     append=[self.fake_baremetal_node['uuid'],
                             'maintenance']),
                 validate=dict(json={'reason': 'no reason'})),
+            dict(
+                method='GET',
+                uri=self.get_mock_url(
+                    resource='nodes',
+                    append=[self.fake_baremetal_node['uuid']]),
+                json=self.fake_baremetal_node),
         ])
         self.cloud.set_machine_maintenance_state(
             self.fake_baremetal_node['uuid'], True, reason='no reason')
@@ -606,6 +612,12 @@ class TestBaremetalNode(base.IronicTestCase):
                     resource='nodes',
                     append=[self.fake_baremetal_node['uuid'],
                             'maintenance'])),
+            dict(
+                method='GET',
+                uri=self.get_mock_url(
+                    resource='nodes',
+                    append=[self.fake_baremetal_node['uuid']]),
+                json=self.fake_baremetal_node),
         ])
         self.cloud.set_machine_maintenance_state(
             self.fake_baremetal_node['uuid'], False)
@@ -620,6 +632,12 @@ class TestBaremetalNode(base.IronicTestCase):
                     resource='nodes',
                     append=[self.fake_baremetal_node['uuid'],
                             'maintenance'])),
+            dict(
+                method='GET',
+                uri=self.get_mock_url(
+                    resource='nodes',
+                    append=[self.fake_baremetal_node['uuid']]),
+                json=self.fake_baremetal_node),
         ])
         self.cloud.remove_machine_from_maintenance(
             self.fake_baremetal_node['uuid'])

releasenotes/notes/baremetal-maintenance-5cb95c6d898d4d72.yaml

@@ -0,0 +1,4 @@
+---
+features:
+  - |
+    Implements updating the baremetal Node's ``maintenance_reason``.