Merge lp:~hopem/charms/trusty/keystone/lp1519035 into lp:~openstack-charmers-archive/charms/trusty/keystone/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
Reviewer Review Type Date Requested Status
Liam Young (community) Approve
Review via email: mp+281851@code.launchpad.net
To post a comment you must log in.
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

charm_lint_check #16748 keystone-next for hopem mp281851
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/16748/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

charm_unit_test #15640 keystone-next for hopem mp281851
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/15640/

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://10.245.162.77:8080/job/charm_amulet_test/8573/

Revision history for this message
Liam Young (gnuoy) wrote :

Approve

review: Approve

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)

Subscribers

People subscribed via source and target branches