Merge lp:~jjo/charms/precise/postgresql/support-no-explicit-version_and_trusty-pg93 into lp:charms/postgresql

[JuanJo Ciarlante]

- support deploying with version unset in config.yaml (new default)
- add pg_version(): if no explicit version set, find it from apt.Cache() install-able candidate, save it in local_state later usage.
- add support for postgresql>=9.3 (trusty): deprecated unix_socket_directory in favor of unix_socket_directories (list)
- move ssl cert symlinking into create_ssl_cert()

[lp:~jjo/charms/precise/postgresql/support-no-explicit-version_and_trusty-pg93]

81. By JuanJo Ciarlante on 2014-01-17

add some logging

[James Troup (elmo)]

Trivial: 'canditate' should be 'candidate' throughout.

pg_version() makes several assumptions (e.g. that a postgresql package
exists and that it's version matches a specific regex (which won't be
the case if, e.g. the package grows an epoch) that are probably fine,
but it caught my eye.

| + # expensive operation, save it

What operation does this refer to? The function as a whole?

[lp:~jjo/charms/precise/postgresql/support-no-explicit-version_and_trusty-pg93]

82. By JuanJo Ciarlante on 2014-01-20

s/canditate/candidate/, clarify apt.Cache being the expensive op

[JuanJo Ciarlante (jjo)]

> Trivial: 'canditate' should be 'candidate' throughout.

thanks for spotting, fixed.

>
> pg_version() makes several assumptions (e.g. that a postgresql package
> exists and that it's version matches a specific regex (which won't be
> the case if, e.g. the package grows an epoch) that are probably fine,
> but it caught my eye.

I found this the less painful way to find out which "short-version"
($major.$minor) is available.

>
> | + # expensive operation, save it
>
> What operation does this refer to? The function as a whole?

apt.Cache(), I clarified the comment there.

[James Troup (elmo)]

+1

review: Approve

[Stuart Bishop (stub)]

I don't think allowing an unspecified PostgreSQL version is a good idea. It means charm deployment is not repeatable, as which version is deployed depends on the state of the archive. I don't want 9.4 (or worse, 9.4 beta) being installed when my apps are tested with 9.3 just because a newer version is available in the archive. This would also break replication, as a unit does not know what version the other units in the service are using until after the install hook is run and the unit attempt's to join the service's peer relation. Archives will not always be under user control, in particular apt.postgresql.org for official Debian and Ubuntu backports.

If we really need this so the default version under trusty is 9.3, and the default version under precise is 9.1, I think we just want to check 'lsb_release -sc' and hard code the default version to the codename. This may still cause surprises, but would be repeatable.

If we keep the 'apt' import, do we need to add python-apt to the default list of packages to install?

review: Needs Fixing

[JuanJo Ciarlante (jjo)]

> I don't think allowing an unspecified PostgreSQL version is a good idea. It
> means charm deployment is not repeatable, as which version is deployed depends
> on the state of the archive. I don't want 9.4 (or worse, 9.4 beta) being
> installed when my apps are tested with 9.3 just because a newer version is
> available in the archive. This would also break replication, as a unit does
> not know what version the other units in the service are using until after the
> install hook is run and the unit attempt's to join the service's peer
> relation. Archives will not always be under user control, in particular
> apt.postgresql.org for official Debian and Ubuntu backports.
>
> If we really need this so the default version under trusty is 9.3, and the
> default version under precise is 9.1, I think we just want to check
> 'lsb_release -sc' and hard code the default version to the codename. This may
> still cause surprises, but would be repeatable.

I'm fine with hardcoding it, as far as we have a single charm that can be
deployed to either precise,trusty without requiring the user to overload
the config with a version that's already well-known/mapped for this distro
release.

>
> If we keep the 'apt' import, do we need to add python-apt to the default list
> of packages to install?

Good point, fwiw afaicT both precise,trusty it was already installed there.

[lp:~jjo/charms/precise/postgresql/support-no-explicit-version_and_trusty-pg93]

83. By JuanJo Ciarlante on 2014-01-21

hardcode pg_version map by (LTS) distro release

[JuanJo Ciarlante (jjo)]

Thanks stub for the review, PTAL (CID testing case against precise,trusty running atm, will let you know once it finishes, ETA: ~+1.5hr).

[Stuart Bishop (stub)]

This no longer worries me and looks good.

review: Approve

[lp:~jjo/charms/precise/postgresql/support-no-explicit-version_and_trusty-pg93]

84. By JuanJo Ciarlante on 2014-01-21

need to: distro_release.rstrip()

85. By JuanJo Ciarlante on 2014-01-21

move rstrip above, let distro_release value "clean" from there

