openstack/swift
Code Issues Proposed changes

Make swift-ring-builder filename usage more consistent

Browse Source 
Sometimes the given argument is internally altered and another filename is used
without a note to the operator. Even worse, a given .ring.gz filename is
sometimes written out as builder file, without updating the corresponding
.builder file.

There is already a method to parse the given argv and return the name of the
builder and ring file. However, it's rarely used and no warning is given to the
user if it is altered. This patch uses the already parsed builder and ring file
name instead of argv[1], and also adds a note to the user if the used filename
is differently to the one given as argument.

Closes-Bug: 1482096
Change-Id: I2f8ef23aeab8b07caaa799f7dcd57e684b4b2ad2
This commit is contained in:
Christian Schwede 2015-08-06 07:21:15 +00:00 committed by Matthew Oliver
parent e1683fdb2e
commit eeb0fa40a1
2 changed files with 40 additions and 19 deletions
swift/cli
ringbuilder.py
test/unit/cli
test_ringbuilder.py

42
swift/cli/ringbuilder.py

@ -403,14 +403,15 @@ swift-ring-builder <builder_file> create <part_power> <replicas>
print(Commands.create.__doc__.strip())
exit(EXIT_ERROR)
builder = RingBuilder(int(argv[3]), float(argv[4]), int(argv[5]))
backup_dir = pathjoin(dirname(argv[1]), 'backups')
backup_dir = pathjoin(dirname(builder_file), 'backups')
try:
mkdir(backup_dir)
except OSError as err:
if err.errno != EEXIST:
raise
builder.save(pathjoin(backup_dir, '%d.' % time() + basename(argv[1])))
builder.save(argv[1])
builder.save(pathjoin(backup_dir,
'%d.' % time() + basename(builder_file)))
builder.save(builder_file)
exit(EXIT_SUCCESS)
def default():
@ -418,7 +419,7 @@ swift-ring-builder <builder_file> create <part_power> <replicas>
swift-ring-builder <builder_file>
Shows information about the ring and the devices within.
"""
print('%s, build version %d' % (argv[1], builder.version))
print('%s, build version %d' % (builder_file, builder.version))
regions = 0
zones = 0
balance = 0
@ -546,7 +547,7 @@ swift-ring-builder <builder_file> list_parts
if not builder._replica2part2dev:
print('Specified builder file \"%s\" is not rebalanced yet. '
'Please rebalance first.' % argv[1])
'Please rebalance first.' % builder_file)
exit(EXIT_ERROR)
devs = _parse_list_parts_values(argv[3:])
@ -612,7 +613,7 @@ swift-ring-builder <builder_file> add
print('The on-disk ring builder is unchanged.')
exit(EXIT_ERROR)
builder.save(argv[1])
builder.save(builder_file)
exit(EXIT_SUCCESS)
def set_weight():
@ -644,7 +645,7 @@ swift-ring-builder <builder_file> set_weight
_parse_set_weight_values(argv[3:])
builder.save(argv[1])
builder.save(builder_file)
exit(EXIT_SUCCESS)
def set_info():
@ -689,7 +690,7 @@ swift-ring-builder <builder_file> set_info
print(err)
exit(EXIT_ERROR)
builder.save(argv[1])
builder.save(builder_file)
exit(EXIT_SUCCESS)
def remove():
@ -754,7 +755,7 @@ swift-ring-builder <builder_file> search
print('%s marked for removal and will '
'be removed next rebalance.' % format_device(dev))
builder.save(argv[1])
builder.save(builder_file)
exit(EXIT_SUCCESS)
def rebalance():
@ -856,9 +857,9 @@ swift-ring-builder <builder_file> rebalance [options]
ts = time()
builder.get_ring().save(
pathjoin(backup_dir, '%d.' % ts + basename(ring_file)))
builder.save(pathjoin(backup_dir, '%d.' % ts + basename(argv[1])))
builder.save(pathjoin(backup_dir, '%d.' % ts + basename(builder_file)))
builder.get_ring().save(ring_file)
builder.save(argv[1])
builder.save(builder_file)
exit(status)
def dispersion():
@ -893,7 +894,7 @@ swift-ring-builder <builder_file> dispersion <search_filter> [options]
status = EXIT_SUCCESS
if not builder._replica2part2dev:
print('Specified builder file \"%s\" is not rebalanced yet. '
'Please rebalance first.' % argv[1])
'Please rebalance first.' % builder_file)
exit(EXIT_ERROR)
usage = Commands.dispersion.__doc__.strip()
parser = optparse.OptionParser(usage)
@ -1015,7 +1016,7 @@ swift-ring-builder <ring_file> write_builder [min_part_hours]
def pretend_min_part_hours_passed():
builder.pretend_min_part_hours_passed()
builder.save(argv[1])
builder.save(builder_file)
exit(EXIT_SUCCESS)
def set_min_part_hours():
@ -1031,7 +1032,7 @@ swift-ring-builder <builder_file> set_min_part_hours <hours>
builder.change_min_part_hours(int(argv[3]))
print('The minimum number of hours before a partition can be '
'reassigned is now set to %s' % argv[3])
builder.save(argv[1])
builder.save(builder_file)
exit(EXIT_SUCCESS)
def set_replicas():
@ -1063,7 +1064,7 @@ swift-ring-builder <builder_file> set_replicas <replicas>
builder.set_replicas(new_replicas)
print('The replica count is now %.6f.' % builder.replicas)
print('The change will take effect after the next rebalance.')
builder.save(argv[1])
builder.save(builder_file)
exit(EXIT_SUCCESS)
def set_overload():
@ -1106,7 +1107,7 @@ swift-ring-builder <builder_file> set_overload <overload>[%]
print('The overload factor is now %0.2f%% (%.6f)' % (
builder.overload * 100, builder.overload))
print('The change will take effect after the next rebalance.')
builder.save(argv[1])
builder.save(builder_file)
exit(status)
@ -1139,6 +1140,9 @@ def main(arguments=None):
exit(EXIT_SUCCESS)
builder_file, ring_file = parse_builder_ring_filename_args(argv)
if builder_file != argv[1]:
print('Note: using %s instead of %s as builder file' % (
builder_file, argv[1]))
try:
builder = RingBuilder.load(builder_file)
@ -1151,10 +1155,10 @@ def main(arguments=None):
exit(EXIT_ERROR)
except Exception as e:
print('Problem occurred while reading builder file: %s. %s' %
(argv[1], e))
(builder_file, e))
exit(EXIT_ERROR)
backup_dir = pathjoin(dirname(argv[1]), 'backups')
backup_dir = pathjoin(dirname(builder_file), 'backups')
try:
mkdir(backup_dir)
except OSError as err:
@ -1167,7 +1171,7 @@ def main(arguments=None):
command = argv[2]
if argv[0].endswith('-safe'):
try:
with lock_parent_directory(abspath(argv[1]), 15):
with lock_parent_directory(abspath(builder_file), 15):
Commands.__dict__.get(command, Commands.unknown.im_func)()
except exceptions.LockTimeout:
print("Ring/builder dir currently locked.")

17
test/unit/cli/test_ringbuilder.py

@ -1741,6 +1741,23 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
err = exc
self.assertEquals(err.code, 2)
def test_use_ringfile_as_builderfile(self):
mock_stdout = six.StringIO()
mock_stderr = six.StringIO()
argv = ["", "object.ring.gz"]
try:
with mock.patch("sys.stdout", mock_stdout):
with mock.patch("sys.stderr", mock_stderr):
ringbuilder.main(argv)
except SystemExit:
pass
expected = "Note: using object.builder instead of object.ring.gz " \
"as builder file\n" \
"Ring Builder file does not exist: object.builder\n"
self.assertEqual(expected, mock_stdout.getvalue())
class TestRebalanceCommand(unittest.TestCase, RunSwiftRingBuilderMixin):