diff --git a/doc/source/user/proxies/baremetal.rst b/doc/source/user/proxies/baremetal.rst index cace96a7a..d877f400a 100644 --- a/doc/source/user/proxies/baremetal.rst +++ b/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 ^^^^^^^^^^^^^^^ diff --git a/openstack/baremetal/v1/_proxy.py b/openstack/baremetal/v1/_proxy.py index 77cc77a32..e24b2cf43 100644 --- a/openstack/baremetal/v1/_proxy.py +++ b/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. diff --git a/openstack/baremetal/v1/node.py b/openstack/baremetal/v1/node.py index 4a2471401..21c41d3e3 100644 --- a/openstack/baremetal/v1/node.py +++ b/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 diff --git a/openstack/cloud/openstackcloud.py b/openstack/cloud/openstackcloud.py index 5037cb904..83fdfcddb 100755 --- a/openstack/cloud/openstackcloud.py +++ b/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 diff --git a/openstack/resource.py b/openstack/resource.py index 718037020..acd227301 100644 --- a/openstack/resource.py +++ b/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: diff --git a/openstack/tests/functional/baremetal/test_baremetal_node.py b/openstack/tests/functional/baremetal/test_baremetal_node.py index 359cd9a07..c38fbf52a 100644 --- a/openstack/tests/functional/baremetal/test_baremetal_node.py +++ b/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): diff --git a/openstack/tests/unit/baremetal/v1/test_node.py b/openstack/tests/unit/baremetal/v1/test_node.py index 5db11397e..765250ef7 100644 --- a/openstack/tests/unit/baremetal/v1/test_node.py +++ b/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) diff --git a/openstack/tests/unit/cloud/test_baremetal_node.py b/openstack/tests/unit/cloud/test_baremetal_node.py index e40262897..61882ea48 100644 --- a/openstack/tests/unit/cloud/test_baremetal_node.py +++ b/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']) diff --git a/releasenotes/notes/baremetal-maintenance-5cb95c6d898d4d72.yaml b/releasenotes/notes/baremetal-maintenance-5cb95c6d898d4d72.yaml new file mode 100644 index 000000000..4a309b9fd --- /dev/null +++ b/releasenotes/notes/baremetal-maintenance-5cb95c6d898d4d72.yaml @@ -0,0 +1,4 @@ +--- +features: + - | + Implements updating the baremetal Node's ``maintenance_reason``.