swift-ring-builder output corrected for ipv6

Adjusted width of ip and port columns in swift-ring-builder command
output to dynamically span to the longest ip or the longest port in
the devices list. Also combined the port and ip address columns for
better visual clarity. Took care of ipv6 format [ipv6]:port

Modified the corresponding test case with expected output.

Change-Id: I65837f8fed70be60b53d5a817a4ce529ad0f070e
Closes-Bug: #1567105

swift/cli/ringbuilder.py

@@ -39,7 +39,7 @@ from swift.common.ring.utils import validate_args, \
     parse_builder_ring_filename_args, parse_search_value, \
     parse_search_values_from_opts, parse_change_values_from_opts, \
     dispersion_report, parse_add_value
-from swift.common.utils import lock_parent_directory
+from swift.common.utils import lock_parent_directory, is_valid_ipv6
 
 MAJOR_VERSION = 1
 MINOR_VERSION = 3
@@ -376,6 +376,58 @@ def _parse_remove_values(argvish):
         exit(EXIT_ERROR)
 
 
+def _make_display_device_table(builder):
+    ip_width = 10
+    port_width = 4
+    rep_ip_width = 14
+    rep_port_width = 4
+    ip_ipv6 = rep_ipv6 = False
+    for dev in builder._iter_devs():
+        if is_valid_ipv6(dev['ip']):
+            ip_ipv6 = True
+        if is_valid_ipv6(dev['replication_ip']):
+            rep_ipv6 = True
+        ip_width = max(len(dev['ip']), ip_width)
+        rep_ip_width = max(len(dev['replication_ip']), rep_ip_width)
+        port_width = max(len(str(dev['port'])), port_width)
+        rep_port_width = max(len(str(dev['replication_port'])),
+                             rep_port_width)
+    if ip_ipv6:
+        ip_width += 2
+    if rep_ipv6:
+        rep_ip_width += 2
+    header_line = ('Devices:%5s %6s %4s %' + str(ip_width)
+                   + 's:%-' + str(port_width) + 's %' +
+                   str(rep_ip_width) + 's:%-' + str(rep_port_width) +
+                   's %5s %6s %10s %7s %5s %s') % (
+                       'id', 'region', 'zone', 'ip address',
+                       'port', 'replication ip', 'port', 'name',
+                       'weight', 'partitions', 'balance', 'flags',
+                       'meta')
+
+    def print_dev_f(dev, balance_per_dev=0.00, flags=''):
+        def get_formated_ip(key):
+            value = dev[key]
+            if ':' in value:
+                value = '[%s]' % value
+            return value
+        dev_ip = get_formated_ip('ip')
+        dev_replication_ip = get_formated_ip('replication_ip')
+        format_string = ''.join(['%13d %6d %4d ',
+                                 '%', str(ip_width), 's:%-',
+                                 str(port_width), 'd ', '%',
+                                 str(rep_ip_width), 's', ':%-',
+                                 str(rep_port_width), 'd %5s %6.02f'
+                                 ' %10s %7.02f %5s %s'])
+        args = (dev['id'], dev['region'], dev['zone'], dev_ip, dev['port'],
+                dev_replication_ip, dev['replication_port'], dev['device'],
+                dev['weight'], dev['parts'], balance_per_dev, flags,
+                dev['meta'])
+        print(format_string % args)
+
+    return header_line, print_dev_f
+
+
 class Commands(object):
     @staticmethod
     def unknown():
@@ -458,18 +510,11 @@ swift-ring-builder <builder_file>
 
         if builder.devs:
             balance_per_dev = builder._build_balance_per_dev()
-            print('Devices:    id  region  zone      ip address  port  '
+            header_line, print_dev_f = _make_display_device_table(builder)
-                  'replication ip  replication port      name '
+            print(header_line)
-                  'weight partitions balance flags meta')
             for dev in builder._iter_devs():
                 flags = 'DEL' if dev in builder._remove_devs else ''
