Cleanup ring manager storage relation settings

If local unit is no longer leader, clear rings_url on
storage relations to avoid storage units getting
rings from wrong proxy unit.

Also send broker-timestamp to storage units when providing
rings_url so that we have a means of knowing which is the most
recent. Broker timestamp is the same for peer and storage
sync so this enables identifying most recent.

Change-Id: I2c7e9028f345791bad0a736cb89979284b144e33
Closes-Bug: #1765203

hooks/swift_hooks.py

@@ -64,6 +64,7 @@ from lib.swift_utils import (
     timestamps_available,
     assess_status,
     try_initialize_swauth,
+    clear_storage_rings_available,
 )
 
 import charmhelpers.contrib.openstack.utils as openstack
@@ -251,6 +252,8 @@ def storage_joined(rid=None):
         # possibility of storage nodes getting out-of-date rings by deprecating
         # any existing ones from the www dir.
         mark_www_rings_deleted()
+        clear_storage_rings_available()
+
     try_initialize_swauth()
 
     # NOTE(jamespage): Ensure private-address is set to any network-space

lib/swift_utils.py

@@ -261,12 +261,16 @@ class SwiftProxyClusterRPC(object):
         rq['peers-only'] = echo_peers_only
         return rq
 
-    def sync_rings_request(self, broker_token, builders_only=False):
+    def sync_rings_request(self, broker_token, broker_timestamp,
+                           builders_only=False):
         """Request for peers to sync rings from leader.
 
         NOTE: this action must only be performed by the cluster leader.
 
         :param broker_token: token to identify sync request.
+        @param broker_timestamp: timestamp for peer and storage sync - this
+                                 MUST bethe same as the one used for storage
+                                 sync.
         :param builders_only: if False, tell peers to sync builders only (not
                               rings).
         """
@@ -281,7 +285,7 @@ class SwiftProxyClusterRPC(object):
             rq['sync-only-builders'] = 1
 
         rq['broker-token'] = broker_token
-        rq['broker-timestamp'] = "{:f}".format(time.time())
+        rq['broker-timestamp'] = broker_timestamp
         rq['builder-broker'] = self._hostname
         return rq
 
@@ -893,11 +897,15 @@ def mark_www_rings_deleted():
             os.rename(path, "{}.deleted".format(path))
 
 
