Merge lp:~jjo/charms/trusty/swift-proxy/fix-multiple-devices-per-node_lp1479938 into lp:~openstack-charmers-archive/charms/trusty/swift-proxy/next

[JuanJo Ciarlante]

[jjo, r=] also consider blockdev in exists_in_ring(), fixes lp#1479938

[JuanJo Ciarlante (jjo)]

Sorry for not providing tests also, please use is as a reference
for a possible lp#1479938 fix.

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7295 swift-proxy-next for jjo mp266462
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/7295/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6749 swift-proxy-next for jjo mp266462
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6749/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7296 swift-proxy-next for jjo mp266462
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/7296/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6750 swift-proxy-next for jjo mp266462
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6750/

[lp:~jjo/charms/trusty/swift-proxy/fix-multiple-devices-per-node_lp1479938]

103. By JuanJo Ciarlante on 2015-07-30

add unittest: test_update_rings_multiple_devs()

[JuanJo Ciarlante (jjo)]

FYI, added unittests at r103.

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7297 swift-proxy-next for jjo mp266462
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/7297/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6751 swift-proxy-next for jjo mp266462
    UNIT FAIL: unit-test failed

UNIT Results (max last 2 lines):
make: *** [unit_test] Error 1
ERROR:root:Make target returned non-zero.

Full unit test output: http://paste.ubuntu.com/11969573/
Build: http://10.245.162.77:8080/job/charm_unit_test/6751/

[lp:~jjo/charms/trusty/swift-proxy/fix-multiple-devices-per-node_lp1479938]

104. By JuanJo Ciarlante on 2015-07-30

improve test_update_rings_multiple_devs() by comparing added devices with requested ones

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7298 swift-proxy-next for jjo mp266462
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/7298/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6752 swift-proxy-next for jjo mp266462
    UNIT FAIL: unit-test failed

UNIT Results (max last 2 lines):
make: *** [unit_test] Error 1
ERROR:root:Make target returned non-zero.

Full unit test output: http://paste.ubuntu.com/11969743/
Build: http://10.245.162.77:8080/job/charm_unit_test/6752/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #5509 swift-proxy-next for jjo mp266462
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/5509/

[lp:~jjo/charms/trusty/swift-proxy/fix-multiple-devices-per-node_lp1479938]

105. By JuanJo Ciarlante on 2015-07-30

add a basic-enough mock_ring class, to avoid the need for swift.common.ring library

[JuanJo Ciarlante (jjo)]

r105 should now pass both unittest, and amulet ones (already ok for the latter).

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7299 swift-proxy-next for jjo mp266462
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/7299/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6753 swift-proxy-next for jjo mp266462
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6753/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #5533 swift-proxy-next for jjo mp266462
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/5533/

[David Ames (thedac)]

Juan Jo, I have superseded this MP with https://code.launchpad.net/~thedac/charms/trusty/swift-proxy/multiple-device-per-node/+merge/274192

I wanted to see a slightly different approach and it did not make sense to ask you to do it. I have merged your MP into mine so it is included for historical sake.

=== modified file 'hooks/swift_utils.py'
--- hooks/swift_utils.py	2015-03-26 18:37:57 +0000
+++ hooks/swift_utils.py	2015-07-30 23:13:39 +0000
@@ -398,13 +398,18 @@
     _write_ring(ring, path)
 
 
-def exists_in_ring(ring_path, node):
+def exists_in_ring(ring_path, node, blockdev):
     ring = _load_builder(ring_path).to_dict()
     node['port'] = ring_port(ring_path, node)
+    # Make a nodecopy dict, to add a 'device' key for the purpose
+    # of below comparison, as it doesn't have a single-valued 'device'
+    # but a list as e.g. 'devices': ['sdb', 'sdc', ...]
+    nodecopy = {k: v for k, v in node.items()}
+    nodecopy['device'] = blockdev
 
     for dev in ring['devs']:
