openstack
/
charm-ceph-mon
Code Issues Proposed changes

Fix handling of recursion in notify_rbd_mirrors 

The committed iteration had a problem:
In the event the ``ceph-rbd-mirror`` charm created a new pool
on a remote cluster and set the relation data on the ceph
leader unit, members of other instances of the ``rbd-mirror``
relation would not be updated with pool information.

Also limit number of times notify_mons() is run

Change-Id: I2a03ca02285e7a99c2cae48dbafc014fb478fb84
This commit is contained in:
Frode Nordahl 2019-02-28 00:07:26 +01:00
parent e3bf76a1c4
commit f6d9a61cb6
2 changed files with 27 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
hooks/ceph_hooks.py
View File

@ -39,7 +39,6 @@ from charmhelpers.core.hookenv import (
is_relation_made, is_relation_made,
relation_get, relation_get,
relation_set, relation_set,
relation_type,
leader_set, leader_get, leader_set, leader_get,
is_leader, is_leader,
remote_unit, remote_unit,
@ -475,7 +474,7 @@ def notify_radosgws():
def notify_rbd_mirrors(): def notify_rbd_mirrors():
for relid in relation_ids('rbd-mirror'): for relid in relation_ids('rbd-mirror'):
for unit in related_units(relid): for unit in related_units(relid):
rbd_mirror_relation(relid=relid, unit=unit) rbd_mirror_relation(relid=relid, unit=unit, recurse=False)
def notify_client(): def notify_client():
@ -509,7 +508,8 @@ def notify_mons():
relation_settings={'nonce': nonce}) relation_settings={'nonce': nonce})
def handle_broker_request(relid, unit, add_legacy_response=False): def handle_broker_request(relid, unit, add_legacy_response=False,
recurse=True):
"""Retrieve broker request from relation, process, return response data. """Retrieve broker request from relation, process, return response data.
:param relid: Realtion ID :param relid: Realtion ID
@ -519,6 +519,10 @@ def handle_broker_request(relid, unit, add_legacy_response=False):
:param add_legacy_response: (Optional) Adds the legacy ``broker_rsp`` key :param add_legacy_response: (Optional) Adds the legacy ``broker_rsp`` key
to the response in addition to the new way. to the response in addition to the new way.
:type add_legacy_response: bool :type add_legacy_response: bool
:param recurse: Whether we should call out to update relation functions or
not. Mainly used to handle recursion when called from
notify_rbd_mirrors()
:type recurse: bool
:returns: Dictionary of response data ready for use with relation_set. :returns: Dictionary of response data ready for use with relation_set.
:rtype: dict :rtype: dict
""" """
@ -537,8 +541,7 @@ def handle_broker_request(relid, unit, add_legacy_response=False):
if add_legacy_response: if add_legacy_response:
response.update({'broker_rsp': rsp}) response.update({'broker_rsp': rsp})
# prevent recursion when called from rbd_mirror_relation() if relation_ids('rbd-mirror') and recurse:
if relation_type() != 'rbd-mirror':
# update ``rbd-mirror`` relations for this unit with # update ``rbd-mirror`` relations for this unit with
# information about new pools. # information about new pools.
log('Notifying this units rbd-mirror relations after ' log('Notifying this units rbd-mirror relations after '
@ -546,7 +549,8 @@ def handle_broker_request(relid, unit, add_legacy_response=False):
notify_rbd_mirrors() notify_rbd_mirrors()
# notify mons to flag that the other mon units should update # notify mons to flag that the other mon units should update
# their ``rbd-mirror`` relations with information about new pools. # their ``rbd-mirror`` relations with information about new
# pools.
log('Notifying peers after processing broker request.', log('Notifying peers after processing broker request.',
level=DEBUG) level=DEBUG)
notify_mons() notify_mons()
@ -672,14 +676,14 @@ def radosgw_relation(relid=None, unit=None):
@hooks.hook('rbd-mirror-relation-joined') @hooks.hook('rbd-mirror-relation-joined')
@hooks.hook('rbd-mirror-relation-changed') @hooks.hook('rbd-mirror-relation-changed')
def rbd_mirror_relation(relid=None, unit=None): def rbd_mirror_relation(relid=None, unit=None, recurse=True):
if ready_for_service(): if ready_for_service():
log('mon cluster in quorum and osds bootstrapped ' log('mon cluster in quorum and osds bootstrapped '
'- providing rbd-mirror client with keys') '- providing rbd-mirror client with keys')
if not unit: if not unit:
unit = remote_unit() unit = remote_unit()
# handle broker requests first to get a updated pool map # handle broker requests first to get a updated pool map
data = (handle_broker_request(relid, unit)) data = (handle_broker_request(relid, unit, recurse=recurse))
data.update({ data.update({
'auth': config('auth-supported'), 'auth': config('auth-supported'),
'ceph-public-address': get_public_addr(), 'ceph-public-address': get_public_addr(),

14
unit_tests/test_ceph_hooks.py
View File

@ -261,7 +261,8 @@ class CephHooksTestCase(unittest.TestCase):
mock_relation_ids.assert_called_once_with('rbd-mirror') mock_relation_ids.assert_called_once_with('rbd-mirror')
mock_related_units.assert_called_once_with('arelid') mock_related_units.assert_called_once_with('arelid')
mock_rbd_mirror_relation.assert_called_once_with(relid='arelid', mock_rbd_mirror_relation.assert_called_once_with(relid='arelid',
unit='aunit') unit='aunit',
recurse=False)
@patch.object(ceph_hooks, 'uuid') @patch.object(ceph_hooks, 'uuid')
@patch.object(ceph_hooks, 'relation_set') @patch.object(ceph_hooks, 'relation_set')
@ -360,6 +361,7 @@ class RelatedUnitsTestCase(unittest.TestCase):
'broker-rsp-glance-0': 'AOK', 'broker-rsp-glance-0': 'AOK',
'broker_rsp': 'AOK'}) 'broker_rsp': 'AOK'})
@patch.object(ceph_hooks, 'relation_ids')
@patch.object(ceph_hooks, 'notify_mons') @patch.object(ceph_hooks, 'notify_mons')
@patch.object(ceph_hooks, 'notify_rbd_mirrors') @patch.object(ceph_hooks, 'notify_rbd_mirrors')
@patch.object(ceph_hooks, 'process_requests') @patch.object(ceph_hooks, 'process_requests')
@ -370,7 +372,8 @@ class RelatedUnitsTestCase(unittest.TestCase):
mock_ceph_is_leader, mock_ceph_is_leader,
mock_broker_process_requests, mock_broker_process_requests,
mock_notify_rbd_mirrors, mock_notify_rbd_mirrors,
mock_notify_mons): mock_notify_mons,
mock_relation_ids):
mock_remote_unit.return_value = 'glance/0' mock_remote_unit.return_value = 'glance/0'
ceph_hooks.handle_broker_request('rel1', None) ceph_hooks.handle_broker_request('rel1', None)
mock_remote_unit.assert_called_once_with() mock_remote_unit.assert_called_once_with()
@ -388,6 +391,11 @@ class RelatedUnitsTestCase(unittest.TestCase):
ceph_hooks.handle_broker_request('rel1', 'glance/0', ceph_hooks.handle_broker_request('rel1', 'glance/0',
add_legacy_response=True), add_legacy_response=True),
{'broker_rsp': 'AOK', 'broker-rsp-glance-0': 'AOK'}) {'broker_rsp': 'AOK', 'broker-rsp-glance-0': 'AOK'})
mock_notify_rbd_mirrors.reset_mock()
mock_notify_mons.reset_mock()
ceph_hooks.handle_broker_request('rel1', None, recurse=False)
self.assertFalse(mock_notify_rbd_mirrors.called)
self.assertFalse(mock_notify_mons.called)
class BootstrapSourceTestCase(test_utils.CharmTestCase): class BootstrapSourceTestCase(test_utils.CharmTestCase):
@ -615,7 +623,7 @@ class RBDMirrorRelationTestCase(test_utils.CharmTestCase):
} }
ceph_hooks.rbd_mirror_relation('rbd-mirror:51', 'ceph-rbd-mirror/0') ceph_hooks.rbd_mirror_relation('rbd-mirror:51', 'ceph-rbd-mirror/0')
self.handle_broker_request.assert_called_with( self.handle_broker_request.assert_called_with(
'rbd-mirror:51', 'ceph-rbd-mirror/0') 'rbd-mirror:51', 'ceph-rbd-mirror/0', recurse=True)
self.relation_set.assert_called_with( self.relation_set.assert_called_with(
relation_id='rbd-mirror:51', relation_id='rbd-mirror:51',
relation_settings=base_relation_settings) relation_settings=base_relation_settings)