openstack
/
openstacksdk
Code Issues Proposed changes

Make PATCH a first class operation and support it for baremetal 

The existing pattern of working with ironicclient is to use JSON patch to
update resources. To simplify migration let us support it as well.
Patching is also useful for granular updating of nested structures.

Deprecate two openstackcloud calls that are meaningless wrappers on
top of patch_machine/update_machine.

Change-Id: Idffaf8947f51e5854461808d9d42c576640bec56
This commit is contained in:
Dmitry Tantsur 2019-03-26 13:45:35 +01:00
parent b33079bc9c
commit 9ace77e69e
15 changed files with 304 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
doc/source/user/proxies/baremetal.rst
View File

@ -18,6 +18,7 @@ Node Operations
.. automethod:: openstack.baremetal.v1._proxy.Proxy.create_node
.. automethod:: openstack.baremetal.v1._proxy.Proxy.update_node
.. automethod:: openstack.baremetal.v1._proxy.Proxy.patch_node
.. automethod:: openstack.baremetal.v1._proxy.Proxy.delete_node
.. automethod:: openstack.baremetal.v1._proxy.Proxy.get_node
.. automethod:: openstack.baremetal.v1._proxy.Proxy.find_node
@ -36,6 +37,7 @@ Port Operations
.. automethod:: openstack.baremetal.v1._proxy.Proxy.create_port
.. automethod:: openstack.baremetal.v1._proxy.Proxy.update_port
.. automethod:: openstack.baremetal.v1._proxy.Proxy.patch_port
.. automethod:: openstack.baremetal.v1._proxy.Proxy.delete_port
.. automethod:: openstack.baremetal.v1._proxy.Proxy.get_port
.. automethod:: openstack.baremetal.v1._proxy.Proxy.find_port
@ -47,6 +49,7 @@ Port Group Operations
.. automethod:: openstack.baremetal.v1._proxy.Proxy.create_port_group
.. automethod:: openstack.baremetal.v1._proxy.Proxy.update_port_group
.. automethod:: openstack.baremetal.v1._proxy.Proxy.patch_port_group
.. automethod:: openstack.baremetal.v1._proxy.Proxy.delete_port_group
.. automethod:: openstack.baremetal.v1._proxy.Proxy.get_port_group
.. automethod:: openstack.baremetal.v1._proxy.Proxy.find_port_group
@ -65,6 +68,7 @@ Chassis Operations
.. automethod:: openstack.baremetal.v1._proxy.Proxy.create_chassis
.. automethod:: openstack.baremetal.v1._proxy.Proxy.update_chassis
.. automethod:: openstack.baremetal.v1._proxy.Proxy.patch_chassis
.. automethod:: openstack.baremetal.v1._proxy.Proxy.delete_chassis
.. automethod:: openstack.baremetal.v1._proxy.Proxy.get_chassis
.. automethod:: openstack.baremetal.v1._proxy.Proxy.find_chassis

62
openstack/baremetal/v1/_proxy.py
View File

