Merge "Add ring-builder analyzer."

bin/swift-ring-builder-analyzer

@@ -0,0 +1,22 @@
+#!/usr/bin/python
+# Copyright (c) 2015 Samuel Merritt <sam@swiftstack.com>
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import sys
+from swift.cli.ring_builder_analyzer import main
+
+
+if __name__ == "__main__":
+    sys.exit(main())

doc/source/overview_ring.rst

@@ -237,6 +237,12 @@ when the balance doesn't improve by at least 1% (indicating we probably can't
 get perfect balance due to wildly imbalanced zones or too many partitions
 recently moved).
 
+---------------------
+Ring Builder Analyzer
+---------------------
+.. automodule:: swift.cli.ring_builder_analyzer
+
+
 -------
 History
 -------

setup.cfg

@@ -60,6 +60,7 @@ scripts =
     bin/swift-recon
     bin/swift-recon-cron
     bin/swift-ring-builder
+    bin/swift-ring-builder-analyzer
     bin/swift-temp-url
 
 [entry_points]

swift/cli/ring_builder_analyzer.py

@@ -0,0 +1,325 @@
+#! /usr/bin/env python
+# Copyright (c) 2015 Samuel Merritt <sam@swiftstack.com>
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""
+This is a tool for analyzing how well the ring builder performs its job
+in a particular scenario. It is intended to help developers quantify any
+improvements or regressions in the ring builder; it is probably not useful
+to others.
+
+The ring builder analyzer takes a scenario file containing some initial
+parameters for a ring builder plus a certain number of rounds. In each
+round, some modifications are made to the builder, e.g. add a device, remove
+a device, change a device's weight. Then, the builder is repeatedly
+rebalanced until it settles down. Data about that round is printed, and the
+next round begins.
+
+Scenarios are specified in JSON. Example scenario for a gradual device
+addition::
+
+    {
+        "part_power": 12,
+        "replicas": 3,
+        "overload": 0.1,
+        "random_seed": 203488,
+
+        "rounds": [
+            [
+                ["add", "r1z2-10.20.30.40:6000/sda", 8000],
+                ["add", "r1z2-10.20.30.40:6000/sdb", 8000],
+                ["add", "r1z2-10.20.30.40:6000/sdc", 8000],
+                ["add", "r1z2-10.20.30.40:6000/sdd", 8000],
+
+                ["add", "r1z2-10.20.30.41:6000/sda", 8000],
+                ["add", "r1z2-10.20.30.41:6000/sdb", 8000],
+                ["add", "r1z2-10.20.30.41:6000/sdc", 8000],
+                ["add", "r1z2-10.20.30.41:6000/sdd", 8000],
+
+                ["add", "r1z2-10.20.30.43:6000/sda", 8000],
+                ["add", "r1z2-10.20.30.43:6000/sdb", 8000],
+                ["add", "r1z2-10.20.30.43:6000/sdc", 8000],
+                ["add", "r1z2-10.20.30.43:6000/sdd", 8000],
+
+                ["add", "r1z2-10.20.30.44:6000/sda", 8000],
+                ["add", "r1z2-10.20.30.44:6000/sdb", 8000],
+                ["add", "r1z2-10.20.30.44:6000/sdc", 8000]
+            ], [
+                ["add", "r1z2-10.20.30.44:6000/sdd", 1000]
+            ], [
+                ["set_weight", 15, 2000]
+            ], [
+                ["remove", 3],
+                ["set_weight", 15, 3000]
+            ], [
+                ["set_weight", 15, 4000]
+            ], [
+                ["set_weight", 15, 5000]
+            ], [
+                ["set_weight", 15, 6000]
+            ], [
+                ["set_weight", 15, 7000]
+            ], [
+                ["set_weight", 15, 8000]
+            ]]
+    }
+
+"""
+
+import argparse
+import json
+import sys
+
+from swift.common.ring import builder
+from swift.common.ring.utils import parse_add_value
+
+
+ARG_PARSER = argparse.ArgumentParser(
+    description='Put the ring builder through its paces')
+ARG_PARSER.add_argument(
+    '--check', '-c', action='store_true',
+    help="Just check the scenario, don't execute it.")
+ARG_PARSER.add_argument(
+    'scenario_path',
+    help="Path to the scenario file")
+
+
+def _parse_weight(round_index, command_index, weight_str):
+    try:
+        weight = float(weight_str)
+    except ValueError as err:
+        raise ValueError(
+            "Invalid weight %r (round %d, command %d): %s"
+            % (weight_str, round_index, command_index, err))
+    if weight < 0:
+        raise ValueError(
+            "Negative weight (round %d, command %d)"
+            % (round_index, command_index))
+    return weight
+
+
+def _parse_add_command(round_index, command_index, command):
+    if len(command) != 3:
+        raise ValueError(
+            "Invalid add command (round %d, command %d): expected array of "
+            "length 3, but got %d"
+            % (round_index, command_index, len(command)))
+
+    dev_str = command[1]
+    weight_str = command[2]
+
+    try:
+        dev = parse_add_value(dev_str)
+    except ValueError as err:
+        raise ValueError(
+            "Invalid device specifier '%s' in add (round %d, command %d): %s"
+            % (dev_str, round_index, command_index, err))
+
+    dev['weight'] = _parse_weight(round_index, command_index, weight_str)
+
+    if dev['region'] is None:
+        dev['region'] = 1
+
+    return ['add', dev]
+
+
+def _parse_remove_command(round_index, command_index, command):
+    if len(command) != 2:
+        raise ValueError(
+            "Invalid remove command (round %d, command %d): expected array of "
+            "length 2, but got %d"
+            % (round_index, command_index, len(command)))
+
+    dev_str = command[1]
+
+    try:
+        dev_id = int(dev_str)
+    except ValueError as err:
+        raise ValueError(
+            "Invalid device ID '%s' in remove (round %d, command %d): %s"
+            % (dev_str, round_index, command_index, err))
+
+    return ['remove', dev_id]
+
+
+def _parse_set_weight_command(round_index, command_index, command):
+    if len(command) != 3:
+        raise ValueError(
+            "Invalid remove command (round %d, command %d): expected array of "
+            "length 3, but got %d"
+            % (round_index, command_index, len(command)))
+
+    dev_str = command[1]
+    weight_str = command[2]
+
+    try:
+        dev_id = int(dev_str)
+    except ValueError as err:
+        raise ValueError(
+            "Invalid device ID '%s' in set_weight (round %d, command %d): %s"
+            % (dev_str, round_index, command_index, err))
+
+    weight = _parse_weight(round_index, command_index, weight_str)
+    return ['set_weight', dev_id, weight]
+
+
+def parse_scenario(scenario_data):
+    """
+    Takes a serialized scenario and turns it into a data structure suitable
+    for feeding to run_scenario().
+
+    :returns: scenario
+    :raises: ValueError on invalid scenario
+    """
+
+    parsed_scenario = {}
+
+    try:
+        raw_scenario = json.loads(scenario_data)
+    except ValueError as err:
+        raise ValueError("Invalid JSON in scenario file: %s" % err)
+
+    if not isinstance(raw_scenario, dict):
+        raise ValueError("Scenario must be a JSON object, not array or string")
+
+    if 'part_power' not in raw_scenario:
+        raise ValueError("part_power missing")
+    try:
+        parsed_scenario['part_power'] = int(raw_scenario['part_power'])
+    except ValueError as err:
+        raise ValueError("part_power not an integer: %s" % err)
+    if not 1 <= parsed_scenario['part_power'] <= 32:
+        raise ValueError("part_power must be between 1 and 32, but was %d"
+                         % raw_scenario['part_power'])
+
+    if 'replicas' not in raw_scenario:
+        raise ValueError("replicas missing")
+    try:
+        parsed_scenario['replicas'] = float(raw_scenario['replicas'])
+    except ValueError as err:
+        raise ValueError("replicas not a float: %s" % err)
+    if parsed_scenario['replicas'] < 1:
+        raise ValueError("replicas must be at least 1, but is %f"
+                         % parsed_scenario['replicas'])
+
+    if 'overload' not in raw_scenario:
+        raise ValueError("overload missing")
+    try:
+        parsed_scenario['overload'] = float(raw_scenario['overload'])
+    except ValueError as err:
+        raise ValueError("overload not a float: %s" % err)
+    if parsed_scenario['overload'] < 0:
+        raise ValueError("overload must be non-negative, but is %f"
+                         % parsed_scenario['overload'])
+
+    if 'random_seed' not in raw_scenario:
+        raise ValueError("random_seed missing")
+    try:
+        parsed_scenario['random_seed'] = int(raw_scenario['random_seed'])
+    except ValueError as err:
+        raise ValueError("replicas not an integer: %s" % err)
+
+    if 'rounds' not in raw_scenario:
+        raise ValueError("rounds missing")
+    if not isinstance(raw_scenario['rounds'], list):
+        raise ValueError("rounds must be an array")
+
+    parser_for_command = {'add': _parse_add_command,
+                          'remove': _parse_remove_command,
+                          'set_weight': _parse_set_weight_command}
+
+    parsed_scenario['rounds'] = []
+    for round_index, raw_round in enumerate(raw_scenario['rounds']):
+        if not isinstance(raw_round, list):
+            raise ValueError("round %d not an array" % round_index)
+
+        parsed_round = []
+        for command_index, command in enumerate(raw_round):
+            if command[0] not in parser_for_command:
+                raise ValueError(
+                    "Unknown command (round %d, command %d): "
+                    "'%s' should be one of %s" %
+                    (round_index, command_index, command[0],
+                     parser_for_command.keys()))
+            parsed_round.append(
+                parser_for_command[command[0]](
+                    round_index, command_index, command))
+        parsed_scenario['rounds'].append(parsed_round)
+    return parsed_scenario
+
+
+def run_scenario(scenario):
+    """
+    Takes a parsed scenario (like from parse_scenario()) and runs it.
+    """
+    seed = scenario['random_seed']
+
+    rb = builder.RingBuilder(scenario['part_power'], scenario['replicas'], 1)
+    rb.set_overload(scenario['overload'])
+    for round_index, commands in enumerate(scenario['rounds']):
+        print "Round %d" % (round_index + 1)
+
+        for command in commands:
+            if command[0] == 'add':
+                rb.add_dev(command[1])
+            elif command[0] == 'remove':
+                rb.remove_dev(command[1])
+            elif command[0] == 'set_weight':
+                rb.set_dev_weight(command[1], command[2])
+            else:
+                raise ValueError("unknown command %r" % (command[0],))
+
+        rebalance_number = 1
+        parts_moved, old_balance = rb.rebalance(seed=seed)
+        rb.pretend_min_part_hours_passed()
+        print "\tRebalance 1: moved %d parts, balance is %.6f" % (
+            parts_moved, old_balance)
+
+        while True:
+            rebalance_number += 1
+            parts_moved, new_balance = rb.rebalance(seed=seed)
+            rb.pretend_min_part_hours_passed()
+            print "\tRebalance %d: moved %d parts, balance is %.6f" % (
+                rebalance_number, parts_moved, new_balance)
+            if parts_moved == 0:
+                break
+            if abs(new_balance - old_balance) < 1 and not (
+                    old_balance == builder.MAX_BALANCE and
+                    new_balance == builder.MAX_BALANCE):
+                break
+            old_balance = new_balance
+
+
+def main(argv=None):
+    args = ARG_PARSER.parse_args(argv)
+
+    try:
+        with open(args.scenario_path) as sfh:
+            scenario_data = sfh.read()
+    except OSError as err:
+        sys.stderr.write("Error opening scenario %s: %s\n" %
+                         (args.scenario_path, err))
+        return 1
+
+    try:
+        scenario = parse_scenario(scenario_data)
+    except ValueError as err:
+        sys.stderr.write("Invalid scenario %s: %s\n" %
+                         (args.scenario_path, err))
+        return 1
+
+    if not args.check:
+        run_scenario(scenario)
+    return 0

swift/cli/ringbuilder.py

@@ -34,7 +34,7 @@ from swift.common.ring.utils import validate_args, \
     validate_and_normalize_ip, build_dev_from_opts, \
     parse_builder_ring_filename_args, parse_search_value, \
     parse_search_values_from_opts, parse_change_values_from_opts, \
-    dispersion_report, validate_device_name
+    dispersion_report, parse_add_value
 from swift.common.utils import lock_parent_directory
 
 MAJOR_VERSION = 1
@@ -129,37 +129,6 @@ def _parse_list_parts_values(argvish):
         exit(EXIT_ERROR)
 
 
-def _parse_address(rest):
-    if rest.startswith('['):
-        # remove first [] for ip
-        rest = rest.replace('[', '', 1).replace(']', '', 1)
-
-    pos = 0
-    while (pos < len(rest) and
-           not (rest[pos] == 'R' or rest[pos] == '/')):
-        pos += 1
-    address = rest[:pos]
-    rest = rest[pos:]
-
-    port_start = address.rfind(':')
-    if port_start == -1:
-        raise ValueError('Invalid port in add value')
-
-    ip = address[:port_start]
-    try:
-        port = int(address[(port_start + 1):])
-    except (TypeError, ValueError):
-        raise ValueError(
-            'Invalid port %s in add value' % address[port_start:])
-
-    # if this is an ipv6 address then we want to convert it
-    # to all lowercase and use its fully expanded representation
-    # to make searches easier
-    ip = validate_and_normalize_ip(ip)
-
-    return (ip, port, rest)
-
-
 def _parse_add_values(argvish):
     """
     Parse devices to add as specified on the command line.
