Ensure broker requests are re-processed on upgrade-charm

When broker-request caching was added, it broke functionality
that ensured that clients were updated on charm-upgrade, this
change enables a bypass of that cache functionality and uses
it to re-process broker requests in the upgrade-charm hook.

Depends-On: https://review.opendev.org/c/openstack/charms.ceph/+/848311
Func-Test-Pr: openstack-charmers/zaza-openstack-tests#1066
Closes-Bug: #1968369
Change-Id: Ibdad1fd5976fdf2d5f3384f1b120b0d5dda34947

src/ceph_client.py

@@ -139,7 +139,7 @@ def _req_already_treated(self, request_id):
         return request_id in self._stored.processed
 
     def _handle_broker_request(
-            self, relation, unit, add_legacy_response=False):
+            self, relation, unit, add_legacy_response=False, force=False):
         """Retrieve broker request from relation, process, return response data.
 
         :param event: Operator event for the relation
@@ -149,6 +149,8 @@ def _handle_broker_request(
                                     new way.
         :type add_legacy_response: bool
         :returns: Dictionary of response data ready for use with relation_set.
+        :param force: Whether to re-process broker requests.
+        :type force: bool
         :rtype: dict
         """
         def _get_broker_req_id(request):
@@ -186,7 +188,7 @@ def _get_broker_req_id(request):
                         broker_req_id))
                 return {}
 
-            if self._req_already_treated(broker_req_id):
+            if self._req_already_treated(broker_req_id) and not force:
                 logger.debug(
                     "Ignoring already executed broker request {}".format(
                         broker_req_id))

src/ceph_hooks.py

@@ -595,10 +595,10 @@ def attempt_mon_cluster_bootstrap():
     return True
 
 
-def notify_relations():
-    notify_osds()
-    notify_radosgws()
-    notify_rbd_mirrors()
+def notify_relations(reprocess_broker_requests=False):
+    notify_osds(reprocess_broker_requests=reprocess_broker_requests)
+    notify_radosgws(reprocess_broker_requests=reprocess_broker_requests)
+    notify_rbd_mirrors(reprocess_broker_requests=reprocess_broker_requests)
     notify_prometheus()
 
 
@@ -614,22 +614,29 @@ def notify_prometheus():
                                 module_enabled=module_enabled)
 
 
-def notify_osds():
+def notify_osds(reprocess_broker_requests=False):
     for relid in relation_ids('osd'):
         for unit in related_units(relid):
-            osd_relation(relid=relid, unit=unit)
+            osd_relation(
+                relid=relid, unit=unit,
+                reprocess_broker_requests=reprocess_broker_requests)
 
 
-def notify_radosgws():
+def notify_radosgws(reprocess_broker_requests=False):
     for relid in relation_ids('radosgw'):
         for unit in related_units(relid):
-            radosgw_relation(relid=relid, unit=unit)
+            radosgw_relation(
+                relid=relid, unit=unit,
+                reprocess_broker_requests=reprocess_broker_requests)
 
 
-def notify_rbd_mirrors():
+def notify_rbd_mirrors(reprocess_broker_requests=False):
     for relid in relation_ids('rbd-mirror'):
         for unit in related_units(relid):
-            rbd_mirror_relation(relid=relid, unit=unit, recurse=False)
+            rbd_mirror_relation(
+                relid=relid, unit=unit,
+                recurse=False,
+                reprocess_broker_requests=reprocess_broker_requests)
 
 
 def req_already_treated(request_id, relid, req_unit):
@@ -738,7 +745,7 @@ def _get_request(relation_data):
 
 
 def handle_broker_request(relid, unit, add_legacy_response=False,
-                          recurse=True):
+                          recurse=True, force=False):
     """Retrieve broker request from relation, process, return response data.
 
     :param relid: Realtion ID
@@ -752,6 +759,9 @@ def handle_broker_request(relid, unit, add_legacy_response=False,
                     not.  Mainly used to handle recursion when called from
                     notify_rbd_mirrors()
     :type recurse: bool
+    :param force: Process broker requests even if they have already been
+                    processed.
+    :type force: bool
     :returns: Dictionary of response data ready for use with relation_set.
     :rtype: dict
     """
@@ -784,7 +794,7 @@ def _get_broker_req_id(request):
                 level=DEBUG)
             return {}
 
-        if req_already_treated(broker_req_id, relid, unit):
+        if req_already_treated(broker_req_id, relid, unit) and not force:
             log("Ignoring already executed broker request {}".format(
                 broker_req_id),
                 level=DEBUG)
@@ -820,7 +830,7 @@ def _get_broker_req_id(request):
 
 @hooks.hook('osd-relation-joined')
 @hooks.hook('osd-relation-changed')
-def osd_relation(relid=None, unit=None):
+def osd_relation(relid=None, unit=None, reprocess_broker_requests=False):
     if ceph.is_quorum():
         log('mon cluster in quorum - providing fsid & keys')
         public_addr = get_public_addr()
@@ -855,7 +865,8 @@ def osd_relation(relid=None, unit=None):
             )
         }
 
-        data.update(handle_broker_request(relid, unit))
+        data.update(handle_broker_request(
+            relid, unit, force=reprocess_broker_requests))
         relation_set(relation_id=relid,
                      relation_settings=data)
 
@@ -968,7 +979,7 @@ def dashboard_relation(relid=None):
 
 @hooks.hook('radosgw-relation-changed')
 @hooks.hook('radosgw-relation-joined')