-                print('         %5d %7d %5d %15s %5d %15s %17d %9s %6.02f '
+                print_dev_f(dev, balance_per_dev[dev['id']], flags)
-                      '%10s %7.02f %5s %s' %
-                      (dev['id'], dev['region'], dev['zone'], dev['ip'],
-                       dev['port'], dev['replication_ip'],
-                       dev['replication_port'], dev['device'], dev['weight'],
-                       dev['parts'], balance_per_dev[dev['id']], flags,
-                       dev['meta']))
         exit(EXIT_SUCCESS)
 
     @staticmethod
@@ -905,7 +950,7 @@ swift-ring-builder <builder_file> dispersion <search_filter> [options]
     --verbose option will display dispersion graph broken down by tier
 
     You can filter which tiers are evaluated to drill down using a regex
-    in the optional search_filter arguemnt.  i.e.
+    in the optional search_filter argument.  i.e.
 
         swift-ring-builder <builder_file> dispersion "r\d+z\d+$" -v
 

test/unit/cli/test_default_output.stub

@@ -0,0 +1,11 @@
+__RINGFILE__, build version 4
+64 partitions, 3.000000 replicas, 4 regions, 4 zones, 4 devices, 100.00 balance, 0.00 dispersion
+The minimum number of hours before a partition can be reassigned is 1 (0:00:00 remaining)
+The overload factor is 0.00% (0.000000)
+Ring file __RINGFILE__.ring.gz not found, probably it hasn't been written yet
+Devices:   id region zone ip address:port replication ip:port  name weight partitions balance flags meta
+            0      0    0  127.0.0.1:6200      127.0.0.1:6200  sda1 100.00          0 -100.00       some meta data
+            1      1    1  127.0.0.2:6201      127.0.0.2:6201  sda2 100.00          0 -100.00
+            2      2    2  127.0.0.3:6202      127.0.0.3:6202  sdc3 100.00          0 -100.00
+            3      3    3  127.0.0.4:6203      127.0.0.4:6203  sdd4 100.00          0 -100.00
+

test/unit/cli/test_ipv6_output.stub

@@ -0,0 +1,10 @@
+__RINGFILE__, build version 4
+256 partitions, 3.000000 replicas, 4 regions, 4 zones, 4 devices, 100.00 balance, 0.00 dispersion
+The minimum number of hours before a partition can be reassigned is 1 (0:00:00 remaining)
+The overload factor is 0.00% (0.000000)
+Ring file __RINGFILE__.ring.gz not found, probably it hasn't been written yet
+Devices:   id region zone                     ip address:port                  replication ip:port   name weight partitions balance flags meta
+            0      0    0 [2001:db8:85a3::8a2e:370:7334]:6200  [2001:db8:85a3::8a2e:370:7334]:6200   sda1 100.00          0 -100.00       some meta data
+            1      1    1                      127.0.0.1:66201                      127.0.0.1:66201  sda2 100.00          0 -100.00
+            2      2    2 [2001:db8:85a3::8a2e:370:7336]:6202                    127.0.10.127:7070   sdc3 100.00          0 -100.00
+            3      3    3 [2001:db8:85a3::8a2e:370:7337]:6203  [7001:db8:85a3::8a2e:370:7337]:11664  sdd4 100.00          0 -100.00

test/unit/cli/test_ringbuilder.py

@@ -13,6 +13,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import errno
+import itertools
 import logging
 import mock
 import os
@@ -92,6 +94,43 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
         except OSError:
             pass
 