@@ -183,62 +152,25 @@ def _parse_add_values(argvish):
                                 islice(args, 1, len(args), 2))
 
         for devstr, weightstr in devs_and_weights:
-            region = 1
+            dev_dict = parse_add_value(devstr)
-            rest = devstr
+
-            if devstr.startswith('r'):
+            if dev_dict['region'] is None:
-                i = 1
-                while i < len(devstr) and devstr[i].isdigit():
-                    i += 1
-                region = int(devstr[1:i])
-                rest = devstr[i:]
-            else:
                 stderr.write('WARNING: No region specified for %s. '
                              'Defaulting to region 1.\n' % devstr)
+                dev_dict['region'] = 1
 
-            if not rest.startswith('z'):
+            if dev_dict['replication_ip'] is None:
-                raise ValueError('Invalid add value: %s' % devstr)
+                dev_dict['replication_ip'] = dev_dict['ip']
-            i = 1
-            while i < len(rest) and rest[i].isdigit():
-                i += 1
-            zone = int(rest[1:i])
-            rest = rest[i:]
 
-            if not rest.startswith('-'):
+            if dev_dict['replication_port'] is None:
-                raise ValueError('Invalid add value: %s' % devstr)
+                dev_dict['replication_port'] = dev_dict['port']
-
-            ip, port, rest = _parse_address(rest[1:])
-
-            replication_ip = ip
-            replication_port = port
-            if rest.startswith('R'):
-                replication_ip, replication_port, rest =  \
-                    _parse_address(rest[1:])
-            if not rest.startswith('/'):
-                raise ValueError(
-                    'Invalid add value: %s' % devstr)
-            i = 1
-            while i < len(rest) and rest[i] != '_':
-                i += 1
-            device_name = rest[1:i]
-            if not validate_device_name(device_name):
-                raise ValueError('Invalid device name')
-
-            rest = rest[i:]
-
-            meta = ''
-            if rest.startswith('_'):
-                meta = rest[1:]
 
             weight = float(weightstr)
