From 54e0df2b5efe03f2b3681696057543d276394129 Mon Sep 17 00:00:00 2001
From: Nikolay Stanchev <ns17@it-innovation.soton.ac.uk>
Date: Fri, 29 Mar 2019 10:12:52 +0000
Subject: [PATCH] Code refactoring

---
 src/service/clmcservice/alertsapi/tests.py |  8 +++----
 src/service/clmcservice/alertsapi/views.py | 25 ++++++++++++----------
 src/test/clmctest/alerts/conftest.py       | 18 +---------------
 src/test/clmctest/alerts/test_alerts.py    |  8 ++++---
 4 files changed, 23 insertions(+), 36 deletions(-)

diff --git a/src/service/clmcservice/alertsapi/tests.py b/src/service/clmcservice/alertsapi/tests.py
index 482660c..a4eb816 100644
--- a/src/service/clmcservice/alertsapi/tests.py
+++ b/src/service/clmcservice/alertsapi/tests.py
@@ -323,13 +323,11 @@ class TestAlertsConfigurationAPI(object):
                 kapacitor_response = get("http://{0}:{1}/kapacitor/v1/tasks/{2}".format(kapacitor_host, kapacitor_port, alert_id))
                 assert kapacitor_response.status_code == 404, "Alert with ID {0} was not deleted - test file {1}.".format(alert_id, alerts_test_file)
 
-            # check that all topic IDs were deleted from Kapacitor
-            for topic_id in topic_handlers.keys():
+            # check that all topic IDs and handler IDs were deleted from Kapacitor
+            for topic_id in topic_handlers:
                 kapacitor_response = get("http://{0}:{1}/kapacitor/v1/alerts/topics/{2}".format(kapacitor_host, kapacitor_port, topic_id))
                 assert kapacitor_response.status_code == 404, "Topic with ID {0} was not deleted - test file {1}".format(topic_id, alerts_test_file)
 
-            # check that all handler IDs were deleted from Kapacitor
-            for topic_id in topic_handlers:
                 for handler_id, handler_url in topic_handlers[topic_id]:
                     kapacitor_response = get("http://{0}:{1}/kapacitor/v1/alerts/topics/{2}/handlers/{3}".format(kapacitor_host, kapacitor_port, topic_id, handler_id))
                     assert kapacitor_response.status_code == 404, "Handler with ID {0} for topic with ID {1} was not deleted - test file {2}".format(handler_id, topic_id, alerts_test_file)
@@ -447,7 +445,7 @@ def get_alert_type(event_type, alert_period):
     """
 
     if event_type in AlertsConfigurationAPI.DUAL_VERSION_TEMPLATES:
-        if alert_period <= AlertsConfigurationAPI.STREAM_PERIOD_LIMIT:
+        if alert_period < AlertsConfigurationAPI.STREAM_PERIOD_LIMIT:
             return "stream"
         else:
             return "batch"
diff --git a/src/service/clmcservice/alertsapi/views.py b/src/service/clmcservice/alertsapi/views.py
index 74683bc..b8ee7db 100644
--- a/src/service/clmcservice/alertsapi/views.py
+++ b/src/service/clmcservice/alertsapi/views.py
@@ -249,12 +249,9 @@ class AlertsConfigurationAPI(object):
 
         db = sfc  # database per service function chain, named after the service function chain ID
 
-        # two lists to keep track of any errors while interacting with the Kapacitor HTTP API
-        alert_tasks_errors = []
-        alert_handlers_errors = []
-
-        # iterate through every policy and extract all triggers of the given policy
-        self._config_kapacitor_alerts(tosca_tpl, sfc, sfc_instance, db, kapacitor_host, kapacitor_port, resource_spec_policy_triggers, alert_tasks_errors, alert_handlers_errors)
+        # iterate through every policy and extract all triggers of the given policy - the returned lists of errors will be empty if no errors were encountered
+        # while interacting with the Kapacitor HTTP API
+        alert_tasks_errors, alert_handlers_errors = self._config_kapacitor_alerts(tosca_tpl, sfc, sfc_instance, db, kapacitor_host, kapacitor_port, resource_spec_policy_triggers)
 
         return_msg = {"msg": "Alerts specification has been successfully validated and forwarded to Kapacitor", "service_function_chain_id": sfc,
                       "service_function_chain_instance_id": sfc_instance}
@@ -302,7 +299,7 @@ class AlertsConfigurationAPI(object):
         if len(missing_policy_triggers) > 0:
             raise HTTPBadRequest("Couldn't match the following policy triggers from the alerts specification with triggers defined in the resource specification: {0}".format(missing_policy_triggers))
 
-    def _config_kapacitor_alerts(self, tosca_tpl, sfc, sfc_instance, db, kapacitor_host, kapacitor_port, resource_spec_policy_triggers, alert_tasks_errors, alert_handlers_errors):
+    def _config_kapacitor_alerts(self, tosca_tpl, sfc, sfc_instance, db, kapacitor_host, kapacitor_port, resource_spec_policy_triggers):
         """
         Configures the alerts task and alert handlers within Kapacitor.
 
