Merge ~stub/postgresql-charm:upgrade-series into postgresql-charm:master

[Stuart Bishop]

upgrade-series support, resurrect unit tests

The charm would already probably survive a series upgrade, but
these changes should ensure the service always gets back up on
its feet. PostgreSQL major version upgrades might even work too,
although that would be better left to an action.

Also gets the unit tests back running, which had atrophied.
They are still rather awkward to run (they need to run in the
built charm directory), but are passing again.

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

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

[Barry Price (barryprice)]

Couple of nitpick questions on the Makefile, otherwise LGTM

review: Approve

[Barry Price (barryprice)]

Actually save the second of those questions :)

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

Change cannot be self approved, setting status to needs review.

diff --git a/Makefile b/Makefile
index b05ce5b..089e02e 100644
--- a/Makefile
+++ b/Makefile
@@ -27,24 +27,7 @@ _success_ex:
 	true | ts
 
 
-# Munge the path so the requested version of Juju is found, and thus used
-# by Amulet and juju-deployer.
-export PATH := /usr/lib/juju-$(shell $(JUJU) --version | perl -p -e "s/-.*//")/bin:$(PATH)
-
-
-test: testdeps lint unittest integration
-
-testdeps:
-ifeq ($(HOST_SERIES),trusty)
-	sudo apt-get install -y python-tox python3-psycopg2 bzr moreutils \
-	    software-properties-common
-else
-	sudo apt-get install -y tox python3-psycopg2 bzr moreutils \
-	    software-properties-common
-endif
-	sudo add-apt-repository -y ppa:juju/stable
-	sudo apt-get install charm-tools
-
+test: lint unittest integration
 
 CHARM_NAME := postgresql
 
@@ -69,7 +52,7 @@ $(BUILD_DIR):
 # updates.
 .PHONY: build
 build: | $(BUILD_DIR)
-	charm build -f -o $(BUILD_ROOT) -n $(CHARM_NAME)
+	charm build -f -d $(BUILD_DIR)/.. -n $(CHARM_NAME)
 
 # Generate a fresh development build and commit it to $(TEST_BRANCH).
 # Only builds work committed to $(LAYER_BRANCH).
diff --git a/reactive/postgresql/postgresql.py b/reactive/postgresql/postgresql.py
index 373080d..35d3e02 100644
--- a/reactive/postgresql/postgresql.py
+++ b/reactive/postgresql/postgresql.py
@@ -79,6 +79,13 @@ def version():
     if version:
         return version
 
+    # If no cached version, use the version reported by
+    # pg_lsclusters
+    version = lsclusters_version()
+    if version:
+        unitdata.kv().set("postgresql.pg_version", version)
+        return version
+
     # We use the charm configuration here, as multiple versions
     # of PostgreSQL may be installed.
     config = hookenv.config()
@@ -98,6 +105,19 @@ def version():
     return version
 
 
+def clear_version_cache():
+    unitdata.kv().set("postgresql.pg_version", None)
+
+
+def lsclusters_version():
+    if not os.path.exists("/usr/bin/pg_lsclusters"):
+        return None
+    results = subprocess.check_output(["pg_lsclusters", "--no-header"]).decode().splitlines()
+    if len(results) == 1:
+        return results[0].split()[0]
+    return None
+
+
 def point_version():
     """PostgreSQL version. major.minor.patch or major.patch, as a string."""
     output = subprocess.check_output([postgres_path(), "-V"], universal_newlines=True)
@@ -473,10 +493,7 @@ def ensure_role(con, role):
 def ensure_extensions(con, extensions):
     """extensions is a list of (name, schema) tuples"""
     cur = con.cursor()
-    cur.execute(
-        """SELECT extname,nspname FROM pg_extension,pg_namespace
-                   WHERE pg_namespace.oid = extnamespace"""
-    )
+    cur.execute("SELECT extname, nspname FROM pg_extension, pg_namespace WHERE pg_namespace.oid = extnamespace")
     installed_extensions = frozenset((x[0], x[1]) for x in cur.fetchall())
     hookenv.log("ensure_extensions({}), have {}".format(extensions, installed_extensions), DEBUG)
     extensions_set = frozenset(set(extensions))
