From 9b8b81a0bc8406f03b2de884eeec91b8e8f2d442 Mon Sep 17 00:00:00 2001 From: Chris MacNaughton Date: Mon, 16 Aug 2021 16:55:14 -0500 Subject: [PATCH] Use the application data bag to set id and id_service notifications When purely using relation-set from a leader, updates after the leader has changed can lead to old data being persisted on a relation in addition to newer data being set by the new leader. When this happens, there can be issues with services using old data to talk to other related services. This change introduces the use of the application data bag to ensure that all units related to keystone get the same data from the leader, regardless of leadership changes. While this change enables the application data bag for these relations, it still sends the per-unit relation data as well to maintain backwards compatibility. Charms that consume the identity-service and identity-notification relations will need an update to use the application data bag to complete this change. Partial-Bug: #1902264 Change-Id: Iadd795fec605e7704e5a6673906452279bbecb34 --- hooks/keystone_hooks.py | 2 +- hooks/keystone_utils.py | 27 +++++++++++++++- unit_tests/test_keystone_hooks.py | 2 +- unit_tests/test_keystone_utils.py | 52 ++++++++++++++++++++++--------- 4 files changed, 66 insertions(+), 17 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index fda34745..79da4f6f 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -427,7 +427,7 @@ def db_departed_or_broken(): def identity_changed(relation_id=None, remote_unit=None): notifications_checksums = {} notifications_endpoints = {} - if is_elected_leader(CLUSTER_RES): + if is_leader(): if not is_db_ready(): log("identity-service-relation-changed hook fired before db " "ready - deferring until db ready", level=WARNING) diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index c1055f17..2774f991 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -2240,6 +2240,20 @@ def send_id_service_notifications(data): relation_settings={ 'catalog_ttl': config('catalog-cache-expiration'), 'ep_changed': json.dumps(changed, sort_keys=True)}) + # As a first step in resolving a race condition when updating + # identity and identity-notifications (LP: #1902264), start sending + # this data into the application data-bag instead of just the + # per-unit relation data. The consuming charms will need updating + # to look in the app data bag as well. + # + # After the charms consuming thie relation have been updated, + # it will be possible to remove the per-relation set above. + relation_set( + relation_id=rid, + relation_settings={ + 'catalog_ttl': config('catalog-cache-expiration'), + 'ep_changed': json.dumps(changed, sort_keys=True)}, + app=True) def send_notifications(checksum_data, endpoint_data, force=False): @@ -2262,7 +2276,7 @@ def send_id_notifications(data, force=False): :param force: Determines whether a trigger value is set to ensure the remote hook is fired. """ - if not data or not is_elected_leader(CLUSTER_RES): + if not data or not is_leader(): log("Not sending notifications (no data or not leader)", level=INFO) return @@ -2311,6 +2325,17 @@ def send_id_notifications(data, force=False): level=DEBUG) for rid in rel_ids: relation_set(relation_id=rid, relation_settings=_notifications) + # As a first step in resolving a race condition when updating + # identity and identity-notifications (LP: #1902264), start sending + # this data into the application data-bag instead of just the + # per-unit relation data. The consuming charms will need updating + # to look in the app data bag as well. + # + # After the charms consuming thie relation have been updated, + # it will be possible to remove the per-relation set above. + relation_set(relation_id=rid, + relation_settings=_notifications, + app=True) def is_db_ready(use_current_context=False, db_rel=None): diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index 1f38aab7..2079bb40 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/unit_tests/test_keystone_hooks.py @@ -466,7 +466,7 @@ class KeystoneRelationTests(CharmTestCase): @patch('keystone_utils.log') def test_identity_changed_no_leader(self, mock_log): - self.is_elected_leader.return_value = False + self.is_leader.return_value = False hooks.identity_changed( relation_id='identity-service:0', remote_unit='unit/0') diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index 3ce32261..e1f79717 100644 --- a/unit_tests/test_keystone_utils.py +++ b/unit_tests/test_keystone_utils.py @@ -893,32 +893,50 @@ class TestKeystoneUtils(CharmTestCase): @patch.object(utils, 'relation_set') @patch.object(utils, 'relation_get') @patch.object(utils, 'relation_ids') - @patch.object(utils, 'is_elected_leader') - def test_send_id_notifications(self, mock_is_elected_leader, + @patch.object(utils, 'is_leader') + def test_send_id_notifications(self, mock_is_leader, mock_relation_ids, mock_relation_get, mock_relation_set, mock_uuid): checksums = {'foo-endpoint-changed': 1} relation_id = 'testrel:0' mock_uuid.uuid4.return_value = '1234' mock_relation_ids.return_value = [relation_id] - mock_is_elected_leader.return_value = False + mock_is_leader.return_value = False utils.send_id_notifications(checksums) self.assertFalse(mock_relation_set.called) - mock_is_elected_leader.return_value = True + mock_is_leader.return_value = True utils.send_id_notifications({}) self.assertFalse(mock_relation_set.called) utils.send_id_notifications(checksums) self.assertTrue(mock_relation_set.called) - mock_relation_set.assert_called_once_with(relation_id=relation_id, - relation_settings=checksums) + mock_relation_set.assert_has_calls([ + call( + relation_id=relation_id, + relation_settings=checksums, + app=True + ), + call( + relation_id=relation_id, + relation_settings=checksums + ), + ], any_order=True) mock_relation_set.reset_mock() utils.send_id_notifications(checksums, force=True) self.assertTrue(mock_relation_set.called) checksums['trigger'] = '1234' - mock_relation_set.assert_called_once_with(relation_id=relation_id, - relation_settings=checksums) + mock_relation_set.assert_has_calls([ + call( + relation_id=relation_id, + relation_settings=checksums, + app=True + ), + call( + relation_id=relation_id, + relation_settings=checksums + ), + ], any_order=True) @patch.object(utils, 'service_endpoint_dict') @patch.object(utils, 'relation_ids') @@ -984,7 +1002,8 @@ class TestKeystoneUtils(CharmTestCase): 'ep_changed': ('{"neutron": {"internal": ' '"http://neutron.demo.com:9696"}}') - } + }, + app=True ), call( relation_id='identity-service:2', @@ -994,7 +1013,8 @@ class TestKeystoneUtils(CharmTestCase): ('{"neutron": {"internal": ' '"http://neutron.demo.com:9696"},' ' "placement": {"internal": "http://demo.com"}}') - } + }, + app=True ) ], any_order=True) @@ -1009,7 +1029,8 @@ class TestKeystoneUtils(CharmTestCase): 'catalog_ttl': 60, 'ep_changed': '{"neutron": {"internal": "http://demo.com"}}' - } + }, + app=True ), call( relation_id='identity-service:2', @@ -1017,7 +1038,8 @@ class TestKeystoneUtils(CharmTestCase): 'catalog_ttl': 60, 'ep_changed': '{"neutron": {"internal": "http://demo.com"}}' - } + }, + app=True ) ] mock_relation_set.assert_has_calls( @@ -1036,7 +1058,8 @@ class TestKeystoneUtils(CharmTestCase): 'catalog_ttl': 60, 'ep_changed': '{"neutron": {"internal": "http://demo.com"}}' - } + }, + app=True ), call( relation_id='identity-service:2', @@ -1046,7 +1069,8 @@ class TestKeystoneUtils(CharmTestCase): '{"neutron": {"internal": "http://demo.com"}, ' '"placement": {"internal": "http://demo.com"}}' ) - } + }, + app=True ) ] mock_relation_set.assert_has_calls(