Merge lp:~stub/charms/precise/postgresql/pgtune into lp:charms/postgresql

[Stuart Bishop]

Tune the database using the pgtune tool, rather than attempt to reproduce its logic ourselves.

[Stuart Bishop (stub)]

Note that although I change the default value in config.yaml to 'mixed', we still accept the old 'auto' value so existing environments will not need config changes.

syslinux package seems to be unused and unnecessary, so removing it resolves Bug #1297987.

A bug caused the postgresql-debversion package to not be installed. This has been fixed, which could have confused our redeployments.

The changes in jujufixture.py were me futzing around trying to get test detail reporting working. I'm giving up for now, at least on this branch.

[lp:~stub/charms/precise/postgresql/pgtune]

132. By Stuart Bishop on 2014-04-01

delint

133. By Stuart Bishop on 2014-04-01

Merged tests into pgtune.

134. By Stuart Bishop on 2014-04-01

Remove dead code, fix another test wait race

135. By Stuart Bishop on 2014-04-04

Fix tests and config documentation

[Marco Ceppi (marcoceppi)]

LGTM

review: Approve

=== modified file 'Makefile'
--- Makefile	2014-01-27 12:59:00 +0000
+++ Makefile	2014-04-04 12:07:49 +0000
@@ -1,6 +1,16 @@
 CHARM_DIR := $(shell pwd)
 TEST_TIMEOUT := 900
 
+default:
+	@echo "One of:"
+	@echo "    make lint"
+	@echo "    make test"
+	@echo "    make unit_test"
+	@echo "    make integration_test"
+	@echo "    make integration_test_91"
+	@echo "    make integration_test_92"
+	@echo "    make integration_test_93"
+
 test: lint unit_test integration_test
 
 unit_test:

=== modified file 'config.yaml'
--- config.yaml	2014-04-04 12:07:49 +0000
+++ config.yaml	2014-04-04 12:07:49 +0000
@@ -225,21 +225,25 @@
   work_mem:
     default: "1MB"
     type: string
-    description: Working Memory
+    description: |
+        Working Memory.
+        Ignored unless 'performance_tuning' is set to 'manual'.
   maintenance_work_mem:
     default: "1MB"
     type: string
-    description: Maintenance working memory
+    description: |
+        Maintenance working memory.
+        Ignored unless 'performance_tuning' is set to 'manual'.
   performance_tuning:
-    default: "auto"
+    default: "Mixed"
     type: string
     description: |
-        Possible values here are "auto" or "manual". If we set "auto" then the
-        charm will attempt to automatically tune all the performance parameters
-        for kernel_shmall, kernel_shmmax, shared_buffers and
-        effective_cache_size below, unless those config values are explicitly
-        set. If manual, then it will use the defaults below unless set.
-        "auto" gathers information about the node on which you are deployed and
+        Possible values here are "manual", "DW" (data warehouse),
+        "OLTP" (online transaction processing), "Web" (web application),
+        "Desktop" or "Mixed". When this is set to a value other than
+        "manual", the charm invokes the pgtune tool to tune a number
+        of performance parameters based on the specified load type.
+        pgtune gathers information about the node on which you are deployed and
         tries to make intelligent guesses about what tuning parameters to set
         based on available RAM and CPU under the assumption that it's the only
         significant service running on this node.
@@ -257,13 +261,15 @@
     description: |
         The amount of memory the database server uses for shared memory
         buffers. This string should be of the format '###MB'.
+        Ignored unless 'performance_tuning' is set to 'manual'.
   effective_cache_size:
     default: ""
     type: string
     description: |
         Effective cache size is an estimate of how much memory is available for
         disk caching within the database. (50% to 75% of system memory). This
-        string should be of the format '###MB'.
+        string should be of the format '###MB'. Ignored unless
+        'performance_tuning' is set to 'manual'.
   temp_buffers:
     default: "1MB"
     type: string
@@ -273,11 +279,14 @@
     default: "-1"
     type: string
     description: |
-        min 32kB, -1 sets based on shared_buffers (change requires restart)
+        min 32kB, -1 sets based on shared_buffers (change requires restart).
+        Ignored unless 'performance_tuning' is set to 'manual'.
   checkpoint_segments:
     default: 3
     type: int