@ -114,6 +114,18 @@ class Proxy(proxy.Proxy):
"""
return self._update(_chassis.Chassis, chassis, **attrs)
def patch_chassis(self, chassis, patch):
"""Apply a JSON patch to the chassis.
:param chassis: The value can be the ID of a chassis or a
:class:`~openstack.baremetal.v1.chassis.Chassis` instance.
:param patch: JSON patch to apply.
:returns: The updated chassis.
:rtype: :class:`~openstack.baremetal.v1.chassis.Chassis`
"""
return self._get_resource(_chassis.Chassis, chassis).patch(self, patch)
def delete_chassis(self, chassis, ignore_missing=True):
"""Delete a chassis.
@ -246,8 +258,8 @@ class Proxy(proxy.Proxy):
def update_node(self, node, retry_on_conflict=True, **attrs):
"""Update a node.
:param chassis: Either the name or the ID of a node or an instance
of :class:`~openstack.baremetal.v1.node.Node`.
:param node: The value can be the name or ID of a node or a
:class:`~openstack.baremetal.v1.node.Node` instance.
:param bool retry_on_conflict: Whether to retry HTTP CONFLICT error.
Most of the time it can be retried, since it is caused by the node
being locked. However, when setting ``instance_id``, this is
@ -261,6 +273,23 @@ class Proxy(proxy.Proxy):
res = self._get_resource(_node.Node, node, **attrs)
return res.commit(self, retry_on_conflict=retry_on_conflict)
def patch_node(self, node, patch, retry_on_conflict=True):
"""Apply a JSON patch to the node.
:param node: The value can be the name or ID of a node or a
:class:`~openstack.baremetal.v1.node.Node` instance.
:param patch: JSON patch to apply.
:param bool retry_on_conflict: Whether to retry HTTP CONFLICT error.
Most of the time it can be retried, since it is caused by the node
being locked. However, when setting ``instance_id``, this is
a normal code and should not be retried.
:returns: The updated node.
:rtype: :class:`~openstack.baremetal.v1.node.Node`
"""
res = self._get_resource(_node.Node, node)
return res.patch(self, patch, retry_on_conflict=retry_on_conflict)
def set_node_provision_state(self, node, target, config_drive=None,
clean_steps=None, rescue_password=None,
wait=False, timeout=None):
@ -527,7 +556,7 @@ class Proxy(proxy.Proxy):
def update_port(self, port, **attrs):
"""Update a port.
:param chassis: Either the ID of a port or an instance
:param port: Either the ID of a port or an instance
of :class:`~openstack.baremetal.v1.port.Port`.
:param dict attrs: The attributes to update on the port represented
by the ``port`` parameter.
@ -537,6 +566,18 @@ class Proxy(proxy.Proxy):
"""
return self._update(_port.Port, port, **attrs)
def patch_port(self, port, patch):
"""Apply a JSON patch to the port.
:param port: The value can be the ID of a port or a
:class:`~openstack.baremetal.v1.port.Port` instance.
:param patch: JSON patch to apply.
:returns: The updated port.
:rtype: :class:`~openstack.baremetal.v1.port.Port`
"""
return self._get_resource(_port.Port, port).patch(self, patch)
def delete_port(self, port, ignore_missing=True):
"""Delete a port.
@ -638,7 +679,7 @@ class Proxy(proxy.Proxy):
def update_port_group(self, port_group, **attrs):
"""Update a port group.
:param chassis: Either the name or the ID of a port group or
:param port_group: Either the name or the ID of a port group or
an instance of
:class:`~openstack.baremetal.v1.port_group.PortGroup`.
:param dict attrs: The attributes to update on the port group
@ -649,6 +690,19 @@ class Proxy(proxy.Proxy):
"""
return self._update(_portgroup.PortGroup, port_group, **attrs)
def patch_port_group(self, port_group, patch):
"""Apply a JSON patch to the port_group.
:param port_group: The value can be the ID of a port group or a
:class:`~openstack.baremetal.v1.port_group.PortGroup` instance.
:param patch: JSON patch to apply.
:returns: The updated port group.
:rtype: :class:`~openstack.baremetal.v1.port_group.PortGroup`
"""
res = self._get_resource(_portgroup.PortGroup, port_group)
return res.patch(self, patch)
def delete_port_group(self, port_group, ignore_missing=True):
"""Delete a port group.

1
openstack/baremetal/v1/chassis.py
View File

@ -28,6 +28,7 @@ class Chassis(_common.ListMixin, resource.Resource):
allow_commit = True
allow_delete = True
allow_list = True
allow_patch = True
commit_method = 'PATCH'
commit_jsonpatch = True

1
openstack/baremetal/v1/node.py
View File

@ -45,6 +45,7 @@ class Node(_common.ListMixin, resource.Resource):
allow_commit = True
allow_delete = True
allow_list = True
allow_patch = True
commit_method = 'PATCH'
commit_jsonpatch = True

1
openstack/baremetal/v1/port.py
View File

@ -25,6 +25,7 @@ class Port(_common.ListMixin, resource.Resource):
allow_commit = True
allow_delete = True
allow_list = True
allow_patch = True
commit_method = 'PATCH'
commit_jsonpatch = True

1
openstack/baremetal/v1/port_group.py
View File

@ -25,6 +25,7 @@ class PortGroup(_common.ListMixin, resource.Resource):
allow_commit = True
allow_delete = True
allow_list = True
allow_patch = True
commit_method = 'PATCH'
commit_jsonpatch = True

34
openstack/cloud/_baremetal.py
View File

@ -434,17 +434,8 @@ class BaremetalCloudMixin(_normalize.Normalizer):
:returns: ``munch.Munch`` representing the newly updated node.
"""
node = self.baremetal.get_node(name_or_id)
microversion = node._get_microversion_for(self._baremetal_client,
'commit')
msg = ("Error updating machine via patch operation on node "
"{node}".format(node=name_or_id))
url = '/nodes/{node_id}'.format(node_id=node.id)
return self._normalize_machine(
self._baremetal_client.patch(url,
json=patch,
microversion=microversion,
error_message=msg))
self.baremetal.patch_node(name_or_id, patch))
def update_machine(self, name_or_id, **attrs):
"""Update a machine with new configuration information
@ -687,22 +678,17 @@ class BaremetalCloudMixin(_normalize.Normalizer):
uuid, 'deleted', wait=wait, timeout=timeout)
def set_node_instance_info(self, uuid, patch):
msg = ("Error updating machine via patch operation on node "
"{node}".format(node=uuid))
url = '/nodes/{node_id}'.format(node_id=uuid)
return self._baremetal_client.patch(url,
json=patch,
error_message=msg)
warnings.warn("The set_node_instance_info call is deprecated, "
"use patch_machine or update_machine instead",
DeprecationWarning)
return self.patch_machine(uuid, patch)
def purge_node_instance_info(self, uuid):
patch = []
patch.append({'op': 'remove', 'path': '/instance_info'})
msg = ("Error updating machine via patch operation on node "
"{node}".format(node=uuid))
url = '/nodes/{node_id}'.format(node_id=uuid)
return self._baremetal_client.patch(url,
json=patch,
error_message=msg)
warnings.warn("The purge_node_instance_info call is deprecated, "
"use patch_machine or update_machine instead",
DeprecationWarning)
return self.patch_machine(uuid,
dict(path='/instance_info', op='remove'))
def wait_for_baremetal_node_lock(self, node, timeout=30):
"""Wait for a baremetal node to have no lock.

