Merge ~hloeung/charm-graylog:master into ~graylog-charmers/charm-graylog:master

[Haw Loeung]

Add option to tune JVM heap size - LP: #1807565

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

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

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

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[Joel Sing (jsing)]

review: Needs Fixing

[Joel Sing (jsing)]

LGTM, couple of minor comments inline.

review: Approve (+1)

[Haw Loeung (hloeung)]

Minor fixes without having to change existing code.

review: Needs Resubmitting

[🤖 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)]

Change successfully merged at revision efc18399977fbf5e6bd8a6fd2b8d745293b24382

diff --git a/.gitignore b/.gitignore
index ca44404..874e257 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,5 +1,9 @@
-deps/
-__pycache__
-*~
 *.swp
+*~
+.coverage
+.pytest_cache/
+.tox/
+.unit-state.db
+__pycache__
 builds/
+deps/
diff --git a/Makefile b/Makefile
index 6483241..dc50e2b 100644
--- a/Makefile
+++ b/Makefile
@@ -1,21 +1,16 @@
 BUILDDEST:=$(if $(JUJU_REPOSITORY),$(JUJU_REPOSITORY),"../graylog-built")
 BUILTCHARMDIR:="$(BUILDDEST)/builds/graylog"
 
-PYTHON_SCRIPTS = \
-	./actions/actions.py \
-	./files/check_graylog_health.py \
-	./lib/graylogapi.py \
-	./reactive/graylog.py \
-	./tests/*.py \
-	./unit_tests/graylog.py
-
 all: unittest
 
 .PHONY: clean
 clean:
+	@echo "Cleaning files"
 	@rm -f .unit-state.db
 	@find . -name "*.pyc" -type f -exec rm -f '{}' \;
 	@find . -name "__pycache__" -type d -prune -exec rm -rf '{}' \;
+	@rm -rf ./.tox
+	@rm -rf ./.pytest_cache
 
 .PHONY: sysdeps
 sysdeps:
@@ -35,12 +30,12 @@ testdeps:
 .PHONY: lint
 lint: sysdeps
 	@echo "Checking Python syntax..."
-	@flake8 --max-complexity=10 --filename=\* --ignore=E501,E402 $(PYTHON_SCRIPTS) && echo OK
+	@tox -e lint
 
 .PHONY: unittest
 unittest: sysdeps lint
 	@echo "Running unit tests..."
-	nosetests3 unit_tests/*
+	@tox -e unit
 
 .PHONY: charmbuild
 charmbuild:
diff --git a/config.yaml b/config.yaml
index 451f73f..21e76c6 100644
--- a/config.yaml
+++ b/config.yaml
@@ -88,3 +88,8 @@ options:
     default: 5044
     type: int
     description: TCP port for Beats input when relation is joined
+  jvm_heap_size:
+    default: "1G"
+    type: string
+    description: |
+      JVM Heap memory size (default 1G)
diff --git a/files/check_graylog_health.py b/files/check_graylog_health.py
index 5842727..a349d4a 100755
--- a/files/check_graylog_health.py
+++ b/files/check_graylog_health.py
@@ -11,7 +11,7 @@ libs_dir = os.path.abspath(os.path.join(os.path.dirname(__file__),
 if libs_dir not in sys.path:
     sys.path.append(libs_dir)
 
-from graylogapi import GraylogAPI
+from graylogapi import GraylogAPI  # NOQA: E402
 
 STATE_OK = 0
 STATE_WARNING = 1
diff --git a/lib/logextract.py b/lib/logextract.py
index 07e4c6f..2447da4 100644
--- a/lib/logextract.py
+++ b/lib/logextract.py
@@ -4,6 +4,7 @@ extraction
 
 import re
 
+
 class GraylogTitleSourceItems:
 
     def __init__(self, graylogapi=None):
@@ -22,7 +23,7 @@ class GraylogTitleSourceItems:
     def post(self, title, source):
         return self.graylogapi.request(
             self.url, method='POST',
-            data={'title' : title, 'source': source})
+            data={'title': title, 'source': source})
 
     def delete(self, title):
         id_ = self.get_for_title(title)
@@ -44,17 +45,16 @@ class GraylogTitleSourceItems:
         else:
             self.post(title, source)
 
-class GraylogPipelines(GraylogTitleSourceItems):
 
+class GraylogPipelines(GraylogTitleSourceItems):
     url = '/plugins/org.graylog.plugins.pipelineprocessor/system/pipelines/pipeline'
 
-class GraylogRules(GraylogTitleSourceItems):
 
+class GraylogRules(GraylogTitleSourceItems):
     url = '/plugins/org.graylog.plugins.pipelineprocessor/system/pipelines/rule'
 
 
 class GraylogStreams:
-
     def __init__(self, graylogapi=None):
         self.graylogapi = graylogapi
 
@@ -85,20 +85,21 @@ class GraylogStreams:
         if pipeline:
             self.graylogapi.request(url, method="POST",
                                     data={"stream_id": streamid,
-                                          "pipeline_ids" : [pipeline['id']]})
-
+                                          "pipeline_ids": [pipeline['id']]})
 
 
 class LogExtractPipelineException(Exception):
     """Error handling log extraction
     """
 
+
 _logextract_pipe_template = '''
 pipeline "{}"
 stage 0 match either
 {}
 end'''
 
+
 class LogExtractPipeline:
     """Manage a pipeline with rules for log extraction
     The pipeline comprises a single stage which holds
@@ -139,4 +140,3 @@ class LogExtractPipeline:
         pipesrc = _logextract_pipe_template.format(self.title, ''.join(titlerefs))
         self.pipelines.set(self.title, pipesrc)
         self.streams.connect_pipeline(self.title, self.all_messages)
-
diff --git a/reactive/graylog.py b/reactive/graylog.py
index 0ce774b..c3895ff 100644
--- a/reactive/graylog.py
+++ b/reactive/graylog.py
@@ -18,11 +18,15 @@ if libs_dir not in sys.path:
     sys.path.append(libs_dir)
 
 from graylogapi import GraylogAPI  # NOQA: E402
-import logextract
+import logextract  # NOQA: E402
 
 API_PORT = '9001'  # This is the default set by the snap
 API_URL = 'http://127.0.0.1:9001/api/'  # This is the default set by the snap
 CONF_FILE = '/var/snap/graylog/common/server.conf'
+# /snap/graylog is a read-only squashfs so we use the initial settings
+# and write out an override to SERVER_DEFAULT_CONF_FILE
+SHIPPED_SNAP_SERVER_DEFAULT_CONF_FILE = '/snap/graylog/current/etc/default/graylog-server'
+SERVER_DEFAULT_CONF_FILE = '/var/snap/graylog/current/default-graylog-server'
 ELASTICSEARCH_DISCOVERY_PORT = '9300'
 SERVICE_NAME = 'snap.graylog.graylog'
 
@@ -97,6 +101,8 @@ def configure_graylog():
         if rules_config and isinstance(rules_config, list):
             _configure_logextraction(rules_config)
 
+    set_jvm_heap_size(conf['jvm_heap_size'])
+
     remove_state('graylog_api.configured')
     set_state('graylog.configured')
     report_status()
@@ -530,7 +536,7 @@ def set_conf(key, value, conf_path=CONF_FILE):
         so setting the configuration this way avoids having to regenerate those values.
     """
     conf = []
-    key_re = re.compile('^#*\s*{}'.format(key))
+    key_re = re.compile('^#*\\s*{}'.format(key))
 
     changed = False
     replaced = False
@@ -563,6 +569,64 @@ def set_conf(key, value, conf_path=CONF_FILE):
         with open(conf_path, 'wb') as conf_file:
             conf_file.write('\n'.join(conf).encode())
         return True
+
+    return False
+
+
+def parse_java_opts(opts, size):
+    foundXms = False
+    foundXmx = False
+    new_opts = []
+    for opt in opts.split():
+        if opt.startswith('-Xms'):
+            foundXms = True
+            if size == '':
+                continue
+            new_opts.append('-Xms{}'.format(size))
+        elif opt.startswith('-Xmx'):
+            foundXmx = True
+            if size == '':
+                continue
+            new_opts.append('-Xmx{}'.format(size))
+        else:
+            new_opts.append(opt)
+    if not foundXms:
+        new_opts.append('-Xms{}'.format(size))
+    if not foundXmx:
+        new_opts.append('-Xmx{}'.format(size))
+    return new_opts
+
+
+def set_jvm_heap_size(heap_size='1G', conf_path=SERVER_DEFAULT_CONF_FILE):
+    size = heap_size.lower()
+    conf = []
+    key_re = re.compile('^\\s*GRAYLOG_SERVER_JAVA_OPTS\\s*="(.*)"$')
+    changed = False
+    # If specified /etc/default/graylog-server doesn't exist, use the default
+    # file shipped out by the snap
+    if not os.path.exists(conf_path):
+        updated_conf_path = SHIPPED_SNAP_SERVER_DEFAULT_CONF_FILE
+    else:
+        updated_conf_path = conf_path
+    with open(updated_conf_path, 'rb') as conf_file:
+        for line in conf_file:
+            line = line.decode()
+            m = key_re.match(line)
+            if not m:
+                conf.append(line)
+                continue
+            new_opts = parse_java_opts(m.group(1), size)
+            new_line = 'GRAYLOG_SERVER_JAVA_OPTS="{}"\n'.format(' '.join(new_opts))
+            if new_line != line:
+                hookenv.log('Updating GRAYLOG_SERVER_JAVA_OPTS="{}".'.format(''.join(new_opts)))
+                changed = True
+            conf.append(new_line)
+
+    if changed:
+        set_state('graylog.needs_restart')
+        with open(conf_path, 'wb') as conf_file:
+            conf_file.write(''.join(conf).encode())
+        return True
     return False
 
 
@@ -586,7 +650,7 @@ def configure_nagios(nagios):
     if conf['web_listen_uri']:
         url = urlparse(conf['web_listen_uri'])
         # LP:#1706265: Work around this bug by escaping some characters.
-        check_string = '\<title\>Graylog\ Web\ Interface\</title\>'
+        check_string = '\\<title\\>Graylog\\ Web\\ Interface\\</title\\>'
         url_path = url.path
         # Catch URI without an explicit path
         if not url_path:
@@ -597,7 +661,8 @@ def configure_nagios(nagios):
         nrpe_setup.add_check(
             'graylog_http',
             'Graylog Web UI check',
-            '/usr/lib/nagios/plugins/check_http -I {} -p {} -u "{}" -s "{}"'.format(url.hostname, str(url.port), url_path, check_string)
+            '/usr/lib/nagios/plugins/check_http -I {} -p {} -u "{}" -s "{}"'
+            .format(url.hostname, str(url.port), url_path, check_string)
         )
     check_string = 'cluster_id'
     nrpe_setup.add_check(
diff --git a/requirements.txt b/requirements.txt
 new file mode 100644
index 0000000..8462291
--- /dev/null
+++ b/requirements.txt
@@ -0,0 +1 @@
+# Include python requirements here
diff --git a/tox.ini b/tox.ini
 new file mode 100644
index 0000000..1cdd9ba
--- /dev/null
+++ b/tox.ini
@@ -0,0 +1,27 @@
+[tox]
+skipsdist=True
+envlist = unit
+skip_missing_interpreters = True
+
+[testenv]
+basepython = python3
+setenv =
+  PYTHONPATH = .
+
+[testenv:unit]
+commands = pytest -v --ignore {toxinidir}/tests --cov=lib --cov=reactive --cov=actions --cov-report=term-missing --cov-branch
+deps = -r{toxinidir}/unit_tests/requirements.txt
+       -r{toxinidir}/requirements.txt
+setenv = PYTHONPATH={toxinidir}/lib
+
+[testenv:lint]
+commands = flake8
+deps = flake8
+
+[flake8]
+exclude =
+    .git,
+    __pycache__,
+    .tox,
+max-line-length = 120
+max-complexity = 10
diff --git a/unit_tests/requirements.txt b/unit_tests/requirements.txt
 new file mode 100644
index 0000000..29f9318
--- /dev/null
+++ b/unit_tests/requirements.txt
@@ -0,0 +1,6 @@
+charmhelpers
+charms.reactive
+mock
+pytest
+pytest-cov
+requests
diff --git a/unit_tests/graylog.py b/unit_tests/test_graylog.py
 similarity index 75%
 rename from unit_tests/graylog.py
 rename to unit_tests/test_graylog.py
index 3394201..79cdaf0 100644
--- a/unit_tests/graylog.py
+++ b/unit_tests/test_graylog.py
@@ -8,9 +8,10 @@ sys.path.append(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))
 
 from reactive.graylog import (
     set_conf,
-    _check_input_exists)
-from lib.graylogapi import GraylogAPI
-from files import check_graylog_health
+    set_jvm_heap_size,
+    _check_input_exists)  # NOQA: E402
+from lib.graylogapi import GraylogAPI  # NOQA: E402
+from files import check_graylog_health  # NOQA: E402
 
 initial_conf = u"""#key1 = value1
 key2 = value2
@@ -173,3 +174,93 @@ class TestHealthCheck(unittest.TestCase):
                     'uncommitted_journal_entries': ucommitted}
                 state, _ = check_graylog_health.journal_state(mock_api, args)
                 self.assertEqual(state, exp_state)
+
+
+etc_default_conf = u"""
+# Path to the java executable.
+JAVA=/usr/bin/java
+
+# Default Java options for heap and garbage collection.
+{}
+
+# Pass some extra args to graylog-server. (i.e. "-d" to enable debug mode)
+GRAYLOG_SERVER_ARGS=""
+
+# Program that will be used to wrap the graylog-server command. Useful to
+# support programs like authbind.
+GRAYLOG_COMMAND_WRAPPER=""
+"""
+
+heap_default = u"""
+GRAYLOG_SERVER_JAVA_OPTS="-Xms1g -Xmx1g -XX:NewRatio=1 -server -XX:+ResizeTLAB -XX:+UseConcMarkSweepGC \
+-XX:+CMSConcurrentMTEnabled -XX:+CMSClassUnloadingEnabled -XX:+UseParNewGC -XX:-OmitStackTraceInFastThrow"
+"""
+
+heap_2g = u"""
+GRAYLOG_SERVER_JAVA_OPTS="-Xms2g -Xmx2g -XX:NewRatio=1 -server -XX:+ResizeTLAB -XX:+UseConcMarkSweepGC \
+-XX:+CMSConcurrentMTEnabled -XX:+CMSClassUnloadingEnabled -XX:+UseParNewGC -XX:-OmitStackTraceInFastThrow"
+"""
+
+heap_not_present = u"""
+GRAYLOG_SERVER_JAVA_OPTS="-XX:NewRatio=1 -server -XX:+ResizeTLAB -XX:+UseConcMarkSweepGC -XX:+CMSConcurrentMTEnabled \
+-XX:+CMSClassUnloadingEnabled -XX:+UseParNewGC -XX:-OmitStackTraceInFastThrow -Xms4g -Xmx4g"
+"""
+
+heap_unset = u"""
+GRAYLOG_SERVER_JAVA_OPTS="-XX:NewRatio=1 -server -XX:+ResizeTLAB -XX:+UseConcMarkSweepGC -XX:+CMSConcurrentMTEnabled \
+-XX:+CMSClassUnloadingEnabled -XX:+UseParNewGC -XX:-OmitStackTraceInFastThrow"
+"""
+
+heap_not_present = u"""
+GRAYLOG_SERVER_JAVA_OPTS="-XX:NewRatio=1 -server -XX:+ResizeTLAB -XX:+UseConcMarkSweepGC -XX:+CMSConcurrentMTEnabled \
+-XX:+CMSClassUnloadingEnabled -XX:+UseParNewGC -XX:-OmitStackTraceInFastThrow -Xms4g -Xmx4g"
+"""
+
+heap_twice = u"""
+GRAYLOG_SERVER_JAVA_OPTS="-XX:NewRatio=1 -server -XX:+ResizeTLAB -XX:+UseConcMarkSweepGC -XX:+CMSConcurrentMTEnabled \
+-XX:+CMSClassUnloadingEnabled -XX:+UseParNewGC -XX:-OmitStackTraceInFastThrow -Xms8g -Xmx8g"
+"""
+
+
+class TestSetJVMHeapSize(unittest.TestCase):
+
+    def setUp(self):
+        temp_file = tempfile.NamedTemporaryFile(delete=False)
+        temp_file.write(etc_default_conf.format(heap_default).encode())
+        temp_file.close()
+        self.conf_file = temp_file.name
+
+    def tearDown(self):
+        os.remove(self.conf_file)
+
+    def conf_equals(self, want):
+        with open(self.conf_file, 'r') as conf:
+            got = conf.read()
+            if got == want:
+                return True
+            print('{}\n--- != ---\n{}'.format(got, want))
+        return False
+
+    def test_same(self):
+        set_jvm_heap_size('1g', self.conf_file)
+        self.assertTrue(self.conf_equals(etc_default_conf.format(heap_default)))
+
+    def test_update(self):
+        set_jvm_heap_size('2g', self.conf_file)
+        self.assertTrue(self.conf_equals(etc_default_conf.format(heap_2g)))
+
+    def test_unset(self):
+        # Remove Xms and Xmx
+        set_jvm_heap_size('', self.conf_file)
+        self.assertTrue(self.conf_equals(etc_default_conf.format(heap_unset)))
+
+    def test_add(self):
+        set_jvm_heap_size('', self.conf_file)
+        set_jvm_heap_size('4g', self.conf_file)
+        self.assertTrue(self.conf_equals(etc_default_conf.format(heap_not_present)))
+
+    def test_add_twice(self):
+        set_jvm_heap_size('', self.conf_file)
+        set_jvm_heap_size('6g', self.conf_file)
+        set_jvm_heap_size('8g', self.conf_file)
+        self.assertTrue(self.conf_equals(etc_default_conf.format(heap_twice)))
diff --git a/unit_tests/test_logextract.py b/unit_tests/test_logextract.py
index fd04bf1..69d87d8 100644
--- a/unit_tests/test_logextract.py
+++ b/unit_tests/test_logextract.py
@@ -2,20 +2,19 @@ import unittest
 from unittest import mock
 
 from lib import logextract
-from lib.graylogapi import GraylogAPI
 
 
 testdata_simple_pipeline_source = '\npipeline "testpipe"\nstage 1 match either\n  rule "test-rule";\nend\n'
 
 testdata_simple_pipeline = [{'description': None, 'stages':
-                    [{'match_all': False, 'rules':
-                    ['test-rule'], 'stage': 1}],
-                    'source': testdata_simple_pipeline_source,
-                    'created_at': '2018-06-18T14:49:27.427Z',
-                    'modified_at': '2018-06-18T14:49:27.427Z',
-                    'id': '5b27c6778ac7b4647301a562',
-                    'errors': None,
-                    'title': 'juju-logextract-pipeline'}]
+                             [{'match_all': False, 'rules':
+                               ['test-rule'], 'stage': 1}],
+                             'source': testdata_simple_pipeline_source,
+                             'created_at': '2018-06-18T14:49:27.427Z',
+                             'modified_at': '2018-06-18T14:49:27.427Z',
+                             'id': '5b27c6778ac7b4647301a562',
+                             'errors': None,
+                             'title': 'juju-logextract-pipeline'}]
 
 testdata_streams = {'total': 1,
                     'streams': [
@@ -30,18 +29,19 @@ testdata_rules = [
   {
     "title": "from ceph mon",
     "description": "",
-    "source": "rule \"from ceph mon\"\nwhen\n  has_field(\"logdir\") && $message.logdir == \"ceph\" && regex(\"^ceph-mon\", to_string($message.juju_principal_unit)).matches == true\nthen\n  set_field(\"subsystem\", \"ceph-mon\");\nend;\n",
+    "source": "rule \"from ceph mon\"\nwhen\n  has_field(\"logdir\") && $message.logdir == \"ceph\" && regex(\"^ceph-mon\", to_string($message.juju_principal_unit)).matches == true\nthen\n  set_field(\"subsystem\", \"ceph-mon\");\nend;\n",  # NOQA: E501
     "created_at": "2018-06-17T14:32:13.632Z",
     "modified_at": "2018-06-17T14:42:25.818Z",
     "errors": None,
     "id": "5b2670ed6140b943aef99ae9"
-  },]
+  }]
 
 testdata_stream_connect = {
     "stream_id": "000000000000000000000001",
-    "pipeline_ids" : ["5b35061c6140b943ae096769"]
+    "pipeline_ids": ["5b35061c6140b943ae096769"]
 }
 
+
 class TestGraylogPipelines(unittest.TestCase):
 
     @mock.patch('lib.graylogapi.GraylogAPI')
@@ -69,7 +69,6 @@ class TestGraylogRules(unittest.TestCase):
         self.assertEqual(rules[0]['title'], "from ceph mon")
 
 
-
 class TestGraylogStreams(unittest.TestCase):
 
     @mock.patch('lib.graylogapi.GraylogAPI')
@@ -77,18 +76,16 @@ class TestGraylogStreams(unittest.TestCase):
         mock_api.request.return_value = testdata_streams
         gstreams = logextract.GraylogStreams(mock_api)
         allmsgs = gstreams.get_stream_id_for("All messages")
-        self.assertEqual(allmsgs,
-                          testdata_streams['streams'][0]['id'])
+        self.assertEqual(allmsgs, testdata_streams['streams'][0]['id'])
         nope = gstreams.get_stream_id_for("I don't exist")
         self.assertIsNone(nope)
 
+
 class TestLogExtractPipeline(unittest.TestCase):
 
     @mock.patch('lib.graylogapi.GraylogAPI')
     def test_get_rules(self, mock_api):
         mock_api.request.return_value = testdata_simple_pipeline
-        logext = logextract.LogExtractPipeline(mock_api)
-        rules = logext.get_rules()
 
     def test_parse_rule_title(self):
         logext = logextract.LogExtractPipeline()