Wire in retries for all baremetal actions

The baremetal API can output HTTP 409 during its normal operation and
503 under load. This change enables retries for them.

Since node update can naturally result in HTTP 409, provide a way
to opt out of retrying on it. Additionally, allow retrying on HTTP 409
for all services via commit().

Change-Id: I765c4066e26706abe5f576c98353a48a17da6f9c

openstack/baremetal/v1/_proxy.py

@@ -11,6 +11,7 @@
 # under the License.
 
 from openstack import _log
+from openstack.baremetal.v1 import _common
 from openstack.baremetal.v1 import chassis as _chassis
 from openstack.baremetal.v1 import driver as _driver
 from openstack.baremetal.v1 import node as _node
@@ -25,6 +26,8 @@ _logger = _log.setup_logging('openstack')
 
 class Proxy(proxy.Proxy):
 
+    retriable_status_codes = _common.RETRIABLE_STATUS_CODES
+
     def chassis(self, details=False, **query):
         """Retrieve a generator of chassis.
 
@@ -238,18 +241,23 @@ class Proxy(proxy.Proxy):
         """
         return self._get(_node.Node, node)
 
-    def update_node(self, node, **attrs):
+    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 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.
         :param dict attrs: The attributes to update on the node represented
             by the ``node`` parameter.
 
         :returns: The updated node.
         :rtype: :class:`~openstack.baremetal.v1.node.Node`
         """
-        return self._update(_node.Node, node, **attrs)
+        res = self._get_resource(_node.Node, node, **attrs)
+        return res.commit(self, retry_on_conflict=retry_on_conflict)
 
     def set_node_provision_state(self, node, target, config_drive=None,
                                  clean_steps=None, rescue_password=None,

openstack/proxy.py

@@ -46,6 +46,20 @@ def _check_resource(strict=False):
 class Proxy(six.with_metaclass(_meta.ProxyMeta, _adapter.OpenStackSDKAdapter)):
     """Represents a service."""
 
+    retriable_status_codes = None
+    """HTTP status codes that should be retried by default.
+
+    The number of retries is defined by the configuration in parameters called
+    ``<service-type>_status_code_retries``.
+    """
+
+    def __init__(self, *args, **kwargs):
+        # NOTE(dtantsur): keystoneauth defaults retriable_status_codes to None,
+        # override it with a class-level value.
+        kwargs.setdefault('retriable_status_codes',
+                          self.retriable_status_codes)
+        super(Proxy, self).__init__(*args, **kwargs)
+
     def _get_resource(self, resource_type, value, **attrs):
         """Get a resource object to work on
 

openstack/resource.py

@@ -1107,7 +1107,8 @@ class Resource(dict):
         self._translate_response(response, has_body=False)
         return self
 