121
openstack/resource.py
View File

@ -387,6 +387,8 @@ class Resource(dict):
allow_list = False
#: Allow head operation for this resource.
allow_head = False
#: Allow patch operation for this resource.
allow_patch = False
# TODO(mordred) Unused - here for transition with OSC. Remove once
# OSC no longer checks for allow_get
@ -455,7 +457,7 @@ class Resource(dict):
self._computed = _ComponentManager(
attributes=computed,
synchronized=_synchronized)
if self.commit_jsonpatch:
if self.commit_jsonpatch or self.allow_patch:
# We need the original body to compare against
if _synchronized:
self._original_body = self._body.attributes.copy()
@ -698,7 +700,7 @@ class Resource(dict):
def _clean_body_attrs(self, attrs):
"""Mark the attributes as up-to-date."""
self._body.clean(only=attrs)
if self.commit_jsonpatch:
if self.commit_jsonpatch or self.allow_patch:
for attr in attrs:
if attr in self._body:
self._original_body[attr] = self._body[attr]
@ -976,7 +978,7 @@ class Resource(dict):
body = self._consume_body_attrs(body)
self._body.attributes.update(body)
self._body.clean()
if self.commit_jsonpatch:
if self.commit_jsonpatch or self.allow_patch:
# We need the original body to compare against
self._original_body = body.copy()
@ -1037,11 +1039,11 @@ class Resource(dict):
Subclasses can override this method if more complex logic is needed.
:param session: :class`keystoneauth1.adapter.Adapter`
:param action: One of "fetch", "commit", "create", "delete". Unused in
the base implementation.
:param action: One of "fetch", "commit", "create", "delete", "patch".
Unused in the base implementation.
:return: microversion as string or ``None``
"""
if action not in ('fetch', 'commit', 'create', 'delete'):
if action not in ('fetch', 'commit', 'create', 'delete', 'patch'):
raise ValueError('Invalid action: %s' % action)
return self._get_microversion_for_list(session)
@ -1234,6 +1236,14 @@ class Resource(dict):
request = self._prepare_request(prepend_key=prepend_key,
base_path=base_path,
**kwargs)
microversion = self._get_microversion_for(session, 'commit')
return self._commit(session, request, self.commit_method, microversion,
has_body=has_body,
retry_on_conflict=retry_on_conflict)
def _commit(self, session, request, method, microversion, has_body=True,
retry_on_conflict=None):
session = self._get_session(session)
kwargs = {}
@ -1245,28 +1255,95 @@ class Resource(dict):
# overriding it via an explicit retry_on_conflict=False.
kwargs['retriable_status_codes'] = retriable_status_codes - {409}
microversion = self._get_microversion_for(session, 'commit')
if self.commit_method == 'PATCH':
response = session.patch(
request.url, json=request.body, headers=request.headers,
microversion=microversion, **kwargs)
elif self.commit_method == 'POST':
response = session.post(
request.url, json=request.body, headers=request.headers,
microversion=microversion, **kwargs)
elif self.commit_method == 'PUT':
response = session.put(
request.url, json=request.body, headers=request.headers,
microversion=microversion, **kwargs)
else:
try:
call = getattr(session, method.lower())
except AttributeError:
raise exceptions.ResourceFailure(
msg="Invalid commit method: %s" % self.commit_method)
msg="Invalid commit method: %s" % method)
response = call(request.url, json=request.body,
headers=request.headers, microversion=microversion,
**kwargs)
self.microversion = microversion
self._translate_response(response, has_body=has_body)
return self
def _convert_patch(self, patch):
if not isinstance(patch, list):
patch = [patch]
converted = []
for item in patch:
try:
path = item['path']
parts = path.lstrip('/').split('/', 1)
field = parts[0]
except (KeyError, IndexError):
raise ValueError("Malformed or missing path in %s" % item)
try:
component = getattr(self.__class__, field)
except AttributeError:
server_field = field
else:
server_field = component.name
if len(parts) > 1:
new_path = '/%s/%s' % (server_field, parts[1])
else:
new_path = '/%s' % server_field
converted.append(dict(item, path=new_path))
return converted
def patch(self, session, patch=None, prepend_key=True, has_body=True,
retry_on_conflict=None, base_path=None):
"""Patch the remote resource.
Allows modifying the resource by providing a list of JSON patches to
apply to it. The patches can use both the original (server-side) and
SDK field names.
:param session: The session to use for making this request.
:type session: :class:`~keystoneauth1.adapter.Adapter`
:param patch: Additional JSON patch as a list or one patch item.
If provided, it is applied on top of any changes to the
current resource.
:param prepend_key: A boolean indicating whether the resource_key
should be prepended in a resource update request.
Default to True.
:param bool retry_on_conflict: Whether to enable retries on HTTP
CONFLICT (409). Value of ``None`` leaves
the `Adapter` defaults.
:param str base_path: Base part of the URI for modifying resources, if
different from
:data:`~openstack.resource.Resource.base_path`.
:return: This :class:`Resource` instance.
:raises: :exc:`~openstack.exceptions.MethodNotSupported` if
:data:`Resource.allow_patch` is not set to ``True``.
"""
# The id cannot be dirty for an commit
self._body._dirty.discard("id")
# Only try to update if we actually have anything to commit.
if not patch and not self.requires_commit:
return self
if not self.allow_patch:
raise exceptions.MethodNotSupported(self, "patch")
request = self._prepare_request(prepend_key=prepend_key,
base_path=base_path, patch=True)
microversion = self._get_microversion_for(session, 'patch')
if patch:
request.body += self._convert_patch(patch)
return self._commit(session, request, 'PATCH', microversion,
has_body=has_body,
retry_on_conflict=retry_on_conflict)
def delete(self, session, error_message=None):
"""Delete the remote resource based on this instance.