diff --git a/reactive/postgresql/service.py b/reactive/postgresql/service.py
index b7269b1..f5e1936 100644
--- a/reactive/postgresql/service.py
+++ b/reactive/postgresql/service.py
@@ -45,6 +45,18 @@ def install():
         reactive.set_flag("postgresql.upgrade.systemd")
 
 
+@hook("post-series-upgrade")
+def post_series_upgrade():
+    postgresql.clear_version_cache()  # PG version upgrades should work on the master, but will break standbys
+    config = hookenv.config()
+    if config["pgdg"]:
+        add_pgdg_source()
+    if host.init_is_systemd():
+        reactive.set_flag("postgresql.upgrade.systemd")
+    reactive.clear_flag("postgresql.cluster.support-scripts")
+    reactive.clear_flag("postgresql.cluster.configured")
+
+
 @when("config.changed")
 def config_changed():
     reactive.clear_flag("postgresql.cluster.support-scripts")
@@ -143,12 +155,23 @@ def configure_sources():
 
     # Shortcut for the PGDG archive.
     if config["pgdg"]:
-        pgdg_url = "http://apt.postgresql.org/pub/repos/apt/"
-        pgdg_src = "deb {} {}-pgdg main".format(pgdg_url, helpers.distro_codename())
-        pgdg_key_path = os.path.join(hookenv.charm_dir(), "lib", "pgdg.key")
+        add_pgdg_source()
+
+
+def add_pgdg_source():
+    pgdg_url = "http://apt.postgresql.org/pub/repos/apt/"
+    pgdg_src = "deb {} {}-pgdg main".format(pgdg_url, helpers.distro_codename())
+    pgdg_key_path = os.path.join(hookenv.charm_dir(), "lib", "pgdg.key")
+    kv = unitdata.kv()
+    k = "postgresql.service.pgdg_src"
+    # Add the source if the key has changed, or if the src has
+    # changed such as a distro upgrade. Check key change first,
+    # or short circuiting will add the source twice.
+    if reactive.helpers.any_file_changed([pgdg_key_path]) or pgdg_src != kv.get(k):
         with open(pgdg_key_path, "r") as f:
             hookenv.log("Adding PGDG archive")
             apt.add_source(pgdg_src, f.read())
+        kv.set(k, pgdg_src)
 
 
 @when("postgresql.cluster.locale.set")
