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

Proposed by JuanJo Ciarlante
Status: Merged
Merged at revision: 80
Proposed branch: lp:~jjo/charms/precise/postgresql/support-no-explicit-version_and_trusty-pg93
Merge into: lp:charms/postgresql
Diff against target: 247 lines (+73/-22)
3 files modified
config.yaml (+1/-1)
hooks/hooks.py (+68/-21)
templates/postgresql.conf.tmpl (+4/-0)
To merge this branch: bzr merge lp:~jjo/charms/precise/postgresql/support-no-explicit-version_and_trusty-pg93
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
James Troup Approve
Review via email: mp+202076@code.launchpad.net

Commit message

- 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()

To post a comment you must log in.
81. By JuanJo Ciarlante

add some logging

Revision history for this message
James Troup (elmo) wrote :

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?

82. By JuanJo Ciarlante

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

Revision history for this message
JuanJo Ciarlante (jjo) wrote :

> 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.

Revision history for this message
James Troup (elmo) wrote :

+1

review: Approve
Revision history for this message
Stuart Bishop (stub) wrote :

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
Revision history for this message
JuanJo Ciarlante (jjo) wrote :

> 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.

83. By JuanJo Ciarlante

hardcode pg_version map by (LTS) distro release

Revision history for this message
JuanJo Ciarlante (jjo) wrote :

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

Revision history for this message
Stuart Bishop (stub) wrote :

This no longer worries me and looks good.

review: Approve
84. By JuanJo Ciarlante

need to: distro_release.rstrip()