-def notify_peers_builders_available(broker_token, builders_only=False):
+def notify_peers_builders_available(broker_token, broker_timestamp,
+                                    builders_only=False):
     """Notify peer swift-proxy units that they should synchronise ring and
     builder files.
 
     Note that this should only be called from the leader unit.
+
+    @param broker_timestamp: timestamp for peer and storage sync - this MUST be
+    the same as the one used for storage sync.
     """
     if not is_elected_leader(SWIFT_HA_RES):
         log("Ring availability peer broadcast requested by non-leader - "
@@ -922,6 +930,7 @@ def notify_peers_builders_available(broker_token, builders_only=False):
     log("Notifying peer(s) that {} are ready for sync."
         .format(_type), level=INFO)
     rq = SwiftProxyClusterRPC().sync_rings_request(broker_token,
+                                                   broker_timestamp,
                                                    builders_only=builders_only)
     for rid in cluster_rids:
         log("Notifying rid={} ({})".format(rid, rq), level=DEBUG)
@@ -935,16 +944,21 @@ def broadcast_rings_available(storage=True, builders_only=False,
 
     We can opt to only notify peer or storage relations if needs be.
     """
+
+    # NOTE: this MUST be the same for peer and storage sync
+    broker_timestamp = "{:f}".format(time.time())
+
     if storage:
         # TODO: get ack from storage units that they are synced before
         # syncing proxies.
-        notify_storage_rings_available()
+        notify_storage_rings_available(broker_timestamp)
     else:
         log("Skipping notify storage relations", level=DEBUG)
 
     # Always set peer info even if not clustered so that info is present when
     # units join
     notify_peers_builders_available(broker_token,
+                                    broker_timestamp,
                                     builders_only=builders_only)
 
 
@@ -987,11 +1001,14 @@ def cluster_sync_rings(peers_only=False, builders_only=False, token=None):
         relation_set(relation_id=rid, relation_settings=rq)
 
 
-def notify_storage_rings_available():
+def notify_storage_rings_available(broker_timestamp):
     """Notify peer swift-storage relations that they should synchronise ring
     and builder files.
 
     Note that this should only be called from the leader unit.
+
+    @param broker_timestamp: timestamp for peer and storage sync - this MUST be
+                             the same as the one used for peer sync.
     """
     if not is_elected_leader(SWIFT_HA_RES):
         log("Ring availability storage-relation broadcast requested by "
@@ -1002,13 +1019,26 @@ def notify_storage_rings_available():
     hostname = format_ipv6_addr(hostname) or hostname
     path = os.path.basename(get_www_dir())
     rings_url = 'http://{}/{}'.format(hostname, path)
+
+    # TODO(hopem): consider getting rid of this trigger since the timestamp
+    #              should do the job.
     trigger = uuid.uuid4()
+
     # Notify storage nodes that there is a new ring to fetch.
     log("Notifying storage nodes that new rings are ready for sync.",
         level=INFO)
     for relid in relation_ids('swift-storage'):
         relation_set(relation_id=relid, swift_hash=get_swift_hash(),
-                     rings_url=rings_url, trigger=trigger)
+                     rings_url=rings_url, broker_timestamp=broker_timestamp,
+                     trigger=trigger)
+
+
+def clear_storage_rings_available():
+    """Clear the rings_url setting so that storage units don't accidentally try
+    to sync from this unit (which is presumably no longer the leader).
+    """
+    for relid in relation_ids('swift-storage'):
+        relation_set(relation_id=relid, rings_url=None)
 
 
 def fully_synced():

unit_tests/test_swift_hooks.py

@@ -239,6 +239,7 @@ class SwiftHooksTestCase(unittest.TestCase):
         relation_set.assert_called_once_with(
             relation_id='rid:23', rel_data='data')
 
+    @patch.object(swift_hooks, 'clear_storage_rings_available')
     @patch.object(swift_hooks, 'is_elected_leader')
     @patch.object(swift_hooks, 'get_relation_ip')
     @patch.object(swift_hooks, 'try_initialize_swauth')
@@ -249,7 +250,8 @@ class SwiftHooksTestCase(unittest.TestCase):
                                   mark_www_rings_deleted,
                                   try_initialize_swauth,
                                   get_relation_ip,
-                                  is_elected_leader):
+                                  is_elected_leader,
+                                  mock_clear_storage_rings_available):
         is_elected_leader.return_value = False
         get_relation_ip.return_value = '10.10.20.243'
         swift_hooks.storage_joined(rid='swift-storage:23')
@@ -259,3 +261,4 @@ class SwiftHooksTestCase(unittest.TestCase):
             relation_settings={'private-address': '10.10.20.243'}
         )
         try_initialize_swauth.assert_called_once()
+        mock_clear_storage_rings_available.assert_called_once()

unit_tests/test_swift_utils.py

@@ -311,13 +311,11 @@ class SwiftUtilsTestCase(unittest.TestCase):
 
     @mock.patch('lib.swift_utils.is_elected_leader', lambda arg: True)
     @mock.patch.object(swift_utils, 'get_hostaddr', lambda *args: '1.2.3.4')
-    @mock.patch.object(swift_utils, 'time')
     @mock.patch('lib.swift_utils.uuid')
-    def test_cluster_rpc_sync_request(self, mock_uuid, mock_time):
+    def test_cluster_rpc_sync_request(self, mock_uuid):
-        mock_time.time = mock.Mock(return_value=float(1.234))
         mock_uuid.uuid4.return_value = 'token2'
         rpc = swift_utils.SwiftProxyClusterRPC()
-        rq = rpc.sync_rings_request('token1')
+        rq = rpc.sync_rings_request('token1', '1.234000')
         self.assertEqual({'trigger': 'token2',
                           'broker-token': 'token1',
                           'broker-timestamp': '1.234000',
@@ -512,3 +510,45 @@ class SwiftUtilsTestCase(unittest.TestCase):
                                        'http://localhost:8080/auth',
                                        '-K',
                                        'Test'])
+
+    @mock.patch.object(swift_utils.uuid, 'uuid4')
+    @mock.patch.object(swift_utils, 'relation_set')
+    @mock.patch.object(swift_utils, 'get_swift_hash')
+    @mock.patch.object(swift_utils, 'log')
+    @mock.patch.object(swift_utils, 'relation_ids')
+    @mock.patch.object(swift_utils, 'get_www_dir')
+    @mock.patch.object(swift_utils, 'format_ipv6_addr')
+    @mock.patch.object(swift_utils, 'get_hostaddr')
+    @mock.patch.object(swift_utils, 'is_elected_leader')
+    def test_notify_storage_rings_available(self, mock_is_leader,
+                                            mock_get_hostaddr,
+                                            mock_format_ipv6_addr,
+                                            mock_get_www_dir,
+                                            mock_relation_ids,
+                                            mock_log,
+                                            mock_get_swift_hash,
+                                            mock_relation_set,
+                                            mock_uuid):
+        mock_is_leader.return_value = True
+        mock_get_hostaddr.return_value = '10.0.0.1'
+        mock_format_ipv6_addr.return_value = None
+        mock_get_www_dir.return_value = 'some/dir'
+        mock_relation_ids.return_value = ['storage:0']
+        mock_get_swift_hash.return_value = 'greathash'
+        mock_uuid.return_value = 'uuid-1234'
+        swift_utils.notify_storage_rings_available('1.234')
+        mock_relation_set.assert_called_once_with(
+            broker_timestamp='1.234',
+            relation_id='storage:0',
+            rings_url='http://10.0.0.1/dir',
+            swift_hash='greathash',
+            trigger='uuid-1234')
+
+    @mock.patch.object(swift_utils, 'relation_set')
+    @mock.patch.object(swift_utils, 'relation_ids')
+    def test_clear_notify_storage_rings_available(self, mock_relation_ids,
+                                                  mock_relation_set):
+        mock_relation_ids.return_value = ['storage:0']
+        swift_utils.clear_storage_rings_available()
+        mock_relation_set.assert_called_once_with(
+            relation_id='storage:0', rings_url=None)