Merge lp:~hopem/charms/trusty/keystone/lp1519035 into lp:~openstack-charmers-archive/charms/trusty/keystone/next
- Trusty Tahr (14.04)
- lp1519035
- Merge into next
Proposed by
Edward Hope-Morley
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 196 | ||||
Proposed branch: | lp:~hopem/charms/trusty/keystone/lp1519035 | ||||
Merge into: | lp:~openstack-charmers-archive/charms/trusty/keystone/next | ||||
Diff against target: |
294 lines (+95/-40) 2 files modified
hooks/keystone_hooks.py (+33/-23) unit_tests/test_keystone_hooks.py (+62/-17) |
||||
To merge this branch: | bzr merge lp:~hopem/charms/trusty/keystone/lp1519035 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Liam Young (community) | Approve | ||
Review via email: mp+281851@code.launchpad.net |
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote : | # |
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote : | # |
charm_unit_test #15640 keystone-next for hopem mp281851
UNIT OK: passed
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote : | # |
charm_amulet_test #8573 keystone-next for hopem mp281851
AMULET OK: passed
Build: http://
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'hooks/keystone_hooks.py' |
2 | --- hooks/keystone_hooks.py 2015-10-20 16:41:03 +0000 |
3 | +++ hooks/keystone_hooks.py 2016-01-07 12:34:16 +0000 |
4 | @@ -271,6 +271,33 @@ |
5 | update_all_identity_relation_units() |
6 | |
7 | |
8 | +def leader_init_db_if_ready(use_current_context=False): |
9 | + """ Initialise the keystone db if it is ready and mark it as initialised. |
10 | + |
11 | + NOTE: this must be idempotent. |
12 | + """ |
13 | + if not is_elected_leader(CLUSTER_RES): |
14 | + log("Not leader - skipping db init", level=DEBUG) |
15 | + return |
16 | + |
17 | + if is_db_initialised(): |
18 | + log("Database already initialised - skipping db init", level=DEBUG) |
19 | + return |
20 | + |
21 | + # Bugs 1353135 & 1187508. Dbs can appear to be ready before the |
22 | + # units acl entry has been added. So, if the db supports passing |
23 | + # a list of permitted units then check if we're in the list. |
24 | + if not is_db_ready(use_current_context=use_current_context): |
25 | + log('Allowed_units list provided and this unit not present', |
26 | + level=INFO) |
27 | + return |
28 | + |
29 | + migrate_database() |
30 | + # Ensure any existing service entries are updated in the |
31 | + # new database backend. Also avoid duplicate db ready check. |
32 | + update_all_identity_relation_units(check_db_ready=False) |
33 | + |
34 | + |
35 | @hooks.hook('shared-db-relation-changed') |
36 | @restart_on_change(restart_map()) |
37 | @synchronize_ca_if_changed() |
38 | @@ -279,19 +306,7 @@ |
39 | log('shared-db relation incomplete. Peer not ready?') |
40 | else: |
41 | CONFIGS.write(KEYSTONE_CONF) |
42 | - if is_elected_leader(CLUSTER_RES): |
43 | - # Bugs 1353135 & 1187508. Dbs can appear to be ready before the |
44 | - # units acl entry has been added. So, if the db supports passing |
45 | - # a list of permitted units then check if we're in the list. |
46 | - if not is_db_ready(use_current_context=True): |
47 | - log('Allowed_units list provided and this unit not present', |
48 | - level=INFO) |
49 | - return |
50 | - |
51 | - migrate_database() |
52 | - # Ensure any existing service entries are updated in the |
53 | - # new database backend. Also avoid duplicate db ready check. |
54 | - update_all_identity_relation_units(check_db_ready=False) |
55 | + leader_init_db_if_ready(use_current_context=True) |
56 | |
57 | |
58 | @hooks.hook('pgsql-db-relation-changed') |
59 | @@ -302,16 +317,7 @@ |
60 | log('pgsql-db relation incomplete. Peer not ready?') |
61 | else: |
62 | CONFIGS.write(KEYSTONE_CONF) |
63 | - if is_elected_leader(CLUSTER_RES): |
64 | - if not is_db_ready(use_current_context=True): |
65 | - log('Allowed_units list provided and this unit not present', |
66 | - level=INFO) |
67 | - return |
68 | - |
69 | - migrate_database() |
70 | - # Ensure any existing service entries are updated in the |
71 | - # new database backend. Also avoid duplicate db ready check. |
72 | - update_all_identity_relation_units(check_db_ready=False) |
73 | + leader_init_db_if_ready(use_current_context=True) |
74 | |
75 | |
76 | @hooks.hook('identity-service-relation-changed') |
77 | @@ -612,6 +618,10 @@ |
78 | ensure_ssl_dirs() |
79 | |
80 | CONFIGS.write_all() |
81 | + |
82 | + # See LP bug 1519035 |
83 | + leader_init_db_if_ready() |
84 | + |
85 | update_nrpe_config() |
86 | |
87 | if is_elected_leader(CLUSTER_RES): |
88 | |
89 | === modified file 'unit_tests/test_keystone_hooks.py' |
90 | --- unit_tests/test_keystone_hooks.py 2015-10-20 16:41:03 +0000 |
91 | +++ unit_tests/test_keystone_hooks.py 2016-01-07 12:34:16 +0000 |
92 | @@ -60,7 +60,6 @@ |
93 | 'synchronize_ca_if_changed', |
94 | 'update_nrpe_config', |
95 | 'ensure_ssl_dirs', |
96 | - 'is_db_initialised', |
97 | 'is_db_ready', |
98 | # other |
99 | 'check_call', |
100 | @@ -246,14 +245,30 @@ |
101 | configs.write = MagicMock() |
102 | hooks.pgsql_db_changed() |
103 | |
104 | + @patch('keystone_utils.relation_ids') |
105 | + @patch('keystone_utils.peer_retrieve') |
106 | + @patch('keystone_utils.peer_store') |
107 | @patch('keystone_utils.log') |
108 | @patch('keystone_utils.ensure_ssl_cert_master') |
109 | @patch.object(hooks, 'CONFIGS') |
110 | @patch.object(hooks, 'identity_changed') |
111 | def test_db_changed_allowed(self, identity_changed, configs, |
112 | - mock_ensure_ssl_cert_master, |
113 | - mock_log): |
114 | - self.is_db_initialised.return_value = True |
115 | + mock_ensure_ssl_cert_master, mock_log, |
116 | + mock_peer_store, |
117 | + mock_peer_retrieve, mock_relation_ids): |
118 | + mock_relation_ids.return_value = ['peer/0'] |
119 | + peer_settings = {} |
120 | + |
121 | + def fake_peer_store(key, val): |
122 | + peer_settings[key] = val |
123 | + |
124 | + def fake_migrate(): |
125 | + fake_peer_store('db-initialised', 'True') |
126 | + |
127 | + self.migrate_database.side_effect = fake_migrate |
128 | + mock_peer_store.side_effect = fake_peer_store |
129 | + mock_peer_retrieve.side_effect = lambda key: peer_settings.get(key) |
130 | + |
131 | self.is_db_ready.return_value = True |
132 | mock_ensure_ssl_cert_master.return_value = False |
133 | self.relation_ids.return_value = ['identity-service:0'] |
134 | @@ -268,12 +283,15 @@ |
135 | relation_id='identity-service:0', |
136 | remote_unit='unit/0') |
137 | |
138 | + @patch('keystone_utils.relation_ids') |
139 | @patch('keystone_utils.log') |
140 | @patch('keystone_utils.ensure_ssl_cert_master') |
141 | @patch.object(hooks, 'CONFIGS') |
142 | @patch.object(hooks, 'identity_changed') |
143 | def test_db_changed_not_allowed(self, identity_changed, configs, |
144 | - mock_ensure_ssl_cert_master, mock_log): |
145 | + mock_ensure_ssl_cert_master, mock_log, |
146 | + mock_relation_ids): |
147 | + mock_relation_ids.return_value = [] |
148 | self.is_db_ready.return_value = False |
149 | mock_ensure_ssl_cert_master.return_value = False |
150 | self.relation_ids.return_value = ['identity-service:0'] |
151 | @@ -286,13 +304,31 @@ |
152 | self.assertFalse(self.ensure_initial_admin.called) |
153 | self.assertFalse(identity_changed.called) |
154 | |
155 | + @patch('keystone_utils.relation_ids') |
156 | + @patch('keystone_utils.peer_retrieve') |
157 | + @patch('keystone_utils.peer_store') |
158 | @patch('keystone_utils.log') |
159 | @patch('keystone_utils.ensure_ssl_cert_master') |
160 | @patch.object(hooks, 'CONFIGS') |
161 | @patch.object(hooks, 'identity_changed') |
162 | def test_postgresql_db_changed(self, identity_changed, configs, |
163 | - mock_ensure_ssl_cert_master, mock_log): |
164 | - self.is_db_initialised.return_value = True |
165 | + mock_ensure_ssl_cert_master, mock_log, |
166 | + mock_peer_store, mock_peer_retrieve, |
167 | + mock_relation_ids): |
168 | + mock_relation_ids.return_value = ['peer/0'] |
169 | + |
170 | + peer_settings = {} |
171 | + |
172 | + def fake_peer_store(key, val): |
173 | + peer_settings[key] = val |
174 | + |
175 | + def fake_migrate(): |
176 | + fake_peer_store('db-initialised', 'True') |
177 | + |
178 | + self.migrate_database.side_effect = fake_migrate |
179 | + mock_peer_store.side_effect = fake_peer_store |
180 | + mock_peer_retrieve.side_effect = lambda key: peer_settings.get(key) |
181 | + |
182 | self.is_db_ready.return_value = True |
183 | mock_ensure_ssl_cert_master.return_value = False |
184 | self.relation_ids.return_value = ['identity-service:0'] |
185 | @@ -307,6 +343,7 @@ |
186 | relation_id='identity-service:0', |
187 | remote_unit='unit/0') |
188 | |
189 | + @patch.object(hooks, 'is_db_initialised') |
190 | @patch.object(hooks, 'git_install_requested') |
191 | @patch('keystone_utils.log') |
192 | @patch('keystone_utils.ensure_ssl_cert_master') |
193 | @@ -340,11 +377,12 @@ |
194 | mock_ensure_pki_dir_permissions, |
195 | mock_ensure_ssl_dirs, |
196 | mock_ensure_ssl_cert_master, |
197 | - mock_log, git_requested): |
198 | + mock_log, git_requested, |
199 | + mock_is_db_initialised): |
200 | git_requested.return_value = False |
201 | mock_is_pki_enabled.return_value = True |
202 | mock_is_ssl_cert_master.return_value = True |
203 | - self.is_db_initialised.return_value = True |
204 | + mock_is_db_initialised.return_value = True |
205 | self.is_db_ready.return_value = True |
206 | self.openstack_upgrade_available.return_value = False |
207 | self.is_elected_leader.return_value = True |
208 | @@ -421,6 +459,7 @@ |
209 | self.assertFalse(self.ensure_initial_admin.called) |
210 | self.assertFalse(identity_changed.called) |
211 | |
212 | + @patch.object(hooks, 'is_db_initialised') |
213 | @patch.object(hooks, 'git_install_requested') |
214 | @patch('keystone_utils.log') |
215 | @patch('keystone_utils.ensure_ssl_cert_master') |
216 | @@ -453,12 +492,13 @@ |
217 | mock_ensure_pki_permissions, |
218 | mock_ensure_ssl_dirs, |
219 | mock_ensure_ssl_cert_master, |
220 | - mock_log, git_requested): |
221 | + mock_log, git_requested, |
222 | + mock_is_db_initialised): |
223 | git_requested.return_value = False |
224 | mock_is_pki_enabled.return_value = True |
225 | mock_is_ssl_cert_master.return_value = True |
226 | self.is_db_ready.return_value = True |
227 | - self.is_db_initialised.return_value = True |
228 | + mock_is_db_initialised.return_value = True |
229 | self.openstack_upgrade_available.return_value = True |
230 | self.is_elected_leader.return_value = True |
231 | # avoid having to mock syncer |
232 | @@ -544,6 +584,7 @@ |
233 | self.assertFalse(self.openstack_upgrade_available.called) |
234 | self.assertFalse(self.do_openstack_upgrade_reexec.called) |
235 | |
236 | + @patch.object(hooks, 'is_db_initialised') |
237 | @patch.object(hooks, 'git_install_requested') |
238 | @patch.object(hooks, 'config_value_changed') |
239 | @patch.object(hooks, 'ensure_ssl_dir') |
240 | @@ -562,7 +603,8 @@ |
241 | is_pki, config_https, |
242 | ensure_ssl_dir, |
243 | config_value_changed, |
244 | - git_requested): |
245 | + git_requested, |
246 | + mock_db_init): |
247 | ensure_ssl_cert.return_value = False |
248 | is_pki.return_value = False |
249 | peer_units.return_value = [] |
250 | @@ -575,14 +617,15 @@ |
251 | |
252 | self.assertFalse(self.do_openstack_upgrade_reexec.called) |
253 | |
254 | + @patch.object(hooks, 'is_db_initialised') |
255 | @patch('keystone_utils.log') |
256 | @patch('keystone_utils.ensure_ssl_cert_master') |
257 | @patch.object(hooks, 'hashlib') |
258 | @patch.object(hooks, 'send_notifications') |
259 | def test_identity_changed_leader(self, mock_send_notifications, |
260 | mock_hashlib, mock_ensure_ssl_cert_master, |
261 | - mock_log): |
262 | - self.is_db_initialised.return_value = True |
263 | + mock_log, mock_is_db_initialised): |
264 | + mock_is_db_initialised.return_value = True |
265 | self.is_db_ready.return_value = True |
266 | mock_ensure_ssl_cert_master.return_value = False |
267 | hooks.identity_changed( |
268 | @@ -784,6 +827,7 @@ |
269 | self.assertTrue(configs.write_all.called) |
270 | self.assertFalse(mock_synchronize_ca.called) |
271 | |
272 | + @patch.object(hooks, 'is_db_initialised') |
273 | @patch('keystone_utils.log') |
274 | @patch('keystone_utils.ensure_ssl_cert_master') |
275 | @patch.object(hooks, 'identity_changed') |
276 | @@ -791,8 +835,9 @@ |
277 | def test_ha_relation_changed_clustered_leader(self, configs, |
278 | identity_changed, |
279 | mock_ensure_ssl_cert_master, |
280 | - mock_log): |
281 | - self.is_db_initialised.return_value = True |
282 | + mock_log, |
283 | + mock_is_db_initialised): |
284 | + mock_is_db_initialised.return_value = True |
285 | self.is_db_ready.return_value = True |
286 | mock_ensure_ssl_cert_master.return_value = False |
287 | self.relation_get.return_value = True |
288 | @@ -906,5 +951,5 @@ |
289 | ssh_authorized_peers.assert_called_with( |
290 | user=self.ssh_user, group='juju_keystone', |
291 | peer_interface='cluster', ensure_local_user=True) |
292 | - self.assertFalse(self.log.called) |
293 | + self.assertTrue(self.log.called) |
294 | self.assertFalse(self.ensure_initial_admin.called) |
charm_lint_check #16748 keystone-next for hopem mp281851
LINT OK: passed
Build: http:// 10.245. 162.77: 8080/job/ charm_lint_ check/16748/