Merge ~peter-sabaini/charm-graylog:add-functtests into ~graylog-charmers/charm-graylog:master

[Peter Sabaini]

Add functional testing

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Joe Guo (guoqiao)]

Hi Peter,
I've added a few inline comments, hope it make sense:)

review: Needs Fixing

[Peter Sabaini (peter-sabaini)]

Hey Joe,

thanks for the review, some answers inline

On 15.01.20 23:14, Joe Guo wrote:
> Review: Needs Fixing
>
> Hi Peter,
> I've added a few inline comments, hope it make sense:)
>
> Diff comments:
>
>> diff --git a/Makefile b/Makefile
>> index 44e05cc..c7f5284 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -36,7 +36,12 @@ unittest: sysdeps lint
>>
>> .PHONY: charmbuild
>> charmbuild:
>> - charm build --output-dir $(BUILDDEST) --report
>> + charm build --output-dir $(BUILDDEST) --report --force
>> +
>> +.PHONY: functional
>> +functional: charmbuild
>> + @echo "Running functional tests tests..."
>
> redundant "tests"

Heh, indeed -- will fix

>> + @tox -e func
>>
>> .PHONY: charmrelease
>> charmrelease:
>> diff --git a/tests/bundles/overlays/local-charm-overlay.yaml.j2 b/tests/bundles/overlays/local-charm-overlay.yaml.j2
>> index b459ab1..d74aac1 100644
>> --- a/tests/bundles/overlays/local-charm-overlay.yaml.j2
>> +++ b/tests/bundles/overlays/local-charm-overlay.yaml.j2
>> @@ -1,3 +1,3 @@
>> applications:
>> - graylog:
>> - charm: ../../../graylog-built/builds/graylog
>> \ No newline at end of file
>> + {{ charm_name }}:
>> + charm: "{{ JUJU_REPOSITORY }}/builds/{{ charm_name }}"
>
> according to https://old-docs.jujucharms.com/2.5/en/reference-environment-variables, JUJU_REPOSITORY is deprecated.

JUJU_REPOSITORY is being used in the Makefile which I didn't want to touch too much at this point. I'm ok with updateing this in a 2nd iteration (I want to rip out the apt-get installs there too). Note I'm not relying on Juju honoring JUJU_REPOSITORY in any way.

>> diff --git a/tests/test_graylog_charm.py b/tests/test_graylog_charm.py
>> new file mode 100644
>> index 0000000..72702d6
>> --- /dev/null
>> +++ b/tests/test_graylog_charm.py
>> @@ -0,0 +1,112 @@
>> +"""Encapsulate graylog testing."""
>> +
>> +import logging
>> +import os
>> +import pathlib
>> +import time
>> +import sys
>> +import unittest
>> +
>> +import zaza.model as model
>> +import zaza.charm_lifecycle.utils as lifecycle_utils
>> +
>> +
>> +def _add_path(path):
>> + if path not in sys.path:
>> + sys.path.insert(1, path)
>> +
>> +
>> +built_charm = pathlib.Path(os.environ["JUJU_REPOSITORY"]) / "builds/graylog"
>> +# Need to avoid importing c.l.graylog as it wants charmhelpers etc.
>> +_add_path(str(built_charm / "lib/charms/layer/graylog"))
>
> 1. Maybe add python paths in `__init__.py`, so other test_xxx.py file can reuse that.

Makes sense, will do.

> 2. I would prefer to build charm first, and then go to the built charm dir to run tests. So the test itself doesn't need to know/hardcode the builds dir path, etc.

I would prefer to be able to update tests without rebuild. Using JUJU_REPOSITORY/builds/{charmname} is an established convention, also honored by "charm build"

> 3. while adding path, just add `lib`, then import module with `from charms.layer import graylog`, thus any layer modules are under the `charms.layer` namespace.

I unfort. can't do this, as this would trigger import of charmhelpers and require a juju runtime env, which are not available on the local system where tests are run

