Correct update operations for baremetal

The bare metal service only accepts JSON patch format for updating
resources. However, we try to sent the whole bodies. This change
introduces support for JSON patch into the base Resource.

The Image resource, also using JSON patch, is not updated here,
because it may end up a breaking change.

Change-Id: I4c7866a1246f9e1d8025f5bde1e1930b5d1e2122

openstack/baremetal/v1/chassis.py

@@ -27,6 +27,7 @@ class Chassis(resource.Resource):
     allow_delete = True
     allow_list = True
     commit_method = 'PATCH'
+    commit_jsonpatch = True
 
     _query_mapping = resource.QueryParameters(
         'fields'

openstack/baremetal/v1/node.py

@@ -34,6 +34,7 @@ class Node(resource.Resource):
     allow_delete = True
     allow_list = True
     commit_method = 'PATCH'
+    commit_jsonpatch = True
 
     _query_mapping = resource.QueryParameters(
         'associated', 'driver', 'fields', 'provision_state', 'resource_class',

openstack/baremetal/v1/port.py

@@ -27,6 +27,7 @@ class Port(resource.Resource):
     allow_delete = True
     allow_list = True
     commit_method = 'PATCH'
+    commit_jsonpatch = True
 
     _query_mapping = resource.QueryParameters(
         'fields'

openstack/baremetal/v1/port_group.py

@@ -27,6 +27,7 @@ class PortGroup(resource.Resource):
     allow_delete = True
     allow_list = True
     commit_method = 'PATCH'
+    commit_jsonpatch = True
 
     _query_mapping = resource.QueryParameters(
         'node', 'address', 'fields',

openstack/resource.py

@@ -34,6 +34,7 @@ and then returned to the caller.
 import collections
 import itertools
 
+import jsonpatch
 from keystoneauth1 import adapter
 from keystoneauth1 import discover
 import munch
@@ -364,6 +365,8 @@ class Resource(object):
     commit_method = "PUT"
     #: Method for creating a resource (POST, PUT)
     create_method = "POST"
+    #: Whether commit uses JSON patch format.
+    commit_jsonpatch = False
 
     #: Do calls for this resource require an id
     requires_id = True
@@ -382,6 +385,7 @@ class Resource(object):
     _header = None
     _uri = None
     _computed = None
+    _original_body = None
 
     def __init__(self, _synchronized=False, connection=None, **attrs):
         """The base resource
@@ -418,6 +422,9 @@ class Resource(object):
         self._computed = _ComponentManager(
             attributes=computed,
             synchronized=_synchronized)
+        if self.commit_jsonpatch:
+            # We need the original body to compare against
+            self._original_body = self._body.attributes.copy()
 
     def __repr__(self):
         pairs = [
@@ -756,7 +763,8 @@ class Resource(object):
         return munch.Munch(self.to_dict(body=True, headers=False,
                                         original_names=True))
 
-    def _prepare_request(self, requires_id=None, prepend_key=False):
+    def _prepare_request(self, requires_id=None, prepend_key=False,
+                         patch=False):
         """Prepare a request to be sent to the server
 
         Create operations don't require an ID, but all others do,
@@ -765,6 +773,7 @@ class Resource(object):
         their bodies to be contained within an dict -- if the
         instance contains a resource_key and prepend_key=True,
         the body will be wrapped in a dict with that key.
+        If patch=True, a JSON patch is prepared instead of the full body.
 
         Return a _Request object that contains the constructed URI
         as well a body and headers that are ready to send.
@@ -773,9 +782,13 @@ class Resource(object):
         if requires_id is None:
             requires_id = self.requires_id
 
-        body = self._body.dirty
+        if patch:
-        if prepend_key and self.resource_key is not None:
+            new = self._body.attributes
-            body = {self.resource_key: body}
+            body = jsonpatch.make_patch(self._original_body, new).patch
+        else:
+            body = self._body.dirty
+            if prepend_key and self.resource_key is not None:
+                body = {self.resource_key: body}
 
         # TODO(mordred) Ensure headers have string values better than this
         headers = {}
@@ -815,6 +828,9 @@ class Resource(object):
             body = self._consume_body_attrs(body)
             self._body.attributes.update(body)
             self._body.clean()
+            if self.commit_jsonpatch:
+                # We need the original body to compare against
+                self._original_body = body.copy()
 
         headers = self._consume_header_attrs(response.headers)
         self._header.attributes.update(headers)
@@ -1029,7 +1045,13 @@ class Resource(object):
         if not self.allow_commit:
             raise exceptions.MethodNotSupported(self, "commit")
 
-        request = self._prepare_request(prepend_key=prepend_key)
+        # Avoid providing patch unconditionally to avoid breaking subclasses
+        # without it.
+        kwargs = {}
+        if self.commit_jsonpatch:
+            kwargs['patch'] = True
+
+        request = self._prepare_request(prepend_key=prepend_key, **kwargs)
         session = self._get_session(session)
         microversion = self._get_microversion_for(session, 'commit')
 

openstack/tests/functional/baremetal/test_baremetal_chassis.py

@@ -27,6 +27,16 @@ class TestBareMetalChassis(base.BaseBaremetalTest):
         self.assertRaises(exceptions.NotFoundException,
                           self.conn.baremetal.get_chassis, chassis.id)
 
+    def test_chassis_update(self):
+        chassis = self.create_chassis()
+        chassis.extra = {'answer': 42}
+
+        chassis = self.conn.baremetal.update_chassis(chassis)
+        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.NotFoundException,

openstack/tests/functional/baremetal/test_baremetal_node.py

@@ -10,6 +10,8 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import uuid
+
 from openstack import exceptions
 from openstack.tests.functional.baremetal import base
 
@@ -39,6 +41,30 @@ class TestBareMetalNode(base.BaseBaremetalTest):
         self.assertRaises(exceptions.NotFoundException,
                           self.conn.baremetal.get_node, self.node_id)
 
+    def test_node_update(self):
+        node = self.create_node(name='node-name', extra={'foo': 'bar'})
+        node.name = 'new-name'
+        node.extra = {'answer': 42}
+        instance_uuid = str(uuid.uuid4())
+
+        node = self.conn.baremetal.update_node(node,
+                                               instance_uuid=instance_uuid)
+        self.assertEqual('new-name', node.name)
+        self.assertEqual({'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({'answer': 42}, node.extra)
+        self.assertEqual(instance_uuid, node.instance_id)
+
+        node = self.conn.baremetal.update_node(node,
+                                               instance_uuid=None)
+        self.assertIsNone(node.instance_id)
+
+        node = self.conn.baremetal.get_node('new-name')
+        self.assertIsNone(node.instance_id)
+
     def test_node_create_in_enroll_provide(self):
         node = self.create_node(provision_state='enroll')
         self.node_id = node.id
@@ -66,5 +92,8 @@ class TestBareMetalNode(base.BaseBaremetalTest):
         self.assertRaises(exceptions.NotFoundException,
                           self.conn.baremetal.delete_node, uuid,
                           ignore_missing=False)
+        self.assertRaises(exceptions.NotFoundException,
+                          self.conn.baremetal.update_node, uuid,
+                          name='new-name')
         self.assertIsNone(self.conn.baremetal.find_node(uuid))
         self.assertIsNone(self.conn.baremetal.delete_node(uuid))

openstack/tests/functional/baremetal/test_baremetal_port.py

@@ -36,6 +36,19 @@ class TestBareMetalPort(base.BaseBaremetalTest):
         self.assertRaises(exceptions.NotFoundException,
                           self.conn.baremetal.get_port, port.id)
 
+    def test_port_update(self):
+        port = self.create_port(address='11:22:33:44:55:66')
+        port.address = '66:55:44:33:22:11'
+        port.extra = {'answer': 42}
+
+        port = self.conn.baremetal.update_port(port)
+        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.NotFoundException,
@@ -46,5 +59,8 @@ class TestBareMetalPort(base.BaseBaremetalTest):
         self.assertRaises(exceptions.NotFoundException,
                           self.conn.baremetal.delete_port, uuid,
                           ignore_missing=False)
+        self.assertRaises(exceptions.NotFoundException,
+                          self.conn.baremetal.update_port, uuid,
+                          pxe_enabled=True)
         self.assertIsNone(self.conn.baremetal.find_port(uuid))
         self.assertIsNone(self.conn.baremetal.delete_port(uuid))

openstack/tests/functional/baremetal/test_baremetal_port_group.py

@@ -34,6 +34,16 @@ class TestBareMetalPortGroup(base.BaseBaremetalTest):
         self.assertRaises(exceptions.NotFoundException,
                           self.conn.baremetal.get_port_group, port_group.id)
 
+    def test_port_group_update(self):
+        port_group = self.create_port_group()
+        port_group.extra = {'answer': 42}
+
+        port_group = self.conn.baremetal.update_port_group(port_group)
+        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.NotFoundException,

openstack/tests/unit/test_resource.py

@@ -864,6 +864,23 @@ class TestResource(base.TestCase):
         self.assertEqual({key: {"x": body_value}}, result.body)
         self.assertEqual({"y": header_value}, result.headers)
 
+    def test__prepare_request_with_patch(self):
+        class Test(resource.Resource):
+            commit_jsonpatch = True
+            base_path = "/something"
+            x = resource.Body("x")
+            y = resource.Body("y")
+
+        the_id = "id"
+        sot = Test(id=the_id, x=1, y=2)
+        sot.x = 3
+
+        result = sot._prepare_request(requires_id=True, patch=True)
+
+        self.assertEqual("something/id", result.url)
+        self.assertEqual([{'op': 'replace', 'path': '/x', 'value': 3}],
+                         result.body)
+
     def test__translate_response_no_body(self):
         class Test(resource.Resource):
             attr = resource.Header("attr")

releasenotes/notes/baremetal-update-80effb38aae8e02d.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Correct updating bare metal resources. Previously an incorrect body used
+    to be sent.