-
             if weight < 0:
                 raise ValueError('Invalid weight value: %s' % devstr)
+            dev_dict['weight'] = weight
 
-            parsed_devs.append({'region': region, 'zone': zone, 'ip': ip,
+            parsed_devs.append(dev_dict)
-                                'port': port, 'device': device_name,
-                                'replication_ip': replication_ip,
-                                'replication_port': replication_port,
-                                'weight': weight, 'meta': meta})
     else:
         parsed_devs.append(build_dev_from_opts(opts))
 

swift/common/ring/utils.py

@@ -403,7 +403,7 @@ def parse_search_values_from_opts(opts):
     Convert optparse style options into a dictionary for searching.
 
     :param opts: optparse style options
-    :returns: a dictonary with search values to filter devices,
+    :returns: a dictionary with search values to filter devices,
               supported parameters are id, region, zone, ip, port,
               replication_ip, replication_port, device, weight, meta
     """
@@ -440,6 +440,100 @@ def parse_change_values_from_opts(opts):
     return change_values
 
 
+def parse_add_value(add_value):
+    """
+    Convert an add value, like 'r1z2-10.1.2.3:7878/sdf', to a dictionary.
+
+    If the string does not start with 'r<N>', then the value of 'region' in
+    the returned dictionary will be None. Callers should check for this and
+    set a reasonable default. This is done so callers can emit errors or
+    warnings if desired.
+
+    Similarly, 'replication_ip' and 'replication_port' will be None if not
+    specified.
+
+    :returns: dictionary with keys 'region', 'zone', 'ip', 'port', 'device',
+        'replication_ip', 'replication_port', 'meta'
+    :raises: ValueError if add_value is malformed
+    """
+    region = None
+    rest = add_value
+    if add_value.startswith('r'):
+        i = 1
+        while i < len(add_value) and add_value[i].isdigit():
+            i += 1
+        region = int(add_value[1:i])
+        rest = add_value[i:]
+
+    if not rest.startswith('z'):
+        raise ValueError('Invalid add value: %s' % add_value)
+    i = 1
+    while i < len(rest) and rest[i].isdigit():
+        i += 1
+    zone = int(rest[1:i])
+    rest = rest[i:]
+
+    if not rest.startswith('-'):
+        raise ValueError('Invalid add value: %s' % add_value)
+
+    ip, port, rest = parse_address(rest[1:])
+
+    replication_ip = replication_port = None
+    if rest.startswith('R'):
+        replication_ip, replication_port, rest =  \
+            parse_address(rest[1:])
+    if not rest.startswith('/'):
+        raise ValueError(
+            'Invalid add value: %s' % add_value)
+    i = 1
+    while i < len(rest) and rest[i] != '_':
+        i += 1
+    device_name = rest[1:i]
+    if not validate_device_name(device_name):
+        raise ValueError('Invalid device name')
+
+    rest = rest[i:]
+
+    meta = ''
+    if rest.startswith('_'):
+        meta = rest[1:]
+
+    return {'region': region, 'zone': zone, 'ip': ip, 'port': port,
+            'device': device_name, 'replication_ip': replication_ip,
+            'replication_port': replication_port, 'meta': meta}
+
+
+def parse_address(rest):
+    if rest.startswith('['):
+        # remove first [] for ip
+        rest = rest.replace('[', '', 1).replace(']', '', 1)
+
+    pos = 0
+    while (pos < len(rest) and
+           not (rest[pos] == 'R' or rest[pos] == '/')):
+        pos += 1
+    address = rest[:pos]
+    rest = rest[pos:]
+
+    port_start = address.rfind(':')
+    if port_start == -1:
+        raise ValueError('Invalid port in add value')
+
+    ip = address[:port_start]
+    try:
+        port = int(address[(port_start + 1):])
+    except (TypeError, ValueError):
+        raise ValueError(
+            'Invalid port %s in add value' % address[port_start:])
+
+    # if this is an ipv6 address then we want to convert it
+    # to all lowercase and use its fully expanded representation
+    # to make searches easier
+    ip = validate_and_normalize_ip(ip)
+
+    return (ip, port, rest)
+
+
 def validate_args(argvish):
     """
     Build OptionParse and validate it whether the format is new command-line