-    description: in logfile segments, min 1, 16MB each
+    description: |
+        in logfile segments, min 1, 16MB each.
+        Ignored unless 'performance_tuning' is set to 'manual'.
   random_page_cost:
     default: 4.0
     type: float

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-04-04 12:07:49 +0000
+++ hooks/hooks.py	2014-04-04 12:07:49 +0000
@@ -437,27 +437,18 @@
     return int(run("getconf PAGE_SIZE"))   # frequently 4096
 
 
+def _run_sysctl(postgresql_sysctl):
+    """sysctl -p postgresql_sysctl, helper for easy test mocking."""
+    return run("sysctl -p {}".format(postgresql_sysctl))
+
+
 def create_postgresql_config(config_file):
     '''Create the postgresql.conf file'''
     config_data = hookenv.config()
     if not config_data.get('listen_port', None):
         config_data['listen_port'] = get_service_port()
-    if config_data["performance_tuning"] == "auto":
-        # Taken from:
-        # http://wiki.postgresql.org/wiki/Tuning_Your_PostgreSQL_Server
-        # num_cpus is not being used ... commenting it out ... negronjl
-        #num_cpus = run("cat /proc/cpuinfo | grep processor | wc -l")
+    if config_data["performance_tuning"].lower() != "manual":
         total_ram = _get_system_ram()
-        if not config_data["effective_cache_size"]:
-            config_data["effective_cache_size"] = \
-                "%sMB" % (int(int(total_ram) * 0.75),)
-        if not config_data["shared_buffers"]:
-            if total_ram > 1023:
-                config_data["shared_buffers"] = \
-                    "%sMB" % (int(int(total_ram) * 0.25),)
-            else:
-                config_data["shared_buffers"] = \
-                    "%sMB" % (int(int(total_ram) * 0.15),)
         config_data["kernel_shmmax"] = (int(total_ram) * 1024 * 1024) + 1024
         config_data["kernel_shmall"] = config_data["kernel_shmmax"]
 
@@ -473,7 +464,7 @@
     if config_data["kernel_shmmax"] > 0:
         lines.append("kernel.shmmax = %s\n" % config_data["kernel_shmmax"])
     host.write_file(postgresql_sysctl, ''.join(lines), perms=0600)
-    run("sysctl -p {}".format(postgresql_sysctl))
+    _run_sysctl(postgresql_sysctl)
 
     # If we are replicating, some settings may need to be overridden to
     # certain minimum levels.
@@ -504,10 +495,29 @@
     # Create or update files included from postgresql.conf.
     configure_log_destination(os.path.dirname(config_file))
 
+    tune_postgresql_config(config_file)
+
     local_state['saved_config'] = config_data
     local_state.save()
 
 
+def tune_postgresql_config(config_file):
+    tune_workload = hookenv.config('performance_tuning').lower()
+    if tune_workload == "manual":
+        return  # Requested no autotuning.
+
+    if tune_workload == "auto":
+        tune_workload = "mixed"  # Pre-pgtune backwards compatibility.
+
+    with NamedTemporaryFile() as tmp_config:
+        run(['pgtune', '-i', config_file, '-o', tmp_config.name,
+             '-T', tune_workload,
+             '-c', str(hookenv.config('max_connections'))])
+        host.write_file(
+            config_file, open(tmp_config.name, 'r').read(),
+            owner='postgres', group='postgres', perms=0o600)
+
+
 def create_postgresql_ident(output_file):
     '''Create the pg_ident.conf file.'''
     ident_data = {}
@@ -834,6 +844,17 @@
         log("listen_ip values other than '*' do not work per LP:1271837",
             CRITICAL)
 
+    valid_workloads = [
+        'dw',  'oltp', 'web', 'mixed', 'desktop', 'manual', 'auto']
+    requested_workload = config_data['performance_tuning'].lower()
+    if requested_workload not in valid_workloads:
+        valid = False
+        log('Invalid performance_tuning setting {}'.format(requested_workload),
+            CRITICAL)
+    if requested_workload == 'auto':
+        log("'auto' performance_tuning deprecated. Using 'mixed' tuning",
+            WARNING)
+
     unchangeable_config = [
         'locale', 'encoding', 'version', 'cluster_name', 'pgdg']
 
@@ -1554,11 +1575,13 @@
                 "postgresql-{}".format(version),
                 "postgresql-contrib-{}".format(version),
                 "postgresql-plpython-{}".format(version),
-                "python-jinja2", "syslinux", "python-psycopg2"]
+                "python-jinja2", "python-psycopg2"]
     # PGDG currently doesn't have debversion for 9.3. Put this back when
     # it does.
     if not (hookenv.config('pgdg') and version == '9.3'):
