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

Proposed by JuanJo Ciarlante on 2015-07-30
Status: Merged
Merged at revision: 120
Proposed branch: lp:~jjo/charms/trusty/swift-proxy/fix-multiple-devices-per-node_lp1479938
Merge into: lp:~openstack-charmers-archive/charms/trusty/swift-proxy/next
Diff against target: 121 lines (+82/-6)
2 files modified
hooks/swift_utils.py (+11/-6)
unit_tests/test_swift_utils.py (+71/-0)
To merge this branch: bzr merge lp:~jjo/charms/trusty/swift-proxy/fix-multiple-devices-per-node_lp1479938
Reviewer Review Type Date Requested Status
OpenStack Charmers 2015-07-30 Pending
Review via email: mp+266462@code.launchpad.net

Commit message

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

To post a comment you must log in.
JuanJo Ciarlante (jjo) wrote :

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

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/

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/

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/

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/

103. By JuanJo Ciarlante on 2015-07-30

add unittest: test_update_rings_multiple_devs()

JuanJo Ciarlante (jjo) wrote :

FYI, added unittests at r103.

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/

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/

104. By JuanJo Ciarlante on 2015-07-30

improve test_update_rings_multiple_devs() by comparing added devices with requested ones

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/

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/

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/

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) wrote :

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

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/

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/

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/swift_utils.py'
2--- hooks/swift_utils.py 2015-03-26 18:37:57 +0000
3+++ hooks/swift_utils.py 2015-07-30 23:13:39 +0000
4@@ -398,13 +398,18 @@
5 _write_ring(ring, path)
6
7
8-def exists_in_ring(ring_path, node):
9+def exists_in_ring(ring_path, node, blockdev):
10 ring = _load_builder(ring_path).to_dict()
11 node['port'] = ring_port(ring_path, node)
12+ # Make a nodecopy dict, to add a 'device' key for the purpose
13+ # of below comparison, as it doesn't have a single-valued 'device'
14+ # but a list as e.g. 'devices': ['sdb', 'sdc', ...]
15+ nodecopy = {k: v for k, v in node.items()}
16+ nodecopy['device'] = blockdev
17
18 for dev in ring['devs']:
19- d = [(i, dev[i]) for i in dev if i in node and i != 'zone']
20- n = [(i, node[i]) for i in node if i in dev and i != 'zone']
21+ d = [(i, dev[i]) for i in dev if i in nodecopy and i != 'zone']
22+ n = [(i, nodecopy[i]) for i in nodecopy if i in dev and i != 'zone']
23 if sorted(d) == sorted(n):
24
25 log('Node already exists in ring (%s).' % ring_path, level=INFO)
26@@ -812,10 +817,10 @@
27 balance_required = True
28
29 if node_settings:
30- for dev in node_settings.get('devices', []):
31+ for blockdev in node_settings.get('devices', []):
32 for ring in SWIFT_RINGS.itervalues():
33- if not exists_in_ring(ring, node_settings):
34- add_to_ring(ring, node_settings, dev)
35+ if not exists_in_ring(ring, node_settings, blockdev):
36+ add_to_ring(ring, node_settings, blockdev)
37 balance_required = True
38
39 if balance_required:
40
41=== modified file 'unit_tests/test_swift_utils.py'
42--- unit_tests/test_swift_utils.py 2014-12-18 11:58:28 +0000
43+++ unit_tests/test_swift_utils.py 2015-07-30 23:13:39 +0000
44@@ -76,6 +76,77 @@
45 self.assertTrue(mock_set_min_hours.called)
46 self.assertTrue(mock_balance_rings.called)
47
48+ @mock.patch('swift_utils._load_builder')
49+ @mock.patch('swift_utils.initialize_ring')
50+ @mock.patch('swift_utils.get_broker_token')
51+ @mock.patch('swift_utils.update_www_rings')
52+ @mock.patch('swift_utils.get_builders_checksum')
53+ @mock.patch('swift_utils.get_rings_checksum')
54+ @mock.patch('swift_utils.balance_rings')
55+ @mock.patch('swift_utils.log')
56+ @mock.patch('swift_utils.is_elected_leader')
57+ def test_update_rings_multiple_devs(self,
58+ mock_is_elected_leader,
59+ mock_log, mock_balance_rings,
60+ mock_get_rings_checksum,
61+ mock_get_builders_checksum,
62+ mock_update_www_rings,
63+ mock_get_broker_token,
64+ mock_initialize_ring,
65+ mock_load_builder,
66+ ):
67+ # To avoid the need for swift.common.ring library, mock a basic
68+ # rings dictionary, keyed by path.
69+ # Each ring has enough logic to hold a dictionary with a single 'devs'
70+ # key, which stores the list of passed dev(s) by add_dev().
71+ #
72+ # If swift (actual) ring representation diverges (see _load_builder),
73+ # this mock will need to be adapted.
74+ mock_rings = {}
75+
76+ def mock_load_builder_fn(path):
77+ class mock_ring(object):
78+ def __init__(self, path):
79+ self.path = path
80+
81+ def to_dict(self):
82+ return mock_rings[self.path]
83+
84+ def add_dev(self, dev):
85+ mock_rings[self.path]['devs'].append(dev)
86+ return mock_ring(path)
87+
88+ def mock_initialize_ring_fn(path, *args):
89+ mock_rings.setdefault(path, {'devs': []})
90+
91+ mock_load_builder.side_effect = mock_load_builder_fn
92+ mock_initialize_ring.side_effect = mock_initialize_ring_fn
93+
94+ init_ring_paths(tempfile.mkdtemp())
95+ devices = ['sdb', 'sdc']
96+ node_settings = {
97+ 'object_port': 6000,
98+ 'container_port': 6001,
99+ 'account_port': 6002,
100+ 'zone': 1,
101+ 'ip': '1.2.3.4',
102+ 'devices': devices
103+ }
104+ for path in swift_utils.SWIFT_RINGS.itervalues():
105+ swift_utils.initialize_ring(path, 8, 3, 0)
106+
107+ # verify all devices added to each ring
108+ swift_utils.update_rings(node_settings)
109+ for path in swift_utils.SWIFT_RINGS.itervalues():
110+ devs = swift_utils._load_builder(path).to_dict()['devs']
111+ added_devices = [dev['device'] for dev in devs]
112+ self.assertEqual(devices, added_devices)
113+
114+ # try re-adding, assert add_to_ring was not called
115+ with mock.patch('swift_utils.add_to_ring') as mock_add_to_ring:
116+ swift_utils.update_rings(node_settings)
117+ self.assertFalse(mock_add_to_ring.called)
118+
119 @mock.patch('swift_utils.get_broker_token')
120 @mock.patch('swift_utils.balance_rings')
121 @mock.patch('swift_utils.log')

Subscribers

People subscribed via source and target branches