-def radosgw_relation(relid=None, unit=None):
+def radosgw_relation(relid=None, unit=None, reprocess_broker_requests=False):
     # Install radosgw for admin tools
     apt_install(packages=filter_installed_packages(['radosgw']))
     if not unit:
@@ -997,13 +1008,16 @@ def radosgw_relation(relid=None, unit=None):
             # Old style global radosgw key
             data['radosgw_key'] = ceph.get_radosgw_key()
 
-        data.update(handle_broker_request(relid, unit))
+        data.update(handle_broker_request(
+            relid, unit, force=reprocess_broker_requests))
         relation_set(relation_id=relid, relation_settings=data)
 
 
 @hooks.hook('rbd-mirror-relation-joined')
 @hooks.hook('rbd-mirror-relation-changed')
-def rbd_mirror_relation(relid=None, unit=None, recurse=True):
+def rbd_mirror_relation(
+        relid=None, unit=None, recurse=True,
+        reprocess_broker_requests=False):
     '''
     Handle the rbd mirror relation
 
@@ -1029,7 +1043,8 @@ def get_pool_details():
             return ceph.list_pools_detail()
 
         # handle broker requests first to get a updated pool map
-        data = (handle_broker_request(relid, unit, recurse=recurse))
+        data = (handle_broker_request(
+            relid, unit, recurse=recurse, force=reprocess_broker_requests))
         data.update({
             'auth': 'cephx',
             'ceph-public-address': get_public_addr(),
@@ -1061,7 +1076,8 @@ def get_pool_details():
 
 @hooks.hook('mds-relation-changed')
 @hooks.hook('mds-relation-joined')
-def mds_relation_joined(relid=None, unit=None):
+def mds_relation_joined(
+        relid=None, unit=None, reprocess_broker_requests=False):
     if ready_for_service():
         log('mon cluster in quorum and osds bootstrapped '
             '- providing mds client with keys')
@@ -1078,7 +1094,9 @@ def mds_relation_joined(relid=None, unit=None):
                 ceph.get_mds_key(name=mds_name),
             'auth': 'cephx',
             'ceph-public-address': public_addr}
-        data.update(handle_broker_request(relid, unit))
+        data.update(
+            handle_broker_request(
+                relid, unit, force=reprocess_broker_requests))
         relation_set(relation_id=relid, relation_settings=data)
 
 
@@ -1131,7 +1149,7 @@ def upgrade_charm():
     # NOTE(jamespage):
     # Reprocess broker requests to ensure that any cephx
     # key permission changes are applied
-    notify_relations()
+    notify_relations(reprocess_broker_requests=True)
 
 
 @hooks.hook('nrpe-external-master-relation-joined')

tests/tests.yaml

@@ -19,6 +19,7 @@ tests:
     - zaza.openstack.charm_tests.ceph.tests.CephTest
     - zaza.openstack.charm_tests.ceph.osd.tests.SecurityTest
     - zaza.openstack.charm_tests.ceph.tests.CephPrometheusTest
+    - zaza.openstack.charm_tests.ceph.mon.tests.CephPermissionUpgradeTest
   - zaza.openstack.charm_tests.ceph.tests.CheckPoolTypes
   - zaza.openstack.charm_tests.ceph.tests.CephLowLevelTest
   - zaza.openstack.charm_tests.ceph.tests.CephTest
@@ -35,3 +36,4 @@ tests:
   # Tests from quincy.
   - zaza.openstack.charm_tests.ceph.tests.CephAuthTest
   - zaza.openstack.charm_tests.ceph.tests.CephMonActionsTest
+  - zaza.openstack.charm_tests.ceph.mon.tests.CephPermissionUpgradeTest

unit_tests/test_ceph_hooks.py

@@ -240,7 +240,8 @@ def test_upgrade_charm_with_nrpe_relation_installs_dependencies(
             ceph_hooks.upgrade_charm()
         mocks["apt_install"].assert_called_with(
             "lockfile-progs", fatal=True)
-        mock_notify_radosgws.assert_called_once_with()
+        mock_notify_radosgws.assert_called_once_with(
+            reprocess_broker_requests=True)
         mock_ceph.update_monfs.assert_called_once_with()
         mock_notify_prometheus.assert_called_once_with()
         mock_service_pause.assert_called_with('ceph-create-keys')
@@ -255,9 +256,11 @@ def test_notify_rbd_mirrors(self, mock_relation_ids, mock_related_units,
         ceph_hooks.notify_rbd_mirrors()
         mock_relation_ids.assert_called_once_with('rbd-mirror')
         mock_related_units.assert_called_once_with('arelid')
-        mock_rbd_mirror_relation.assert_called_once_with(relid='arelid',
-                                                         unit='aunit',
-                                                         recurse=False)
+        mock_rbd_mirror_relation.assert_called_once_with(
+            relid='arelid',
+            unit='aunit',
+            recurse=False,
+            reprocess_broker_requests=False)
 
     @patch.object(ceph_hooks, 'uuid')
     @patch.object(ceph_hooks, 'relation_set')
@@ -892,7 +895,7 @@ def test_rbd_mirror_relation(self,
         ]
         ceph_hooks.rbd_mirror_relation('rbd-mirror:51', 'ceph-rbd-mirror/0')
         self.handle_broker_request.assert_called_with(
-            'rbd-mirror:51', 'ceph-rbd-mirror/0', recurse=True)
+            'rbd-mirror:51', 'ceph-rbd-mirror/0', recurse=True, force=False)
         self.relation_set.assert_called_with(
             relation_id='rbd-mirror:51',
             relation_settings=base_relation_settings)