From 3b88d50820a2a389e189c8c2a956f20fcc540c65 Mon Sep 17 00:00:00 2001 From: Helena McGough Date: Wed, 17 May 2017 10:44:43 +0000 Subject: [PATCH] Refactor update and create alarm functions - Checks for errors and retries creating/updating alarms which fixes the bug, by removing the need to have a dictionary for alarms. - Included a get_alarm_id function. - Refactored the update and create alarm functions. - Updated and included the relevant tests for this. - Include a reno for the bug fix. Change-Id: I7e998bc8cb55f9ef7564464fcd4eded06643e1eb Closes-Bug: #1677597 --- collectd_ceilometer/aodh/sender.py | 78 +++++----- collectd_ceilometer/tests/aodh/test_plugin.py | 137 +++++++++--------- .../notes/bug-1677597-2871c1af2cdaed94.yaml | 8 + 3 files changed, 108 insertions(+), 115 deletions(-) create mode 100644 releasenotes/notes/bug-1677597-2871c1af2cdaed94.yaml diff --git a/collectd_ceilometer/aodh/sender.py b/collectd_ceilometer/aodh/sender.py index 929e943..e093de9 100644 --- a/collectd_ceilometer/aodh/sender.py +++ b/collectd_ceilometer/aodh/sender.py @@ -52,7 +52,6 @@ class Sender(object): self._auth_lock = threading.Lock() self._failed_auth = False self._alarm_ids = {} - self._alarm_names = list() def _authenticate(self): """Authenticate and renew the authentication token.""" @@ -131,13 +130,9 @@ class Sender(object): # Create alarm name alarm_name = self._get_alarm_name(metername, resource_id) - # check for and/or get alarm_id - alarm_id = self._get_alarm_id(alarm_name, severity, metername, message) - - if alarm_id is not None: - result = self._update_alarm(alarm_id, message, auth_token) - else: - result = None + # Update or create this alarm + result = self._update_or_create_alarm(alarm_name, message, auth_token, + severity, metername) if result is None: return @@ -159,25 +154,19 @@ class Sender(object): auth_token = self._authenticate() if auth_token is not None: - if alarm_id is not None: - result = self._update_alarm(alarm_id, message, auth_token) - else: - result = None + result = self._update_or_create_alarm(alarm_name, message, + auth_token, severity, + metername) if result.status_code == HTTP_NOT_FOUND: - LOGGER.debug("Received 404 error when submitting %s sample, \ - creating a new metric", + LOGGER.debug("Received 404 error when submitting %s notification, \ + creating a new alarm", alarm_name) # check for and/or get alarm_id - alarm_id = self._get_alarm_id(alarm_name, severity, - metername, message) - - LOGGER.info('alarmname: %s, alarm_id: %s', alarm_name, alarm_id) - if alarm_id is not None: - result = self._update_alarm(alarm_id, message, auth_token) - else: - result = None + result = self._update_or_create_alarm(alarm_name, message, + auth_token, severity, + metername) if result.status_code == HTTP_CREATED: LOGGER.debug('Result: %s', HTTP_CREATED) @@ -193,22 +182,23 @@ class Sender(object): Config.instance().CEILOMETER_URL_TYPE) return endpoint - def _get_alarm_id(self, alarm_name, severity, metername, message): - """Check for existing alarm and its id or create a new one.""" + def _update_or_create_alarm(self, alarm_name, message, auth_token, + severity, metername): + # check for an alarm and update try: - return self._alarm_ids[alarm_name] + alarm_id = self._get_alarm_id(alarm_name) + result = self._update_alarm(alarm_id, message, auth_token) + # or create a new alarm except KeyError as ke: LOGGER.warn(ke) endpoint = self._get_endpoint("aodh") - if alarm_name not in self._alarm_names: - LOGGER.warn('No known ID for %s', alarm_name) - self._alarm_names.append(alarm_name) - self._alarm_ids[alarm_name] = \ - self._create_alarm(endpoint, severity, - metername, alarm_name, message) - return None + LOGGER.warn('No known ID for %s', alarm_name) + result, self._alarm_ids[alarm_name] = \ + self._create_alarm(endpoint, severity, + metername, alarm_name, message) + return result def _create_alarm(self, endpoint, severity, metername, alarm_name, message): @@ -226,7 +216,11 @@ class Sender(object): result = self._perform_post_request(url, payload, self._auth_token) alarm_id = json.loads(result.text)['alarm_id'] LOGGER.debug("alarm_id=%s", alarm_id) - return alarm_id + return result, alarm_id + + def _get_alarm_id(self, alarm_name): + """Try and return an alarm_id for an collectd notification""" + return self._alarm_ids[alarm_name] def _get_alarm_state(self, message): """Get the state of the alarm.""" @@ -275,11 +269,10 @@ class Sender(object): except RequestException as exc: LOGGER.error('aodh request error: %s', six.text_type(exc)) finally: - if response is not None: - LOGGER.debug( - "Returning response from _perform_post_request(): %s", - response.status_code) - return response + LOGGER.debug( + "Returning response from _perform_post_request(): %s", + response.status_code) + return response @classmethod def _perform_update_request(cls, url, auth_token, payload): @@ -304,8 +297,7 @@ class Sender(object): except RequestException as exc: LOGGER.error('aodh request error: %s', six.text_type(exc)) finally: - if response is not None: - LOGGER.debug( - 'Returning response from _perform_update_request(): %s', - response.status_code) - return response + LOGGER.debug( + 'Returning response from _perform_update_request(): %s', + response.status_code) + return response diff --git a/collectd_ceilometer/tests/aodh/test_plugin.py b/collectd_ceilometer/tests/aodh/test_plugin.py index d8d9f53..170da40 100644 --- a/collectd_ceilometer/tests/aodh/test_plugin.py +++ b/collectd_ceilometer/tests/aodh/test_plugin.py @@ -25,6 +25,7 @@ import unittest from collectd_ceilometer.aodh import plugin from collectd_ceilometer.aodh import sender from collectd_ceilometer.common.keystone_light import KeystoneException +from collectd_ceilometer.common.meters import base Logger = logging.getLoggerClass() @@ -136,58 +137,55 @@ class TestPlugin(unittest.TestCase): collectd.register_notification.assert_called_once_with(instance.notify) collectd.register_shutdown.assert_called_once_with(instance.shutdown) - @mock.patch.object(sender.Sender, '_get_alarm_id', autospec=True) - @mock.patch.object(requests, 'put', spec=callable) + @mock.patch.object(sender.Sender, '_update_or_create_alarm', autospec=True) + @mock.patch.object(sender.Sender, '_get_alarm_name', autospec=True) + @mock.patch.object(base, 'Meter', autospec=True) @mock.patch.object(sender, 'ClientV3', autospec=True) @mock_collectd() @mock_config() @mock_value() - def test_notify(self, data, config, collectd, ClientV3, put, - _get_alarm_id): - """Test collectd data notifying.""" + def test_update_or_create_alarm(self, data, config, collectd, + ClientV3, meter, + _get_alarm_name, _update_or_create_alarm): + """Test the update/create alarm function""" auth_client = ClientV3.return_value auth_client.get_service_endpoint.return_value = \ 'https://test-aodh.tld' - put.return_value.status_code = sender.HTTP_CREATED - put.return_value.text = 'Updated' + _update_or_create_alarm.return_value = requests.Response() - # Test when the value doesn't get updated - _get_alarm_id.return_value = None + # init sender instance + instance = sender.Sender() - # init instance - instance = plugin.Plugin(collectd=collectd, config=config) + _get_alarm_name.return_value = 'my-alarm' + meter_name = meter.meter_name.return_value + message = meter.message.return_value + severity = meter.severity.return_value + resource_id = meter.resource_id.return_value - # send the value(doesn't update) - instance.notify(data) - collectd.error.assert_not_called() + # send the values + instance.send(meter_name, severity, resource_id, message) - put.assert_not_called() + # check that the function is called + _update_or_create_alarm.assert_called_once_with( + instance, 'my-alarm', message, auth_client.auth_token, + severity, meter_name) - # Test when an alarm will be updated - _get_alarm_id.return_value = 'my-alarm-id' + # reset function + _update_or_create_alarm.reset_mock() - # send the value(updates) - instance.notify(data) - collectd.error.assert_not_called() + # run test again for failed attempt + _update_or_create_alarm.return_value = None - # authentication client that has been created - ClientV3.assert_called_once() + instance.send(meter_name, severity, resource_id, message) # and values that have been sent - put.assert_called_once_with( - 'https://test-aodh.tld' + - '/v2/alarms/my-alarm-id/state', - data='"insufficient data"', - headers={'Content-type': 'application/json', - 'X-Auth-Token': auth_client.auth_token}, - timeout=1.0) + _update_or_create_alarm.assert_called_once_with( + instance, 'my-alarm', message, auth_client.auth_token, + severity, meter_name) # reset post method - put.reset_mock() - - # call shutdown - instance.shutdown() + _update_or_create_alarm.reset_mock() @mock.patch.object(requests, 'put', spec=callable) @mock.patch.object(sender, 'ClientV3', autospec=True) @@ -237,20 +235,17 @@ class TestPlugin(unittest.TestCase): # the value self.assertRaises(requests.RequestException, instance.notify, data) - @mock.patch.object(sender.Sender, '_get_alarm_id', autospec=True) - @mock.patch.object(requests, 'put', spec=callable) + @mock.patch.object(sender.Sender, '_update_or_create_alarm', autospec=True) + @mock.patch.object(sender.Sender, '_get_alarm_name', autospec=True) + @mock.patch.object(base, 'Meter', autospec=True) @mock.patch.object(sender, 'ClientV3', autospec=True) @mock_collectd() @mock_config() @mock_value() def test_reauthentication(self, data, config, collectd, - ClientV3, put, _get_alarm_id): + ClientV3, meter, _get_alarm_name, + _update_or_create_alarm): """Test re-authentication for update request.""" - # init instance - instance = plugin.Plugin(collectd=collectd, config=config) - - # test for non update request - _get_alarm_id.return_value = None # response returned on success response_ok = requests.Response() @@ -260,19 +255,29 @@ class TestPlugin(unittest.TestCase): response_unauthorized = requests.Response() response_unauthorized.status_code = requests.codes["UNAUTHORIZED"] - put.return_value = response_ok + _update_or_create_alarm.return_value = response_ok client = ClientV3.return_value client.auth_token = 'Test auth token' - # notify of the value - instance.notify(data) + # init instance attempt to update/create alarm + instance = sender.Sender() - # de-assert the non-update request - put.assert_not_called() + alarm_name = _get_alarm_name.return_value + meter_name = meter.meter_name.return_value + message = meter.message.return_value + severity = meter.severity.return_value + resource_id = meter.resource_id.return_value - # test for update request - _get_alarm_id.return_value = 'my-alarm-id' + # send the data + instance.send(meter_name, severity, resource_id, message) + + _update_or_create_alarm.assert_called_once_with( + instance, alarm_name, message, client.auth_token, + severity, meter_name) + + # de-assert the request + _update_or_create_alarm.reset_mock() # response returned on success response_ok = requests.Response() @@ -282,40 +287,28 @@ class TestPlugin(unittest.TestCase): response_unauthorized = requests.Response() response_unauthorized.status_code = requests.codes["UNAUTHORIZED"] - put.return_value = response_ok + _update_or_create_alarm.return_value = response_ok client = ClientV3.return_value client.auth_token = 'Test auth token' - # notify of the value - instance.notify(data) + # send the data + instance.send(meter_name, severity, resource_id, message) - # verify the auth token - put.assert_called_once_with( - mock.ANY, data=mock.ANY, - headers={u'Content-type': mock.ANY, - u'X-Auth-Token': 'Test auth token'}, - timeout=1.0) + _update_or_create_alarm.assert_called_once_with( + instance, alarm_name, message, client.auth_token, + severity, meter_name) - # POST response is unauthorized -> new token needs to be acquired - put.side_effect = [response_unauthorized, response_ok] + # update/create response is unauthorized -> new token needs + # to be acquired + _update_or_create_alarm.side_effect = [response_unauthorized, + response_ok] # set a new auth token client.auth_token = 'New test auth token' - instance.notify(data) - - # verify the auth token: - call_list = put.call_args_list - # POST called three times - self.assertEqual(len(call_list), 3) - - # the second call contains the old token - token = call_list[1][1]['headers']['X-Auth-Token'] - self.assertEqual(token, 'Test auth token') - # the third call contains the new token - token = call_list[2][1]['headers']['X-Auth-Token'] - self.assertEqual(token, 'New test auth token') + # send the data again + instance.send(meter_name, severity, resource_id, message) @mock.patch.object(sender, 'ClientV3', autospec=True) @mock.patch.object(plugin, 'Notifier', autospec=True) diff --git a/releasenotes/notes/bug-1677597-2871c1af2cdaed94.yaml b/releasenotes/notes/bug-1677597-2871c1af2cdaed94.yaml new file mode 100644 index 0000000..93f755e --- /dev/null +++ b/releasenotes/notes/bug-1677597-2871c1af2cdaed94.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixes 'bug 1677597 https://bugs.launchpad.net/collectd-ceilometer-plugin/+bug/1677597' + Refactored the update_alarm and create_alarm functions to allow for error + checking by introducing a update_or_create_alarm function. This allows + retrying updating or creating alarms if they fail. Also updated the + corresponding tests.