-        d = [(i, dev[i]) for i in dev if i in node and i != 'zone']
-        n = [(i, node[i]) for i in node if i in dev and i != 'zone']
+        d = [(i, dev[i]) for i in dev if i in nodecopy and i != 'zone']
+        n = [(i, nodecopy[i]) for i in nodecopy if i in dev and i != 'zone']
         if sorted(d) == sorted(n):
 
             log('Node already exists in ring (%s).' % ring_path, level=INFO)
@@ -812,10 +817,10 @@
                         balance_required = True
 
     if node_settings:
-        for dev in node_settings.get('devices', []):
+        for blockdev in node_settings.get('devices', []):
             for ring in SWIFT_RINGS.itervalues():
-                if not exists_in_ring(ring, node_settings):
-                    add_to_ring(ring, node_settings, dev)
+                if not exists_in_ring(ring, node_settings, blockdev):
+                    add_to_ring(ring, node_settings, blockdev)
                     balance_required = True
 
     if balance_required:

=== modified file 'unit_tests/test_swift_utils.py'
--- unit_tests/test_swift_utils.py	2014-12-18 11:58:28 +0000
+++ unit_tests/test_swift_utils.py	2015-07-30 23:13:39 +0000
@@ -76,6 +76,77 @@
         self.assertTrue(mock_set_min_hours.called)
         self.assertTrue(mock_balance_rings.called)
 
+    @mock.patch('swift_utils._load_builder')
+    @mock.patch('swift_utils.initialize_ring')
+    @mock.patch('swift_utils.get_broker_token')
+    @mock.patch('swift_utils.update_www_rings')
+    @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.is_elected_leader')
+    def test_update_rings_multiple_devs(self,
+                                        mock_is_elected_leader,
+                                        mock_log, mock_balance_rings,
+                                        mock_get_rings_checksum,
+                                        mock_get_builders_checksum,
+                                        mock_update_www_rings,
+                                        mock_get_broker_token,
+                                        mock_initialize_ring,
+                                        mock_load_builder,
+                                        ):
+        # To avoid the need for swift.common.ring library, mock a basic
+        # rings dictionary, keyed by path.
+        # Each ring has enough logic to hold a dictionary with a single 'devs'
+        # key, which stores the list of passed dev(s) by add_dev().
+        #
+        # If swift (actual) ring representation diverges (see _load_builder),
+        # this mock will need to be adapted.
+        mock_rings = {}
+
+        def mock_load_builder_fn(path):
+            class mock_ring(object):
+                def __init__(self, path):
+                    self.path = path
+
+                def to_dict(self):
+                    return mock_rings[self.path]
+
+                def add_dev(self, dev):
+                    mock_rings[self.path]['devs'].append(dev)
+            return mock_ring(path)
+
+        def mock_initialize_ring_fn(path, *args):
+            mock_rings.setdefault(path, {'devs': []})
+
+        mock_load_builder.side_effect = mock_load_builder_fn
+        mock_initialize_ring.side_effect = mock_initialize_ring_fn
+
+        init_ring_paths(tempfile.mkdtemp())
+        devices = ['sdb', 'sdc']
+        node_settings = {
+            'object_port': 6000,
+            'container_port': 6001,
+            'account_port': 6002,
+            'zone': 1,
+            'ip': '1.2.3.4',
+            'devices': devices
+        }
+        for path in swift_utils.SWIFT_RINGS.itervalues():
+            swift_utils.initialize_ring(path, 8, 3, 0)
+
+        # verify all devices added to each ring
+        swift_utils.update_rings(node_settings)
+        for path in swift_utils.SWIFT_RINGS.itervalues():
+            devs = swift_utils._load_builder(path).to_dict()['devs']
+            added_devices = [dev['device'] for dev in devs]
+            self.assertEqual(devices, added_devices)
+
+        # try re-adding, assert add_to_ring was not called
+        with mock.patch('swift_utils.add_to_ring') as mock_add_to_ring:
+            swift_utils.update_rings(node_settings)
+            self.assertFalse(mock_add_to_ring.called)
+
     @mock.patch('swift_utils.get_broker_token')
     @mock.patch('swift_utils.balance_rings')
     @mock.patch('swift_utils.log')