Address PR comments

nginx · Aug 7, 2020 · 9a2d1ee · 9a2d1ee
1 parent c32c914
commit 9a2d1ee
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 59 deletions.
diff --git a/perf-tests/README.md b/perf-tests/README.md
@@ -27,19 +27,18 @@ Run the tests:
     ```bash
     $ cd perf_tests
     $ pip3 install -r requirements.txt
-    $ pytest -v -s -m ap_perf --count=<number_of_times_to_run_the_tests> --node-ip=$(minikube ip) --users=<INT> --hatch_rate=<INT> --time=<INT>
+    $ pytest -v -s -m ap_perf --count=<INT> --node-ip=$(minikube ip) --users=<INT> --hatch-rate=<INT> --time=<INT>
     ```
 
-The tests will use the Ingress Controller for NGINX with the default *nginx/nginx-ingress:edge* image. See the section below to learn how to configure the tests including the image and the type of NGINX -- NGINX or NGINX Plus.
+The tests will use the Ingress Controller for NGINX with the image built from `DockerfileWithAppProtectForPlus`. See the section below to learn how to configure the tests including the image and the type of NGINX -- NGINX or NGINX Plus.
 Refer the [Configuring the Tests](#configuring-the-tests) section for valid arguments.
 
 ## Configuring the Tests
 
 The table below shows various configuration options for the performance tests. Use command line arguments to run tests with Python3
 
-
 | Command-line Argument | Description | Default |
-| :----------------------- | :------------ | :------------ | :----------------------- |
+| :----------------------- | :------------ | :----------------------- |
 | `--context` | The context to use in the kubeconfig file. | `""` |
 | `--image` | The Ingress Controller image. | `nginx/nginx-ingress:edge` |
 | `--image-pull-policy` | The pull policy of the Ingress Controller image. | `IfNotPresent` |
@@ -51,6 +50,7 @@ The table below shows various configuration options for the performance tests. U
 | `N/A` | A path to a folder with a kubeconfig file. | `~/.kube/` |
 | `--show-ic-logs` | A flag to control accumulating IC logs in stdout. | `no` |
 | `N/A` | Any additional pytest command-line arguments (i.e `-m "smoke"`) | `""` |
+| `--count` | Number of times to repeat tests | `1` |
 | `--users` | Total no. of users/locusts for response perf tests. | `10` |
-| `--hatch_rate` | No. of users hatched per second. | `5` |
-| `--time` | Duration for AP response perf tests in seconds. | `10` |
+| `--hatch-rate` | No. of users hatched per second. | `5` |
+| `--time` | Duration for AP response perf tests in seconds. | `10` |
diff --git a/perf-tests/conftest.py b/perf-tests/conftest.py
@@ -73,7 +73,7 @@ def pytest_addoption(parser) -> None:
         "--users", action="store", default="10", help="No. of users for response perf tests",
     )
     parser.addoption(
-        "--hatch_rate", action="store", default="5", help="No. of users hatched per second",
+        "--hatch-rate", action="store", default="5", help="No. of users hatched per second",
     )
     parser.addoption(
         "--time",

diff --git a/perf-tests/suite/test_ap_reload_perf.py b/perf-tests/suite/test_ap_reload_perf.py
@@ -121,13 +121,16 @@ def fin():
 
     return AppProtectSetup(req_url)
 
+
 @pytest.fixture
 def setup_users(request):
     return request.config.getoption("--users")
 
+
 @pytest.fixture
 def setup_rate(request):
-    return request.config.getoption("--hatch_rate")
+    return request.config.getoption("--hatch-rate")
+
 
 @pytest.fixture
 def setup_time(request):
@@ -158,7 +161,22 @@ def assert_invalid_responses(response) -> None:
     ],
     indirect=["crd_ingress_controller_with_ap"],
 )
-class TestAppProtect:
+class TestAppProtectPerf:
+    def collect_prom_reload_metrics(self, metric_list, scenario, ip, port) -> None:
+        req_url = f"http://{ip}:{port}/metrics"
+        resp = requests.get(req_url)
+        resp_decoded = resp.content.decode("utf-8")
+        reload_metric = ""
+        for line in resp_decoded.splitlines():
+            if "last_reload_milliseconds{class" in line:
+                reload_metric = re.findall("\d+", line)[0]
+                metric_list.append(
+                    {
+                        f"Reload time ({scenario}) ": f"{reload_metric}ms",
+                        "TimeStamp": str(datetime.utcnow()),
+                    }
+                )
+
     def test_ap_perf_create_ingress(
         self,
         kube_apis,
@@ -186,19 +204,12 @@ def test_ap_perf_create_ingress(
             appprotect_setup.req_url + "/<script>", headers={"host": ingress_host}, verify=False
         )
         print(response.text)
-        req_url = f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.metrics_port}/metrics"
-        resp = requests.get(req_url)
-        resp_decoded = resp.content.decode("utf-8")
-        reload_metric = ""
-        for line in resp_decoded.splitlines():
-            if "last_reload_milliseconds{class" in line:
-                reload_metric = re.findall("\d+", line)[0]
-                reload_ap.append(
-                    {
-                        f"Reload time (creating AP ingress) ": reload_metric,
-                        "TimeStamp": str(datetime.utcnow()),
-                    }
-                )
+        self.collect_prom_reload_metrics(
+            reload_ap,
+            "creating AP ingress",
+            ingress_controller_endpoint.public_ip,
+            ingress_controller_endpoint.metrics_port,
+        )
         delete_items_from_yaml(kube_apis, src_ing_yaml, test_namespace)
         assert_invalid_responses(response)
 