-    def commit(self, session, prepend_key=True, has_body=True):
+    def commit(self, session, prepend_key=True, has_body=True,
+               retry_on_conflict=None):
         """Commit the state of the instance to the remote resource.
 
         :param session: The session to use for making this request.
@@ -1115,6 +1116,9 @@ class Resource(dict):
         :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.
 
         :return: This :class:`Resource` instance.
         :raises: :exc:`~openstack.exceptions.MethodNotSupported` if
@@ -1138,20 +1142,30 @@ class Resource(dict):
 
         request = self._prepare_request(prepend_key=prepend_key, **kwargs)
         session = self._get_session(session)
+
+        kwargs = {}
+        retriable_status_codes = set(session.retriable_status_codes or ())
+        if retry_on_conflict:
+            kwargs['retriable_status_codes'] = retriable_status_codes | {409}
+        elif retry_on_conflict is not None and retriable_status_codes:
+            # The baremetal proxy defaults to retrying on conflict, allow
+            # 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)
+                microversion=microversion, **kwargs)
         elif self.commit_method == 'POST':
             response = session.post(
                 request.url, json=request.body, headers=request.headers,
-                microversion=microversion)
+                microversion=microversion, **kwargs)
         elif self.commit_method == 'PUT':
             response = session.put(
                 request.url, json=request.body, headers=request.headers,
-                microversion=microversion)
+                microversion=microversion, **kwargs)
         else:
             raise exceptions.ResourceFailure(
                 msg="Invalid commit method: %s" % self.commit_method)

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

@@ -87,8 +87,20 @@ class TestBaremetalProxy(test_proxy_base.TestProxyBase):
     def test_get_node(self):
         self.verify_get(self.proxy.get_node, node.Node)
 
-    def test_update_node(self):
+    @mock.patch.object(node.Node, 'commit', autospec=True)
-        self.verify_update(self.proxy.update_node, node.Node)
+    def test_update_node(self, mock_commit):
+        self.proxy.update_node('uuid', instance_id='new value')
+        mock_commit.assert_called_once_with(mock.ANY, self.proxy,
+                                            retry_on_conflict=True)
+        self.assertEqual('new value', mock_commit.call_args[0][0].instance_id)
+
+    @mock.patch.object(node.Node, 'commit', autospec=True)
+    def test_update_node_no_retries(self, mock_commit):
+        self.proxy.update_node('uuid', instance_id='new value',
+                               retry_on_conflict=False)
+        mock_commit.assert_called_once_with(mock.ANY, self.proxy,
+                                            retry_on_conflict=False)
+        self.assertEqual('new value', mock_commit.call_args[0][0].instance_id)
 
     def test_delete_node(self):
         self.verify_delete(self.proxy.delete_node, node.Node, False)

openstack/tests/unit/image/v2/test_image.py

@@ -104,6 +104,7 @@ class TestImage(base.TestCase):
         self.sess = mock.Mock(spec=adapter.Adapter)
         self.sess.post = mock.Mock(return_value=self.resp)
         self.sess.default_microversion = None
+        self.sess.retriable_status_codes = None
 
     def test_basic(self):
         sot = image.Image()

openstack/tests/unit/test_resource.py

@@ -1016,6 +1016,7 @@ class TestResourceActions(base.TestCase):
         self.session.delete = mock.Mock(return_value=self.response)
         self.session.head = mock.Mock(return_value=self.response)
         self.session.default_microversion = None
+        self.session.retriable_status_codes = None
 
         self.endpoint_data = mock.Mock(max_microversion='1.99',
                                        min_microversion=None)
@@ -1157,7 +1158,8 @@ class TestResourceActions(base.TestCase):
         self.assertEqual(result, sot)
 
     def _test_commit(self, commit_method='PUT', prepend_key=True,
-                     has_body=True, microversion=None):
+                     has_body=True, microversion=None,
+                     commit_args=None, expected_args=None):
         self.sot.commit_method = commit_method
 
         # Need to make sot look dirty so we can attempt an update
@@ -1165,7 +1167,7 @@ class TestResourceActions(base.TestCase):
         self.sot._body.dirty = mock.Mock(return_value={"x": "y"})
 
         self.sot.commit(self.session, prepend_key=prepend_key,
-                        has_body=has_body)
+                        has_body=has_body, **(commit_args or {}))
 
         self.sot._prepare_request.assert_called_once_with(
             prepend_key=prepend_key)
@@ -1174,17 +1176,17 @@ class TestResourceActions(base.TestCase):
             self.session.patch.assert_called_once_with(
                 self.request.url,
                 json=self.request.body, headers=self.request.headers,
-                microversion=microversion)
+                microversion=microversion, **(expected_args or {}))
         elif commit_method == 'POST':
             self.session.post.assert_called_once_with(
                 self.request.url,
                 json=self.request.body, headers=self.request.headers,
-                microversion=microversion)
+                microversion=microversion, **(expected_args or {}))
         elif commit_method == 'PUT':
             self.session.put.assert_called_once_with(
                 self.request.url,
                 json=self.request.body, headers=self.request.headers,
-                microversion=microversion)
+                microversion=microversion, **(expected_args or {}))
 
         self.assertEqual(self.sot.microversion, microversion)
         self.sot._translate_response.assert_called_once_with(
@@ -1197,6 +1199,32 @@ class TestResourceActions(base.TestCase):
         self._test_commit(
             commit_method='PATCH', prepend_key=False, has_body=False)
 
+    def test_commit_patch_retry_on_conflict(self):
+        self._test_commit(
+            commit_method='PATCH',
+            commit_args={'retry_on_conflict': True},
+            expected_args={'retriable_status_codes': {409}})
+
+    def test_commit_put_retry_on_conflict(self):
+        self._test_commit(
+            commit_method='PUT',
+            commit_args={'retry_on_conflict': True},
+            expected_args={'retriable_status_codes': {409}})
+
+    def test_commit_patch_no_retry_on_conflict(self):
+        self.session.retriable_status_codes = {409, 503}
+        self._test_commit(
+            commit_method='PATCH',
+            commit_args={'retry_on_conflict': False},
+            expected_args={'retriable_status_codes': {503}})
+
+    def test_commit_put_no_retry_on_conflict(self):
+        self.session.retriable_status_codes = {409, 503}
+        self._test_commit(
+            commit_method='PATCH',
+            commit_args={'retry_on_conflict': False},
+            expected_args={'retriable_status_codes': {503}})
+
     def test_commit_not_dirty(self):
         self.sot._body = mock.Mock()
         self.sot._body.dirty = dict()

releasenotes/notes/baremetal-retries-ff8aa8f73fb97415.yaml

@@ -0,0 +1,6 @@
+---
+features:
+  - |
+    The bare metal operations now retry HTTP 409 and 503 by default. The number
+    of retries can be changes via the ``baremetal_status_code_retries``
+    configuration option (defaulting to 5).