Merge lp:~nelson-crynwr/swift/refactor into lp:~hudson-openstack/swift/trunk

[RussNelson]

At least in theory, the code is not functionally different. It's just physically restructured internally so that the help strings are now docstrings within each command/function. There are other improvements possible, such as including test code in the docstring. I love Python!

[RussNelson (nelson-crynwr)]

Oh, yes, another reason for coding it this way is that now those subroutines are available to anyone, just by importing swift-ring-builder. You'd need to have a symlink to it from /usr/bin/swift-ring-builder to /usr/lib/pymodules/python2.6/swift_ring_builder.py cuz Python is jealous of its .py extension.

[gholt (gholt)]

Russ, did you see my proposed changes to this?

https://code.launchpad.net/~gholt/swift/refactor/+merge/45166

[RussNelson (nelson-crynwr)]

> Russ, did you see my proposed changes to this?
>
> https://code.launchpad.net/~gholt/swift/refactor/+merge/45166

Yes, I just approved it.

[gholt (gholt)]

I'm good with this; but since I have code in here too now I'll just have chuck give it a once over too.

review: Approve

[Chuck Thier (cthier)]

alright

review: Approve

=== modified file 'bin/swift-ring-builder'
--- bin/swift-ring-builder	2011-01-04 23:34:43 +0000
+++ bin/swift-ring-builder	2011-01-06 00:14:08 +0000
@@ -20,20 +20,40 @@
 from os import mkdir
 from os.path import basename, dirname, exists, join as pathjoin
 from sys import argv, exit
+from textwrap import wrap
 from time import time
 
 from swift.common.ring import RingBuilder
 
 
 MAJOR_VERSION = 1
-MINOR_VERSION = 1
+MINOR_VERSION = 2
 EXIT_RING_CHANGED = 0
 EXIT_RING_UNCHANGED = 1
-EXIT_ERROR  = 2
+EXIT_ERROR = 2
 
 
 def search_devs(builder, search_value):
-    # d<device_id>z<zone>-<ip>:<port>/<device_name>_<meta>
+    """
+The <search-value> can be of the form:
+    d<device_id>z<zone>-<ip>:<port>/<device_name>_<meta>
+    Any part is optional, but you must include at least one part.
+    Examples:
+        d74              Matches the device id 74
+        z1               Matches devices in zone 1
+        z1-1.2.3.4       Matches devices in zone 1 with the ip 1.2.3.4
+        1.2.3.4          Matches devices in any zone with the ip 1.2.3.4
+        z1:5678          Matches devices in zone 1 using port 5678
+        :5678            Matches devices that use port 5678
+        /sdb1            Matches devices with the device name sdb1
+        _shiny           Matches devices with shiny in the meta data
+        _"snet: 5.6.7.8" Matches devices with snet: 5.6.7.8 in the meta data
+    Most specific example:
+        d74z1-1.2.3.4:5678/sdb1_"snet: 5.6.7.8"
+    Nerd explanation:
+        All items require their single character prefix except the ip, in which
+        case the - is optional unless the device id or zone is also included.
+    """
     orig_search_value = search_value
     match = []
     if search_value.startswith('d'):
@@ -72,7 +92,8 @@
         match.append(('meta', search_value[1:]))
         search_value = ''
     if search_value:
-        raise ValueError('Invalid <search-value>: %s' % repr(orig_search_value))
+        raise ValueError('Invalid <search-value>: %s' %
+                         repr(orig_search_value))
     devs = []
     for dev in builder.devs:
         if not dev:
@@ -89,142 +110,22 @@
     return devs
 
 