=== modified file 'config.yaml'
--- config.yaml	2014-01-07 19:00:20 +0000
+++ config.yaml	2014-01-21 12:55:42 +0000
@@ -43,7 +43,7 @@
         access details are updated, so disabling either doesn't mean PostgreSQL
         will never be reloaded.
   version:
-    default: "9.1"
+    default: ""
     type: string
     description: Version of PostgreSQL that we want to install
   cluster_name:
 
=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-01-09 09:08:11 +0000
+++ hooks/hooks.py	2014-01-21 12:55:42 +0000
@@ -53,6 +53,39 @@
     hookenv.log(msg, lvl)
 
 
+def pg_version():
+    '''Return pg_version to use.
+
+    Return "version" config item if set, else use version from "postgresql"
+    package candidate, saving it in local_state for later.
+    '''
+    config_data = hookenv.config()
+    if config_data['version']:
+        version = config_data['version']
+    elif 'pg_version' in local_state:
+        version = local_state['pg_version']
+    else:
+        log("map version from distro release ...")
+        distro_release = run("lsb_release -sc")
+        distro_release = distro_release.rstrip()
+        version_map = {'precise': '9.1',
+                       'trusty': '9.3'}
+        version = version_map.get(distro_release)
+        if not version:
+            log("No PG version map for distro_release={}, "
+                "you'll need to explicitly set it".format(distro_release),
+                CRITICAL)
+            sys.exit(1)
+        log("version={} from distro_release='{}'".format(
+            version, distro_release))
+        # save it for later
+        local_state.setdefault('pg_version', version)
+        local_state.save()
+
+    assert version, "pg_version couldn't find a version to use"
+    return version
+
+
 class State(dict):
     """Encapsulate state common to the unit for republishing to relations."""
     def __init__(self, state_file):
@@ -230,7 +263,7 @@
     if status != 0:
         return False
     # e.g. output: "Running clusters: 9.1/main"
-    vc = "%s/%s" % (hookenv.config("version"), hookenv.config("cluster_name"))
+    vc = "%s/%s" % (pg_version(), hookenv.config("cluster_name"))
     return vc in output.decode('utf8').split()
 
 
@@ -253,8 +286,9 @@
             # 'service postgresql restart' fails; it only does a reload.
             # success = host.service_restart('postgresql')
             try:
-                run('pg_ctlcluster -force {version} {cluster_name} '
-                    'restart'.format(**hookenv.config()))
+                run('pg_ctlcluster -force {} {} '
+                    'restart'.format(pg_version(),
+                                     hookenv.config('cluster_name')))
                 success = True
             except subprocess.CalledProcessError:
                 success = False
@@ -405,6 +439,8 @@
     # Return it as pg_config
     charm_dir = hookenv.charm_dir()
     template_file = "{}/templates/postgresql.conf.tmpl".format(charm_dir)