@@ -226,9 +237,7 @@ def test_ap_perf_ingress_path_change(
         ingress_host = get_first_ingress_host_from_yaml(src1_ing_yaml)
 
         print("--------- Run test while AppProtect module is enabled with correct policy ---------")
-        # events_before_ingress = len(get_events(kube_apis.v1, test_namespace))
         ensure_response_from_backend(appprotect_setup.req_url, ingress_host)
-        # wait_status = wait_for_event_increment(kube_apis, test_namespace, events_before_ingress)
         wait_before_test(30)
         replace_ingress_with_ap_annotations(
             kube_apis,
@@ -247,19 +256,12 @@ def test_ap_perf_ingress_path_change(
             appprotect_setup.req_url + "/v1/<script>", headers={"host": ingress_host}, verify=False
         )
         print(response.text)
-        req_url = f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.metrics_port}/metrics"
-        resp = requests.get(req_url)
-        resp_decoded = resp.content.decode("utf-8")
-        reload_metric = ""
-        for line in resp_decoded.splitlines():
-            if "last_reload_milliseconds{class" in line:
-                reload_metric = re.findall("\d+", line)[0]
-                reload_ap_path.append(
-                    {
-                        f"Reload time (changing paths in AP ingress) ": f"{reload_metric} ms",
-                        "TimeStamp": str(datetime.utcnow()),
-                    }
-                )
+        self.collect_prom_reload_metrics(
+            reload_ap_path,
+            "changing paths in AP ingress",
+            ingress_controller_endpoint.public_ip,
+            ingress_controller_endpoint.metrics_port,
+        )
         delete_items_from_yaml(kube_apis, src2_ing_yaml, test_namespace)
         assert_invalid_responses(response)
 
@@ -300,19 +302,12 @@ def test_ap_perf_multiple_ingress(
             appprotect_setup.req_url + "/<script>", headers={"host": ingress_host}, verify=False
         )
         print(response.text)
-        req_url = f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.metrics_port}/metrics"
-        resp = requests.get(req_url)
-        resp_decoded = resp.content.decode("utf-8")
-        reload_metric = ""
-        for line in resp_decoded.splitlines():
-            if "last_reload_milliseconds{class" in line:
-                reload_metric = re.findall("\d+", line)[0]
-                reload_ap_with_ingress.append(
-                    {
-                        f"Reload time (creating AP ingress alongside a simple ingress) ": f"{reload_metric} ms",
-                        "TimeStamp": str(datetime.utcnow()),
-                    }
-                )
+        self.collect_prom_reload_metrics(
+            reload_ap_with_ingress,
+            "creating AP ingress alongside a simple ingress",
+            ingress_controller_endpoint.public_ip,
+            ingress_controller_endpoint.metrics_port,
+        )
         delete_items_from_yaml(kube_apis, src1_ing_yaml, test_namespace)
         delete_items_from_yaml(kube_apis, src2_ing_yaml, test_namespace)
         assert_invalid_responses(response)
@@ -328,11 +323,11 @@ def test_ap_perf_response(
         enable_prometheus_port,
         test_namespace,
         setup_users,
-        setup_time, 
+        setup_time,
         setup_rate,
     ):
         """
-        Test response times for AP ingress by runnig locust as a subprocess.
+        Test response times for AP ingress by running locust as a subprocess.
         """
 
         src_ing_yaml = f"{TEST_DATA}/appprotect/appprotect-ingress.yaml"
@@ -358,19 +353,19 @@ def test_ap_perf_response(
         subprocess.run(
             [
                 "locust",
-                "-f", 
+                "-f",
                 "suite/ap_request_perf.py",
                 "--headless",
                 "--host",
                 appprotect_setup.req_url,
                 "--csv",
-                "ap_response_times", 
+                "ap_response_times",
                 "-u",
-                setup_users, # total no. of users
+                setup_users,  # total no. of users
                 "-r",
-                setup_rate, # no. of users hatched per second
+                setup_rate,  # no. of users hatched per second
                 "-t",
-                setup_time, # locust session duration in seconds
+                setup_time,  # locust session duration in seconds
             ]
         )
         delete_items_from_yaml(kube_apis, src_ing_yaml, test_namespace)

diff --git a/tests/suite/resources_utils.py b/tests/suite/resources_utils.py
@@ -957,7 +957,7 @@ def create_ingress_with_ap_annotations(
     :param namespace: namespace
     :param policy_name: AppProtect policy
     :param ap_log_st: True/False for enabling/disabling AppProtect security logging
-    :param ap_pol_st: True/False for enabling/disabling AppProtect module for partucular ingress
+    :param ap_pol_st: True/False for enabling/disabling AppProtect module for particular ingress
     :param syslog_ep: Destination endpoint for security logs
     :return:
     """
@@ -987,7 +987,7 @@ def replace_ingress_with_ap_annotations(
     :param namespace: namespace
     :param policy_name: AppProtect policy
     :param ap_log_st: True/False for enabling/disabling AppProtect security logging
-    :param ap_pol_st: True/False for enabling/disabling AppProtect module for partucular ingress
+    :param ap_pol_st: True/False for enabling/disabling AppProtect module for particular ingress
     :param syslog_ep: Destination endpoint for security logs
     :return:
     """