PostgreSQL Charm

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

  1. Git
  2. lp:~stub/postgresql-charm
  3. upgrade-series
  4. Merge into master
Proposed by Stuart Bishop
Status: Merged
Merged at revision: a1d109cbbfb999f9c5715e268cd0226668f7d8cf
Proposed branch: ~stub/postgresql-charm:upgrade-series
Merge into: postgresql-charm:master
Diff against target: 476 lines (+114/-125)
7 files modified
Makefile (+2/-19)
reactive/postgresql/postgresql.py (+21/-4)
reactive/postgresql/service.py (+32/-9)
reactive/postgresql/upgrade.py (+6/-0)
requirements.txt (+3/-2)
tox.ini (+4/-3)
unit_tests/test_postgresql.py (+46/-88)
Reviewer Review Type Date Requested Status
Barry Price Approve
Canonical IS Reviewers Pending
PostgreSQL Charm Maintainers Pending
Review via email: mp+395355@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

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

Revision history for this message
Barry Price (barryprice) wrote :

Couple of nitpick questions on the Makefile, otherwise LGTM

review: Approve
Revision history for this message
Barry Price (barryprice) wrote :

Actually save the second of those questions :)

Revision history for this message
Stuart Bishop (stub) :
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/Makefile b/Makefile
2index b05ce5b..089e02e 100644
3--- a/Makefile
4+++ b/Makefile
5@@ -27,24 +27,7 @@ _success_ex:
6 true | ts
7
8
9-# Munge the path so the requested version of Juju is found, and thus used
10-# by Amulet and juju-deployer.
11-export PATH := /usr/lib/juju-$(shell $(JUJU) --version | perl -p -e "s/-.*//")/bin:$(PATH)
12-
13-
14-test: testdeps lint unittest integration
15-
16-testdeps:
17-ifeq ($(HOST_SERIES),trusty)
18- sudo apt-get install -y python-tox python3-psycopg2 bzr moreutils \
19- software-properties-common
20-else
21- sudo apt-get install -y tox python3-psycopg2 bzr moreutils \
22- software-properties-common
23-endif
24- sudo add-apt-repository -y ppa:juju/stable
25- sudo apt-get install charm-tools
26-
27+test: lint unittest integration
28
29 CHARM_NAME := postgresql
30
31@@ -69,7 +52,7 @@ $(BUILD_DIR):
32 # updates.
33 .PHONY: build
34 build: | $(BUILD_DIR)
35- charm build -f -o $(BUILD_ROOT) -n $(CHARM_NAME)
36+ charm build -f -d $(BUILD_DIR)/.. -n $(CHARM_NAME)
37
38 # Generate a fresh development build and commit it to $(TEST_BRANCH).
39 # Only builds work committed to $(LAYER_BRANCH).
40diff --git a/reactive/postgresql/postgresql.py b/reactive/postgresql/postgresql.py
41index 373080d..35d3e02 100644
42--- a/reactive/postgresql/postgresql.py
43+++ b/reactive/postgresql/postgresql.py
44@@ -79,6 +79,13 @@ def version():
45 if version:
46 return version
47
48+ # If no cached version, use the version reported by
49+ # pg_lsclusters
50+ version = lsclusters_version()
51+ if version:
52+ unitdata.kv().set("postgresql.pg_version", version)
53+ return version
54+
55 # We use the charm configuration here, as multiple versions
56 # of PostgreSQL may be installed.
57 config = hookenv.config()
58@@ -98,6 +105,19 @@ def version():
59 return version
60
61
62+def clear_version_cache():
63+ unitdata.kv().set("postgresql.pg_version", None)
64+
65+
66+def lsclusters_version():
67+ if not os.path.exists("/usr/bin/pg_lsclusters"):
68+ return None
69+ results = subprocess.check_output(["pg_lsclusters", "--no-header"]).decode().splitlines()
70+ if len(results) == 1:
71+ return results[0].split()[0]
72+ return None
73+
74+
75 def point_version():
76 """PostgreSQL version. major.minor.patch or major.patch, as a string."""
77 output = subprocess.check_output([postgres_path(), "-V"], universal_newlines=True)
78@@ -473,10 +493,7 @@ def ensure_role(con, role):
79 def ensure_extensions(con, extensions):
80 """extensions is a list of (name, schema) tuples"""
81 cur = con.cursor()
82- cur.execute(
83- """SELECT extname,nspname FROM pg_extension,pg_namespace
84- WHERE pg_namespace.oid = extnamespace"""
85- )
86+ cur.execute("SELECT extname, nspname FROM pg_extension, pg_namespace WHERE pg_namespace.oid = extnamespace")
87 installed_extensions = frozenset((x[0], x[1]) for x in cur.fetchall())
88 hookenv.log("ensure_extensions({}), have {}".format(extensions, installed_extensions), DEBUG)
89 extensions_set = frozenset(set(extensions))
90diff --git a/reactive/postgresql/service.py b/reactive/postgresql/service.py
91index b7269b1..f5e1936 100644
92--- a/reactive/postgresql/service.py
93+++ b/reactive/postgresql/service.py
94@@ -45,6 +45,18 @@ def install():
95 reactive.set_flag("postgresql.upgrade.systemd")
96
97
98+@hook("post-series-upgrade")
99+def post_series_upgrade():
100+ postgresql.clear_version_cache() # PG version upgrades should work on the master, but will break standbys
101+ config = hookenv.config()
102+ if config["pgdg"]:
103+ add_pgdg_source()
104+ if host.init_is_systemd():
105+ reactive.set_flag("postgresql.upgrade.systemd")
106+ reactive.clear_flag("postgresql.cluster.support-scripts")
107+ reactive.clear_flag("postgresql.cluster.configured")
108+
109+
110 @when("config.changed")
111 def config_changed():
112 reactive.clear_flag("postgresql.cluster.support-scripts")
113@@ -143,12 +155,23 @@ def configure_sources():
114
115 # Shortcut for the PGDG archive.
116 if config["pgdg"]:
117- pgdg_url = "http://apt.postgresql.org/pub/repos/apt/"
118- pgdg_src = "deb {} {}-pgdg main".format(pgdg_url, helpers.distro_codename())
119- pgdg_key_path = os.path.join(hookenv.charm_dir(), "lib", "pgdg.key")
120+ add_pgdg_source()
121+
122+
123+def add_pgdg_source():
124+ pgdg_url = "http://apt.postgresql.org/pub/repos/apt/"
125+ pgdg_src = "deb {} {}-pgdg main".format(pgdg_url, helpers.distro_codename())
126+ pgdg_key_path = os.path.join(hookenv.charm_dir(), "lib", "pgdg.key")
127+ kv = unitdata.kv()
128+ k = "postgresql.service.pgdg_src"
129+ # Add the source if the key has changed, or if the src has
130+ # changed such as a distro upgrade. Check key change first,
131+ # or short circuiting will add the source twice.
132+ if reactive.helpers.any_file_changed([pgdg_key_path]) or pgdg_src != kv.get(k):
133 with open(pgdg_key_path, "r") as f:
134 hookenv.log("Adding PGDG archive")
135 apt.add_source(pgdg_src, f.read())
136+ kv.set(k, pgdg_src)
137
138
139 @when("postgresql.cluster.locale.set")
140@@ -192,13 +215,13 @@ def create_cluster():
141 def create_pg_ctl_conf():
142 contents = textwrap.dedent(
143 """\
144- # Managed by Juju
145- # Automatic pg_ctl configuration
146- # This configuration file contains cluster specific options to be passed to
147- # pg_ctl(1).
148+ # Managed by Juju
149+ # Automatic pg_ctl configuration
150+ # This configuration file contains cluster specific options to be passed to
151+ # pg_ctl(1).
152
153- pg_ctl_options = '-w -t 3600'
154- """
155+ pg_ctl_options = '-w -t 3600'
156+ """
157 )
158 helpers.write(
159 postgresql.pg_ctl_conf_path(),
160diff --git a/reactive/postgresql/upgrade.py b/reactive/postgresql/upgrade.py
161index 9379c60..d056319 100644
162--- a/reactive/postgresql/upgrade.py
163+++ b/reactive/postgresql/upgrade.py
164@@ -26,6 +26,7 @@ from reactive import workloadstatus
165 from reactive.postgresql import helpers
166 from reactive.postgresql import postgresql
167 from reactive.postgresql import replication
168+from reactive.postgresql import service
169 from reactive.postgresql import storage
170
171
172@@ -134,6 +135,11 @@ def upgrade_charm():
173 reactive.clear_flag("postgresql.cluster.is_running")
174 postgresql.stop_pgctlcluster()
175
176+ # Update the PGDG source, in case the signing key has changed.
177+ config = hookenv.config()
178+ if config["pgdg"]:
179+ service.add_pgdg_source()
180+
181
182 def migrate_user(old_username, new_username, password, superuser=False):
183 if postgresql.is_primary():
184diff --git a/requirements.txt b/requirements.txt
185index 8aabe77..675a41a 100644
186--- a/requirements.txt
187+++ b/requirements.txt
188@@ -11,5 +11,6 @@ python-swiftclient
189 charms.reactive
190 charmhelpers
191 juju-deployer
192-pip<8.1.2
193-websocket-client<=0.40.0
194+pip
195+websocket-client
196+tenacity
197diff --git a/tox.ini b/tox.ini
198index 71565ed..cb30fb6 100644
199--- a/tox.ini
200+++ b/tox.ini
201@@ -29,9 +29,10 @@ commands=
202 basepython=python3
203 # pip install -I required to ensure scripts are created
204 # even if packages are already installed on the system
205-install_command=pip install -I {opts} {packages}
206-sitepackages=True
207-deps = -r{toxinidir}/requirements.txt
208+sitepackages=False
209+deps =
210+ -r{toxinidir}/requirements.txt
211+ psycopg2
212 passenv=JUJU_*
213 commands=
214 pytest {posargs:--verbose --tb=native unit_tests/}
215diff --git a/unit_tests/test_postgresql.py b/unit_tests/test_postgresql.py
216index 5524745..95cf828 100644
217--- a/unit_tests/test_postgresql.py
218+++ b/unit_tests/test_postgresql.py
219@@ -26,47 +26,47 @@ sys.path.insert(1, ROOT)
220 sys.path.insert(2, os.path.join(ROOT, 'lib'))
221 sys.path.insert(3, os.path.join(ROOT, 'lib', 'testdeps'))
222
223-from charmhelpers.core import hookenv
224-from charmhelpers.core import unitdata
225-
226+from charmhelpers.core import hookenv, host
227 from reactive import workloadstatus
228-from reactive.postgresql import helpers
229-from reactive.postgresql import postgresql
230+from reactive.postgresql import helpers, postgresql
231
232
233 class TestPostgresql(unittest.TestCase):
234+ @patch.object(postgresql, 'lsclusters_version')
235 @patch.object(hookenv, 'config')
236 @patch.object(helpers, 'distro_codename')
237- def test_version(self, codename, config):
238-
239- def clear_cache():
240- unitdata.kv().unset('postgresql.pg_version')
241+ def test_version(self, codename, config, lsclusters_version):
242+ # Installed version
243+ lsclusters_version.return_value = '9.5'
244+ postgresql.clear_version_cache()
245+ self.assertEqual(postgresql.version(), '9.5')
246+ lsclusters_version.return_value = None
247
248 # Explicit version in config.
249 config.return_value = {'version': '23'}
250- clear_cache()
251+ postgresql.clear_version_cache()
252 self.assertEqual(postgresql.version(), '23')
253
254 config.return_value = {'version': ''}
255
256 # Trusty default
257 codename.return_value = 'trusty'
258- clear_cache()
259+ postgresql.clear_version_cache()
260 self.assertEqual(postgresql.version(), '9.3')
261
262 # Xenial default
263 codename.return_value = 'xenial'
264- clear_cache()
265+ postgresql.clear_version_cache()
266 self.assertEqual(postgresql.version(), '9.5')
267
268 # Bionic default
269 codename.return_value = 'bionic'
270- clear_cache()
271+ postgresql.clear_version_cache()
272 self.assertEqual(postgresql.version(), '10')
273
274 # No other fallbacks, yet.
275 codename.return_value = 'whatever'
276- clear_cache()
277+ postgresql.clear_version_cache()
278 with self.assertRaises(NotImplementedError):
279 postgresql.version()
280
281@@ -251,8 +251,11 @@ class TestPostgresql(unittest.TestCase):
282 is_secondary.return_value = False
283 self.assertTrue(postgresql.is_primary())
284
285+ @patch.object(postgresql, 'has_version')
286+ @patch.object(hookenv, 'config')
287 @patch.object(postgresql, 'recovery_conf_path')
288- def test_is_secondary(self, recovery_path):
289+ def test_is_secondary(self, recovery_path, config, has_version):
290+ has_version.return_value = False
291 # if recovery.conf exists, we are a secondary.
292 with tempfile.NamedTemporaryFile() as f:
293 recovery_path.return_value = f.name
294@@ -317,7 +320,7 @@ class TestPostgresql(unittest.TestCase):
295 check_call.assert_called_once_with(['pg_createcluster',
296 '-e', sentinel.encoding,
297 '--locale', sentinel.locale,
298- '9.2', 'main'],
299+ '9.2', 'main', '--', '--data-checksums'],
300 universal_newlines=True)
301
302 @patch('subprocess.check_call')
303@@ -345,7 +348,7 @@ class TestPostgresql(unittest.TestCase):
304 cur.execute.assert_has_calls([
305 call('SELECT datname FROM pg_database WHERE datname=%s',
306 ('hello',)),
307- call('CREATE DATABASE %s', (ANY,))])
308+ call('CREATE DATABASE %s OWNER %s', (ANY, ANY))])
309 # The database name in that last call was correctly quoted.
310 quoted_dbname = cur.execute.call_args[0][1][0]
311 self.assertIsInstance(quoted_dbname, postgresql.AsIs)
312@@ -475,14 +478,16 @@ class TestPostgresql(unittest.TestCase):
313
314 pgidentifier.side_effect = lambda d: 'q_{}'.format(d)
315
316- existing_extensions = set(['extA', 'extB'])
317- wanted_extensions = set(['extB', 'extC'])
318+ existing_extensions = set([('extA', 'public'), ('extB', 'public')])
319+ wanted_extensions = set([('extB', 'public'), ('extC', 'custom')])
320
321- cur.fetchall.return_value = [[x] for x in existing_extensions]
322+ cur.fetchall.return_value = [x for x in existing_extensions]
323 postgresql.ensure_extensions(con, wanted_extensions)
324 cur.execute.assert_has_calls([
325- call('SELECT extname FROM pg_extension'),
326- call('CREATE EXTENSION %s', ('q_extC',))])
327+ call('SELECT extname, nspname FROM pg_extension, pg_namespace WHERE pg_namespace.oid = extnamespace'),
328+ call('CREATE SCHEMA IF NOT EXISTS %s', ('q_custom',)),
329+ call('GRANT USAGE ON SCHEMA %s TO PUBLIC', ('q_custom',)),
330+ call('CREATE EXTENSION %s WITH SCHEMA %s', ('q_extC','q_custom'))])
331
332 def test_addr_to_range(self):
333 eggs = [('hostname', 'hostname'),
334@@ -532,35 +537,18 @@ class TestPostgresql(unittest.TestCase):
335
336 @patch.object(postgresql, 'emit_pg_log')
337 @patch.object(workloadstatus, 'status_set')
338- @patch('subprocess.check_call')
339+ @patch.object(host, 'service_start')
340 @patch.object(postgresql, 'version')
341- def test_start(self, version, check_call, status_set, emit_pg_log):
342+ def test_start(self, version, service_start, status_set, emit_pg_log):
343 version.return_value = '9.9'
344
345 # When it works, it works.
346 postgresql.start()
347- # Both -w and -t options are required to wait for startup.
348- # We wait a long time, as startup might take a long time.
349- # Maybe we should wait a lot longer.
350- check_call.assert_called_once_with(['pg_ctlcluster', '9.9', 'main',
351- 'start', '--', '-w',
352- '-t', '86400'],
353- universal_newlines=True)
354+ service_start.assert_called_once_with('postgresql@9.9-main')
355 self.assertFalse(emit_pg_log.called)
356
357- # If it is already running, pg_ctlcluster returns code 2.
358- # We block, and terminate whatever hook is running.
359- check_call.side_effect = subprocess.CalledProcessError(2, 'whoops')
360- check_call.reset_mock()
361- postgresql.start()
362- check_call.assert_called_once_with(['pg_ctlcluster', '9.9', 'main',
363- 'start', '--', '-w',
364- '-t', '86400'],
365- universal_newlines=True)
366-
367- # Other failures block the unit. Perhaps it is just taking too
368- # perform recovery after a power outage.
369- check_call.side_effect = subprocess.CalledProcessError(42, 'whoops')
370+ # Start failure we block, and terminate whatever hook is running.
371+ service_start.return_value = False
372 with self.assertRaises(SystemExit) as x:
373 postgresql.start()
374 status_set.assert_called_once_with('blocked', ANY) # Set blocked.
375@@ -569,59 +557,29 @@ class TestPostgresql(unittest.TestCase):
376
377 @patch.object(hookenv, 'log')
378 @patch.object(workloadstatus, 'status_set')
379- @patch('subprocess.check_call')
380+ @patch.object(host, 'service_stop')
381 @patch.object(postgresql, 'version')
382- def test_stop(self, version, check_call, status_set, log):
383+ def test_stop(self, version, service_stop, status_set, log):
384 version.return_value = '9.9'
385
386 # Normal shutdown shuts down.
387+ service_stop.return_value = True
388 postgresql.stop()
389- # -t option is required to wait for shutdown to complete. -w not
390- # required unlike 'start', but lets be explicit.
391- check_call.assert_called_once_with(['pg_ctlcluster',
392- '--mode', 'fast', '9.9', 'main',
393- 'stop', '--', '-w', '-t', '300'],
394- universal_newlines=True)
395-
396- # If the server is not running, pg_ctlcluster(1) signals this with
397- # returncode 2.
398- check_call.side_effect = subprocess.CalledProcessError(2, 'whoops')
399- check_call.reset_mock()
400- postgresql.stop()
401- # -t option is required to wait for shutdown to complete. -w not
402- # required unlike 'start', but lets be explicit.
403- check_call.assert_called_once_with(['pg_ctlcluster',
404- '--mode', 'fast', '9.9', 'main',
405- 'stop', '--', '-w', '-t', '300'],
406- universal_newlines=True)
407+ service_stop.assert_called_once_with('postgresql@9.9-main')
408
409- # If 'fast' shutdown fails, we retry with an 'immediate' shutdown
410- check_call.side_effect = iter([subprocess.CalledProcessError(42, 'x'),
411- None])
412- check_call.reset_mock()
413- postgresql.stop()
414- check_call.assert_has_calls([
415- call(['pg_ctlcluster', '--mode', 'fast', '9.9', 'main',
416- 'stop', '--', '-w', '-t', '300'],
417- universal_newlines=True),
418- call(['pg_ctlcluster', '--mode', 'immediate', '9.9', 'main',
419- 'stop', '--', '-w', '-t', '300'],
420- universal_newlines=True)])
421-
422- # If both fail, we block the unit.
423- check_call.side_effect = subprocess.CalledProcessError(42, 'x')
424+ # Failed shutdown blocks and terminates
425+ service_stop.return_value = False
426 with self.assertRaises(SystemExit) as x:
427 postgresql.stop()
428 status_set.assert_called_once_with('blocked', ANY)
429 self.assertEqual(x.exception.code, 0) # Exit cleanly
430
431- @patch('subprocess.check_call')
432+ @patch.object(host, 'service_reload')
433 @patch.object(postgresql, 'version')
434- def test_reload_config(self, version, check_call):
435+ def test_reload_config(self, version, service_reload):
436 version.return_value = '9.9'
437 postgresql.reload_config()
438- check_call.assert_called_once_with(['pg_ctlcluster', '9.9', 'main',
439- 'reload'])
440+ service_reload.assert_called_once_with('postgresql@9.9-main')
441
442 def test_parse_config(self):
443 valid = [(r'# A comment', dict()),
444@@ -653,7 +611,7 @@ class TestPostgresql(unittest.TestCase):
445
446 with self.assertRaises(SyntaxError) as x:
447 postgresql.parse_config("=")
448- self.assertEqual(str(x.exception), 'Missing key (line 1)')
449+ self.assertEqual(str(x.exception), "Missing key '=' (line 1)")
450 self.assertEqual(x.exception.lineno, 1)
451 self.assertEqual(x.exception.text, "=")
452
453@@ -665,19 +623,19 @@ class TestPostgresql(unittest.TestCase):
454
455 with self.assertRaises(SyntaxError) as x:
456 postgresql.parse_config("key='unterminated")
457- self.assertEqual(str(x.exception), 'Badly quoted value (line 1)')
458+ self.assertEqual(str(x.exception), r'''Badly quoted value "'unterminated" (line 1)''')
459
460 with self.assertRaises(SyntaxError) as x:
461 postgresql.parse_config("key='unterminated 2 # comment")
462- self.assertEqual(str(x.exception), 'Badly quoted value (line 1)')
463+ self.assertEqual(str(x.exception), r'''Badly quoted value "'unterminated 2" (line 1)''')
464
465 with self.assertRaises(SyntaxError) as x:
466 postgresql.parse_config("key='unte''''")
467- self.assertEqual(str(x.exception), 'Badly quoted value (line 1)')
468+ self.assertEqual(str(x.exception), r"""Badly quoted value "'unte''''" (line 1)""")
469
470 with self.assertRaises(SyntaxError) as x:
471 postgresql.parse_config(r"key='\'")
472- self.assertEqual(str(x.exception), 'Badly quoted value (line 1)')
473+ self.assertEqual(str(x.exception), r'''Badly quoted value "'\\'" (line 1)''')
474
475 def test_convert_unit(self):
476 c = postgresql.convert_unit

Subscribers

People subscribed via source and target branches

to all changes: