swift-ring-builder shows hint about composite builder file

If swift-ring-builder is erroneously given a composite builder
file, which it will fail to load, it will now print a hint
that the file is a composite builder file.

Co-Authored-By: Clay Gerrard <clay.gerrard@gmail.com>
Change-Id: If4517f3b61977a7f6ca3e08ed5deb182aa87a366

swift/cli/ringbuilder.py

@@ -35,6 +35,7 @@ from six.moves import input
 from swift.common import exceptions
 from swift.common.ring import RingBuilder, Ring, RingData
 from swift.common.ring.builder import MAX_BALANCE
+from swift.common.ring.composite_builder import CompositeRingBuilder
 from swift.common.ring.utils import validate_args, \
     validate_and_normalize_ip, build_dev_from_opts, \
     parse_builder_ring_filename_args, parse_search_value, \
@@ -1483,7 +1484,13 @@ def main(arguments=None):
     try:
         builder = RingBuilder.load(builder_file)
     except exceptions.UnPicklingError as e:
-        print(e)
+        msg = str(e)
+        try:
+            CompositeRingBuilder.load(builder_file)
+            msg += ' (it appears to be a composite ring builder file?)'
+        except Exception:  # noqa
+            pass
+        print(msg)
         exit(EXIT_ERROR)
     except (exceptions.FileNotFoundError, exceptions.PermissionError) as e:
         if len(argv) < 3 or argv[2] not in('create', 'write_builder'):

test/unit/__init__.py

@@ -44,7 +44,7 @@ from swift.common.utils import Timestamp, NOTICE
 from test import get_config
 from swift.common import utils
 from swift.common.header_key_dict import HeaderKeyDict
-from swift.common.ring import Ring, RingData
+from swift.common.ring import Ring, RingData, RingBuilder
 from swift.obj import server
 from hashlib import md5
 import logging.handlers
@@ -304,6 +304,31 @@ def write_fake_ring(path, *devs):
         pickle.dump(RingData(replica2part2dev_id, devs, part_shift), f)
 
 
+def write_stub_builder(tmpdir, region=1, name=''):
+    """
+    Pretty much just a three node, three replica, 8 part power builder...
+
+    :param tmpdir: a place to write the builder, be sure to clean it up!
+    :param region: an integer, fills in region and ip
+    :param name: the name of the builder (i.e. <name>.builder)
+    """
+    name = name or str(region)
+    replicas = 3
+    builder = RingBuilder(8, replicas, 1)
+    for i in range(replicas):
+        dev = {'weight': 100,
+               'region': '%d' % region,
+               'zone': '1',
+               'ip': '10.0.0.%d' % region,
+               'port': '3600',
+               'device': 'sdb%d' % i}
+        builder.add_dev(dev)
+    builder.rebalance()
+    builder_file = os.path.join(tmpdir, '%s.builder' % name)
+    builder.save(builder_file)
+    return builder, builder_file
+
+
 class FabricatedRing(Ring):
     """
     When a FakeRing just won't do - you can fabricate one to meet

test/unit/cli/test_ringbuilder.py

@@ -31,8 +31,9 @@ from swift.cli import ringbuilder
 from swift.cli.ringbuilder import EXIT_SUCCESS, EXIT_WARNING, EXIT_ERROR
 from swift.common import exceptions
 from swift.common.ring import RingBuilder
+from swift.common.ring.composite_builder import CompositeRingBuilder
 
-from test.unit import Timeout
+from test.unit import Timeout, write_stub_builder
 
 try:
     from itertools import zip_longest
@@ -1411,10 +1412,31 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
         argv = ["", self.tmpfile, "validate"]
         self.assertSystemExit(EXIT_SUCCESS, ringbuilder.main, argv)
 
+    def test_validate_composite_builder_file(self):
+        b1, b1_file = write_stub_builder(self.tmpdir, 1)
+        b2, b2_file = write_stub_builder(self.tmpdir, 2)
+        cb = CompositeRingBuilder([b1_file, b2_file])
+        cb.compose()
+        cb_file = os.path.join(self.tmpdir, 'composite.builder')
+        cb.save(cb_file)
+        argv = ["", cb_file, "validate"]
+        with mock.patch("sys.stdout", six.StringIO()) as mock_stdout:
+            self.assertSystemExit(EXIT_ERROR, ringbuilder.main, argv)
+        lines = mock_stdout.getvalue().strip().split('\n')
+        self.assertIn("Ring Builder file is invalid", lines[0])
+        self.assertIn("appears to be a composite ring builder file", lines[0])
+        self.assertFalse(lines[1:])
+
     def test_validate_empty_file(self):
         open(self.tmpfile, 'a').close
         argv = ["", self.tmpfile, "validate"]
+        with mock.patch("sys.stdout", six.StringIO()) as mock_stdout:
             self.assertSystemExit(EXIT_ERROR, ringbuilder.main, argv)
+        lines = mock_stdout.getvalue().strip().split('\n')
+        self.assertIn("Ring Builder file is invalid", lines[0])
+        self.assertNotIn("appears to be a composite ring builder file",
+                         lines[0])
+        self.assertFalse(lines[1:])
 
     def test_validate_corrupted_file(self):
         self.create_sample_ring()
@@ -1427,19 +1449,35 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
         # corrupt the file
         with open(self.tmpfile, 'wb') as f:
             f.write(os.urandom(1024))
+        with mock.patch("sys.stdout", six.StringIO()) as mock_stdout:
             self.assertSystemExit(EXIT_ERROR, ringbuilder.main, argv)
+        lines = mock_stdout.getvalue().strip().split('\n')
+        self.assertIn("Ring Builder file is invalid", lines[0])
+        self.assertNotIn("appears to be a composite ring builder file",
+                         lines[0])
+        self.assertFalse(lines[1:])
 
     def test_validate_non_existent_file(self):
         rand_file = '%s/%s' % (tempfile.gettempdir(), str(uuid.uuid4()))
         argv = ["", rand_file, "validate"]
+        with mock.patch("sys.stdout", six.StringIO()) as mock_stdout:
             self.assertSystemExit(EXIT_ERROR, ringbuilder.main, argv)
+        lines = mock_stdout.getvalue().strip().split('\n')
+        self.assertIn("Ring Builder file does not exist", lines[0])
+        self.assertNotIn("appears to be a composite ring builder file",
+                         lines[0])
+        self.assertFalse(lines[1:])
 
     def test_validate_non_accessible_file(self):
         with mock.patch.object(
                 RingBuilder, 'load',
-                mock.Mock(side_effect=exceptions.PermissionError)):
+                mock.Mock(side_effect=exceptions.PermissionError("boom"))):
             argv = ["", self.tmpfile, "validate"]
+            with mock.patch("sys.stdout", six.StringIO()) as mock_stdout:
                 self.assertSystemExit(EXIT_ERROR, ringbuilder.main, argv)
+        lines = mock_stdout.getvalue().strip().split('\n')
+        self.assertIn("boom", lines[0])
+        self.assertFalse(lines[1:])
 
     def test_validate_generic_error(self):
         with mock.patch.object(

test/unit/cli/test_ringcomposer.py

@@ -21,7 +21,7 @@ import six
 from mock import mock
 
 from swift.cli import ringcomposer
-from swift.common.ring import RingBuilder
+from test.unit import write_stub_builder
 
 
 class TestCommands(unittest.TestCase):
@@ -36,22 +36,6 @@ class TestCommands(unittest.TestCase):
     def tearDown(self):
         shutil.rmtree(self.tmpdir)
 
-    def _write_stub_builder(self, region):
-        replicas = 3
-        builder = RingBuilder(8, replicas, 1)
-        for i in range(replicas):
-            dev = {'weight': 100,
-                   'region': '%d' % region,
-                   'zone': '1',
-                   'ip': '10.0.0.%d' % region,
-                   'port': '3600',
-                   'device': 'sdb%d' % i}
-            builder.add_dev(dev)
-        builder.rebalance()
-        builder_file = os.path.join(self.tmpdir, '%d.builder' % region)
-        builder.save(builder_file)
-        return builder, builder_file
-
     def _run_composer(self, args):
         mock_stdout = six.StringIO()
         mock_stderr = six.StringIO()
@@ -92,8 +76,8 @@ class TestCommands(unittest.TestCase):
                 self.fail('Failed testing command %r due to: %s' % (cmd, err))
 
     def test_compose(self):
-        b1, b1_file = self._write_stub_builder(1)
+        b1, b1_file = write_stub_builder(self.tmpdir, 1)
-        b2, b2_file = self._write_stub_builder(2)
+        b2, b2_file = write_stub_builder(self.tmpdir, 2)
         args = ('', self.composite_builder_file, 'compose', b1_file, b2_file,
                 '--output', self.composite_ring_file)
         exit_code, stdout, stderr = self._run_composer(args)
@@ -102,8 +86,8 @@ class TestCommands(unittest.TestCase):
         self.assertTrue(os.path.exists(self.composite_ring_file))
 
     def test_compose_existing(self):
-        b1, b1_file = self._write_stub_builder(1)
+        b1, b1_file = write_stub_builder(self.tmpdir, 1)
-        b2, b2_file = self._write_stub_builder(2)
+        b2, b2_file = write_stub_builder(self.tmpdir, 2)
         args = ('', self.composite_builder_file, 'compose', b1_file, b2_file,
                 '--output', self.composite_ring_file)
         exit_code, stdout, stderr = self._run_composer(args)
@@ -123,7 +107,7 @@ class TestCommands(unittest.TestCase):
         self.assertTrue(os.path.exists(self.composite_ring_file))
 
     def test_compose_insufficient_component_builder_files(self):
-        b1, b1_file = self._write_stub_builder(1)
+        b1, b1_file = write_stub_builder(self.tmpdir, 1)
         args = ('', self.composite_builder_file, 'compose', b1_file,
                 '--output', self.composite_ring_file)
         exit_code, stdout, stderr = self._run_composer(args)
@@ -134,7 +118,7 @@ class TestCommands(unittest.TestCase):
         self.assertFalse(os.path.exists(self.composite_ring_file))
 
     def test_compose_nonexistent_component_builder_file(self):
-        b1, b1_file = self._write_stub_builder(1)
+        b1, b1_file = write_stub_builder(self.tmpdir, 1)
         bad_file = os.path.join(self.tmpdir, 'non-existent-file')
         args = ('', self.composite_builder_file, 'compose', b1_file, bad_file,
                 '--output', self.composite_ring_file)
@@ -146,8 +130,8 @@ class TestCommands(unittest.TestCase):
         self.assertFalse(os.path.exists(self.composite_ring_file))
 
     def test_compose_fails_to_write_composite_ring_file(self):
-        b1, b1_file = self._write_stub_builder(1)
+        b1, b1_file = write_stub_builder(self.tmpdir, 1)
-        b2, b2_file = self._write_stub_builder(2)
+        b2, b2_file = write_stub_builder(self.tmpdir, 2)
         args = ('', self.composite_builder_file, 'compose', b1_file, b2_file,
                 '--output', self.composite_ring_file)
         with mock.patch('swift.common.ring.RingData.save',
@@ -161,8 +145,8 @@ class TestCommands(unittest.TestCase):
         self.assertFalse(os.path.exists(self.composite_ring_file))
 
     def test_compose_fails_to_write_composite_builder_file(self):
-        b1, b1_file = self._write_stub_builder(1)
+        b1, b1_file = write_stub_builder(self.tmpdir, 1)
-        b2, b2_file = self._write_stub_builder(2)
+        b2, b2_file = write_stub_builder(self.tmpdir, 2)
         args = ('', self.composite_builder_file, 'compose', b1_file, b2_file,
                 '--output', self.composite_ring_file)
         func = 'swift.common.ring.composite_builder.CompositeRingBuilder.save'
@@ -177,8 +161,8 @@ class TestCommands(unittest.TestCase):
         self.assertTrue(os.path.exists(self.composite_ring_file))
 
     def test_show(self):
-        b1, b1_file = self._write_stub_builder(1)
+        b1, b1_file = write_stub_builder(self.tmpdir, 1)
-        b2, b2_file = self._write_stub_builder(2)
+        b2, b2_file = write_stub_builder(self.tmpdir, 2)
         args = ('', self.composite_builder_file, 'compose', b1_file, b2_file,
                 '--output', self.composite_ring_file)
         exit_code, stdout, stderr = self._run_composer(args)