10
openstack/tests/functional/baremetal/test_baremetal_chassis.py
View File

@ -37,6 +37,16 @@ class TestBareMetalChassis(base.BaseBaremetalTest):
chassis = self.conn.baremetal.get_chassis(chassis.id)
self.assertEqual({'answer': 42}, chassis.extra)
def test_chassis_patch(self):
chassis = self.create_chassis()
chassis = self.conn.baremetal.patch_chassis(
chassis, dict(path='/extra/answer', op='add', value=42))
self.assertEqual({'answer': 42}, chassis.extra)
chassis = self.conn.baremetal.get_chassis(chassis.id)
self.assertEqual({'answer': 42}, chassis.extra)
def test_chassis_negative_non_existing(self):
uuid = "5c9dcd04-2073-49bc-9618-99ae634d8971"
self.assertRaises(exceptions.ResourceNotFound,

29
openstack/tests/functional/baremetal/test_baremetal_node.py
View File

@ -86,6 +86,35 @@ class TestBareMetalNode(base.BaseBaremetalTest):
node = self.conn.baremetal.get_node('node-name')
self.assertIsNone(node.instance_id)
def test_node_patch(self):
node = self.create_node(name='node-name', extra={'foo': 'bar'})
node.name = 'new-name'
instance_uuid = str(uuid.uuid4())
node = self.conn.baremetal.patch_node(
node,
[dict(path='/instance_id', op='replace', value=instance_uuid),
dict(path='/extra/answer', op='add', value=42)])
self.assertEqual('new-name', node.name)
self.assertEqual({'foo': 'bar', 'answer': 42}, node.extra)
self.assertEqual(instance_uuid, node.instance_id)
node = self.conn.baremetal.get_node('new-name')
self.assertEqual('new-name', node.name)
self.assertEqual({'foo': 'bar', 'answer': 42}, node.extra)
self.assertEqual(instance_uuid, node.instance_id)
node = self.conn.baremetal.patch_node(
node,
[dict(path='/instance_id', op='remove'),
dict(path='/extra/answer', op='remove')])
self.assertIsNone(node.instance_id)
self.assertNotIn('answer', node.extra)
node = self.conn.baremetal.get_node('new-name')
self.assertIsNone(node.instance_id)
self.assertNotIn('answer', node.extra)
def test_node_list_update_delete(self):
self.create_node(name='node-name', extra={'foo': 'bar'})
node = next(n for n in

13
openstack/tests/functional/baremetal/test_baremetal_port.py
View File

@ -77,6 +77,19 @@ class TestBareMetalPort(base.BaseBaremetalTest):
self.assertEqual('66:55:44:33:22:11', port.address)
self.assertEqual({'answer': 42}, port.extra)
def test_port_patch(self):
port = self.create_port(address='11:22:33:44:55:66')
port.address = '66:55:44:33:22:11'
port = self.conn.baremetal.patch_port(
port, dict(path='/extra/answer', op='add', value=42))
self.assertEqual('66:55:44:33:22:11', port.address)
self.assertEqual({'answer': 42}, port.extra)
port = self.conn.baremetal.get_port(port.id)
self.assertEqual('66:55:44:33:22:11', port.address)
self.assertEqual({'answer': 42}, port.extra)
def test_port_negative_non_existing(self):
uuid = "5c9dcd04-2073-49bc-9618-99ae634d8971"
self.assertRaises(exceptions.ResourceNotFound,

10
openstack/tests/functional/baremetal/test_baremetal_port_group.py
View File

@ -72,6 +72,16 @@ class TestBareMetalPortGroup(base.BaseBaremetalTest):
port_group = self.conn.baremetal.get_port_group(port_group.id)
self.assertEqual({'answer': 42}, port_group.extra)
def test_port_group_patch(self):
port_group = self.create_port_group()
port_group = self.conn.baremetal.patch_port_group(
port_group, dict(path='/extra/answer', op='add', value=42))
self.assertEqual({'answer': 42}, port_group.extra)
port_group = self.conn.baremetal.get_port_group(port_group.id)
self.assertEqual({'answer': 42}, port_group.extra)
def test_port_group_negative_non_existing(self):
uuid = "5c9dcd04-2073-49bc-9618-99ae634d8971"
self.assertRaises(exceptions.ResourceNotFound,

5
openstack/tests/unit/cloud/test_baremetal_node.py
View File

@ -206,11 +206,6 @@ class TestBaremetalNode(base.IronicTestCase):
'path': '/instance_info'}]
self.fake_baremetal_node['instance_info'] = {}
self.register_uris([
dict(method='GET',
uri=self.get_mock_url(
resource='nodes',
append=[self.fake_baremetal_node['uuid']]),
json=self.fake_baremetal_node),
dict(method='PATCH',
uri=self.get_mock_url(
resource='nodes',

54
openstack/tests/unit/test_resource.py
View File

@ -1336,6 +1336,60 @@ class TestResourceActions(base.TestCase):
self.session.put.assert_not_called()
def test_patch_with_sdk_names(self):
class Test(resource.Resource):
allow_patch = True
id = resource.Body('id')
attr = resource.Body('attr')
nested = resource.Body('renamed')
other = resource.Body('other')
test_patch = [{'path': '/attr', 'op': 'replace', 'value': 'new'},
{'path': '/nested/dog', 'op': 'remove'},
{'path': '/nested/cat', 'op': 'add', 'value': 'meow'}]
expected = [{'path': '/attr', 'op': 'replace', 'value': 'new'},
{'path': '/renamed/dog', 'op': 'remove'},
{'path': '/renamed/cat', 'op': 'add', 'value': 'meow'}]
sot = Test.existing(id=1, attr=42, nested={'dog': 'bark'})
sot.patch(self.session, test_patch)
self.session.patch.assert_called_once_with(
'/1', json=expected, headers=mock.ANY, microversion=None)
def test_patch_with_server_names(self):
class Test(resource.Resource):
allow_patch = True
id = resource.Body('id')
attr = resource.Body('attr')
nested = resource.Body('renamed')
other = resource.Body('other')
test_patch = [{'path': '/attr', 'op': 'replace', 'value': 'new'},
{'path': '/renamed/dog', 'op': 'remove'},
{'path': '/renamed/cat', 'op': 'add', 'value': 'meow'}]
sot = Test.existing(id=1, attr=42, nested={'dog': 'bark'})
sot.patch(self.session, test_patch)
self.session.patch.assert_called_once_with(
'/1', json=test_patch, headers=mock.ANY, microversion=None)
def test_patch_with_changed_fields(self):
class Test(resource.Resource):
allow_patch = True
attr = resource.Body('attr')
nested = resource.Body('renamed')
other = resource.Body('other')
sot = Test.existing(id=1, attr=42, nested={'dog': 'bark'})
sot.attr = 'new'
sot.patch(self.session, {'path': '/renamed/dog', 'op': 'remove'})
expected = [{'path': '/attr', 'op': 'replace', 'value': 'new'},
{'path': '/renamed/dog', 'op': 'remove'}]
self.session.patch.assert_called_once_with(
'/1', json=expected, headers=mock.ANY, microversion=None)
def test_delete(self):
result = self.sot.delete(self.session)

13
releasenotes/notes/baremetal-patch-feebd96b1b92f3b9.yaml Normal file
View File

@ -0,0 +1,13 @@
---
features:
- |
Adds support for changing bare metal resources by providing a JSON patch.
Adds the following calls to the bare metal proxy: ``patch_node``,
``patch_port``, ``patch_port_group`` and ``patch_chassis``.
deprecations:
- |
The ``set_node_instance_info`` call is deprecated, use ``patch_machine``
with the same arguments instead.
- |
The ``purge_node_instance_info`` call is deprecated, use ``patch_machine``
or ``update_machine`` instead.