openstack/python-ironicclient
Code Issues Proposed changes

Implement and enable retries on Conflict

Browse Source 
Getting Conflict errors is pretty annoying, especially when writing
automation scripts. Nova and ironic-discoverd already have their
own logic for retrying on such errors.

This patch adds 'max_retries' and 'retry_interval' arguments to
get_client for tuning or disabling retries.

Change-Id: Id9720c87ce3846faec61eb40ccf00970ca798ad2
This commit is contained in:
Dmitry Tantsur
2015-04-16 14:43:46 +02:00
parent 06a62bc60e
commit 28bdd0baeb
4 changed files with 259 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
ironicclient/client.py
View File

@@ -103,14 +103,11 @@ def get_client(api_version, **kwargs):
cli_kwargs = {
'token': token,
'insecure': kwargs.get('insecure'),
'timeout': kwargs.get('timeout'),
'ca_file': kwargs.get('ca_file'),
'cert_file': kwargs.get('cert_file'),
'key_file': kwargs.get('key_file'),
'auth_ref': auth_ref,
'os_ironic_api_version': kwargs.get('os_ironic_api_version'),
}
for key in ('insecure', 'timeout', 'ca_file', 'cert_file', 'key_file',
'os_ironic_api_version', 'max_retries', 'retry_interval'):
cli_kwargs[key] = kwargs.get(key)
return Client(api_version, endpoint, **cli_kwargs)

46
ironicclient/common/http.py
View File