>> +
>> +from api import GraylogApi # noqa...

[Peter Sabaini (peter-sabaini)]

Please take a look time permitting

review: Needs Resubmitting

[Drew Freiberger (afreiberger)]

Please see minor comment regarding replacing the "ubuntu/0" literal and re-allowing for --keep-model.

review: Needs Fixing

[Joe Guo (guoqiao)]

Just notice another minor one, other then that looks good to me.

[Peter Sabaini (peter-sabaini)]

review: Needs Resubmitting

[Zachary Zehring (zzehring)]

A couple of small comments. After addressing, LGTM.

[Peter Sabaini (peter-sabaini)]

Please take a look

review: Needs Resubmitting

[Zachary Zehring (zzehring)]

Approving.

review: Approve

[Paul Goins (vultaire)]

Looks good overall; I'm in favor of merging. Only one concern, and not too serious; I don't want to block merging for it.

[Alvaro Uria (aluria)]

Minor code style suggestions. I won't block on them either. +1.

OTOH, I think IS reviewers is not needed anymore, so feel free to mark it for mergebot to merge.

review: Approve

[Peter Sabaini (peter-sabaini)]

Thanks Paul, answer inline

[Peter Sabaini (peter-sabaini)]

Hey thanks Alvaro, I've addressed some style issues.

I'm a fan of f-string formatting but they're in Python3.6+ only, and in the interest of xenial compat I'll forgo them

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Merge proposal is approved, but source revision has changed, setting status to needs review.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Failed to merge change (no changes to merge), setting status to needs review.

[Jeremy Lounder (jldev)]

review: Approve

diff --git a/.gitignore b/.gitignore
index 874e257..26bbf28 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,6 +1,7 @@
 *.swp
 *~
 .coverage
+.idea/
 .pytest_cache/
 .tox/
 .unit-state.db
diff --git a/Makefile b/Makefile
index 44e05cc..ded904d 100644
--- a/Makefile
+++ b/Makefile
@@ -36,7 +36,12 @@ unittest: sysdeps lint
 
 .PHONY: charmbuild
 charmbuild:
-	charm build --output-dir $(BUILDDEST) --report
+	charm build --output-dir $(BUILDDEST) --report --force
+
+.PHONY: functional
+functional: charmbuild
+	@echo "Running functional tests..."
+	@tox -e func
 
 .PHONY: charmrelease
 charmrelease:
diff --git a/lib/charms/layer/graylog/api.py b/lib/charms/layer/graylog/api.py
index 3f87229..87ded7f 100644
--- a/lib/charms/layer/graylog/api.py
+++ b/lib/charms/layer/graylog/api.py
@@ -32,6 +32,8 @@ class GraylogApi:
         self.token_name = token_name
         self.token = None
         self.input_types = None
+        self.req_timeout = 3
+        self.req_retries = 4
 
     def request(self, path, method='GET', data={}, params=None):
         if path.startswith('/'):
@@ -46,12 +48,12 @@ class GraylogApi:
         if data:
             data = json.dumps(data, indent=True)
         tries = 0
-        while tries < 3:
+        while tries < self.req_retries:
             tries += 1
             try:
                 resp = requests.api.request(method, url, auth=self.auth,
                                             data=data, params=params,
-                                            headers=headers, timeout=3)
+                                            headers=headers, timeout=self.req_timeout)
                 if resp.ok:
                     if method == 'DELETE':
                         return True