@@ -192,13 +215,13 @@ def create_cluster():
 def create_pg_ctl_conf():
     contents = textwrap.dedent(
         """\
-                        # Managed by Juju
-                        # Automatic pg_ctl configuration
-                        # This configuration file contains cluster specific options to be passed to
-                        # pg_ctl(1).
+        # Managed by Juju
+        # Automatic pg_ctl configuration
+        # This configuration file contains cluster specific options to be passed to
+        # pg_ctl(1).
 
-                        pg_ctl_options = '-w -t 3600'
-                        """
+        pg_ctl_options = '-w -t 3600'
+        """
     )
     helpers.write(
         postgresql.pg_ctl_conf_path(),
diff --git a/reactive/postgresql/upgrade.py b/reactive/postgresql/upgrade.py
index 9379c60..d056319 100644
--- a/reactive/postgresql/upgrade.py
+++ b/reactive/postgresql/upgrade.py
@@ -26,6 +26,7 @@ from reactive import workloadstatus
 from reactive.postgresql import helpers
 from reactive.postgresql import postgresql
 from reactive.postgresql import replication
+from reactive.postgresql import service
 from reactive.postgresql import storage
 
 
@@ -134,6 +135,11 @@ def upgrade_charm():
             reactive.clear_flag("postgresql.cluster.is_running")
             postgresql.stop_pgctlcluster()
 
+    # Update the PGDG source, in case the signing key has changed.
+    config = hookenv.config()
+    if config["pgdg"]:
+        service.add_pgdg_source()
+
 
 def migrate_user(old_username, new_username, password, superuser=False):
     if postgresql.is_primary():
diff --git a/requirements.txt b/requirements.txt
index 8aabe77..675a41a 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -11,5 +11,6 @@ python-swiftclient
 charms.reactive
 charmhelpers
 juju-deployer
-pip<8.1.2
-websocket-client<=0.40.0
+pip
+websocket-client
+tenacity
diff --git a/tox.ini b/tox.ini
index 71565ed..cb30fb6 100644
--- a/tox.ini
+++ b/tox.ini
@@ -29,9 +29,10 @@ commands=
 basepython=python3
 # pip install -I required to ensure scripts are created
 # even if packages are already installed on the system
-install_command=pip install -I {opts} {packages}
-sitepackages=True
-deps = -r{toxinidir}/requirements.txt
+sitepackages=False
+deps =
+    -r{toxinidir}/requirements.txt
+    psycopg2
 passenv=JUJU_*
 commands=
     pytest {posargs:--verbose --tb=native unit_tests/}
diff --git a/unit_tests/test_postgresql.py b/unit_tests/test_postgresql.py
index 5524745..95cf828 100644
--- a/unit_tests/test_postgresql.py
+++ b/unit_tests/test_postgresql.py
@@ -26,47 +26,47 @@ sys.path.insert(1, ROOT)
 sys.path.insert(2, os.path.join(ROOT, 'lib'))
 sys.path.insert(3, os.path.join(ROOT, 'lib', 'testdeps'))
 
-from charmhelpers.core import hookenv
-from charmhelpers.core import unitdata
-
+from charmhelpers.core import hookenv, host
 from reactive import workloadstatus
-from reactive.postgresql import helpers
-from reactive.postgresql import postgresql
+from reactive.postgresql import helpers, postgresql
 
 
 class TestPostgresql(unittest.TestCase):
+    @patch.object(postgresql, 'lsclusters_version')
     @patch.object(hookenv, 'config')
     @patch.object(helpers, 'distro_codename')
-    def test_version(self, codename, config):
-
-        def clear_cache():
-            unitdata.kv().unset('postgresql.pg_version')
+    def test_version(self, codename, config, lsclusters_version):
+        # Installed version
+        lsclusters_version.return_value = '9.5'
+        postgresql.clear_version_cache()
+        self.assertEqual(postgresql.version(), '9.5')
+        lsclusters_version.return_value = None
 
         # Explicit version in config.
         config.return_value = {'version': '23'}
-        clear_cache()
+        postgresql.clear_version_cache()
         self.assertEqual(postgresql.version(), '23')
 
         config.return_value = {'version': ''}
 
         # Trusty default
         codename.return_value = 'trusty'
-        clear_cache()
+        postgresql.clear_version_cache()
         self.assertEqual(postgresql.version(), '9.3')
 
         # Xenial default
         codename.return_value = 'xenial'
-        clear_cache()
+        postgresql.clear_version_cache()
         self.assertEqual(postgresql.version(), '9.5')
 
         # Bionic default
         codename.return_value = 'bionic'
-        clear_cache()
+        postgresql.clear_version_cache()
         self.assertEqual(postgresql.version(), '10')
 
         # No other fallbacks, yet.
         codename.return_value = 'whatever'
-        clear_cache()
+        postgresql.clear_version_cache()
         with self.assertRaises(NotImplementedError):
             postgresql.version()
 
@@ -251,8 +251,11 @@ class TestPostgresql(unittest.TestCase):
         is_secondary.return_value = False
         self.assertTrue(postgresql.is_primary())
 
+    @patch.object(postgresql, 'has_version')
+    @patch.object(hookenv, 'config')
     @patch.object(postgresql, 'recovery_conf_path')
-    def test_is_secondary(self, recovery_path):
+    def test_is_secondary(self, recovery_path, config, has_version):
+        has_version.return_value = False
         # if recovery.conf exists, we are a secondary.
         with tempfile.NamedTemporaryFile() as f:
             recovery_path.return_value = f.name
@@ -317,7 +320,7 @@ class TestPostgresql(unittest.TestCase):
         check_call.assert_called_once_with(['pg_createcluster',
                                             '-e', sentinel.encoding,
                                             '--locale', sentinel.locale,
-                                            '9.2', 'main'],
+                                            '9.2', 'main', '--', '--data-checksums'],
                                            universal_newlines=True)
 
     @patch('subprocess.check_call')
@@ -345,7 +348,7 @@ class TestPostgresql(unittest.TestCase):
         cur.execute.assert_has_calls([
             call('SELECT datname FROM pg_database WHERE datname=%s',
                  ('hello',)),
-            call('CREATE DATABASE %s', (ANY,))])
+            call('CREATE DATABASE %s OWNER %s', (ANY, ANY))])
         # The database name in that last call was correctly quoted.
         quoted_dbname = cur.execute.call_args[0][1][0]
         self.assertIsInstance(quoted_dbname, postgresql.AsIs)
@@ -475,14 +478,16 @@ class TestPostgresql(unittest.TestCase):
 
         pgidentifier.side_effect = lambda d: 'q_{}'.format(d)
 
-        existing_extensions = set(['extA', 'extB'])
-        wanted_extensions = set(['extB', 'extC'])
+        existing_extensions = set([('extA', 'public'), ('extB', 'public')])
+        wanted_extensions = set([('extB', 'public'), ('extC', 'custom')])
 
-        cur.fetchall.return_value = [[x] for x in existing_extensions]
+        cur.fetchall.return_value = [x for x in existing_extensions]
         postgresql.ensure_extensions(con, wanted_extensions)
         cur.execute.assert_has_calls([
-            call('SELECT extname FROM pg_extension'),
-            call('CREATE EXTENSION %s', ('q_extC',))])
+            call('SELECT extname, nspname FROM pg_extension, pg_namespace WHERE pg_namespace.oid = extnamespace'),
+            call('CREATE SCHEMA IF NOT EXISTS %s', ('q_custom',)),
+            call('GRANT USAGE ON SCHEMA %s TO PUBLIC', ('q_custom',)),
+            call('CREATE EXTENSION %s WITH SCHEMA %s', ('q_extC','q_custom'))])
 
     def test_addr_to_range(self):
         eggs = [('hostname', 'hostname'),
@@ -532,35 +537,18 @@ class TestPostgresql(unittest.TestCase):
 
     @patch.object(postgresql, 'emit_pg_log')
     @patch.object(workloadstatus, 'status_set')
-    @patch('subprocess.check_call')
+    @patch.object(host, 'service_start')
     @patch.object(postgresql, 'version')
-    def test_start(self, version, check_call, status_set, emit_pg_log):
+    def test_start(self, version, service_start, status_set, emit_pg_log):
         version.return_value = '9.9'
 
         # When it works, it works.
         postgresql.start()
-        # Both -w and -t options are required to wait for startup.
-        # We wait a long time, as startup might take a long time.
-        # Maybe we should wait a lot longer.
-        check_call.assert_called_once_with(['pg_ctlcluster', '9.9', 'main',
-                                            'start', '--', '-w',
-                                            '-t', '86400'],
-                                           universal_newlines=True)
+        service_start.assert_called_once_with('postgresql@9.9-main')
         self.assertFalse(emit_pg_log.called)
 
-        # If it is already running, pg_ctlcluster returns code 2.
-        # We block, and terminate whatever hook is running.
-        check_call.side_effect = subprocess.CalledProcessError(2, 'whoops')
-        check_call.reset_mock()
-        postgresql.start()
-        check_call.assert_called_once_with(['pg_ctlcluster', '9.9', 'main',
-                                            'start', '--', '-w',
-                                            '-t', '86400'],
-                                           universal_newlines=True)
-
-        # Other failures block the unit. Perhaps it is just taking too
-        # perform recovery after a power outage.
-        check_call.side_effect = subprocess.CalledProcessError(42, 'whoops')
+        # Start failure we block, and terminate whatever hook is running.
+        service_start.return_value = False
         with self.assertRaises(SystemExit) as x:
             postgresql.start()
         status_set.assert_called_once_with('blocked', ANY)  # Set blocked.
@@ -569,59 +557,29 @@ class TestPostgresql(unittest.TestCase):
 
     @patch.object(hookenv, 'log')
     @patch.object(workloadstatus, 'status_set')
-    @patch('subprocess.check_call')
+    @patch.object(host, 'service_stop')
     @patch.object(postgresql, 'version')
-    def test_stop(self, version, check_call, status_set, log):
+    def test_stop(self, version, service_stop, status_set, log):
         version.return_value = '9.9'
 
         # Normal shutdown shuts down.
+        service_stop.return_value = True
         postgresql.stop()
-        # -t option is required to wait for shutdown to complete. -w not
-        # required unlike 'start', but lets be explicit.
-        check_call.assert_called_once_with(['pg_ctlcluster',
-                                            '--mode', 'fast', '9.9', 'main',
-                                            'stop', '--', '-w', '-t', '300'],
-                                           universal_newlines=True)
-
-        # If the server is not running, pg_ctlcluster(1) signals this with
-        # returncode 2.
-        check_call.side_effect = subprocess.CalledProcessError(2, 'whoops')
-        check_call.reset_mock()
-        postgresql.stop()
-        # -t option is required to wait for shutdown to complete. -w not
-        # required unlike 'start', but lets be explicit.
-        check_call.assert_called_once_with(['pg_ctlcluster',
-                                            '--mode', 'fast', '9.9', 'main',
-                                            'stop', '--', '-w', '-t', '300'],
-                                           universal_newlines=True)
+        service_stop.assert_called_once_with('postgresql@9.9-main')
 
-        # If 'fast' shutdown fails, we retry with an 'immediate' shutdown
-        check_call.side_effect = iter([subprocess.CalledProcessError(42, 'x'),
-                                       None])
-        check_call.reset_mock()
-        postgresql.stop()
-        check_call.assert_has_calls([
-            call(['pg_ctlcluster', '--mode', 'fast', '9.9', 'main',
-                  'stop', '--', '-w', '-t', '300'],
-                 universal_newlines=True),
-            call(['pg_ctlcluster', '--mode', 'immediate', '9.9', 'main',
-                  'stop', '--', '-w', '-t', '300'],
-                 universal_newlines=True)])
-
-        # If both fail, we block the unit.
-        check_call.side_effect = subprocess.CalledProcessError(42, 'x')
+        # Failed shutdown blocks and terminates
+        service_stop.return_value = False
         with self.assertRaises(SystemExit) as x:
             postgresql.stop()
         status_set.assert_called_once_with('blocked', ANY)
         self.assertEqual(x.exception.code, 0)  # Exit cleanly
 
-    @patch('subprocess.check_call')
+    @patch.object(host, 'service_reload')
     @patch.object(postgresql, 'version')
-    def test_reload_config(self, version, check_call):
+    def test_reload_config(self, version, service_reload):
         version.return_value = '9.9'
         postgresql.reload_config()
-        check_call.assert_called_once_with(['pg_ctlcluster', '9.9', 'main',
-                                            'reload'])
+        service_reload.assert_called_once_with('postgresql@9.9-main')
 
     def test_parse_config(self):
         valid = [(r'# A comment', dict()),
@@ -653,7 +611,7 @@ class TestPostgresql(unittest.TestCase):
 
         with self.assertRaises(SyntaxError) as x:
             postgresql.parse_config("=")
-        self.assertEqual(str(x.exception), 'Missing key (line 1)')
+        self.assertEqual(str(x.exception), "Missing key '=' (line 1)")
         self.assertEqual(x.exception.lineno, 1)
         self.assertEqual(x.exception.text, "=")
 
@@ -665,19 +623,19 @@ class TestPostgresql(unittest.TestCase):
 
         with self.assertRaises(SyntaxError) as x:
             postgresql.parse_config("key='unterminated")
-        self.assertEqual(str(x.exception), 'Badly quoted value (line 1)')
+        self.assertEqual(str(x.exception), r'''Badly quoted value "'unterminated" (line 1)''')
 
         with self.assertRaises(SyntaxError) as x:
             postgresql.parse_config("key='unterminated 2 # comment")
-        self.assertEqual(str(x.exception), 'Badly quoted value (line 1)')
+        self.assertEqual(str(x.exception), r'''Badly quoted value "'unterminated 2" (line 1)''')
 
         with self.assertRaises(SyntaxError) as x:
             postgresql.parse_config("key='unte''''")
-        self.assertEqual(str(x.exception), 'Badly quoted value (line 1)')
+        self.assertEqual(str(x.exception), r"""Badly quoted value "'unte''''" (line 1)""")
 
         with self.assertRaises(SyntaxError) as x:
             postgresql.parse_config(r"key='\'")
-        self.assertEqual(str(x.exception), 'Badly quoted value (line 1)')
+        self.assertEqual(str(x.exception), r'''Badly quoted value "'\\'" (line 1)''')
 
     def test_convert_unit(self):
         c = postgresql.convert_unit