@@ -16,12 +16,14 @@
# under the License.
import copy
import functools
import json
import logging
import os
import socket
import ssl
import textwrap
import time
from keystoneclient import adapter
import six
@@ -46,6 +48,10 @@ API_VERSION = '/v1'
API_VERSION_SELECTED_STATES = ('user', 'negotiated', 'default')
DEFAULT_MAX_RETRIES = 5
DEFAULT_RETRY_INTERVAL = 2
def _trim_endpoint_api_version(url):
"""Trim API version and trailing slash from endpoint."""
return url.rstrip('/').rstrip(API_VERSION)
@@ -141,6 +147,35 @@ class VersionNegotiationMixin(object):
raise NotImplementedError()
def with_retries(func):
"""Wrapper for _http_request adding support for retries."""
@functools.wraps(func)
def wrapper(self, url, method, **kwargs):
if self.conflict_max_retries is None:
self.conflict_max_retries = DEFAULT_MAX_RETRIES
if self.conflict_retry_interval is None:
self.conflict_retry_interval = DEFAULT_RETRY_INTERVAL
num_attempts = self.conflict_max_retries + 1
for attempt in range(1, num_attempts + 1):
try:
return func(self, url, method, **kwargs)
except exc.Conflict as error:
msg = ("Error contacting Ironic server: %(error)s. "
"Attempt %(attempt)d of %(total)d" %
{'attempt': attempt,
'total': num_attempts,
'error': error})
if attempt == num_attempts:
LOG.error(msg)
raise
else:
LOG.warn(msg)
time.sleep(self.conflict_retry_interval)
return wrapper
class HTTPClient(VersionNegotiationMixin):
def __init__(self, endpoint, **kwargs):
@@ -152,6 +187,10 @@ class HTTPClient(VersionNegotiationMixin):
DEFAULT_VER)
self.api_version_select_state = kwargs.get(
'api_version_select_state', 'default')
self.conflict_max_retries = kwargs.pop('max_retries',
DEFAULT_MAX_RETRIES)
self.conflict_retry_interval = kwargs.pop('retry_interval',
DEFAULT_RETRY_INTERVAL)
self.connection_params = self.get_connection_params(endpoint, **kwargs)
@staticmethod
@@ -234,6 +273,7 @@ class HTTPClient(VersionNegotiationMixin):
conn.request(method, self._make_connection_url(url))
return conn.getresponse()
@with_retries
def _http_request(self, url, method, **kwargs):
"""Send an http request with the specified characteristics.
@@ -401,6 +441,9 @@ class VerifiedHTTPSConnection(six.moves.http_client.HTTPSConnection):
class SessionClient(VersionNegotiationMixin, adapter.LegacyJsonAdapter):
"""HTTP client based on Keystone client session."""
conflict_max_retries = DEFAULT_MAX_RETRIES
conflict_retry_interval = DEFAULT_RETRY_INTERVAL
def _parse_version_headers(self, resp):
return self._generic_parse_version_headers(resp.headers.get)
@@ -408,6 +451,7 @@ class SessionClient(VersionNegotiationMixin, adapter.LegacyJsonAdapter):
# NOTE: conn is self.session for this class
return conn.request(url, method, raise_exc=False)
@with_retries
def _http_request(self, url, method, **kwargs):
kwargs.setdefault('user_agent', USER_AGENT)
kwargs.setdefault('auth', self.auth)
@@ -509,6 +553,8 @@ def _construct_http_client(*args, **kwargs):
session_client.os_ironic_api_version = os_ironic_api_version
session_client.api_version_select_state = (
kwargs.pop('api_version_select_state', 'default'))
session_client.conflict_max_retries = kwargs.get('max_retries')
session_client.conflict_retry_interval = kwargs.get('retry_interval')
return session_client
else:
return HTTPClient(*args, **kwargs)

28
ironicclient/shell.py
View File

@@ -34,6 +34,7 @@ import six.moves.urllib.parse as urlparse
import ironicclient
from ironicclient import client as iroclient
from ironicclient.common import http
from ironicclient.common.i18n import _
from ironicclient.common import utils
from ironicclient import exc
@@ -221,6 +222,25 @@ class IronicShell(object):
parser.add_argument('--os_endpoint_type',
help=argparse.SUPPRESS)
parser.add_argument('--max-retries', type=int,
help='Maximum number of retries in case of '
'conflict error (HTTP 409). '
'Defaults to env[IRONIC_MAX_RETRIES] or %d. '
'Use 0 to disable retrying.'
% http.DEFAULT_MAX_RETRIES,
default=cliutils.env(
'IRONIC_MAX_RETRIES',
default=str(http.DEFAULT_MAX_RETRIES)))
parser.add_argument('--retry-interval', type=int,
help='Amount of time (in seconds) between retries '
'in case of conflict error (HTTP 409). '
'Defaults to env[IRONIC_RETRY_INTERVAL] or %d.'
% http.DEFAULT_RETRY_INTERVAL,
default=cliutils.env(
'IRONIC_RETRY_INTERVAL',
default=str(http.DEFAULT_RETRY_INTERVAL)))
# FIXME(gyee): this method should come from python-keystoneclient.
# Will refactor this code once it is available.
# https://bugs.launchpad.net/python-keystoneclient/+bug/1332337
@@ -502,6 +522,14 @@ class IronicShell(object):
'password': args.os_password,
}
kwargs['os_ironic_api_version'] = os_ironic_api_version
if args.max_retries < 0:
raise exc.CommandError(_("You must provide value >= 0 for "
"--max-retries"))
if args.retry_interval < 1:
raise exc.CommandError(_("You must provide value >= 1 for "
"--retry-interval"))
kwargs['max_retries'] = args.max_retries
kwargs['retry_interval'] = args.retry_interval
client = iroclient.Client(api_major_version, endpoint, **kwargs)
try:

183
ironicclient/tests/unit/test_http.py
View File

@@ -397,7 +397,7 @@ class HttpClientTest(utils.BaseTestCase):
utils.FakeConnection(good_resp)])
response, body_iter = client._http_request('/v1/resources', 'GET')
self.assertEqual(200, response.status)
self.assertTrue(1, mock_negotiate.call_count)
self.assertEqual(1, mock_negotiate.call_count)
class SessionClientTest(utils.BaseTestCase):
@@ -467,3 +467,184 @@ class SessionClientTest(utils.BaseTestCase):
service_name=None)
result = client._parse_version_headers(fake_session)
self.assertEqual(expected_result, result)
class RetriesTestCase(utils.BaseTestCase):
def setUp(self):
super(RetriesTestCase, self).setUp()
old_retry_delay = http.DEFAULT_RETRY_INTERVAL
http.DEFAULT_RETRY_INTERVAL = 0.001
http.SessionClient.conflict_retry_interval = 0.001
self.addCleanup(setattr, http, 'DEFAULT_RETRY_INTERVAL',
old_retry_delay)
self.addCleanup(setattr, http.SessionClient, 'conflict_retry_interval',
old_retry_delay)
@mock.patch.object(http.HTTPClient, 'get_connection', autospec=True)
def test_http_no_retry(self, mock_getcon):
error_body = _get_error_body()
bad_resp = utils.FakeResponse(
{'content-type': 'text/plain'},
six.StringIO(error_body),
version=1,
status=409)
client = http.HTTPClient('http://localhost/', max_retries=0)
mock_getcon.return_value = utils.FakeConnection(bad_resp)
self.assertRaises(exc.Conflict, client._http_request,
'/v1/resources', 'GET')
self.assertEqual(1, mock_getcon.call_count)
@mock.patch.object(http.HTTPClient, 'get_connection', autospec=True)
def test_http_retry(self, mock_getcon):
error_body = _get_error_body()
bad_resp = utils.FakeResponse(
{'content-type': 'text/plain'},
six.StringIO(error_body),
version=1,
status=409)
good_resp = utils.FakeResponse(
{'content-type': 'text/plain'},
six.StringIO("meow"),
version=1,
status=200)
client = http.HTTPClient('http://localhost/')
mock_getcon.side_effect = iter((utils.FakeConnection(bad_resp),
utils.FakeConnection(good_resp)))
response, body_iter = client._http_request('/v1/resources', 'GET')
self.assertEqual(200, response.status)
self.assertEqual(2, mock_getcon.call_count)
@mock.patch.object(http.HTTPClient, 'get_connection', autospec=True)
def test_http_failed_retry(self, mock_getcon):
error_body = _get_error_body()
bad_resp = utils.FakeResponse(
{'content-type': 'text/plain'},
six.StringIO(error_body),
version=1,
status=409)
client = http.HTTPClient('http://localhost/')
mock_getcon.return_value = utils.FakeConnection(bad_resp)
self.assertRaises(exc.Conflict, client._http_request,
'/v1/resources', 'GET')
self.assertEqual(http.DEFAULT_MAX_RETRIES + 1, mock_getcon.call_count)
@mock.patch.object(http.HTTPClient, 'get_connection', autospec=True)
def test_http_max_retries_none(self, mock_getcon):
error_body = _get_error_body()
bad_resp = utils.FakeResponse(
{'content-type': 'text/plain'},
six.StringIO(error_body),
version=1,
status=409)
client = http.HTTPClient('http://localhost/', max_retries=None)
mock_getcon.return_value = utils.FakeConnection(bad_resp)
self.assertRaises(exc.Conflict, client._http_request,
'/v1/resources', 'GET')
self.assertEqual(http.DEFAULT_MAX_RETRIES + 1, mock_getcon.call_count)
@mock.patch.object(http.HTTPClient, 'get_connection', autospec=True)
def test_http_change_max_retries(self, mock_getcon):
error_body = _get_error_body()
bad_resp = utils.FakeResponse(
{'content-type': 'text/plain'},
six.StringIO(error_body),
version=1,
status=409)
client = http.HTTPClient('http://localhost/',
max_retries=http.DEFAULT_MAX_RETRIES + 1)
mock_getcon.return_value = utils.FakeConnection(bad_resp)
self.assertRaises(exc.Conflict, client._http_request,
'/v1/resources', 'GET')
self.assertEqual(http.DEFAULT_MAX_RETRIES + 2, mock_getcon.call_count)
def test_session_retry(self):
error_body = _get_error_body()
fake_resp = utils.FakeSessionResponse(
{'Content-Type': 'application/json'},
error_body,
409)
ok_resp = utils.FakeSessionResponse(
{'Content-Type': 'application/json'},
b"OK",
200)
fake_session = mock.Mock(spec=utils.FakeSession)
fake_session.request.side_effect = iter((fake_resp, ok_resp))
client = http.SessionClient(session=fake_session,
auth=None,
interface=None,
service_type='publicURL',
region_name='',
service_name=None)
client.json_request('GET', '/v1/resources')
self.assertEqual(2, fake_session.request.call_count)
def test_session_retry_fail(self):
error_body = _get_error_body()
fake_resp = utils.FakeSessionResponse(
{'Content-Type': 'application/json'},
error_body,
409)
fake_session = mock.Mock(spec=utils.FakeSession)
fake_session.request.return_value = fake_resp
client = http.SessionClient(session=fake_session,
auth=None,
interface=None,
service_type='publicURL',
region_name='',
service_name=None)
self.assertRaises(exc.Conflict, client.json_request,
'GET', '/v1/resources')
self.assertEqual(http.DEFAULT_MAX_RETRIES + 1,
fake_session.request.call_count)
def test_session_max_retries_none(self):
error_body = _get_error_body()
fake_resp = utils.FakeSessionResponse(
{'Content-Type': 'application/json'},
error_body,
409)
fake_session = mock.Mock(spec=utils.FakeSession)
fake_session.request.return_value = fake_resp
client = http.SessionClient(session=fake_session,
auth=None,
interface=None,
service_type='publicURL',
region_name='',
service_name=None)
client.conflict_max_retries = None
self.assertRaises(exc.Conflict, client.json_request,
'GET', '/v1/resources')
self.assertEqual(http.DEFAULT_MAX_RETRIES + 1,
fake_session.request.call_count)
def test_session_change_max_retries(self):
error_body = _get_error_body()
fake_resp = utils.FakeSessionResponse(
{'Content-Type': 'application/json'},
error_body,
409)
fake_session = mock.Mock(spec=utils.FakeSession)
fake_session.request.return_value = fake_resp
client = http.SessionClient(session=fake_session,
auth=None,
interface=None,
service_type='publicURL',
region_name='',
service_name=None)
client.conflict_max_retries = http.DEFAULT_MAX_RETRIES + 1
self.assertRaises(exc.Conflict, client.json_request,
'GET', '/v1/resources')
self.assertEqual(http.DEFAULT_MAX_RETRIES + 2,
fake_session.request.call_count)