applied post-review changes

hooks/swift_hooks.py

@@ -21,7 +21,6 @@ from swift_utils import (
     balance_rings,
     fully_synced,
     sync_proxy_rings,
-    update_min_part_hours,
     broadcast_rings_available,
     mark_www_rings_deleted,
     SwiftProxyClusterRPC,
@@ -126,7 +125,7 @@ def config_changed():
     if openstack.openstack_upgrade_available('python-swift'):
         do_openstack_upgrade(CONFIGS)
 
-    update_min_part_hours()
+    update_rings(min_part_hours=config('min-hours'))
 
     if not config('disable-ring-balance') and is_elected_leader(SWIFT_HA_RES):
         # Try ring balance. If rings are balanced, no sync will occur.

hooks/swift_utils.py

@@ -455,7 +455,7 @@ def set_min_part_hours(path, value):
 
 
 def get_zone(assignment_policy):
-    """Determine the appropriate zone depending on configured assignment policy.
+    """Determine appropriate zone based on configured assignment policy.
 
     Manual assignment relies on each storage zone being deployed as a
     separate service unit with its desired zone set as a configuration
@@ -717,18 +717,46 @@ def sync_builders_and_rings_if_changed(f):
 
 
 @sync_builders_and_rings_if_changed
-def update_rings(node_settings=None):
+def update_rings(node_settings=None, min_part_hours=None):
-    """Update builder with node settings and balance rings if necessary."""
+    """Update builder with node settings and balance rings if necessary.
+
+    Also update min_part_hours if provided.
+    """
     if not is_elected_leader(SWIFT_HA_RES):
         log("Update rings called by non-leader - skipping", level=WARNING)
         return
 
+    balance_required = False
+
+    if min_part_hours:
+        # NOTE: no need to stop the proxy since we are not changing the rings,
+        # only the builder.
+
+        # Only update if all exist
+        if all([os.path.exists(p) for p in SWIFT_RINGS.itervalues()]):
+            for ring, path in SWIFT_RINGS.iteritems():
+                current_min_part_hours = get_min_part_hours(path)
+                if min_part_hours != current_min_part_hours:
+                    log("Setting ring %s min_part_hours to %s" %
+                        (ring, min_part_hours), level=INFO)
+                    try:
+                        set_min_part_hours(path, min_part_hours)
+                    except SwiftProxyCharmException as exc:
+                        # TODO: ignore for now since this should not be
+                        # critical but in the future we should support a
+                        # rollback.
+                        log(str(exc), level=WARNING)
+                    else:
+                        balance_required = True
+
     if node_settings:
         if 'device' in node_settings:
             for ring in SWIFT_RINGS.itervalues():
                 if not exists_in_ring(ring, node_settings):
                     add_to_ring(ring, node_settings)
+                    balance_required = True
 
+    if balance_required:
         balance_rings()
 
 
@@ -852,8 +880,8 @@ def cluster_sync_rings(peers_only=False, builders_only=False):
 
 
 def notify_storage_rings_available():
-    """Notify peer swift-storage relations that they should synchronise ring and
+    """Notify peer swift-storage relations that they should synchronise ring
-    builder files.
+    and builder files.
 
     Note that this should only be called from the leader unit.
     """
@@ -905,39 +933,3 @@ def get_hostaddr():
         return get_ipv6_addr(exc_list=[config('vip')])[0]
 
     return unit_get('private-address')
-
-
-def update_min_part_hours():
-    """Update the min_part_hours setting on swift rings.
-
-    This should only be called by the leader unit. Once update has been
-    performed and if setting has changed, rings will be resynced across the
-    cluster.
-    """
-    if not is_elected_leader(SWIFT_HA_RES):
-        # Only the leader can do this.
-        return
-
-    # NOTE: no need to stop the proxy since we are not changing the rings,
-    # only the builder.
-
-    new_min_part_hours = config('min-hours')
-    resync_builders = False
-    # Only update if all exist
-    if all([os.path.exists(p) for p in SWIFT_RINGS.itervalues()]):
-        for ring, path in SWIFT_RINGS.iteritems():
-            min_part_hours = get_min_part_hours(path)
-            if min_part_hours != new_min_part_hours:
-                log("Setting ring %s min_part_hours to %s" %
-                    (ring, new_min_part_hours), level=INFO)
-                try:
-                    set_min_part_hours(path, new_min_part_hours)
-                except SwiftProxyCharmException as exc:
-                    # TODO: ignore for now since this should not be critical
-                    # but in the future we should support a rollback.
-                    log(str(exc), level=WARNING)
-                else:
-                    resync_builders = True
-
-    if resync_builders:
-        balance_rings()

unit_tests/test_swift_utils.py

@@ -21,51 +21,53 @@ def init_ring_paths(tmpdir):
 
 class SwiftUtilsTestCase(unittest.TestCase):
 
-    @mock.patch('swift_utils.sync_builders_and_rings_if_changed')
+    @mock.patch('swift_utils.get_builders_checksum')
+    @mock.patch('swift_utils.get_rings_checksum')
     @mock.patch('swift_utils.balance_rings')
     @mock.patch('swift_utils.log')
     @mock.patch('swift_utils.os.path.exists')
     @mock.patch('swift_utils.is_elected_leader')
-    @mock.patch('swift_utils.config')
     @mock.patch('swift_utils.get_min_part_hours')
     @mock.patch('swift_utils.set_min_part_hours')
-    def test_update_min_part_hours(self, mock_set_min_hours,
+    def test_update_rings(self, mock_set_min_hours,
-                                   mock_get_min_hours, mock_config,
+                          mock_get_min_hours,
                           mock_is_elected_leader, mock_path_exists,
                           mock_log, mock_balance_rings,
-                                   mock_sync_builders_and_rings_if_changed):
+                          mock_get_rings_checksum,
+                          mock_get_builders_checksum):
+
+        # Make sure same is returned for both so that we don't try to sync
+        mock_get_rings_checksum.return_value = None
+        mock_get_builders_checksum.return_value = None
 
         # Test blocker 1
         mock_is_elected_leader.return_value = False
-        swift_utils.update_min_part_hours()
+        swift_utils.update_rings()
-        self.assertFalse(mock_config.called)
         self.assertFalse(mock_balance_rings.called)
 
         # Test blocker 2
         mock_path_exists.return_value = False
         mock_is_elected_leader.return_value = True
-        swift_utils.update_min_part_hours()
+        swift_utils.update_rings()
-        self.assertTrue(mock_config.called)
         self.assertFalse(mock_get_min_hours.called)
         self.assertFalse(mock_balance_rings.called)
 
         # Test blocker 3
         mock_path_exists.return_value = True
         mock_is_elected_leader.return_value = True
-        mock_config.return_value = 10
         mock_get_min_hours.return_value = 10
-        swift_utils.update_min_part_hours()
+        swift_utils.update_rings(min_part_hours=10)
         self.assertTrue(mock_get_min_hours.called)
         self.assertFalse(mock_set_min_hours.called)
         self.assertFalse(mock_balance_rings.called)
 
+        mock_get_min_hours.reset_mock()
+
         # Test go through
         mock_path_exists.return_value = True
         mock_is_elected_leader.return_value = True
-        mock_config.return_value = 10
         mock_get_min_hours.return_value = 0
-        swift_utils.update_min_part_hours()
+        swift_utils.update_rings(min_part_hours=10)
-        self.assertTrue(mock_config.called)
         self.assertTrue(mock_get_min_hours.called)
         self.assertTrue(mock_set_min_hours.called)
         self.assertTrue(mock_balance_rings.called)