test/unit/cli/test_ring_builder_analyzer.py

@@ -0,0 +1,227 @@
+#! /usr/bin/env python
+# Copyright (c) 2015 Samuel Merritt <sam@swiftstack.com>
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import json
+import mock
+import unittest
+from StringIO import StringIO
+
+from swift.cli.ring_builder_analyzer import parse_scenario, run_scenario
+
+
+class TestRunScenario(unittest.TestCase):
+    def test_it_runs(self):
+        scenario = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0,
+            'rounds': [[['add', 'r1z2-3.4.5.6:7/sda8', 100],
+                        ['add', 'z2-3.4.5.6:7/sda9', 200]],
+                       [['set_weight', 0, 150]],
+                       [['remove', 1]]]}
+        parsed = parse_scenario(json.dumps(scenario))
+
+        fake_stdout = StringIO()
+        with mock.patch('sys.stdout', fake_stdout):
+            run_scenario(parsed)
+
+        # Just test that it produced some output as it ran; the fact that
+        # this doesn't crash and produces output that resembles something
+        # useful is good enough.
+        self.assertTrue('Rebalance' in fake_stdout.getvalue())
+
+
+class TestParseScenario(unittest.TestCase):
+    def test_good(self):
+        scenario = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0,
+            'rounds': [[['add', 'r1z2-3.4.5.6:7/sda8', 100],
+                        ['add', 'z2-3.4.5.6:7/sda9', 200]],
+                       [['set_weight', 0, 150]],
+                       [['remove', 1]]]}
+        parsed = parse_scenario(json.dumps(scenario))
+
+        self.assertEqual(parsed['replicas'], 3)
+        self.assertEqual(parsed['part_power'], 8)
+        self.assertEqual(parsed['random_seed'], 123)
+        self.assertEqual(parsed['overload'], 0)
+        self.assertEqual(parsed['rounds'], [
+            [['add', {'device': 'sda8',
+                      'ip': '3.4.5.6',
+                      'meta': '',
+                      'port': 7,
+                      'region': 1,
+                      'replication_ip': None,
+                      'replication_port': None,
+                      'weight': 100.0,
+                      'zone': 2}],
+             ['add', {'device': u'sda9',
+                      'ip': u'3.4.5.6',
+                      'meta': '',
+                      'port': 7,
+                      'region': 1,
+                      'replication_ip': None,
+                      'replication_port': None,
+                      'weight': 200.0,
+                      'zone': 2}]],
+            [['set_weight', 0, 150.0]],
+            [['remove', 1]]])
+
+    # The rest of this test class is just a catalog of the myriad ways that
+    # the input can be malformed.
+    def test_invalid_json(self):
+        self.assertRaises(ValueError, parse_scenario, "{")
+
+    def test_json_not_object(self):
+        self.assertRaises(ValueError, parse_scenario, "[]")
+        self.assertRaises(ValueError, parse_scenario, "\"stuff\"")
+
+    def test_bad_replicas(self):
+        working_scenario = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0,
+            'rounds': [[['add', 'r1z2-3.4.5.6:7/sda8', 100]]]}
+
+        busted = dict(working_scenario)
+        del busted['replicas']
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(working_scenario, replicas='blahblah')
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(working_scenario, replicas=-1)
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+    def test_bad_part_power(self):
+        working_scenario = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0,
+            'rounds': [[['add', 'r1z2-3.4.5.6:7/sda8', 100]]]}
+
+        busted = dict(working_scenario)
+        del busted['part_power']
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(working_scenario, part_power='blahblah')
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(working_scenario, part_power=0)
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(working_scenario, part_power=33)
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+    def test_bad_random_seed(self):
+        working_scenario = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0,
+            'rounds': [[['add', 'r1z2-3.4.5.6:7/sda8', 100]]]}
+
+        busted = dict(working_scenario)
+        del busted['random_seed']
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(working_scenario, random_seed='blahblah')
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+    def test_bad_overload(self):
+        working_scenario = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0,
+            'rounds': [[['add', 'r1z2-3.4.5.6:7/sda8', 100]]]}
+
+        busted = dict(working_scenario)
+        del busted['overload']
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(working_scenario, overload='blahblah')
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(working_scenario, overload=-0.01)
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+    def test_bad_rounds(self):
+        base = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0}
+
+        self.assertRaises(ValueError, parse_scenario, json.dumps(base))
+
+        busted = dict(base, rounds={})
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(base, rounds=[{}])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        busted = dict(base, rounds=[[['bork']]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+    def test_bad_add(self):
+        base = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0}
+
+        # no dev
+        busted = dict(base, rounds=[[['add']]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # no weight
+        busted = dict(base, rounds=[[['add', 'r1z2-1.2.3.4:6000/d7']]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # too many fields
+        busted = dict(base, rounds=[[['add', 'r1z2-1.2.3.4:6000/d7', 1, 2]]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # can't parse
+        busted = dict(base, rounds=[[['add', 'not a good value', 100]]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # negative weight
+        busted = dict(base, rounds=[[['add', 'r1z2-1.2.3.4:6000/d7', -1]]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+    def test_bad_remove(self):
+        base = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0}
+
+        # no dev
+        busted = dict(base, rounds=[[['remove']]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # bad dev id
+        busted = dict(base, rounds=[[['remove', 'not an int']]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # too many fields
+        busted = dict(base, rounds=[[['remove', 1, 2]]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+    def test_bad_set_weight(self):
+        base = {
+            'replicas': 3, 'part_power': 8, 'random_seed': 123, 'overload': 0}
+
+        # no dev
+        busted = dict(base, rounds=[[['set_weight']]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # no weight
+        busted = dict(base, rounds=[[['set_weight', 0]]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # bad dev id
+        busted = dict(base, rounds=[[['set_weight', 'not an int', 90]]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # negative weight
+        busted = dict(base, rounds=[[['set_weight', 1, -1]]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))
+
+        # bogus weight
+        busted = dict(base, rounds=[[['set_weight', 1, 'bogus']]])
+        self.assertRaises(ValueError, parse_scenario, json.dumps(busted))

test/unit/cli/test_ringbuilder.py

@@ -180,14 +180,6 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
             err = e
         self.assertEquals(err.code, 2)
 
-    def test_parse_address_old_format(self):
-        # Test old format
-        argv = "127.0.0.1:6000R127.0.0.1:6000/sda1_some meta data"
-        ip, port, rest = ringbuilder._parse_address(argv)
-        self.assertEqual(ip, '127.0.0.1')
-        self.assertEqual(port, 6000)
-        self.assertEqual(rest, 'R127.0.0.1:6000/sda1_some meta data')
-
     def test_parse_add_values_number_of_arguments(self):
         # Test Number of arguments abnormal
         argv = ["--region", "2", "test"]

test/unit/common/ring/test_utils.py

@@ -26,7 +26,8 @@ from swift.common.ring.utils import (tiers_for_dev, build_tier_tree,
                                      parse_change_values_from_opts,
                                      validate_args, parse_args,
                                      parse_builder_ring_filename_args,
-                                     build_dev_from_opts, dispersion_report)
+                                     build_dev_from_opts, dispersion_report,
+                                     parse_address)
 
 
 class TestUtils(unittest.TestCase):
@@ -694,6 +695,14 @@ class TestUtils(unittest.TestCase):
         self.assertEqual(report['worst_tier'], 'r1z0-127.0.0.1')
         self.assertEqual(report['max_dispersion'], 30.078125)
 
+    def test_parse_address_old_format(self):
+        # Test old format
+        argv = "127.0.0.1:6000R127.0.0.1:6000/sda1_some meta data"
+        ip, port, rest = parse_address(argv)
+        self.assertEqual(ip, '127.0.0.1')
+        self.assertEqual(port, 6000)
+        self.assertEqual(rest, 'R127.0.0.1:6000/sda1_some meta data')
+
 
 if __name__ == '__main__':
     unittest.main()