diff --git a/tests/__init__.py b/tests/__init__.py
new file mode 100644
index 0000000..9abea7a
--- /dev/null
+++ b/tests/__init__.py
@@ -0,0 +1,13 @@
+import os
+import pathlib
+import sys
+
+
+def _add_path(path):
+    if path not in sys.path:
+        sys.path.insert(1, path)
+
+
+built_charm = pathlib.Path(os.environ["JUJU_REPOSITORY"]) / "builds/graylog"
+# Need to avoid importing c.l.graylog as it wants charmhelpers etc.
+_add_path(str(built_charm / "lib/charms/layer/graylog"))
diff --git a/tests/bundles/bionic-ha.yaml b/tests/bundles/bionic-ha.yaml
index e266cf5..516cff6 100644
--- a/tests/bundles/bionic-ha.yaml
+++ b/tests/bundles/bionic-ha.yaml
@@ -10,7 +10,6 @@ applications:
     num_units: 0
   
   graylog:
-    charm: ../../../graylog-built/builds/graylog
     num_units: 3
     series: bionic
   
@@ -36,4 +35,4 @@ relations:
   - - graylog
     - elastic
   - - graylog
-    - haproxy
\ No newline at end of file
+    - haproxy
diff --git a/tests/bundles/bionic.yaml b/tests/bundles/bionic.yaml
index e196355..b646c7a 100644
--- a/tests/bundles/bionic.yaml
+++ b/tests/bundles/bionic.yaml
@@ -10,7 +10,6 @@ applications:
     num_units: 0
   
   graylog:
-    charm: ../../../graylog-built/builds/graylog
     num_units: 1
     series: bionic
   
@@ -36,4 +35,4 @@ relations:
   - - graylog
     - elastic
   - - graylog
-    - haproxy
\ No newline at end of file
+    - haproxy
diff --git a/tests/bundles/overlays/local-charm-overlay.yaml.j2 b/tests/bundles/overlays/local-charm-overlay.yaml.j2
index b459ab1..d74aac1 100644
--- a/tests/bundles/overlays/local-charm-overlay.yaml.j2
+++ b/tests/bundles/overlays/local-charm-overlay.yaml.j2
@@ -1,3 +1,3 @@
 applications:
