Merge lp:~verterok/charms/trusty/tanuki-spec-manager/sane_db-relation-changed_restarts into lp:~tanuki/charms/trusty/tanuki-spec-manager/trunk

Proposed by Guillermo Gonzalez on 2015-11-03
Status: Merged
Approved by: Guillermo Gonzalez on 2015-11-04
Approved revision: 39
Merged at revision: 34
Proposed branch: lp:~verterok/charms/trusty/tanuki-spec-manager/sane_db-relation-changed_restarts
Merge into: lp:~tanuki/charms/trusty/tanuki-spec-manager/trunk
Diff against target: 150 lines (+69/-30)
2 files modified
hooks/actions.py (+68/-27)
hooks/services.py (+1/-3)
To merge this branch: bzr merge lp:~verterok/charms/trusty/tanuki-spec-manager/sane_db-relation-changed_restarts
Reviewer Review Type Date Requested Status
Celso Providelo (community) 2015-11-03 Approve on 2015-11-04
Review via email: mp+276530@code.launchpad.net

Commit message

Fix handling of db-relation-changed hook and avoid pointless restarts.

Description of the change

"reuse" some knowledge from landscape charm on how handle db-relation-changed hook and avoid pointless restarts.

This is to fix the case of adding a new service/unit relation to postgresql and getting a db-relation-changed triggered on all already related services, with a change on allowed-units field which shouldn't trigger a restart on all other services.

To post a comment you must log in.
Celso Providelo (cprov) wrote :

Thanks, Guillo.

I am happy to try it on staging and if it works fine propagate the Restart(ManagerCallback) to other charms, since it's very portable.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/actions.py'
2--- hooks/actions.py 2015-10-29 21:27:35 +0000
3+++ hooks/actions.py 2015-11-03 23:08:19 +0000
4@@ -5,13 +5,15 @@
5 import subprocess
6 import yaml
7
8+from hashlib import md5
9+
10 from charmhelpers import fetch
11 from charmhelpers.core import (
12 hookenv,
13 host,
14 )
15 from charmhelpers.core.services import helpers
16-from charmhelpers.core.services.base import service_restart
17+from charmhelpers.core.services.base import service_restart, ManagerCallback
18 from charmhelpers.core.templating import render
19 from charmhelpers.payload import (
20 archive,
21@@ -36,31 +38,70 @@
22 ETC_DIR = os.path.join(BASE_DIR, 'etc')
23
24
25-def restart(service_name):
26- """Restart the service, only if the config changed"""
27- current_config_hash = get_config_file_hash()
28- if current_config_hash != config['current_config_hash']:
29- service_restart(service_name)
30-
31-
32-def get_config_file_hash():
33- from hashlib import md5
34- config_file = os.path.join(SERVICE_DIR, 'service.conf')
35- if not os.path.exists(config_file):
36- return None
37- with open(config_file, 'r') as fd:
38- h = md5(fd.read())
39- return h.hexdigest()
40-
41-
42-def refresh_config_hash(service_name):
43- current_config_hash = get_config_file_hash()
44- config['current_config_hash'] = current_config_hash
45- config.save()
46-
47-
48-def log_start(service_name):
49- hookenv.log('spec-manager starting')
50+class Restart(ManagerCallback):
51+ """Restart the service, only if the config or relevant bits of the
52+ db-relation changed
53+ """
54+
55+ no_restart_db_keys = ('allowed-units',)
56+
57+ def _get_required_data(self, manager, service_name, key):
58+ """Get the service manager required_data entry matching the given key"""
59+ service = manager.get_service(service_name)
60+ for data in service["required_data"]:
61+ if key in data:
62+ return data[key]
63+
64+ def _update_persisted_data(self, key, value):
65+ """Persist the given 'value' for the given 'key' and return the old value.
66+
67+ @param key: The key to update.
68+ @param value: The value to persist.
69+ @return: The old value of the key, or None if it's a new value.
70+ """
71+ old = config.get(key, None)
72+ config[key] = value
73+ config.save()
74+ return old
75+
76+ def _needs_restart_db_changed(self, db_new, db_old):
77+ if db_old is None:
78+ return True
79+ new = db_new.copy()
80+ old = db_old.copy()
81+ for key in self.no_restart_db_keys:
82+ new.pop(key)
83+ old.pop(key, None)
84+ return new != old
85+
86+ def get_config_file_hash(self):
87+ config_file = os.path.join(SERVICE_DIR, 'service.conf')
88+ if not os.path.exists(config_file):
89+ return None
90+ with open(config_file, 'r') as fd:
91+ h = md5(fd.read())
92+ return h.hexdigest()
93+
94+ def __call__(self, manager, service_name, event_name):
95+
96+ if hookenv.hook_name() == 'db-relation-changed':
97+ db_new = self._get_required_data(manager, service_name, 'db')
98+ # for some unknown reason we can get a list or dict
99+ if isinstance(db_new, list):
100+ db_new = db_new[0]
101+ db_old = self._update_persisted_data('db', db_new)
102+ if self._needs_restart_db_changed(db_new, db_old):
103+ hookenv.log('Restarting service, db config changed')
104+ service_restart(service_name)
105+ return
106+ elif hookenv.hook_name() == 'config-changed':
107+ config_new = self.get_config_file_hash()
108+ config_old = self._update_persisted_data('config_hash', config_new)
109+ if config_new != config_old:
110+ hookenv.log('Restarting service, config file changed')
111+ service_restart(service_name)
112+ return
113+ hookenv.log('Not restarting service, config and db-relation unchanged')
114
115
116 def ensure_directories(service_name):
117@@ -323,7 +364,7 @@
118 username=parsed.username,
119 password=parsed.password,
120 use_tls=use_tls,
121- vhost=vhost)
122+ vhost=vhost)
123 checks.append(amqp_check)
124 return checks
125
126
127=== modified file 'hooks/services.py'
128--- hooks/services.py 2015-10-29 21:27:35 +0000
129+++ hooks/services.py 2015-11-03 23:08:19 +0000
130@@ -18,7 +18,6 @@
131 'required_data': [config,
132 actions.PostgresqlRelation(required=True)],
133 'data_ready': [
134- actions.refresh_config_hash,
135 actions.ensure_directories,
136 actions.get_cloud_service_from_tarball,
137 actions.install_python_packages,
138@@ -30,11 +29,10 @@
139 actions.write_db_config,
140 actions.setup_store_poller,
141 actions.publish_conn_check_relation_data,
142- actions.log_start,
143 actions.nrpe_external_master_relation,
144 ],
145 'start': [
146- actions.restart
147+ actions.Restart()
148 ],
149 },
150 ])

Subscribers

People subscribed via source and target branches