-SEARCH_VALUE_HELP = '''
-    The <search-value> can be of the form:
-    d<device_id>z<zone>-<ip>:<port>/<device_name>_<meta>
-    Any part is optional, but you must include at least one part.
-    Examples:
-        d74              Matches the device id 74
-        z1               Matches devices in zone 1
-        z1-1.2.3.4       Matches devices in zone 1 with the ip 1.2.3.4
-        1.2.3.4          Matches devices in any zone with the ip 1.2.3.4
-        z1:5678          Matches devices in zone 1 using port 5678
-        :5678            Matches devices that use port 5678
-        /sdb1            Matches devices with the device name sdb1
-        _shiny           Matches devices with shiny in the meta data
-        _"snet: 5.6.7.8" Matches devices with snet: 5.6.7.8 in the meta data
-    Most specific example:
-        d74z1-1.2.3.4:5678/sdb1_"snet: 5.6.7.8"
-    Nerd explanation:
-        All items require their single character prefix except the ip, in which
-        case the - is optional unless the device id or zone is also included.
-'''.strip()
-
-CREATE_HELP = '''
-swift-ring-builder <builder_file> create <part_power> <replicas> <min_part_hours>
+class Commands:
+
+    def unknown():
+        print 'Unknown command: %s' % argv[2]
+        exit(EXIT_ERROR)
+
+    def create():
+        """
+swift-ring-builder <builder_file> create <part_power> <replicas>
+                                         <min_part_hours>
     Creates <builder_file> with 2^<part_power> partitions and <replicas>.
     <min_part_hours> is number of hours to restrict moving a partition more
     than once.
-'''.strip()
-
-SEARCH_HELP = '''
-swift-ring-builder <builder_file> search <search-value>
-    Shows information about matching devices.
-
-    %(SEARCH_VALUE_HELP)s
-'''.strip() % globals()
-
-ADD_HELP = '''
-swift-ring-builder <builder_file> add z<zone>-<ip>:<port>/<device_name>_<meta> <wght>
-    Adds a device to the ring with the given information. No partitions will be
-    assigned to the new device until after running 'rebalance'. This is so you
-    can make multiple device changes and rebalance them all just once.
-'''.strip()
-
-SET_WEIGHT_HELP = '''
-swift-ring-builder <builder_file> set_weight <search-value> <weight>
-    Resets the device's weight. No partitions will be reassigned to or from the
-    device until after running 'rebalance'. This is so you can make multiple
-    device changes and rebalance them all just once.
-
-    %(SEARCH_VALUE_HELP)s
-'''.strip() % globals()
-
-SET_INFO_HELP = '''
-swift-ring-builder <builder_file> set_info <search-value>
-                                     <ip>:<port>/<device_name>_<meta>
-    Resets the device's information. This information isn't used to assign
-    partitions, so you can use 'write_ring' afterward to rewrite the current
-    ring with the newer device information. Any of the parts are optional
-    in the final <ip>:<port>/<device_name>_<meta> parameter; just give what you
-    want to change. For instance set_info d74 _"snet: 5.6.7.8" would just
-    update the meta data for device id 74.
-
-    %(SEARCH_VALUE_HELP)s
-'''.strip() % globals()
-
-REMOVE_HELP = '''
-swift-ring-builder <builder_file> remove <search-value>
-    Removes the device(s) from the ring. This should normally just be used for
-    a device that has failed. For a device you wish to decommission, it's best
-    to set its weight to 0, wait for it to drain all its data, then use this
-    remove command. This will not take effect until after running 'rebalance'.
-    This is so you can make multiple device changes and rebalance them all just
-    once.
-
-    %(SEARCH_VALUE_HELP)s
-'''.strip() % globals()
-
-SET_MIN_PART_HOURS_HELP = '''
-swift-ring-builder <builder_file> set_min_part_hours <hours>
-    Changes the <min_part_hours> to the given <hours>. This should be set to
-    however long a full replication/update cycle takes. We're working on a way
-    to determine this more easily than scanning logs.
-'''.strip()
-
-
-if __name__ == '__main__':
-    if len(argv) < 2:
-        print '''
-swift-ring-builder %(MAJOR_VERSION)s.%(MINOR_VERSION)s
-
-%(CREATE_HELP)s
-
-swift-ring-builder <builder_file>
-    Shows information about the ring and the devices within.
-
-%(SEARCH_HELP)s
-
-%(ADD_HELP)s
-
-%(SET_WEIGHT_HELP)s
-
-%(SET_INFO_HELP)s
-
-%(REMOVE_HELP)s
-
-swift-ring-builder <builder_file> rebalance
-    Attempts to rebalance the ring by reassigning partitions that haven't been
-    recently reassigned.
-
-swift-ring-builder <builder_file> validate
-    Just runs the validation routines on the ring.
-
-swift-ring-builder <builder_file> write_ring
-    Just rewrites the distributable ring file. This is done automatically after
-    a successful rebalance, so really this is only useful after one or more
-    'set_info' calls when no rebalance is needed but you want to send out the
-    new device information.
-
-%(SET_MIN_PART_HOURS_HELP)s
-
-Quick list: create search add set_weight set_info remove rebalance write_ring
-            set_min_part_hours
-Exit codes: 0 = ring changed, 1 = ring did not change, 2 = error
-'''.strip() % globals()
-        exit(EXIT_RING_UNCHANGED)
-
-    if exists(argv[1]):
-        builder = pickle.load(open(argv[1], 'rb'))
-        for dev in builder.devs:
-            if dev and 'meta' not in dev:
-                dev['meta'] = ''
-    elif len(argv) < 3 or argv[2] != 'create':
-        print 'Ring Builder file does not exist: %s' % argv[1]
-        exit(EXIT_ERROR)
-    elif argv[2] == 'create':
+        """
         if len(argv) < 6:
-            print CREATE_HELP
+            print Commands.create.__doc__.strip()
             exit(EXIT_RING_UNCHANGED)
         builder = RingBuilder(int(argv[3]), int(argv[4]), int(argv[5]))
         backup_dir = pathjoin(dirname(argv[1]), 'backups')
@@ -238,19 +139,11 @@
         pickle.dump(builder, open(argv[1], 'wb'), protocol=2)
         exit(EXIT_RING_CHANGED)
 
-    backup_dir = pathjoin(dirname(argv[1]), 'backups')
-    try:
-        mkdir(backup_dir)
-    except OSError, err:
-        if err.errno != EEXIST:
-            raise
-
-    ring_file = argv[1]
-    if ring_file.endswith('.builder'):
-        ring_file = ring_file[:-len('.builder')]
-    ring_file += '.ring.gz'
-
-    if len(argv) == 2:
+    def default():
+        """
+swift-ring-builder <builder_file>
+    Shows information about the ring and the devices within.
+        """
         print '%s, build version %d' % (argv[1], builder.version)
         zones = 0
         balance = 0
@@ -284,9 +177,15 @@
                      dev['meta'])
         exit(EXIT_RING_UNCHANGED)
 
-    if argv[2] == 'search':
+    def search():
+        """
+swift-ring-builder <builder_file> search <search-value>
+    Shows information about matching devices.
+        """
         if len(argv) < 4:
-            print SEARCH_HELP
+            print Commands.search.__doc__.strip()
+            print
+            print search_devs.__doc__.strip()
             exit(EXIT_RING_UNCHANGED)
         devs = search_devs(builder, argv[3])
         if not devs:
@@ -311,10 +210,16 @@
                  dev['meta'])
         exit(EXIT_RING_UNCHANGED)
 
-    elif argv[2] == 'add':
-        # add z<zone>-<ip>:<port>/<device_name>_<meta> <wght>
+    def add():
+        """
+swift-ring-builder <builder_file> add z<zone>-<ip>:<port>/<device_name>_<meta>
+                                      <wght>
+    Adds a device to the ring with the given information. No partitions will be
+    assigned to the new device until after running 'rebalance'. This is so you
+    can make multiple device changes and rebalance them all just once.
+        """
         if len(argv) < 5:
-            print ADD_HELP
+            print Commands.add.__doc__.strip()
             exit(EXIT_RING_UNCHANGED)
 
         if not argv[3].startswith('z'):
@@ -379,9 +284,17 @@
         pickle.dump(builder, open(argv[1], 'wb'), protocol=2)
         exit(EXIT_RING_UNCHANGED)
 
-    elif argv[2] == 'set_weight':
+    def set_weight():
+        """
+swift-ring-builder <builder_file> set_weight <search-value> <weight>
+    Resets the device's weight. No partitions will be reassigned to or from the
+    device until after running 'rebalance'. This is so you can make multiple
+    device changes and rebalance them all just once.
+        """
         if len(argv) != 5:
-            print SET_WEIGHT_HELP
+            print Commands.set_weight.__doc__.strip()
+            print
+            print search_devs.__doc__.strip()
             exit(EXIT_RING_UNCHANGED)
         devs = search_devs(builder, argv[3])
         weight = float(argv[4])
@@ -404,9 +317,21 @@
         pickle.dump(builder, open(argv[1], 'wb'), protocol=2)
         exit(EXIT_RING_UNCHANGED)
 
-    elif argv[2] == 'set_info':
+    def set_info():
+        """
+swift-ring-builder <builder_file> set_info <search-value>
+                                           <ip>:<port>/<device_name>_<meta>
+    Resets the device's information. This information isn't used to assign
+    partitions, so you can use 'write_ring' afterward to rewrite the current
+    ring with the newer device information. Any of the parts are optional
+    in the final <ip>:<port>/<device_name>_<meta> parameter; just give what you
+    want to change. For instance set_info d74 _"snet: 5.6.7.8" would just
+    update the meta data for device id 74.
+        """
         if len(argv) != 5:
-            print SET_INFO_HELP
+            print Commands.set_info.__doc__.strip()
+            print
+            print search_devs.__doc__.strip()
             exit(EXIT_RING_UNCHANGED)
         devs = search_devs(builder, argv[3])
         change_value = argv[4]
@@ -471,9 +396,20 @@
         pickle.dump(builder, open(argv[1], 'wb'), protocol=2)
         exit(EXIT_RING_UNCHANGED)
 
-    elif argv[2] == 'remove':
+    def remove():
+        """
+swift-ring-builder <builder_file> remove <search-value>
+    Removes the device(s) from the ring. This should normally just be used for
+    a device that has failed. For a device you wish to decommission, it's best
+    to set its weight to 0, wait for it to drain all its data, then use this
+    remove command. This will not take effect until after running 'rebalance'.
+    This is so you can make multiple device changes and rebalance them all just
+    once.
+        """
         if len(argv) < 4:
-            print REMOVE_HELP
+            print Commands.remove.__doc__.strip()
+            print
+            print search_devs.__doc__.strip()
             exit(EXIT_RING_UNCHANGED)
         devs = search_devs(builder, argv[3])
         if not devs:
@@ -491,11 +427,17 @@
         for dev in devs:
             builder.remove_dev(dev['id'])
             print 'd%(id)sz%(zone)s-%(ip)s:%(port)s/%(device)s_"%(meta)s" ' \
-                  'marked for removal and will be removed next rebalance.' % dev
+                  'marked for removal and will be removed next rebalance.' \
+                  % dev
         pickle.dump(builder, open(argv[1], 'wb'), protocol=2)
         exit(EXIT_RING_UNCHANGED)
 
-    elif argv[2] == 'rebalance':
+    def rebalance():
+        """
+swift-ring-builder <builder_file> rebalance
+    Attempts to rebalance the ring by reassigning partitions that haven't been
+    recently reassigned.
+        """
         devs_changed = builder.devs_changed
         last_balance = builder.get_balance()
         parts, balance = builder.rebalance()
@@ -528,31 +470,50 @@
         pickle.dump(builder, open(argv[1], 'wb'), protocol=2)
         exit(EXIT_RING_CHANGED)
 
-    elif argv[2] == 'validate':
+    def validate():
+        """
+swift-ring-builder <builder_file> validate
+    Just runs the validation routines on the ring.
+        """
         builder.validate()
         exit(EXIT_RING_UNCHANGED)
 
-    elif argv[2] == 'write_ring':
+    def write_ring():
+        """
+swift-ring-builder <builder_file> write_ring
+    Just rewrites the distributable ring file. This is done automatically after
+    a successful rebalance, so really this is only useful after one or more
+    'set_info' calls when no rebalance is needed but you want to send out the
+    new device information.
+        """
         ring_data = builder.get_ring()
         if not ring_data._replica2part2dev_id:
-           if ring_data.devs:
-               print 'Warning: Writing a ring with no partition assignments but with devices; did you forget to run "rebalance"?'
-           else:
-               print 'Warning: Writing an empty ring'
+            if ring_data.devs:
+                print 'Warning: Writing a ring with no partition ' \
+                      'assignments but with devices; did you forget to run ' \
+                      '"rebalance"?'
+            else:
+                print 'Warning: Writing an empty ring'
         pickle.dump(ring_data,
                     GzipFile(pathjoin(backup_dir, '%d.' % time() +
                         basename(ring_file)), 'wb'), protocol=2)
         pickle.dump(ring_data, GzipFile(ring_file, 'wb'), protocol=2)
         exit(EXIT_RING_CHANGED)
 
-    elif argv[2] == 'pretend_min_part_hours_passed':
+    def pretend_min_part_hours_passed():
         builder.pretend_min_part_hours_passed()
         pickle.dump(builder, open(argv[1], 'wb'), protocol=2)
         exit(EXIT_RING_UNCHANGED)
 
-    elif argv[2] == 'set_min_part_hours':
+    def set_min_part_hours():
+        """
+swift-ring-builder <builder_file> set_min_part_hours <hours>
+    Changes the <min_part_hours> to the given <hours>. This should be set to
+    however long a full replication/update cycle takes. We're working on a way
+    to determine this more easily than scanning logs.
+        """
         if len(argv) < 4:
-            print SET_MIN_PART_HOURS_HELP
+            print Commands.set_min_part_hours.__doc__.strip()
             exit(EXIT_RING_UNCHANGED)
         builder.change_min_part_hours(int(argv[3]))
         print 'The minimum number of hours before a partition can be ' \
@@ -560,5 +521,51 @@
         pickle.dump(builder, open(argv[1], 'wb'), protocol=2)
         exit(EXIT_RING_UNCHANGED)
 
-    print 'Unknown command: %s' % argv[2]
-    exit(EXIT_ERROR)
+
+if __name__ == '__main__':
+    if len(argv) < 2:
+        print "swift-ring-builder %(MAJOR_VERSION)s.%(MINOR_VERSION)s\n" % \
+              globals()
+        print Commands.default.__doc__.strip()
+        print
+        cmds = [c for c, f in Commands.__dict__.iteritems()
+                    if f.__doc__ and c[0] != '_' and c != 'default']
+        cmds.sort()
+        for cmd in cmds:
+            print Commands.__dict__[cmd].__doc__.strip()
+            print
+        print search_devs.__doc__.strip()
+        print
+        for line in wrap(' '.join(cmds), 79, initial_indent='Quick list: ',
+                         subsequent_indent='            '):
+            print line
+        print 'Exit codes: 0 = ring changed, 1 = ring did not change, ' \
+              '2 = error'
+        exit(EXIT_RING_UNCHANGED)
+
+    if exists(argv[1]):
+        builder = pickle.load(open(argv[1], 'rb'))
+        for dev in builder.devs:
+            if dev and 'meta' not in dev:
+                dev['meta'] = ''
+    elif len(argv) < 3 or argv[2] != 'create':
+        print 'Ring Builder file does not exist: %s' % argv[1]
+        exit(EXIT_ERROR)
+
+    backup_dir = pathjoin(dirname(argv[1]), 'backups')
+    try:
+        mkdir(backup_dir)
+    except OSError, err:
+        if err.errno != EEXIST:
+            raise
+
+    ring_file = argv[1]
+    if ring_file.endswith('.builder'):
+        ring_file = ring_file[:-len('.builder')]
+    ring_file += '.ring.gz'
+
+    if len(argv) == 2:
+        command = "default"
+    else:
+        command = argv[2]
+    Commands.__dict__.get(command, Commands.unknown)()