-        "postgresql-{}-debversion".format(version)
+        packages.append("postgresql-{}-debversion".format(version))
+    if hookenv.config('performance_tuning').lower() != 'manual':
+        packages.append('pgtune')
     packages.extend((hookenv.config('extra-packages') or '').split())
     packages = fetch.filter_installed_packages(packages)
     fetch.apt_install(packages, fatal=True)

=== modified file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	2014-01-08 08:55:50 +0000
+++ hooks/test_hooks.py	2014-04-04 12:07:49 +0000
@@ -159,11 +159,11 @@
     def log(self, *args, **kwargs):
         pass
 
-    def config_get(self, scope=None):
+    def config(self, scope=None):
         if scope is None:
-            return self.config
+            return dict(self._config)
         else:
-            return self.config[scope]
+            return self._config[scope]
 
     def relation_get(self, scope=None, unit_name=None, relation_id=None):
         pass
@@ -175,14 +175,13 @@
         hooks.hookenv = TestJuju()
         hooks.host = TestJujuHost()
         hooks.juju_log_dir = self.makeDir()
-        hooks.hookenv.config = lambda: hooks.hookenv._config
-        #hooks.hookenv.localunit = lambda: "localhost"
         hooks.os.environ["JUJU_UNIT_NAME"] = "landscape/1"
         hooks.os.environ["CHARM_DIR"] = os.path.abspath(
             os.path.join(os.path.dirname(__file__), os.pardir))
         hooks.postgresql_sysctl = self.makeFile()
         hooks._get_system_ram = lambda: 1024   # MB
         hooks._get_page_size = lambda: 1024 * 1024  # bytes
+        hooks._run_sysctl = lambda x: ""
         self.maxDiff = None
 
     def assertFileContains(self, filename, lines):
