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

@@ -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
+        # Update or create this alarm
-        alarm_id = self._get_alarm_id(alarm_name, severity, metername, message)
+        result = self._update_or_create_alarm(alarm_name, message, auth_token,
-
+                                              severity, metername)
-        if alarm_id is not None:
-            result = self._update_alarm(alarm_id, message, auth_token)
-        else:
-            result = None
 
         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_or_create_alarm(alarm_name, message,
-                    result = self._update_alarm(alarm_id, message, auth_token)
+                                                      auth_token, severity,
-                else:
+                                                      metername)
-                    result = None
 
         if result.status_code == HTTP_NOT_FOUND:
-            LOGGER.debug("Received 404 error when submitting %s sample, \
+            LOGGER.debug("Received 404 error when submitting %s notification, \
-                         creating a new metric",
+                         creating a new alarm",
                          alarm_name)
 
             # check for and/or get alarm_id
-            alarm_id = self._get_alarm_id(alarm_name, severity,
+            result = self._update_or_create_alarm(alarm_name, message,
-                                          metername, message)
+                                                  auth_token, severity,
-
+                                                  metername)
-            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
 
         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):
+    def _update_or_create_alarm(self, alarm_name, message, auth_token,
-        """Check for existing alarm and its id or create a new one."""
+                                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)
+            result, self._alarm_ids[alarm_name] = \
-                self._alarm_ids[alarm_name] = \
                 self._create_alarm(endpoint, severity,
                                    metername, alarm_name, message)
-            return None
+        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,7 +269,6 @@ 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)
@@ -304,7 +297,6 @@ 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)

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(sender.Sender, '_update_or_create_alarm', autospec=True)
-    @mock.patch.object(requests, 'put', spec=callable)
+    @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,
+    def test_update_or_create_alarm(self, data, config, collectd,
-                    _get_alarm_id):
+                                    ClientV3, meter,
-        """Test collectd data notifying."""
+                                    _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
+        _update_or_create_alarm.return_value = requests.Response()
-        put.return_value.text = 'Updated'
 
-        # Test when the value doesn't get updated
+        # init sender instance
-        _get_alarm_id.return_value = None
+        instance = sender.Sender()
 
-        # init instance
+        _get_alarm_name.return_value = 'my-alarm'
-        instance = plugin.Plugin(collectd=collectd, config=config)
+        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)
+        # send the values
-        instance.notify(data)
+        instance.send(meter_name, severity, resource_id, message)
-        collectd.error.assert_not_called()
 
-        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
+        # reset function
-        _get_alarm_id.return_value = 'my-alarm-id'
+        _update_or_create_alarm.reset_mock()
 
-        # send the value(updates)
+        # run test again for failed attempt
-        instance.notify(data)
+        _update_or_create_alarm.return_value = None
-        collectd.error.assert_not_called()
 
-        # authentication client that has been created
+        instance.send(meter_name, severity, resource_id, message)
-        ClientV3.assert_called_once()
 
         # and values that have been sent
-        put.assert_called_once_with(
+        _update_or_create_alarm.assert_called_once_with(
-            'https://test-aodh.tld' +
+            instance, 'my-alarm', message, auth_client.auth_token,
-            '/v2/alarms/my-alarm-id/state',
+            severity, meter_name)
-            data='"insufficient data"',
-            headers={'Content-type': 'application/json',
-                     'X-Auth-Token': auth_client.auth_token},
-            timeout=1.0)
 
         # reset post method
-        put.reset_mock()
+        _update_or_create_alarm.reset_mock()
-
-        # call shutdown
-        instance.shutdown()
 
     @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(sender.Sender, '_update_or_create_alarm', autospec=True)
-    @mock.patch.object(requests, 'put', spec=callable)
+    @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
+        # init instance attempt to update/create alarm
-        instance.notify(data)
+        instance = sender.Sender()
 
-        # de-assert the non-update request
+        alarm_name = _get_alarm_name.return_value
-        put.assert_not_called()
+        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
+        # send the data
-        _get_alarm_id.return_value = 'my-alarm-id'
+        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
+        # send the data
-        instance.notify(data)
+        instance.send(meter_name, severity, resource_id, message)
 
-        # verify the auth token
+        _update_or_create_alarm.assert_called_once_with(
-        put.assert_called_once_with(
+            instance, alarm_name, message, client.auth_token,
-            mock.ANY, data=mock.ANY,
+            severity, meter_name)
-            headers={u'Content-type': mock.ANY,
-                     u'X-Auth-Token': 'Test auth token'},
-            timeout=1.0)
 
-        # POST response is unauthorized -> new token needs to be acquired
+        # update/create response is unauthorized -> new token needs
-        put.side_effect = [response_unauthorized, response_ok]
+        # 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)
+        # send the data again
-
+        instance.send(meter_name, severity, resource_id, message)
-        # 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')
 
     @mock.patch.object(sender, 'ClientV3', autospec=True)
     @mock.patch.object(plugin, 'Notifier', autospec=True)

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.