From 3b022e8d70e7e4a10da009f1c3a1615090bf2786 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 18 Jan 2019 18:15:19 +0100 Subject: [PATCH] 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 | 8 ++++++-- openstack/baremetal/v1/node.py | 13 +++++++++---- openstack/tests/unit/baremetal/v1/test_node.py | 10 +++++++++- .../notes/baremetal-retries-804f553b4e22b3bf.yaml | 8 ++++++++ 4 files changed, 32 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/baremetal-retries-804f553b4e22b3bf.yaml diff --git a/openstack/baremetal/v1/_proxy.py b/openstack/baremetal/v1/_proxy.py index 59d2635ec..ae10c54dd 100644 --- a/openstack/baremetal/v1/_proxy.py +++ b/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. diff --git a/openstack/baremetal/v1/node.py b/openstack/baremetal/v1/node.py index edfb6e9c0..9c2356ebc 100644 --- a/openstack/baremetal/v1/node.py +++ b/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)) diff --git a/openstack/tests/unit/baremetal/v1/test_node.py b/openstack/tests/unit/baremetal/v1/test_node.py index 71aed2e08..466e6a7e4 100644 --- a/openstack/tests/unit/baremetal/v1/test_node.py +++ b/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)) diff --git a/releasenotes/notes/baremetal-retries-804f553b4e22b3bf.yaml b/releasenotes/notes/baremetal-retries-804f553b4e22b3bf.yaml new file mode 100644 index 000000000..b54dc1f19 --- /dev/null +++ b/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``.