+    def assertOutputStub(self, output, ext='stub'):
+        """
+        assert that the given output string is equal to a in-tree stub file,
+        if a test needs to check multiple outputs it can use custom ext's
+        """
+        filepath = os.path.abspath(
+            os.path.join(os.path.dirname(__file__), self.id().split('.')[-1]))
+        print(filepath)
+        filepath = '%s.%s' % (filepath, ext)
+        try:
+            with open(filepath, 'r') as f:
+                stub = f.read()
+        except (IOError, OSError) as e:
+            if e.errno == errno.ENOENT:
+                self.fail('%r does not exist' % filepath)
+            else:
+                self.fail('%r could not be read (%s)' % (filepath, e))
+        output = output.replace(self.tempfile, '__RINGFILE__')
+        for i, (value, expected) in enumerate(
+                itertools.izip_longest(
+                    output.splitlines(), stub.splitlines())):
+            # N.B. differences in trailing whitespace are ignored!
+            value = (value or '').rstrip()
+            expected = (expected or '').rstrip()
+            try:
+                self.assertEqual(value, expected)
+            except AssertionError:
+                msg = 'Line #%s value is not like expected:\n%r\n%r' % (
+                    i, value, expected)
+                msg += '\n\nFull output was:\n'
+                for i, line in enumerate(output.splitlines()):
+                    msg += '%3d: %s\n' % (i, line)
+                msg += '\n\nCompared to stub:\n'
+                for i, line in enumerate(stub.splitlines()):
+                    msg += '%3d: %s\n' % (i, line)
+                self.fail(msg)
+
     def create_sample_ring(self, part_power=6):
         """ Create a sample ring with four devices
 
@@ -1614,6 +1653,50 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
         argv = ["", self.tmpfile]
         self.assertSystemExit(EXIT_SUCCESS, ringbuilder.main, argv)
 
+    def test_default_output(self):
+        self.create_sample_ring()
+        out, err = self.run_srb('')
+        self.assertOutputStub(out)
+
+    def test_ipv6_output(self):
+        ring = RingBuilder(8, 3, 1)
+        ring.add_dev({'weight': 100.0,
+                      'region': 0,
+                      'zone': 0,
+                      'ip': '2001:db8:85a3::8a2e:370:7334',
+                      'port': 6200,
+                      'device': 'sda1',
+                      'meta': 'some meta data',
+                      })
+        ring.add_dev({'weight': 100.0,
+                      'region': 1,
+                      'zone': 1,
+                      'ip': '127.0.0.1',
+                      'port': 66201,
+                      'device': 'sda2',
+                      })
+        ring.add_dev({'weight': 100.0,
+                      'region': 2,
+                      'zone': 2,
+                      'ip': '2001:db8:85a3::8a2e:370:7336',
+                      'port': 6202,
+                      'device': 'sdc3',
+                      'replication_ip': '127.0.10.127',
+                      'replication_port': 7070,
+                      })
+        ring.add_dev({'weight': 100.0,
+                      'region': 3,
+                      'zone': 3,
+                      'ip': '2001:db8:85a3::8a2e:370:7337',
+                      'port': 6203,
+                      'device': 'sdd4',
+                      'replication_ip': '7001:db8:85a3::8a2e:370:7337',
+                      'replication_port': 11664,
+                      })
+        ring.save(self.tmpfile)
+        out, err = self.run_srb('')
+        self.assertOutputStub(out)
+
     def test_default_show_removed(self):
         mock_stdout = six.StringIO()
         mock_stderr = six.StringIO()
@@ -1642,20 +1725,20 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
             "The overload factor is 0.00%% (0.000000)\n" \
             "Ring file %s.ring.gz not found, probably " \
             "it hasn't been written yet\n" \
-            "Devices:    id  region  zone      ip address  port  " \
+            "Devices:   id region zone ip address:port " \
-            "replication ip  replication port      name weight " \
+            "replication ip:port  name weight " \
             "partitions balance flags meta\n" \
-            "             0       0     0       127.0.0.1  6200       " \
+            "            0      0    0  127.0.0.1:6200 " \
-            "127.0.0.1              6200      sda1 100.00" \
+            "     127.0.0.1:6200  sda1 100.00" \
             "          0 -100.00       some meta data\n" \
-            "             1       1     1       127.0.0.2  6201       " \
+            "            1      1    1  127.0.0.2:6201 " \
-            "127.0.0.2              6201      sda2   0.00" \
+            "     127.0.0.2:6201  sda2   0.00" \
             "          0    0.00   DEL \n" \
-            "             2       2     2       127.0.0.3  6202       " \
+            "            2      2    2  127.0.0.3:6202 " \
-            "127.0.0.3              6202      sdc3 100.00" \
+            "     127.0.0.3:6202  sdc3 100.00" \
             "          0 -100.00       \n" \
-            "             3       3     3       127.0.0.4  6203       " \
+            "            3      3    3  127.0.0.4:6203 " \
-            "127.0.0.4              6203      sdd4   0.00" \
+            "     127.0.0.4:6203  sdd4   0.00" \
             "          0    0.00       \n" % (self.tmpfile, self.tmpfile)
         self.assertEqual(expected, mock_stdout.getvalue())