@@ -216,8 +215,8 @@
         C{replication} relations, default wal settings will be present.
         """
         config_outfile = self.makeFile()
-        run = self.mocker.replace(hooks.run)
-        run("sysctl -p %s" % hooks.postgresql_sysctl)
+        _run_sysctl = self.mocker.replace(hooks._run_sysctl)
+        _run_sysctl(hooks.postgresql_sysctl)
         self.mocker.result(True)
         self.mocker.replay()
         hooks.create_postgresql_config(config_outfile)
@@ -240,8 +239,8 @@
         hooks.hookenv._relation_ids = {
             "replication/0": "db-admin:5", "replication/1": "db-admin:6"}
         config_outfile = self.makeFile()
-        run = self.mocker.replace(hooks.run)
-        run("sysctl -p %s" % hooks.postgresql_sysctl)
+        _run_sysctl = self.mocker.replace(hooks._run_sysctl)
+        _run_sysctl(hooks.postgresql_sysctl)
         self.mocker.result(True)
         self.mocker.replay()
         hooks.create_postgresql_config(config_outfile)
@@ -269,8 +268,8 @@
         hooks.hookenv._config["wal_keep_segments"] = 1000
         hooks.hookenv._config["replicated_wal_keep_segments"] = 999
         config_outfile = self.makeFile()
-        run = self.mocker.replace(hooks.run)
-        run("sysctl -p %s" % hooks.postgresql_sysctl)
+        _run_sysctl = self.mocker.replace(hooks._run_sysctl)
+        _run_sysctl(hooks.postgresql_sysctl)
         self.mocker.result(True)
         self.mocker.replay()
         hooks.create_postgresql_config(config_outfile)
@@ -279,79 +278,70 @@
             ["hot_standby = True", "wal_level = hot_standby",
              "max_wal_senders = 3", "wal_keep_segments = 1000"])
 
-    def test_create_postgresql_config_performance_tune_auto_large_ram(self):
-        """
-        When configuration attribute C{performance_tune} is set to C{auto} and
-        total RAM on a system is > 1023MB. It will automatically calculate
-        values for the following attributes if these attributes were left as
-        default values:
-           - C{effective_cache_size} set to 75% of total RAM in MegaBytes
-           - C{shared_buffers} set to 25% of total RAM in MegaBytes
-           - C{kernel_shmmax} set to total RAM in bytes
-           - C{kernel_shmall} equal to kernel_shmmax in pages
-        """
-        config_outfile = self.makeFile()
-        run = self.mocker.replace(hooks.run)
-        run("sysctl -p %s" % hooks.postgresql_sysctl)
-        self.mocker.result(True)
-        self.mocker.replay()
-        hooks.create_postgresql_config(config_outfile)
-        self.assertFileContains(
-            config_outfile,
-            ["shared_buffers = 256MB", "effective_cache_size = 768MB"])
+    def test_auto_tuned_postgresql_config(self):
+        """
+        When automatic performance tuning is specified, pgtune will
+        modify postgresql.conf. Automatic performance tuning is the default.
+        """
+        config_outfile = self.makeFile()
+        _run_sysctl = self.mocker.replace(hooks._run_sysctl)
+        _run_sysctl(hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+
+        hooks.create_postgresql_config(config_outfile)
+
+        raw_config = open(config_outfile, 'r').read()
+        self.assert_('# pgtune wizard' in raw_config)
+
+    def test_auto_tuned_kernel_settings(self):
+        """
+        Kernel settings are automatically set to max RAM values
+        """
+        config_outfile = self.makeFile()
+        _run_sysctl = self.mocker.replace(hooks._run_sysctl)
+        _run_sysctl(hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+
+        hooks.create_postgresql_config(config_outfile)
+
         self.assertFileContains(
             hooks.postgresql_sysctl,
             ["kernel.shmall = 1025\nkernel.shmmax = 1073742848"])
 
-    def test_create_postgresql_config_performance_tune_auto_small_ram(self):
-        """
-        When configuration attribute C{performance_tune} is set to C{auto} and
-        total RAM on a system is <= 1023MB. It will automatically calculate
-        values for the following attributes if these attributes were left as
-        default values:
-           - C{effective_cache_size} set to 75% of total RAM in MegaBytes
-           - C{shared_buffers} set to 15% of total RAM in MegaBytes
-           - C{kernel_shmmax} set to total RAM in bytes
-           - C{kernel_shmall} equal to kernel_shmmax in pages
-        """
-        hooks._get_system_ram = lambda: 1023   # MB
-        config_outfile = self.makeFile()
-        run = self.mocker.replace(hooks.run)
-        run("sysctl -p %s" % hooks.postgresql_sysctl)
-        self.mocker.result(True)
-        self.mocker.replay()
-        hooks.create_postgresql_config(config_outfile)
-        self.assertFileContains(
-            config_outfile,
-            ["shared_buffers = 153MB", "effective_cache_size = 767MB"])
-        self.assertFileContains(
-            hooks.postgresql_sysctl,
-            ["kernel.shmall = 1024\nkernel.shmmax = 1072694272"])
-
-    def test_create_postgresql_config_performance_tune_auto_overridden(self):
-        """
-        When configuration attribute C{performance_tune} is set to C{auto} any
-        non-default values for the configuration parameters below will be used
-        instead of the automatically calculated values.
-           - C{effective_cache_size}
-           - C{shared_buffers}
-           - C{kernel_shmmax}
-           - C{kernel_shmall}
-        """
-        hooks.hookenv._config["effective_cache_size"] = "999MB"
-        hooks.hookenv._config["shared_buffers"] = "101MB"
-        hooks.hookenv._config["kernel_shmmax"] = 50000
-        hooks.hookenv._config["kernel_shmall"] = 500
-        hooks._get_system_ram = lambda: 1023   # MB
-        config_outfile = self.makeFile()
-        run = self.mocker.replace(hooks.run)
-        run("sysctl -p %s" % hooks.postgresql_sysctl)
-        self.mocker.result(True)
-        self.mocker.replay()
-        hooks.create_postgresql_config(config_outfile)
-        self.assertFileContains(
-            config_outfile,
-            ["shared_buffers = 101MB", "effective_cache_size = 999MB"])
-        self.assertFileContains(
-            hooks.postgresql_sysctl,
-            ["kernel.shmall = 1024\nkernel.shmmax = 1072694272"])
+    def test_auto_tuning_preserves_max_connections(self):
+        """
+        pgtune with choose max_connections unless you tell it not too
+        """
+        # Note that the charm does not yet make use of automatic
+        # max_connections. We may want to change the default
+        # max_connections to null and autotune then.
+        hooks.hookenv._config["max_connections"] = 42
+        config_outfile = self.makeFile()
+        _run_sysctl = self.mocker.replace(hooks._run_sysctl)
+        _run_sysctl(hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+
+        hooks.create_postgresql_config(config_outfile)
+
+        raw_config = open(config_outfile, 'r').read()
+        self.assert_('\nmax_connections = 42\n' in raw_config)
+
+    def test_manually_tuned_postgresql_config(self):
+        """
+        When automatic performance tuning is specified, pgtune will
+        modify postgresql.conf. Automatic performance tuning is the default.
+        """
+        hooks.hookenv._config["performance_tuning"] = "maNual"
+        config_outfile = self.makeFile()
+        _run_sysctl = self.mocker.replace(hooks._run_sysctl)
+        _run_sysctl(hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+
+        hooks.create_postgresql_config(config_outfile)
+
+        raw_config = open(config_outfile, 'r').read()
+        self.assert_('# pgtune wizard' not in raw_config)

=== modified file 'test.py'
--- test.py	2014-04-04 12:07:49 +0000
+++ test.py	2014-04-04 12:07:49 +0000
@@ -94,6 +94,10 @@
                                 'database', None)
                             num_masters = 0
                             for unit, unit_rel_info in rel_info.items():
+                                if not unit_rel_info:
+                                    raise NotReady(
+                                        '{} {} is not setup'.format(
+                                            unit, rel_id))
                                 if not unit.startswith('postgresql/'):
                                     continue
                                 if 'user' not in unit_rel_info:

=== modified file 'testing/jujufixture.py'
--- testing/jujufixture.py	2014-04-04 12:07:49 +0000
+++ testing/jujufixture.py	2014-04-04 12:07:49 +0000
@@ -139,11 +139,18 @@
                     self, juju_run_cmd + [
                         'relation-list -r {}'.format(rel_id)]).split()
                 for rel_unit in relation_units:
-                    json_rel_info = run(
-                        self, juju_run_cmd + [
-                            'relation-get --format=json -r {} - {}'.format(
-                                rel_id, rel_unit)])
-                    res[rel_name][rel_id][rel_unit] = json.loads(json_rel_info)
+                    try:
+                        json_rel_info = run(
+                            self, juju_run_cmd + [
+                                'relation-get --format=json -r {} - {}'.format(
+                                    rel_id, rel_unit)])
+                        res[rel_name][rel_id][rel_unit] = json.loads(
+                            json_rel_info)
+                    except subprocess.CalledProcessError as x:
+                        if x.returncode == 2:
+                            res[rel_name][rel_id][rel_unit] = None
+                        else:
+                            raise
         return res
 
     def wait_until_ready(self, extra=60):
@@ -238,27 +245,34 @@
 
 
 def run(detail_collector, cmd, input=''):
+    global _run_seq
+    _run_seq = _run_seq + 1
+
+    # This helper primarily exists to capture the subprocess details,
+    # but this is failing. Details are being captured, but those added
+    # inside the actual test (not setup) are not being reported.
+
+    out, err, returncode = None, None, None
     try:
         proc = subprocess.Popen(
             cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE,
             stderr=subprocess.PIPE)
-    except subprocess.CalledProcessError:
+        (out, err) = proc.communicate(input)
+        returncode = proc.returncode
+        if returncode != 0:
+            raise subprocess.CalledProcessError(returncode, cmd, err)
+        return out
+    except subprocess.CalledProcessError as x:
+        returncode = x.returncode
         raise
-
-    (out, err) = proc.communicate(input)
-    if detail_collector:
-        global _run_seq
-        _run_seq += 1
-        m = {
-            'cmd': ' '.join(cmd),
-            'rc': proc.returncode,
-            'stdout': out,
-            'stderr': err,
-        }
-        detail_collector.addDetail(
-            'run_{}'.format(_run_seq),
-            text_content(yaml.safe_dump(m, default_flow_style=False)))
-    if proc.returncode != 0:
-        raise subprocess.CalledProcessError(
-            proc.returncode, cmd, err)
-    return out
+    finally:
+        if detail_collector is not None:
+            m = {
+                'cmd': ' '.join(cmd),
+                'rc': returncode,
+                'stdout': out,
+                'stderr': err,
+            }
+            detail_collector.addDetail(
+                'run_{}'.format(_run_seq),
+                text_content(yaml.safe_dump(m, default_flow_style=False)))

=== modified file 'tests/00_setup.test'
--- tests/00_setup.test	2014-02-07 07:33:23 +0000
+++ tests/00_setup.test	2014-04-04 12:07:49 +0000
@@ -8,5 +8,6 @@
     python-psycopg2 \
     python-testtools \
     python-twisted-core \
-    python-yaml
+    python-yaml \
+    pgtune