@@ -313,12 +310,16 @@ class AlertsConfigurationAPI(object):
         :param kapacitor_host: default host is localhost (CLMC service running on the same machine as Kapacitor)
         :param kapacitor_port: default value to use is 9092
         :param resource_spec_policy_triggers: the extracted policy-trigger strings from the resource specification
-        :param alert_tasks_errors: the list for tracking errors while interacting with Kapacitor tasks
-        :param alert_handlers_errors: the list for tracking errors while interacting with Kapacitor alert handlers
+
+        :return: the list for tracking errors while interacting with Kapacitor tasks and the list for tracking errors while interacting with Kapacitor alert handlers
         """
 
         kapacitor_api_tasks_url = "http://{0}:{1}/kapacitor/v1/tasks".format(kapacitor_host, kapacitor_port)
 
+        # two lists to keep track of any errors while interacting with the Kapacitor HTTP API
+        alert_tasks_errors = []
+        alert_handlers_errors = []
+
         for policy in tosca_tpl.policies:
             for trigger in policy.triggers:
                 event_id = trigger.name
@@ -356,7 +357,7 @@ class AlertsConfigurationAPI(object):
 
                 # check whether the template needs to be a stream or a batch
                 if event_type in self.DUAL_VERSION_TEMPLATES:
-                    if alert_period_integer <= self.STREAM_PERIOD_LIMIT:
+                    if alert_period_integer < self.STREAM_PERIOD_LIMIT:
                         template_id = "{0}-stream-template".format(event_type)
                         event_type = "{0}_stream".format(event_type)
                     else:
@@ -399,6 +400,8 @@ class AlertsConfigurationAPI(object):
                 self._config_kapacitor_alert_handlers(kapacitor_host, kapacitor_port, sfc, sfc_instance, policy_id, resource_spec_trigger_id, topic_id, event_id,
                                                       http_handlers, alert_handlers_errors)
 
+        return alert_tasks_errors, alert_handlers_errors
+
     def _config_kapacitor_alert_handlers(self, kapacitor_host, kapacitor_port, sfc, sfc_i, policy_id, trigger_id,
                                          topic_id, event_id, http_handlers, alert_handlers_errors):
         """
@@ -448,7 +451,7 @@ class AlertsConfigurationAPI(object):
 
         :param message: the message to hash
 
-        :return: the value of the has
+        :return: the value of the hash
         """
 
         byte_str = bytes(message, encoding="utf-8")
diff --git a/src/test/clmctest/alerts/conftest.py b/src/test/clmctest/alerts/conftest.py
index 8ba8e39..ae23443 100644
--- a/src/test/clmctest/alerts/conftest.py
+++ b/src/test/clmctest/alerts/conftest.py
@@ -31,13 +31,9 @@ from shutil import rmtree
 from signal import SIGKILL
 from json import load
 from pkg_resources import resource_filename
-from requests import delete, get
 from clmctest.alerts.alert_handler_server import LOG_TEST_FOLDER_PATH
 
 
-KAPACITOR_PORT = 9092
-
-
 @fixture(scope="module")
 def rspec_config():
     """
@@ -55,23 +51,11 @@ def rspec_config():
 
 
 @fixture(scope="module")
-def set_up_tear_down_fixture(rspec_config):
+def set_up_tear_down_fixture():
     """
     Set up/tear down fixture for the alerts integration test.
     """
 
-    global KAPACITOR_PORT
-
-    kapacitor_host = None
-    for host in rspec_config:
-        if host["name"] == "clmc-service":
-            kapacitor_host = host["ip_address"]
-            break
-
-    assert kapacitor_host is not None
-
-    kapacitor_url = "http://{0}:{1}".format(kapacitor_host, KAPACITOR_PORT)
-
     if exists(LOG_TEST_FOLDER_PATH):
         rmtree(LOG_TEST_FOLDER_PATH)  # clean out the log directory
     makedirs(LOG_TEST_FOLDER_PATH)  # create the log directory
diff --git a/src/test/clmctest/alerts/test_alerts.py b/src/test/clmctest/alerts/test_alerts.py
index d1ded93..64a30a3 100644
--- a/src/test/clmctest/alerts/test_alerts.py
+++ b/src/test/clmctest/alerts/test_alerts.py
@@ -84,11 +84,13 @@ class TestAlerts(object):
     def test_alert_triggers(self, rspec_config, set_up_tear_down_fixture):
         """
         Test is implemented using the following steps:
-            * Send clmc service a TOSCA alert spec. file
-            * Wait 15 seconds for Kapacitor to configure and start executing the defined tasks
+            * Send to clmc service a POST request with TOSCA alert spec. and resource spec. files
+            * Wait 10 seconds for Kapacitor to configure and start executing the defined tasks
             * Send some test requests to nginx to increase the load
-            * Wait 20 seconds for alerts to be triggered
+            * Wait 15 seconds for alerts to be triggered
             * Check that 4 log files have been created - one for each alert defined in the alert spec.
+            * Send to clmc service a DELETE request with TOSCA alert spec. file
+            * Check that the returned lists of deleted handlers and alerts are correct
 
         :param rspec_config: fixture from conftest.py
         """
-- 
GitLab