+    if not config_data['version']:
+        config_data['version'] = pg_version()
     pg_config = Template(
         open(template_file).read()).render(config_data)
     host.write_file(
@@ -580,7 +616,7 @@
 
 
 def create_recovery_conf(master_host, restart_on_change=False):
-    version = hookenv.config('version')
+    version = pg_version()
     cluster_name = hookenv.config('cluster_name')
     postgresql_cluster_dir = os.path.join(
         postgresql_data_dir, version, cluster_name)
@@ -632,6 +668,18 @@
         hookenv.open_port(new_service_port)
 
 
+def create_ssl_cert(cluster_dir):
+    # Debian by default expects SSL certificates in the datadir.
+    server_crt = os.path.join(cluster_dir, 'server.crt')
+    server_key = os.path.join(cluster_dir, 'server.key')
+    if not os.path.exists(server_crt):
+        os.symlink('/etc/ssl/certs/ssl-cert-snakeoil.pem',
+                   server_crt)
+    if not os.path.exists(server_key):
+        os.symlink('/etc/ssl/private/ssl-cert-snakeoil.key',
+                   server_key)
+
+
 def set_password(user, password):
     if not os.path.isdir("passwords"):
         os.makedirs("passwords")
@@ -711,7 +759,7 @@
 #     - manipulate /var/lib/postgresql/VERSION/CLUSTER symlink
 #------------------------------------------------------------------------------
 def config_changed_volume_apply():
-    version = hookenv.config('version')
+    version = pg_version()
     cluster_name = hookenv.config('cluster_name')
     data_directory_path = os.path.join(
         postgresql_data_dir, version, cluster_name)
@@ -820,7 +868,7 @@
 @hooks.hook()
 def config_changed(force_restart=False):
     config_data = hookenv.config()
-    update_repos_and_packages(config_data["version"])
+    update_repos_and_packages()
 
     # Trigger volume initialization logic for permanent storage
     volid = volume_get_volume_id()
@@ -862,6 +910,8 @@
     create_postgresql_config(postgresql_config)
     generate_postgresql_hba(postgresql_hba)
     create_postgresql_ident(postgresql_ident)
+    create_ssl_cert(os.path.join(
+        postgresql_data_dir, pg_version(), config_data['cluster_name']))
 
     updated_service_port = config_data["listen_port"]
     update_service_port(current_service_port, updated_service_port)
@@ -879,7 +929,7 @@
                 subprocess.check_call(['sh', '-c', f])
 
     config_data = hookenv.config()
-    update_repos_and_packages(config_data["version"])
+    update_repos_and_packages()
     if not 'state' in local_state:
         # Fresh installation. Because this function is invoked by both
         # the install hook and the upgrade-charm hook, we need to guard
@@ -890,9 +940,10 @@
 
         # Drop the cluster created when the postgresql package was
         # installed, and rebuild it with the requested locale and encoding.
-        run("pg_dropcluster --stop 9.1 main")
-        run("pg_createcluster --locale='{}' --encoding='{}' 9.1 main".format(
-            config_data['locale'], config_data['encoding']))
+        version = pg_version()
+        run("pg_dropcluster --stop {} main".format(version))
+        run("pg_createcluster --locale='{}' --encoding='{}' {} main".format(
+            config_data['locale'], config_data['encoding'], version))
 
     postgresql_backups_dir = (
         config_data['backup_dir'].strip() or
@@ -1322,7 +1373,7 @@
     snapshot_relations()
 
 
-def update_repos_and_packages(version):
+def update_repos_and_packages():
     extra_repos = hookenv.config('extra_archives')
     extra_repos_added = local_state.setdefault('extra_repos_added', set())
     if extra_repos:
@@ -1336,6 +1387,7 @@
             fetch.apt_update(fatal=True)
             local_state.save()
 
+    version = pg_version()
     # It might have been better for debversion and plpython to only get
     # installed if they were listed in the extra-packages config item,
     # but they predate this feature.
@@ -1409,7 +1461,7 @@
 def promote_database():
     '''Take the database out of recovery mode.'''
     config_data = hookenv.config()
-    version = config_data['version']
+    version = pg_version()
     cluster_name = config_data['cluster_name']
     postgresql_cluster_dir = os.path.join(
         postgresql_data_dir, version, cluster_name)
@@ -1814,7 +1866,7 @@
         log("Cloning master {}".format(master_unit))
 
         config_data = hookenv.config()
-        version = config_data['version']
+        version = pg_version()
         cluster_name = config_data['cluster_name']
         postgresql_cluster_dir = os.path.join(
             postgresql_data_dir, version, cluster_name)
@@ -1837,12 +1889,7 @@
                 output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
             log(output, DEBUG)
             # Debian by default expects SSL certificates in the datadir.
-            os.symlink(
-                '/etc/ssl/certs/ssl-cert-snakeoil.pem',
-                os.path.join(postgresql_cluster_dir, 'server.crt'))
-            os.symlink(
-                '/etc/ssl/private/ssl-cert-snakeoil.key',
-                os.path.join(postgresql_cluster_dir, 'server.key'))
+            create_ssl_cert(postgresql_cluster_dir)
             create_recovery_conf(master_host)
         except subprocess.CalledProcessError as x:
             # We failed, and this cluster is broken. Rebuild a
@@ -1872,7 +1919,7 @@
 
 
 def postgresql_is_in_backup_mode():
-    version = hookenv.config('version')
+    version = pg_version()
     cluster_name = hookenv.config('cluster_name')
     postgresql_cluster_dir = os.path.join(
         postgresql_data_dir, version, cluster_name)
@@ -1998,7 +2045,7 @@
     """ Return the directory path of the postgresql configuration files. """
     if config_data is None:
         config_data = hookenv.config()
-    version = config_data['version']
+    version = pg_version()
     cluster_name = config_data['cluster_name']
     return os.path.join("/etc/postgresql", version, cluster_name)
 
 
=== modified file 'templates/postgresql.conf.tmpl'
--- templates/postgresql.conf.tmpl	2014-01-02 22:50:28 +0000
+++ templates/postgresql.conf.tmpl	2014-01-21 12:55:42 +0000
@@ -17,7 +17,11 @@
 # CONNECTIONS AND AUTHENTICATION
 #------------------------------------------------------------------------------
 
+{% if version >= "9.3" -%}
+unix_socket_directories = '/var/run/postgresql'
+{% else -%}
 unix_socket_directory = '/var/run/postgresql'
+{% endif -%}
 
 {% if listen_ip != "" -%}
 listen_addresses = '{{listen_ip}}'