-  graylog:
-    charm: ../../../graylog-built/builds/graylog
\ No newline at end of file
+  {{ charm_name }}:
+    charm: "{{ JUJU_REPOSITORY }}/builds/{{ charm_name }}"
diff --git a/tests/requirements.txt b/tests/requirements.txt
index 3da5fb4..b7c9112 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1 +1 @@
-git+https://github.com/openstack-charmers/zaza.git#egg=zaza
\ No newline at end of file
+git+https://github.com/openstack-charmers/zaza.git#egg=zaza
diff --git a/tests/test_graylog_charm.py b/tests/test_graylog_charm.py
new file mode 100644
index 0000000..caea523
--- /dev/null
+++ b/tests/test_graylog_charm.py
@@ -0,0 +1,109 @@
+"""Encapsulate graylog testing."""
+
+import logging
+import time
+import unittest
+
+import zaza.model as model
+
+from api import GraylogApi
+
+
+CURL_TIMEOUT = 180
+REQ_TIMEOUT = 12
+DEFAULT_API_PORT = "9001"
+DEFAULT_WEB_PORT = "9000"
+
+
+class BaseGraylogTest(unittest.TestCase):
+    """Base for Graylog charm tests."""
+
+    @classmethod
+    def setUpClass(cls):
+        super(BaseGraylogTest, cls).setUpClass()
+        cls.model_name = model.get_juju_model()
+        cls.application_name = "graylog"
+        cls.lead_unit_name = model.get_lead_unit_name(
+            cls.application_name, model_name=cls.model_name
+        )
+        cls.units = model.get_units(
+            cls.application_name, model_name=cls.model_name
+        )
+        logging.debug("Leader unit is {}".format(cls.lead_unit_name))
+        cls.graylog_ip = model.get_app_ips(cls.application_name)[0]
+        api_url = "http://{}:{}/api".format(cls.graylog_ip, DEFAULT_API_PORT)
+        action = model.run_action_on_leader(
+            cls.application_name, "show-admin-password"
+        )
+        res = action.data["results"]
+        cls.api = GraylogApi(api_url, "admin", res["admin-password"])
+        cls.api.req_timeout = (
+            REQ_TIMEOUT
+        )  # try harder to get an api connection to cater for overloaded testsystems
+        logging.debug("API at {}".format(api_url))
+
+
+class CharmOperationTest(BaseGraylogTest):
+    """Verify operations"""
+
+    def test_05_api_ready(self):
+        """Test if the API is ready
+
+        Curl the api endpoint on the sentried graylog unit. We'll retry
+        until the CURL_TIMEOUT because it may take a few seconds for the
+        graylog systemd service to start.
+        """
+        curl_command = "curl http://localhost:{}".format(DEFAULT_API_PORT)
+        timeout = time.time() + CURL_TIMEOUT
+        while time.time() < timeout:
+            response = model.run_on_unit(self.lead_unit_name, curl_command)
+            if response["Code"] == "0":
+                return
+            logging.warning(
+                "Unexpected curl response: {}. Retrying in 30s.".format(
+                    response
+                )
+            )
+            time.sleep(30)
+
+        # we didn't get rc=0 in the alloted time, fail the test
+        self.fail(
+            "Graylog didn't respond to the command \n"
+            "'{curl_command}' as expected.\n"
+            "Result: {result}".format(
+                curl_command=curl_command, result=response
+            )
+        )
+
+    def test_06_elasticsearch_healthy(self):
+        resp = self.api.indexer_cluster_health()
+        self.assertEqual(resp["status"], "green")
+
+    def test_07_elasticsearch_no_faults(self):
+        resp = self.api.indexer_failures_count("2020-01-01")
+        self.assertEqual(int(resp["count"]), 0)
+
+    def test_08_plugins(self):
+        resp = self.api.request("/system/plugins")
+        plugin_set = set([p["unique_id"] for p in resp["plugins"]])
+        expected_plugins = {
+            "org.graylog.plugins.pipelineprocessor.ProcessorPlugin",
+            "org.graylog.plugins.beats.BeatsInputPlugin",
+            "org.graylog.plugins.collector.CollectorPlugin",
+        }
+        self.assertTrue(expected_plugins <= plugin_set)
+
+    def test_09_cluster(self):
+        resp = self.api.cluster_get()
+        self.assertGreater(len(resp.values()), 0)
+        node = list(resp.values()).pop()
+        self.assertTrue(node["is_processing"])
+        self.assertEqual(node["lifecycle"], "running")
+
+    def test_10_keyword_search(self):
+        params = {
+            "query": 'file: "/var/log/cloud-init-output.log" AND ci-info',
+            "keyword": "4 hours ago",
+        }
+        resp = self.api.request("/search/universal/keyword", params=params)
+        self.assertTrue(int(resp["total_results"]) > 0)
diff --git a/tests/tests.yaml b/tests/tests.yaml
index 1bb1e58..3a0510d 100644
--- a/tests/tests.yaml
+++ b/tests/tests.yaml
@@ -6,7 +6,9 @@ smoke_bundles:
   - bionic
 dev_bundles:
   - bionic
-configure:
-  - zaza.charm_tests.noop.setup.basic_setup
 tests:
-  - zaza.charm_tests.noop.tests.NoopTest
+  - tests.test_graylog_charm.CharmOperationTest
+target_deploy_status:
+  filebeat:
+    workload-status: active
+    workload-status-message: 'Filebeat ready.'
diff --git a/tox.ini b/tox.ini
index a1f299d..a44fa4a 100644
--- a/tox.ini
+++ b/tox.ini
@@ -17,6 +17,10 @@ deps = -r{toxinidir}/unit_tests/requirements.txt
 [testenv:func]
 commands = functest-run-suite --keep-model
 deps = -r{toxinidir}/tests/requirements.txt
+passenv =
+  HOME
+  JUJU_REPOSITORY
+  MODEL_SETTINGS
 
 [testenv:lint]
 commands = flake8