85. By JuanJo Ciarlante

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'config.yaml'
2--- config.yaml 2014-01-07 19:00:20 +0000
3+++ config.yaml 2014-01-21 12:55:42 +0000
4@@ -43,7 +43,7 @@
5 access details are updated, so disabling either doesn't mean PostgreSQL
6 will never be reloaded.
7 version:
8- default: "9.1"
9+ default: ""
10 type: string
11 description: Version of PostgreSQL that we want to install
12 cluster_name:
13
14=== modified file 'hooks/hooks.py'
15--- hooks/hooks.py 2014-01-09 09:08:11 +0000
16+++ hooks/hooks.py 2014-01-21 12:55:42 +0000
17@@ -53,6 +53,39 @@
18 hookenv.log(msg, lvl)
19
20
21+def pg_version():
22+ '''Return pg_version to use.
23+
24+ Return "version" config item if set, else use version from "postgresql"
25+ package candidate, saving it in local_state for later.
26+ '''
27+ config_data = hookenv.config()
28+ if config_data['version']:
29+ version = config_data['version']
30+ elif 'pg_version' in local_state:
31+ version = local_state['pg_version']
32+ else:
33+ log("map version from distro release ...")
34+ distro_release = run("lsb_release -sc")
35+ distro_release = distro_release.rstrip()
36+ version_map = {'precise': '9.1',
37+ 'trusty': '9.3'}
38+ version = version_map.get(distro_release)
39+ if not version:
40+ log("No PG version map for distro_release={}, "
41+ "you'll need to explicitly set it".format(distro_release),
42+ CRITICAL)
43+ sys.exit(1)
44+ log("version={} from distro_release='{}'".format(
45+ version, distro_release))
46+ # save it for later
47+ local_state.setdefault('pg_version', version)
48+ local_state.save()
49+
50+ assert version, "pg_version couldn't find a version to use"
51+ return version
52+
53+
54 class State(dict):
55 """Encapsulate state common to the unit for republishing to relations."""
56 def __init__(self, state_file):
57@@ -230,7 +263,7 @@
58 if status != 0:
59 return False
60 # e.g. output: "Running clusters: 9.1/main"
61- vc = "%s/%s" % (hookenv.config("version"), hookenv.config("cluster_name"))
62+ vc = "%s/%s" % (pg_version(), hookenv.config("cluster_name"))
63 return vc in output.decode('utf8').split()
64
65
66@@ -253,8 +286,9 @@
67 # 'service postgresql restart' fails; it only does a reload.
68 # success = host.service_restart('postgresql')
69 try:
70- run('pg_ctlcluster -force {version} {cluster_name} '
71- 'restart'.format(**hookenv.config()))
72+ run('pg_ctlcluster -force {} {} '
73+ 'restart'.format(pg_version(),
74+ hookenv.config('cluster_name')))
75 success = True
76 except subprocess.CalledProcessError:
77 success = False
78@@ -405,6 +439,8 @@
79 # Return it as pg_config
80 charm_dir = hookenv.charm_dir()
81 template_file = "{}/templates/postgresql.conf.tmpl".format(charm_dir)
82+ if not config_data['version']:
83+ config_data['version'] = pg_version()
84 pg_config = Template(
85 open(template_file).read()).render(config_data)
86 host.write_file(
87@@ -580,7 +616,7 @@
88
89
90 def create_recovery_conf(master_host, restart_on_change=False):
91- version = hookenv.config('version')
92+ version = pg_version()
93 cluster_name = hookenv.config('cluster_name')
94 postgresql_cluster_dir = os.path.join(
95 postgresql_data_dir, version, cluster_name)
96@@ -632,6 +668,18 @@
97 hookenv.open_port(new_service_port)
98
99
100+def create_ssl_cert(cluster_dir):
101+ # Debian by default expects SSL certificates in the datadir.
102+ server_crt = os.path.join(cluster_dir, 'server.crt')
103+ server_key = os.path.join(cluster_dir, 'server.key')
104+ if not os.path.exists(server_crt):
105+ os.symlink('/etc/ssl/certs/ssl-cert-snakeoil.pem',
106+ server_crt)
107+ if not os.path.exists(server_key):
108+ os.symlink('/etc/ssl/private/ssl-cert-snakeoil.key',
109+ server_key)
110+
111+
112 def set_password(user, password):
113 if not os.path.isdir("passwords"):
114 os.makedirs("passwords")
115@@ -711,7 +759,7 @@
116 # - manipulate /var/lib/postgresql/VERSION/CLUSTER symlink
117 #------------------------------------------------------------------------------
118 def config_changed_volume_apply():
119- version = hookenv.config('version')
120+ version = pg_version()
121 cluster_name = hookenv.config('cluster_name')
122 data_directory_path = os.path.join(
123 postgresql_data_dir, version, cluster_name)
124@@ -820,7 +868,7 @@
125 @hooks.hook()
126 def config_changed(force_restart=False):
127 config_data = hookenv.config()
128- update_repos_and_packages(config_data["version"])
129+ update_repos_and_packages()
130
131 # Trigger volume initialization logic for permanent storage
132 volid = volume_get_volume_id()
133@@ -862,6 +910,8 @@
134 create_postgresql_config(postgresql_config)
135 generate_postgresql_hba(postgresql_hba)
136 create_postgresql_ident(postgresql_ident)
137+ create_ssl_cert(os.path.join(
138+ postgresql_data_dir, pg_version(), config_data['cluster_name']))
139
140 updated_service_port = config_data["listen_port"]
141 update_service_port(current_service_port, updated_service_port)
142@@ -879,7 +929,7 @@
143 subprocess.check_call(['sh', '-c', f])
144
145 config_data = hookenv.config()
146- update_repos_and_packages(config_data["version"])
147+ update_repos_and_packages()
148 if not 'state' in local_state:
149 # Fresh installation. Because this function is invoked by both
150 # the install hook and the upgrade-charm hook, we need to guard
151@@ -890,9 +940,10 @@
152
153 # Drop the cluster created when the postgresql package was
154 # installed, and rebuild it with the requested locale and encoding.
155- run("pg_dropcluster --stop 9.1 main")
156- run("pg_createcluster --locale='{}' --encoding='{}' 9.1 main".format(
157- config_data['locale'], config_data['encoding']))
158+ version = pg_version()
159+ run("pg_dropcluster --stop {} main".format(version))
160+ run("pg_createcluster --locale='{}' --encoding='{}' {} main".format(
161+ config_data['locale'], config_data['encoding'], version))
162
163 postgresql_backups_dir = (
164 config_data['backup_dir'].strip() or
165@@ -1322,7 +1373,7 @@
166 snapshot_relations()
167
168
169-def update_repos_and_packages(version):
170+def update_repos_and_packages():
171 extra_repos = hookenv.config('extra_archives')
172 extra_repos_added = local_state.setdefault('extra_repos_added', set())
173 if extra_repos:
174@@ -1336,6 +1387,7 @@
175 fetch.apt_update(fatal=True)
176 local_state.save()
177
178+ version = pg_version()
179 # It might have been better for debversion and plpython to only get
180 # installed if they were listed in the extra-packages config item,
181 # but they predate this feature.
182@@ -1409,7 +1461,7 @@
183 def promote_database():
184 '''Take the database out of recovery mode.'''
185 config_data = hookenv.config()
186- version = config_data['version']
187+ version = pg_version()
188 cluster_name = config_data['cluster_name']
189 postgresql_cluster_dir = os.path.join(
190 postgresql_data_dir, version, cluster_name)
191@@ -1814,7 +1866,7 @@
192 log("Cloning master {}".format(master_unit))
193
194 config_data = hookenv.config()
195- version = config_data['version']
196+ version = pg_version()
197 cluster_name = config_data['cluster_name']
198 postgresql_cluster_dir = os.path.join(
199 postgresql_data_dir, version, cluster_name)
200@@ -1837,12 +1889,7 @@
201 output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
202 log(output, DEBUG)
203 # Debian by default expects SSL certificates in the datadir.
204- os.symlink(
205- '/etc/ssl/certs/ssl-cert-snakeoil.pem',
206- os.path.join(postgresql_cluster_dir, 'server.crt'))
207- os.symlink(
208- '/etc/ssl/private/ssl-cert-snakeoil.key',
209- os.path.join(postgresql_cluster_dir, 'server.key'))
210+ create_ssl_cert(postgresql_cluster_dir)
211 create_recovery_conf(master_host)
212 except subprocess.CalledProcessError as x:
213 # We failed, and this cluster is broken. Rebuild a
214@@ -1872,7 +1919,7 @@
215
216
217 def postgresql_is_in_backup_mode():
218- version = hookenv.config('version')
219+ version = pg_version()
220 cluster_name = hookenv.config('cluster_name')
221 postgresql_cluster_dir = os.path.join(
222 postgresql_data_dir, version, cluster_name)
223@@ -1998,7 +2045,7 @@
224 """ Return the directory path of the postgresql configuration files. """
225 if config_data is None:
226 config_data = hookenv.config()
227- version = config_data['version']
228+ version = pg_version()
229 cluster_name = config_data['cluster_name']
230 return os.path.join("/etc/postgresql", version, cluster_name)
231
232
233=== modified file 'templates/postgresql.conf.tmpl'
234--- templates/postgresql.conf.tmpl 2014-01-02 22:50:28 +0000
235+++ templates/postgresql.conf.tmpl 2014-01-21 12:55:42 +0000
236@@ -17,7 +17,11 @@
237 # CONNECTIONS AND AUTHENTICATION
238 #------------------------------------------------------------------------------
239
240+{% if version >= "9.3" -%}
241+unix_socket_directories = '/var/run/postgresql'
242+{% else -%}
243 unix_socket_directory = '/var/run/postgresql'
244+{% endif -%}
245
246 {% if listen_ip != "" -%}
247 listen_addresses = '{{listen_ip}}'

Subscribers

People subscribed via source and target branches