x/collectd-openstack-plugins
Code Issues Proposed changes

Fix POST result verification.

Browse Source 
The result of POST request is threated using a more explicit
approach via raise_for_status method instead of using ambigous
bool operator.

Requests library is mocked in plugin test instead of base test class.
The test for this behaviour has been fixed.

Closes-Bug: #1614772
Change-Id: I9079246b1b501b69e850878bef13c16c7c6d0b59
This commit is contained in:
Federico Ressi
2016-08-19 00:02:11 +01:00
parent 3df52a97bc
commit 652ed97af1
3 changed files with 106 additions and 87 deletions
Download Patch File Download Diff File Expand all files Collapse all files

107
collectd_ceilometer/sender.py
View File

@@ -16,21 +16,32 @@
from __future__ import division
from __future__ import unicode_literals
from collectd_ceilometer.keystone_light import ClientV2 as keystoneClientV2
import logging
import threading
import requests
import six
from collectd_ceilometer.keystone_light import ClientV2
from collectd_ceilometer.keystone_light import KeystoneException
from collectd_ceilometer.settings import Config
import logging
import requests
from requests.exceptions import RequestException
import six
import threading
LOGGER = logging.getLogger(__name__)
# HTTP status codes
HTTP_CREATED = 201
HTTP_UNAUTHORIZED = 401
HTTP_CREATED = requests.codes['CREATED']
HTTP_UNAUTHORIZED = requests.codes['UNAUTHORIZED']
# Lookup dictionary for getting code status name from code status number
# this is the inverse mapping of requests.codes dictionary
STATUS_NAMES = {
status_code: status_name
for status_name, status_code in six.iteritems(requests.codes.__dict__)}
def get_status_name(status_code):
"""Get an human friendly name for given status code"""
return STATUS_NAMES.get(status_code, str(status_code))
class Sender(object):
@@ -66,7 +77,7 @@ class Sender(object):
# create a keystone client if it doesn't exist
if self._keystone is None:
cfg = Config.instance()
self._keystone = keystoneClientV2(
self._keystone = ClientV2(
auth_url=cfg.OS_AUTH_URL,
username=cfg.OS_USERNAME,
password=cfg.OS_PASSWORD,
@@ -125,39 +136,34 @@ class Sender(object):
url = self._url_base % metername
# send the POST request
result = self._perform_request(url, payload, auth_token)
if not result:
return
try:
return self._perform_request(url, payload, auth_token)
except requests.exceptions.HTTPError as exc:
response = exc.response
# if the request failed due to an auth error
if result.status_code == HTTP_UNAUTHORIZED:
# reset the auth token in order to force the subsequent
# _authenticate() call to renew it
# Here, it can happen that the token is reset right after
# another thread has finished the authentication and thus
# the authentication may be performed twice
self._auth_token = None
# if the request failed due to an auth error
if response.status_code == HTTP_UNAUTHORIZED:
LOGGER.info('Renewing authentication.')
LOGGER.debug('Result: %s %s',
six.text_type(result.status_code),
result.text)
# reset the auth token in order to force the subsequent
# _authenticate() call to renew it
# Here, it can happen that the token is reset right after
# another thread has finished the authentication and thus
# the authentication may be performed twice
self._auth_token = None
# renew the authentication token
auth_token = self._authenticate()
# renew the authentication token
auth_token = self._authenticate()
if auth_token is not None:
# and try to repost
result = self._perform_request(url, payload, auth_token)
if auth_token is not None:
# and try to repost
return self._perform_request(url, payload, auth_token)
else:
# This is an error and it has to be forwarded
raise
if result.status_code == HTTP_CREATED:
LOGGER.debug('Result: %s', HTTP_CREATED)
else:
LOGGER.info('Result: %s %s',
result.status_code,
result.text)
@classmethod
def _perform_request(cls, url, payload, auth_token):
@staticmethod
def _perform_request(url, payload, auth_token):
"""Perform the POST request"""
LOGGER.debug('Performing request to %s', url)
@@ -166,10 +172,23 @@ class Sender(object):
headers = {'X-Auth-Token': auth_token,
'Content-type': 'application/json'}
# perform request and return its result
response = requests.post(
url, data=payload, headers=headers,
timeout=(Config.instance().CEILOMETER_TIMEOUT / 1000.))
# Raises exception if there was an error
try:
return requests.post(
url, data=payload, headers=headers,
timeout=(Config.instance().CEILOMETER_TIMEOUT / 1000.))
except RequestException as exc:
LOGGER.error('Ceilometer request error: %s', six.text_type(exc))
return None
response.raise_for_status()
# pylint: disable=broad-except
except Exception:
exc_info = 1
raise
else:
exc_info = 0
finally:
# Log out the result of the request for debugging purpose
LOGGER.debug(
'Result: %s, %d, %r',
get_status_name(response.status_code),
response.status_code, response.text, exc_info=exc_info)
return response

8
collectd_ceilometer/tests/base.py
View File

@@ -130,16 +130,10 @@ class TestCase(unittest.TestCase):
super(TestCase, self).setUp()
modules = ['collectd', 'libvirt', 'requests',
'collectd_ceilometer.keystone_light']
modules = ['collectd', 'libvirt', 'collectd_ceilometer.keystone_light']
self._mocked = {module: Mock() for module in modules}
# requests
requests = self.get_mock('requests')
requests.exceptions.RequestException = Exception
self._mocked.update({'requests.exceptions': requests.exceptions})
keystone = self.get_mock('collectd_ceilometer.keystone_light')
keystone.KeystoneException = KeystoneException
self._mocked.update(

78
collectd_ceilometer/tests/test_plugin.py
View File

@@ -24,6 +24,7 @@ from collectd_ceilometer.tests.base import Value
from collections import namedtuple
import json
import mock
import requests
class PluginTest(TestCase):
@@ -53,13 +54,13 @@ class PluginTest(TestCase):
self.assertTrue(collectd.register_write.called)
self.assertTrue(collectd.register_shutdown.called)
def test_write(self):
@mock.patch.object(requests, 'post', spec=callable)
def test_write(self, post):
"""Test collectd data writing"""
from collectd_ceilometer.sender import HTTP_CREATED
requests = self.get_mock('requests')
requests.post.return_value.status_code = HTTP_CREATED
requests.post.return_value.text = 'Created'
post.return_value = response = requests.Response()
response.status_code = HTTP_CREATED
client_class \
= self.get_mock('collectd_ceilometer.keystone_light').ClientV2
@@ -79,7 +80,7 @@ class PluginTest(TestCase):
self._write_value(data)
# no value has been sent to ceilometer
self.assertFalse(requests.post.called)
post.assert_not_called()
# send the second value
self._write_value(data)
@@ -88,8 +89,7 @@ class PluginTest(TestCase):
self.assertTrue(client_class.called)
self.assertEqual(client_class.call_count, 1)
# and values has been sent
self.assertTrue(requests.post.called)
self.assertEqual(requests.post.call_count, 1)
post.assert_called_once()
expected_args = ('https://test-ceilometer.tld/v2/meters/cpu.freq',)
expected_kwargs = {
@@ -118,26 +118,26 @@ class PluginTest(TestCase):
# we cannot compare JSON directly because the original data
# dictionary is unordered
called_kwargs = requests.post.call_args[1]
called_kwargs = post.call_args[1]
called_kwargs['data'] = json.loads(called_kwargs['data'])
# verify data sent to ceilometer
self.assertEqual(requests.post.call_args[0], expected_args)
self.assertEqual(post.call_args[0], expected_args)
self.assertEqual(called_kwargs, expected_kwargs)
# reset post method
requests.post.reset_mock()
post.reset_mock()
# write another values
self._write_value(data)
# nothing has been sent
self.assertFalse(requests.post.called)
post.assert_not_called()
# call shutdown
self.plugin_instance.shutdown()
self.assertNoError()
# previously written value has been sent
self.assertTrue(requests.post.called)
post.assert_called_once()
# no more authentication required
self.assertEqual(client_class.call_count, 1)
@@ -158,14 +158,15 @@ class PluginTest(TestCase):
# we cannot compare JSON directly because the original data
# dictionary is unordered
called_kwargs = requests.post.call_args[1]
called_kwargs = post.call_args[1]
called_kwargs['data'] = json.loads(called_kwargs['data'])
# verify data sent to ceilometer
self.assertEqual(requests.post.call_args[0], expected_args)
self.assertEqual(post.call_args[0], expected_args)
self.assertEqual(called_kwargs, expected_kwargs)
def test_write_auth_failed(self):
@mock.patch.object(requests, 'post', spec=callable)
def test_write_auth_failed(self, post):
"""Test authentication failure"""
# tell the auth client to rise an exception
@@ -182,10 +183,10 @@ class PluginTest(TestCase):
self._write_value(self._create_value(), errors)
# no requests method has been called
self.assertFalse(self.get_mock('requests').post.called,
"requests method has been called")
post.assert_not_called()
def test_write_auth_failed2(self):
@mock.patch.object(requests, 'post', spec=callable)
def test_write_auth_failed2(self, post):
"""Test authentication failure2"""
# tell the auth client to rise an exception
@@ -209,18 +210,17 @@ class PluginTest(TestCase):
self._write_value(self._create_value(), errors)
# no requests method has been called
self.assertFalse(self.get_mock('requests').post.called,
"requests method has been called")
post.assert_not_called()
def test_request_error(self):
@mock.patch.object(requests, 'post', spec=callable)
def test_request_error(self, post):
"""Test error raised by underlying requests module"""
# we have to import the RequestException here as it has been mocked
from requests.exceptions import RequestException
# tell POST request to raise an exception
requests = self.get_mock('requests')
requests.post.side_effect = RequestException('Test POST exception')
post.side_effect = RequestException('Test POST exception')
# init instance
self._init_instance()
@@ -228,13 +228,12 @@ class PluginTest(TestCase):
# write the value
self._write_value(
self._create_value(),
['Ceilometer request error: Test POST exception'])
['Exception during write: Test POST exception'])
def test_reauthentication(self):
@mock.patch.object(requests, 'post', spec=callable)
def test_reauthentication(self, post):
"""Test re-authentication"""
from collectd_ceilometer.sender import HTTP_UNAUTHORIZED
requests = self.get_mock('requests')
client_class \
= self.get_mock('collectd_ceilometer.keystone_light').ClientV2
client_class.return_value.auth_token = 'Test auth token'
@@ -242,26 +241,33 @@ class PluginTest(TestCase):
# init instance
self._init_instance()
# write the first value
# response returned on success
response_ok = requests.Response()
response_ok.status_code = requests.codes["OK"]
# response returned on failure
response_unauthorized = requests.Response()
response_unauthorized.status_code = requests.codes["UNAUTHORIZED"]
# write the first value with success
# subsequent call of POST method will fail due to the authentication
post.side_effect = [response_ok, response_unauthorized, response_ok]
self._write_value(self._create_value())
# verify the auth token
call_list = requests.post.call_args_list
call_list = post.call_args_list
self.assertEqual(len(call_list), 1)
# 0 = first call > 1 = call kwargs > headers argument > auth token
token = call_list[0][1]['headers']['X-Auth-Token']
self.assertEqual(token, 'Test auth token')
# subsequent call of POST method will fail due to the authentication
requests.post.return_value.status_code = HTTP_UNAUTHORIZED
requests.post.return_value.text = 'Unauthorized'
# set a new auth token
client_class.return_value.auth_token = 'New test auth token'
self._write_value(self._create_value())
# verify the auth token
call_list = requests.post.call_args_list
call_list = post.call_args_list
# POST called three times
self.assertEqual(len(call_list), 3)
@@ -272,7 +278,8 @@ class PluginTest(TestCase):
token = call_list[2][1]['headers']['X-Auth-Token']
self.assertEqual(token, 'New test auth token')
def test_authentication_in_multiple_threads(self):
@mock.patch.object(requests, 'post', spec=callable)
def test_authentication_in_multiple_threads(self, post):
"""Test authentication in muliple threads
This test simulates the authentication performed from different thread
@@ -306,7 +313,6 @@ class PluginTest(TestCase):
self._write_value(self._create_value())
# verify the results
requests = self.get_mock('requests')
client_class \
= self.get_mock('collectd_ceilometer.keystone_light').ClientV2
@@ -314,7 +320,7 @@ class PluginTest(TestCase):
self.assertFalse(client_class.called)
# verify the auth token
call_list = requests.post.call_args_list
call_list = post.call_args_list
self.assertEqual(len(call_list), 1)
# 0 = first call > 1 = call kwargs > headers argument > auth token
token = call_list[0][1]['headers']['X-Auth-Token']