Return retries on HTTP CONFLICT to baremetal.attach_vif_to_node

Unfortunately, it is very easy to hit the "node locked" error when retries
are disabled, so add retries to this call as well.

Change-Id: I076cf2537a20687932aca9fd85358bf02882b736

openstack/baremetal/v1/_proxy.py

@@ -632,7 +632,7 @@ class Proxy(proxy.Proxy):
         return self._delete(_portgroup.PortGroup, port_group,
                             ignore_missing=ignore_missing)
 
-    def attach_vif_to_node(self, node, vif_id):
+    def attach_vif_to_node(self, node, vif_id, retry_on_conflict=True):
         """Attach a VIF to the node.
 
         The exact form of the VIF ID depends on the network interface used by
@@ -643,12 +643,16 @@ class Proxy(proxy.Proxy):
         :param node: The value can be either the name or ID of a node or
             a :class:`~openstack.baremetal.v1.node.Node` instance.
         :param string vif_id: Backend-specific VIF ID.
+        :param retry_on_conflict: Whether to retry HTTP CONFLICT errors.
+            This can happen when either the VIF is already used on a node or
+            the node is locked. Since the latter happens more often, the
+            default value is True.
         :return: ``None``
         :raises: :exc:`~openstack.exceptions.NotSupported` if the server
             does not support the VIF API.
         """
         res = self._get_resource(_node.Node, node)
-        res.attach_vif(self, vif_id)
+        res.attach_vif(self, vif_id, retry_on_conflict=retry_on_conflict)
 
     def detach_vif_from_node(self, node, vif_id, ignore_missing=True):
         """Detach a VIF from the node.

openstack/baremetal/v1/node.py

@@ -447,7 +447,7 @@ class Node(_common.ListMixin, resource.Resource):
                 "the last error is %(error)s" %
                 {'node': self.id, 'error': self.last_error})
 
-    def attach_vif(self, session, vif_id):
+    def attach_vif(self, session, vif_id, retry_on_conflict=True):
         """Attach a VIF to the node.
 
         The exact form of the VIF ID depends on the network interface used by
@@ -458,6 +458,10 @@ class Node(_common.ListMixin, resource.Resource):
         :param session: The session to use for making this request.
         :type session: :class:`~keystoneauth1.adapter.Adapter`
         :param string vif_id: Backend-specific VIF ID.
+        :param retry_on_conflict: Whether to retry HTTP CONFLICT errors.
+            This can happen when either the VIF is already used on a node or
+            the node is locked. Since the latter happens more often, the
+            default value is True.
         :return: ``None``
         :raises: :exc:`~openstack.exceptions.NotSupported` if the server
             does not support the VIF API.
@@ -470,12 +474,13 @@ class Node(_common.ListMixin, resource.Resource):
         request = self._prepare_request(requires_id=True)
         request.url = utils.urljoin(request.url, 'vifs')
         body = {'id': vif_id}
+        retriable_status_codes = _common.RETRIABLE_STATUS_CODES
+        if not retry_on_conflict:
+            retriable_status_codes = set(retriable_status_codes) - {409}
         response = session.post(
             request.url, json=body,
             headers=request.headers, microversion=version,
-            # NOTE(dtantsur): do not retry CONFLICT, it's a valid status code
-            # in this API when the VIF is already attached to another node.
-            retriable_status_codes=[503])
+            retriable_status_codes=retriable_status_codes)
 
         msg = ("Failed to attach VIF {vif} to bare metal node {node}"
                .format(node=self.id, vif=vif_id))

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

@@ -342,7 +342,15 @@ class TestNodeVif(base.TestCase):
         self.session.post.assert_called_once_with(
             'nodes/%s/vifs' % self.node.id, json={'id': self.vif_id},
             headers=mock.ANY, microversion='1.28',
-            retriable_status_codes=[503])
+            retriable_status_codes=[409, 503])
+
+    def test_attach_vif_no_retries(self):
+        self.assertIsNone(self.node.attach_vif(self.session, self.vif_id,
+                                               retry_on_conflict=False))
+        self.session.post.assert_called_once_with(
+            'nodes/%s/vifs' % self.node.id, json={'id': self.vif_id},
+            headers=mock.ANY, microversion='1.28',
+            retriable_status_codes={503})
 
     def test_detach_vif_existing(self):
         self.assertTrue(self.node.detach_vif(self.session, self.vif_id))

releasenotes/notes/baremetal-retries-804f553b4e22b3bf.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Changes the ``baremetal.attach_vif_to_node`` call to retry HTTP CONFLICT
+    by default. While it's a valid error code when a VIF is already attached
+    to a node, the same code is also used when the target node is locked.
+    The latter happens more often, so the retries are now on by default and
+    can be disabled by setting ``retry_on_conflict`` to ``False``.