From cfcbc7a66fd136fc4cf910c213d43c65b3900691 Mon Sep 17 00:00:00 2001 From: Gary Kotton Date: Mon, 7 Jan 2013 13:25:26 +0000 Subject: [PATCH] Port to argparse based cfg Implements blueprint argparse-based-cfg Sync the following changes from oslo-incubator: 479f19c Add deprecated --logdir common opt 27b2ff4 Add deprecated --logfile common opt. 9b81289 Allow nova and others to override some logging defaults 3557d84 Fix ListOpt to trim whitespace 01ab910 Fix set_default() with boolean CLI options af18eaa Improve cfg's argparse sub-parsers support f21e1d9 Fix regression with cfg CLI arguments ceb4aa7 Fix broken --help with CommonConfigOpts 5e9503b Hide the GroupAttr conf and group attributes b6d24bb updating sphinx documentation 403509e Don't reference argparse._StoreAction e17deb8 Fix minor coding style issue 0c29e1d Remove ConfigCliParser class 5b9cb41 Add support for positional arguments dbc72a6 Use stock argparse behaviour for optional args 768a147 Use stock argparse --usage behaviour ac180b9 Use stock argparse --version behaviour 0787e38 Remove add_option() method 5afead0 Completely remove cfg's disable_interspersed_args() 5f564b2 argparse support for cfg d7b6397 Add a missing comma in a docstring. aca1805 cfg: fix required if option has a dash Note: various utilities/services with command line parameters needed to be updated due to the changes mentioned above. The downside is that these are not backward compatible. Unknown arguments are no ignored. Only bound arguments will be treated. Thanks to Mark McLoughlin for migration cli code changes. Change-Id: Ia776e78cc55f4ed33ace5efa8d726b70e3fa4131 --- bin/quantum-usage-audit | 4 +- quantum/agent/dhcp_agent.py | 3 +- quantum/agent/l3_agent.py | 2 +- quantum/agent/metadata/agent.py | 3 +- quantum/agent/metadata/namespace_proxy.py | 5 +- quantum/agent/netns_cleanup_util.py | 3 +- quantum/agent/ovs_cleanup_util.py | 6 +- quantum/common/config.py | 6 +- quantum/db/migration/cli.py | 143 +++--- quantum/openstack/common/cfg.py | 462 +++++++++++------- .../agent/linuxbridge_quantum_agent.py | 3 +- .../plugins/nec/agent/nec_quantum_agent.py | 2 +- .../openvswitch/agent/ovs_quantum_agent.py | 2 +- .../plugins/ryu/agent/ryu_quantum_agent.py | 3 +- quantum/server/__init__.py | 2 +- quantum/tests/unit/test_db_migration.py | 114 ++--- quantum/tests/unit/test_debug_commands.py | 2 +- quantum/tests/unit/test_dhcp_agent.py | 16 +- tools/pip-requires | 1 + 19 files changed, 468 insertions(+), 314 deletions(-) diff --git a/bin/quantum-usage-audit b/bin/quantum-usage-audit index 91c9ac4ffe..1d121f0668 100755 --- a/bin/quantum-usage-audit +++ b/bin/quantum-usage-audit @@ -21,8 +21,6 @@ subnets. """ -import sys - from quantum import context from quantum import manager from quantum.common import config @@ -30,7 +28,7 @@ from quantum.openstack.common import cfg from quantum.openstack.common.notifier import api as notifier_api if __name__ == '__main__': - cfg.CONF(args=sys.argv, project='quantum') + cfg.CONF(project='quantum') config.setup_logging(cfg.CONF) context = context.get_admin_context() diff --git a/quantum/agent/dhcp_agent.py b/quantum/agent/dhcp_agent.py index ffa8d10435..a85ce02ae7 100644 --- a/quantum/agent/dhcp_agent.py +++ b/quantum/agent/dhcp_agent.py @@ -17,7 +17,6 @@ import os import socket -import sys import uuid import eventlet @@ -544,7 +543,7 @@ def main(): cfg.CONF.register_opts(DhcpLeaseRelay.OPTS) cfg.CONF.register_opts(dhcp.OPTS) cfg.CONF.register_opts(interface.OPTS) - cfg.CONF(args=sys.argv, project='quantum') + cfg.CONF(project='quantum') config.setup_logging(cfg.CONF) mgr = DhcpAgent(cfg.CONF) diff --git a/quantum/agent/l3_agent.py b/quantum/agent/l3_agent.py index 9dffca55e2..cceb824c5c 100644 --- a/quantum/agent/l3_agent.py +++ b/quantum/agent/l3_agent.py @@ -611,7 +611,7 @@ def main(): conf.register_opts(L3NATAgent.OPTS) conf.register_opts(interface.OPTS) conf.register_opts(external_process.OPTS) - conf(sys.argv) + conf() config.setup_logging(conf) server = quantum_service.Service.create(binary='quantum-l3-agent', topic=topics.L3_AGENT) diff --git a/quantum/agent/metadata/agent.py b/quantum/agent/metadata/agent.py index fe8529904e..61a8727c3f 100644 --- a/quantum/agent/metadata/agent.py +++ b/quantum/agent/metadata/agent.py @@ -20,7 +20,6 @@ import hashlib import hmac import os import socket -import sys import urlparse import eventlet @@ -207,7 +206,7 @@ def main(): eventlet.monkey_patch() cfg.CONF.register_opts(UnixDomainMetadataProxy.OPTS) cfg.CONF.register_opts(MetadataProxyHandler.OPTS) - cfg.CONF(args=sys.argv, project='quantum') + cfg.CONF(project='quantum') config.setup_logging(cfg.CONF) proxy = UnixDomainMetadataProxy(cfg.CONF) diff --git a/quantum/agent/metadata/namespace_proxy.py b/quantum/agent/metadata/namespace_proxy.py index 7d677100b0..0f6849dbba 100644 --- a/quantum/agent/metadata/namespace_proxy.py +++ b/quantum/agent/metadata/namespace_proxy.py @@ -18,7 +18,6 @@ import httplib import socket -import sys import urlparse import eventlet @@ -149,8 +148,8 @@ def main(): help="TCP Port to listen for metadata server requests."), ] - cfg.CONF.register_opts(opts) - cfg.CONF(args=sys.argv, project='quantum') + cfg.CONF.register_cli_opts(opts) + cfg.CONF(project='quantum') config.setup_logging(cfg.CONF) proxy = ProxyDaemon(cfg.CONF.pid_file, diff --git a/quantum/agent/netns_cleanup_util.py b/quantum/agent/netns_cleanup_util.py index f3b0b6fc38..85c822951e 100644 --- a/quantum/agent/netns_cleanup_util.py +++ b/quantum/agent/netns_cleanup_util.py @@ -16,7 +16,6 @@ # under the License. import re -import sys import eventlet @@ -164,7 +163,7 @@ def main(): eventlet.monkey_patch() conf = setup_conf() - conf(sys.argv) + conf() # Identify namespaces that are candidates for deletion. candidates = [ns for ns in diff --git a/quantum/agent/ovs_cleanup_util.py b/quantum/agent/ovs_cleanup_util.py index ac4f7ee5bb..22038317fe 100644 --- a/quantum/agent/ovs_cleanup_util.py +++ b/quantum/agent/ovs_cleanup_util.py @@ -15,8 +15,6 @@ # License for the specific language governing permissions and limitations # under the License. -import sys - from quantum.agent import l3_agent from quantum.agent.linux import interface from quantum.agent.linux import ovs_lib @@ -43,7 +41,7 @@ def setup_conf(): ] conf = cfg.CommonConfigOpts() - conf.register_opts(opts) + conf.register_cli_opts(opts) conf.register_opts(l3_agent.L3NATAgent.OPTS) conf.register_opts(interface.OPTS) config.setup_logging(conf) @@ -57,7 +55,7 @@ def main(): """ conf = setup_conf() - conf(sys.argv) + conf() configuration_bridges = set([conf.ovs_integration_bridge, conf.external_network_bridge]) diff --git a/quantum/common/config.py b/quantum/common/config.py index fb273b465b..079714de61 100644 --- a/quantum/common/config.py +++ b/quantum/common/config.py @@ -47,7 +47,6 @@ core_opts = [ cfg.BoolOpt('allow_bulk', default=True), cfg.IntOpt('max_dns_nameservers', default=5), cfg.IntOpt('max_subnet_host_routes', default=20), - cfg.StrOpt('state_path', default='/var/lib/quantum'), cfg.IntOpt('dhcp_lease_duration', default=120), cfg.BoolOpt('allow_overlapping_ips', default=False), cfg.StrOpt('control_exchange', @@ -58,8 +57,13 @@ core_opts = [ help=_("Ensure that configured gateway is on subnet")), ] +core_cli_opts = [ + cfg.StrOpt('state_path', default='/var/lib/quantum'), +] + # Register the configuration options cfg.CONF.register_opts(core_opts) +cfg.CONF.register_cli_opts(core_cli_opts) def parse(args): diff --git a/quantum/db/migration/cli.py b/quantum/db/migration/cli.py index 50dd24ec1b..ec9a32c3d1 100644 --- a/quantum/db/migration/cli.py +++ b/quantum/db/migration/cli.py @@ -44,31 +44,84 @@ _db_opts = [ help='URL to database'), ] -_cmd_opts = [ - cfg.StrOpt('message', - short='m', - default='', - help="Message string to use with 'revision'"), - cfg.BoolOpt('autogenerate', - default=False, - help=("Populate revision script with candidate " - "migration operations, based on comparison " - "of database to model.")), - cfg.BoolOpt('sql', - default=False, - help=("Don't emit SQL to database - dump to " - "standard output/file instead")), - cfg.IntOpt('delta', - default=0, - help='Number of relative migrations to upgrade/downgrade'), - -] - CONF = cfg.CommonConfigOpts() CONF.register_opts(_core_opts) CONF.register_opts(_db_opts, 'DATABASE') CONF.register_opts(_quota_opts, 'QUOTAS') -CONF.register_cli_opts(_cmd_opts) + + +def do_alembic_command(config, cmd, *args, **kwargs): + try: + getattr(alembic_command, cmd)(config, *args, **kwargs) + except alembic_util.CommandError, e: + alembic_util.err(str(e)) + + +def do_check_migration(config, cmd): + do_alembic_command(config, 'branches') + + +def do_upgrade_downgrade(config, cmd): + if not CONF.command.revision and not CONF.command.delta: + raise SystemExit(_('You must provide a revision or relative delta')) + + revision = CONF.command.revision + + if CONF.command.delta: + sign = '+' if CONF.command.name == 'upgrade' else '-' + revision = sign + str(CONF.command.delta) + else: + revision = CONF.command.revision + + do_alembic_command(config, cmd, revision, sql=CONF.command.sql) + + +def do_stamp(config, cmd): + do_alembic_command(config, cmd, + CONF.command.revision, + sql=CONF.command.sql) + + +def do_revision(config, cmd): + do_alembic_command(config, cmd, + message=CONF.command.message, + autogenerate=CONF.command.autogenerate, + sql=CONF.command.sql) + + +def add_command_parsers(subparsers): + for name in ['current', 'history', 'branches']: + parser = subparsers.add_parser(name) + parser.set_defaults(func=do_alembic_command) + + parser = subparsers.add_parser('check_migration') + parser.set_defaults(func=do_check_migration) + + for name in ['upgrade', 'downgrade']: + parser = subparsers.add_parser(name) + parser.add_argument('--delta', type=int) + parser.add_argument('--sql', action='store_true') + parser.add_argument('revision', nargs='?') + parser.set_defaults(func=do_upgrade_downgrade) + + parser = subparsers.add_parser('stamp') + parser.add_argument('--sql', action='store_true') + parser.add_argument('revision') + parser.set_defaults(func=do_stamp) + + parser = subparsers.add_parser('revision') + parser.add_argument('-m', '--message') + parser.add_argument('--autogenerate', action='store_true') + parser.add_argument('--sql', action='store_true') + parser.set_defaults(func=do_revision) + + +command_opt = cfg.SubCommandOpt('command', + title='Command', + help='Available commands', + handler=add_command_parsers) + +CONF.register_cli_opt(command_opt) def main(): @@ -80,49 +133,5 @@ def main(): # attach the Quantum conf to the Alembic conf config.quantum_config = CONF - cmd, args, kwargs = process_argv(sys.argv) - - try: - getattr(alembic_command, cmd)(config, *args, **kwargs) - except alembic_util.CommandError, e: - alembic_util.err(str(e)) - - -def process_argv(argv): - positional = CONF(argv) - - if len(positional) > 1: - cmd = positional[1] - revision = positional[2:] and positional[2:][0] - - args = () - kwargs = {} - - if cmd == 'stamp': - args = (revision,) - kwargs = {'sql': CONF.sql} - elif cmd in ('current', 'history'): - pass # these commands do not require additional args - elif cmd in ('upgrade', 'downgrade'): - if CONF.delta: - revision = '%s%d' % ({'upgrade': '+', 'downgrade': '-'}[cmd], - CONF.delta) - elif not revision: - raise SystemExit( - _('You must provide a revision or relative delta') - ) - args = (revision,) - kwargs = {'sql': CONF.sql} - elif cmd == 'revision': - kwargs = { - 'message': CONF.message, - 'autogenerate': CONF.autogenerate, - 'sql': CONF.sql} - elif cmd == 'check_migration': - cmd = 'branches' - else: - raise SystemExit(_('Unrecognized Command: %s') % cmd) - - return cmd, args, kwargs - else: - raise SystemExit(_('You must provide a sub-command')) + CONF() + CONF.command.func(config, CONF.command.name) diff --git a/quantum/openstack/common/cfg.py b/quantum/openstack/common/cfg.py index c760ede2a0..10a91db31d 100644 --- a/quantum/openstack/common/cfg.py +++ b/quantum/openstack/common/cfg.py @@ -205,27 +205,11 @@ Option values may reference other values using PEP 292 string substitution:: Note that interpolation can be avoided by using '$$'. -For command line utilities that dispatch to other command line utilities, the -disable_interspersed_args() method is available. If this this method is called, -then parsing e.g.:: - - script --verbose cmd --debug /tmp/mything - -will no longer return:: - - ['cmd', '/tmp/mything'] - -as the leftover arguments, but will instead return:: - - ['cmd', '--debug', '/tmp/mything'] - -i.e. argument parsing is stopped at the first non-option argument. - Options may be declared as required so that an error is raised if the user does not supply a value for the option. Options may be declared as secret so that their values are not leaked into -log files: +log files:: opts = [ cfg.StrOpt('s3_store_access_key', secret=True), @@ -234,28 +218,50 @@ log files: ] This module also contains a global instance of the CommonConfigOpts class -in order to support a common usage pattern in OpenStack: +in order to support a common usage pattern in OpenStack:: - from quantum.openstack.common import cfg + from quantum.openstack.common import cfg - opts = [ - cfg.StrOpt('bind_host', default='0.0.0.0'), - cfg.IntOpt('bind_port', default=9292), - ] + opts = [ + cfg.StrOpt('bind_host', default='0.0.0.0'), + cfg.IntOpt('bind_port', default=9292), + ] - CONF = cfg.CONF - CONF.register_opts(opts) + CONF = cfg.CONF + CONF.register_opts(opts) - def start(server, app): - server.start(app, CONF.bind_port, CONF.bind_host) + def start(server, app): + server.start(app, CONF.bind_port, CONF.bind_host) + +Positional command line arguments are supported via a 'positional' Opt +constructor argument:: + + >>> CONF.register_cli_opt(MultiStrOpt('bar', positional=True)) + True + >>> CONF(['a', 'b']) + >>> CONF.bar + ['a', 'b'] + +It is also possible to use argparse "sub-parsers" to parse additional +command line arguments using the SubCommandOpt class: + + >>> def add_parsers(subparsers): + ... list_action = subparsers.add_parser('list') + ... list_action.add_argument('id') + ... + >>> CONF.register_cli_opt(SubCommandOpt('action', handler=add_parsers)) + True + >>> CONF(['list', '10']) + >>> CONF.action.name, CONF.action.id + ('list', '10') """ +import argparse import collections import copy import functools import glob -import optparse import os import string import sys @@ -474,6 +480,13 @@ def _is_opt_registered(opts, opt): return False +def set_defaults(opts, **kwargs): + for opt in opts: + if opt.dest in kwargs: + opt.default = kwargs[opt.dest] + break + + class Opt(object): """Base class for all configuration options. @@ -489,6 +502,8 @@ class Opt(object): a single character CLI option name default: the default value of the option + positional: + True if the option is a positional CLI argument metavar: the name shown as the argument to a CLI option in --help output help: @@ -497,8 +512,8 @@ class Opt(object): multi = False def __init__(self, name, dest=None, short=None, default=None, - metavar=None, help=None, secret=False, required=False, - deprecated_name=None): + positional=False, metavar=None, help=None, + secret=False, required=False, deprecated_name=None): """Construct an Opt object. The only required parameter is the option's name. However, it is @@ -508,6 +523,7 @@ class Opt(object): :param dest: the name of the corresponding ConfigOpts property :param short: a single character CLI option name :param default: the default value of the option + :param positional: True if the option is a positional CLI argument :param metavar: the option argument to show in --help :param help: an explanation of how the option is used :param secret: true iff the value should be obfuscated in log output @@ -521,6 +537,7 @@ class Opt(object): self.dest = dest self.short = short self.default = default + self.positional = positional self.metavar = metavar self.help = help self.secret = secret @@ -561,64 +578,73 @@ class Opt(object): :param parser: the CLI option parser :param group: an optional OptGroup object """ - container = self._get_optparse_container(parser, group) - kwargs = self._get_optparse_kwargs(group) - prefix = self._get_optparse_prefix('', group) - self._add_to_optparse(container, self.name, self.short, kwargs, prefix, - self.deprecated_name) + container = self._get_argparse_container(parser, group) + kwargs = self._get_argparse_kwargs(group) + prefix = self._get_argparse_prefix('', group) + self._add_to_argparse(container, self.name, self.short, kwargs, prefix, + self.positional, self.deprecated_name) - def _add_to_optparse(self, container, name, short, kwargs, prefix='', - deprecated_name=None): - """Add an option to an optparse parser or group. + def _add_to_argparse(self, container, name, short, kwargs, prefix='', + positional=False, deprecated_name=None): + """Add an option to an argparse parser or group. - :param container: an optparse.OptionContainer object + :param container: an argparse._ArgumentGroup object :param name: the opt name :param short: the short opt name - :param kwargs: the keyword arguments for add_option() + :param kwargs: the keyword arguments for add_argument() :param prefix: an optional prefix to prepend to the opt name + :param position: whether the optional is a positional CLI argument :raises: DuplicateOptError if a naming confict is detected """ - args = ['--' + prefix + name] + def hyphen(arg): + return arg if not positional else '' + + args = [hyphen('--') + prefix + name] if short: - args += ['-' + short] + args.append(hyphen('-') + short) if deprecated_name: - args += ['--' + prefix + deprecated_name] - for a in args: - if container.has_option(a): - raise DuplicateOptError(a) - container.add_option(*args, **kwargs) + args.append(hyphen('--') + prefix + deprecated_name) - def _get_optparse_container(self, parser, group): - """Returns an optparse.OptionContainer. + try: + container.add_argument(*args, **kwargs) + except argparse.ArgumentError as e: + raise DuplicateOptError(e) - :param parser: an optparse.OptionParser + def _get_argparse_container(self, parser, group): + """Returns an argparse._ArgumentGroup. + + :param parser: an argparse.ArgumentParser :param group: an (optional) OptGroup object - :returns: an optparse.OptionGroup if a group is given, else the parser + :returns: an argparse._ArgumentGroup if group is given, else parser """ if group is not None: - return group._get_optparse_group(parser) + return group._get_argparse_group(parser) else: return parser - def _get_optparse_kwargs(self, group, **kwargs): - """Build a dict of keyword arguments for optparse's add_option(). + def _get_argparse_kwargs(self, group, **kwargs): + """Build a dict of keyword arguments for argparse's add_argument(). Most opt types extend this method to customize the behaviour of the - options added to optparse. + options added to argparse. :param group: an optional group :param kwargs: optional keyword arguments to add to :returns: a dict of keyword arguments """ - dest = self.dest - if group is not None: - dest = group.name + '_' + dest - kwargs.update({'dest': dest, + if not self.positional: + dest = self.dest + if group is not None: + dest = group.name + '_' + dest + kwargs['dest'] = dest + else: + kwargs['nargs'] = '?' + kwargs.update({'default': None, 'metavar': self.metavar, 'help': self.help, }) return kwargs - def _get_optparse_prefix(self, prefix, group): + def _get_argparse_prefix(self, prefix, group): """Build a prefix for the CLI option name, if required. CLI options in a group are prefixed with the group's name in order @@ -656,6 +682,11 @@ class BoolOpt(Opt): _boolean_states = {'1': True, 'yes': True, 'true': True, 'on': True, '0': False, 'no': False, 'false': False, 'off': False} + def __init__(self, *args, **kwargs): + if 'positional' in kwargs: + raise ValueError('positional boolean args not supported') + super(BoolOpt, self).__init__(*args, **kwargs) + def _get_from_config_parser(self, cparser, section): """Retrieve the opt value as a boolean from ConfigParser.""" def convert_bool(v): @@ -671,21 +702,32 @@ class BoolOpt(Opt): def _add_to_cli(self, parser, group=None): """Extends the base class method to add the --nooptname option.""" super(BoolOpt, self)._add_to_cli(parser, group) - self._add_inverse_to_optparse(parser, group) + self._add_inverse_to_argparse(parser, group) - def _add_inverse_to_optparse(self, parser, group): + def _add_inverse_to_argparse(self, parser, group): """Add the --nooptname option to the option parser.""" - container = self._get_optparse_container(parser, group) - kwargs = self._get_optparse_kwargs(group, action='store_false') - prefix = self._get_optparse_prefix('no', group) + container = self._get_argparse_container(parser, group) + kwargs = self._get_argparse_kwargs(group, action='store_false') + prefix = self._get_argparse_prefix('no', group) kwargs["help"] = "The inverse of --" + self.name - self._add_to_optparse(container, self.name, None, kwargs, prefix, - self.deprecated_name) + self._add_to_argparse(container, self.name, None, kwargs, prefix, + self.positional, self.deprecated_name) - def _get_optparse_kwargs(self, group, action='store_true', **kwargs): - """Extends the base optparse keyword dict for boolean options.""" - return super(BoolOpt, - self)._get_optparse_kwargs(group, action=action, **kwargs) + def _get_argparse_kwargs(self, group, action='store_true', **kwargs): + """Extends the base argparse keyword dict for boolean options.""" + + kwargs = super(BoolOpt, self)._get_argparse_kwargs(group, **kwargs) + + # metavar has no effect for BoolOpt + if 'metavar' in kwargs: + del kwargs['metavar'] + + if action != 'store_true': + action = 'store_false' + + kwargs['action'] = action + + return kwargs class IntOpt(Opt): @@ -697,10 +739,10 @@ class IntOpt(Opt): return [int(v) for v in self._cparser_get_with_deprecated(cparser, section)] - def _get_optparse_kwargs(self, group, **kwargs): - """Extends the base optparse keyword dict for integer options.""" + def _get_argparse_kwargs(self, group, **kwargs): + """Extends the base argparse keyword dict for integer options.""" return super(IntOpt, - self)._get_optparse_kwargs(group, type='int', **kwargs) + self)._get_argparse_kwargs(group, type=int, **kwargs) class FloatOpt(Opt): @@ -712,10 +754,10 @@ class FloatOpt(Opt): return [float(v) for v in self._cparser_get_with_deprecated(cparser, section)] - def _get_optparse_kwargs(self, group, **kwargs): - """Extends the base optparse keyword dict for float options.""" - return super(FloatOpt, - self)._get_optparse_kwargs(group, type='float', **kwargs) + def _get_argparse_kwargs(self, group, **kwargs): + """Extends the base argparse keyword dict for float options.""" + return super(FloatOpt, self)._get_argparse_kwargs(group, + type=float, **kwargs) class ListOpt(Opt): @@ -725,23 +767,26 @@ class ListOpt(Opt): is a list containing these strings. """ + class _StoreListAction(argparse.Action): + """ + An argparse action for parsing an option value into a list. + """ + def __call__(self, parser, namespace, values, option_string=None): + if values is not None: + values = [a.strip() for a in values.split(',')] + setattr(namespace, self.dest, values) + def _get_from_config_parser(self, cparser, section): """Retrieve the opt value as a list from ConfigParser.""" - return [v.split(',') for v in + return [[a.strip() for a in v.split(',')] for v in self._cparser_get_with_deprecated(cparser, section)] - def _get_optparse_kwargs(self, group, **kwargs): - """Extends the base optparse keyword dict for list options.""" - return super(ListOpt, - self)._get_optparse_kwargs(group, - type='string', - action='callback', - callback=self._parse_list, - **kwargs) - - def _parse_list(self, option, opt, value, parser): - """An optparse callback for parsing an option value into a list.""" - setattr(parser.values, self.dest, value.split(',')) + def _get_argparse_kwargs(self, group, **kwargs): + """Extends the base argparse keyword dict for list options.""" + return Opt._get_argparse_kwargs(self, + group, + action=ListOpt._StoreListAction, + **kwargs) class MultiStrOpt(Opt): @@ -752,10 +797,14 @@ class MultiStrOpt(Opt): """ multi = True - def _get_optparse_kwargs(self, group, **kwargs): - """Extends the base optparse keyword dict for multi str options.""" - return super(MultiStrOpt, - self)._get_optparse_kwargs(group, action='append') + def _get_argparse_kwargs(self, group, **kwargs): + """Extends the base argparse keyword dict for multi str options.""" + kwargs = super(MultiStrOpt, self)._get_argparse_kwargs(group) + if not self.positional: + kwargs['action'] = 'append' + else: + kwargs['nargs'] = '*' + return kwargs def _cparser_get_with_deprecated(self, cparser, section): """If cannot find option as dest try deprecated_name alias.""" @@ -765,6 +814,57 @@ class MultiStrOpt(Opt): return cparser.get(section, [self.dest], multi=True) +class SubCommandOpt(Opt): + + """ + Sub-command options allow argparse sub-parsers to be used to parse + additional command line arguments. + + The handler argument to the SubCommandOpt contructor is a callable + which is supplied an argparse subparsers object. Use this handler + callable to add sub-parsers. + + The opt value is SubCommandAttr object with the name of the chosen + sub-parser stored in the 'name' attribute and the values of other + sub-parser arguments available as additional attributes. + """ + + def __init__(self, name, dest=None, handler=None, + title=None, description=None, help=None): + """Construct an sub-command parsing option. + + This behaves similarly to other Opt sub-classes but adds a + 'handler' argument. The handler is a callable which is supplied + an subparsers object when invoked. The add_parser() method on + this subparsers object can be used to register parsers for + sub-commands. + + :param name: the option's name + :param dest: the name of the corresponding ConfigOpts property + :param title: title of the sub-commands group in help output + :param description: description of the group in help output + :param help: a help string giving an overview of available sub-commands + """ + super(SubCommandOpt, self).__init__(name, dest=dest, help=help) + self.handler = handler + self.title = title + self.description = description + + def _add_to_cli(self, parser, group=None): + """Add argparse sub-parsers and invoke the handler method.""" + dest = self.dest + if group is not None: + dest = group.name + '_' + dest + + subparsers = parser.add_subparsers(dest=dest, + title=self.title, + description=self.description, + help=self.help) + + if not self.handler is None: + self.handler(subparsers) + + class OptGroup(object): """ @@ -800,19 +900,20 @@ class OptGroup(object): self.help = help self._opts = {} # dict of dicts of (opt:, override:, default:) - self._optparse_group = None + self._argparse_group = None - def _register_opt(self, opt): + def _register_opt(self, opt, cli=False): """Add an opt to this group. :param opt: an Opt object + :param cli: whether this is a CLI option :returns: False if previously registered, True otherwise :raises: DuplicateOptError if a naming conflict is detected """ if _is_opt_registered(self._opts, opt): return False - self._opts[opt.dest] = {'opt': opt} + self._opts[opt.dest] = {'opt': opt, 'cli': cli} return True @@ -824,16 +925,16 @@ class OptGroup(object): if opt.dest in self._opts: del self._opts[opt.dest] - def _get_optparse_group(self, parser): - """Build an optparse.OptionGroup for this group.""" - if self._optparse_group is None: - self._optparse_group = optparse.OptionGroup(parser, self.title, - self.help) - return self._optparse_group + def _get_argparse_group(self, parser): + if self._argparse_group is None: + """Build an argparse._ArgumentGroup for this group.""" + self._argparse_group = parser.add_argument_group(self.title, + self.help) + return self._argparse_group def _clear(self): """Clear this group's option parsing state.""" - self._optparse_group = None + self._argparse_group = None class ParseError(iniparser.ParseError): @@ -928,26 +1029,31 @@ class ConfigOpts(collections.Mapping): self._groups = {} self._args = None + self._oparser = None self._cparser = None self._cli_values = {} self.__cache = {} self._config_opts = [] - self._disable_interspersed_args = False - def _setup(self, project, prog, version, usage, default_config_files): - """Initialize a ConfigOpts object for option parsing.""" + def _pre_setup(self, project, prog, version, usage, default_config_files): + """Initialize a ConfigCliParser object for option parsing.""" + if prog is None: prog = os.path.basename(sys.argv[0]) if default_config_files is None: default_config_files = find_config_files(project, prog) - self._oparser = optparse.OptionParser(prog=prog, - version=version, - usage=usage) - if self._disable_interspersed_args: - self._oparser.disable_interspersed_args() + self._oparser = argparse.ArgumentParser(prog=prog, usage=usage) + self._oparser.add_argument('--version', + action='version', + version=version) + + return prog, default_config_files + + def _setup(self, project, prog, version, usage, default_config_files): + """Initialize a ConfigOpts object for option parsing.""" self._config_opts = [ MultiStrOpt('config-file', @@ -1017,18 +1123,23 @@ class ConfigOpts(collections.Mapping): :raises: SystemExit, ConfigFilesNotFoundError, ConfigFileParseError, RequiredOptError, DuplicateOptError """ + self.clear() + prog, default_config_files = self._pre_setup(project, + prog, + version, + usage, + default_config_files) + self._setup(project, prog, version, usage, default_config_files) - self._cli_values, leftovers = self._parse_cli_opts(args) + self._cli_values = self._parse_cli_opts(args) self._parse_config_files() self._check_required_opts() - return leftovers - def __getattr__(self, name): """Look up an option value and perform string substitution. @@ -1062,17 +1173,21 @@ class ConfigOpts(collections.Mapping): @__clear_cache def clear(self): - """Clear the state of the object to before it was called.""" + """Clear the state of the object to before it was called. + + Any subparsers added using the add_cli_subparsers() will also be + removed as a side-effect of this method. + """ self._args = None self._cli_values.clear() - self._oparser = None + self._oparser = argparse.ArgumentParser() self._cparser = None self.unregister_opts(self._config_opts) for group in self._groups.values(): group._clear() @__clear_cache - def register_opt(self, opt, group=None): + def register_opt(self, opt, group=None, cli=False): """Register an option schema. Registering an option schema makes any option value which is previously @@ -1080,17 +1195,19 @@ class ConfigOpts(collections.Mapping): as an attribute of this object. :param opt: an instance of an Opt sub-class + :param cli: whether this is a CLI option :param group: an optional OptGroup object or group name :return: False if the opt was already register, True otherwise :raises: DuplicateOptError """ if group is not None: - return self._get_group(group, autocreate=True)._register_opt(opt) + group = self._get_group(group, autocreate=True) + return group._register_opt(opt, cli) if _is_opt_registered(self._opts, opt): return False - self._opts[opt.dest] = {'opt': opt} + self._opts[opt.dest] = {'opt': opt, 'cli': cli} return True @@ -1116,7 +1233,7 @@ class ConfigOpts(collections.Mapping): if self._args is not None: raise ArgsAlreadyParsedError("cannot register CLI option") - return self.register_opt(opt, group, clear_cache=False) + return self.register_opt(opt, group, cli=True, clear_cache=False) @__clear_cache def register_cli_opts(self, opts, group=None): @@ -1243,10 +1360,11 @@ class ConfigOpts(collections.Mapping): for info in group._opts.values(): yield info, group - def _all_opts(self): - """A generator function for iteration opts.""" + def _all_cli_opts(self): + """A generator function for iterating CLI opts.""" for info, group in self._all_opt_infos(): - yield info['opt'], group + if info['cli']: + yield info['opt'], group def _unset_defaults_and_overrides(self): """Unset any default or override on all options.""" @@ -1254,31 +1372,6 @@ class ConfigOpts(collections.Mapping): info.pop('default', None) info.pop('override', None) - def disable_interspersed_args(self): - """Set parsing to stop on the first non-option. - - If this this method is called, then parsing e.g. - - script --verbose cmd --debug /tmp/mything - - will no longer return: - - ['cmd', '/tmp/mything'] - - as the leftover arguments, but will instead return: - - ['cmd', '--debug', '/tmp/mything'] - - i.e. argument parsing is stopped at the first non-option argument. - """ - self._disable_interspersed_args = True - - def enable_interspersed_args(self): - """Set parsing to not stop on the first non-option. - - This it the default behaviour.""" - self._disable_interspersed_args = False - def find_file(self, name): """Locate a file located alongside the config files. @@ -1377,6 +1470,9 @@ class ConfigOpts(collections.Mapping): info = self._get_opt_info(name, group) opt = info['opt'] + if isinstance(opt, SubCommandOpt): + return self.SubCommandAttr(self, group, opt.dest) + if 'override' in info: return info['override'] @@ -1401,6 +1497,10 @@ class ConfigOpts(collections.Mapping): if not opt.multi: return value + # argparse ignores default=None for nargs='*' + if opt.positional and not value: + value = opt.default + return value + values if values: @@ -1523,12 +1623,10 @@ class ConfigOpts(collections.Mapping): """ self._args = args - for opt, group in self._all_opts(): + for opt, group in self._all_cli_opts(): opt._add_to_cli(self._oparser, group) - values, leftovers = self._oparser.parse_args(args) - - return vars(values), leftovers + return vars(self._oparser.parse_args(args)) class GroupAttr(collections.Mapping): @@ -1543,12 +1641,12 @@ class ConfigOpts(collections.Mapping): :param conf: a ConfigOpts object :param group: an OptGroup object """ - self.conf = conf - self.group = group + self._conf = conf + self._group = group def __getattr__(self, name): """Look up an option value and perform template substitution.""" - return self.conf._get(name, self.group) + return self._conf._get(name, self._group) def __getitem__(self, key): """Look up an option value and perform string substitution.""" @@ -1556,16 +1654,50 @@ class ConfigOpts(collections.Mapping): def __contains__(self, key): """Return True if key is the name of a registered opt or group.""" - return key in self.group._opts + return key in self._group._opts def __iter__(self): """Iterate over all registered opt and group names.""" - for key in self.group._opts.keys(): + for key in self._group._opts.keys(): yield key def __len__(self): """Return the number of options and option groups.""" - return len(self.group._opts) + return len(self._group._opts) + + class SubCommandAttr(object): + + """ + A helper class representing the name and arguments of an argparse + sub-parser. + """ + + def __init__(self, conf, group, dest): + """Construct a SubCommandAttr object. + + :param conf: a ConfigOpts object + :param group: an OptGroup object + :param dest: the name of the sub-parser + """ + self._conf = conf + self._group = group + self._dest = dest + + def __getattr__(self, name): + """Look up a sub-parser name or argument value.""" + if name == 'name': + name = self._dest + if self._group is not None: + name = self._group.name + '_' + name + return self._conf._cli_values[name] + + if name in self._conf: + raise DuplicateOptError(name) + + try: + return self._conf._cli_values[name] + except KeyError: + raise NoSuchOptError(name) class StrSubWrapper(object): @@ -1623,19 +1755,21 @@ class CommonConfigOpts(ConfigOpts): metavar='FORMAT', help='A logging.Formatter log message format string which may ' 'use any of the available logging.LogRecord attributes. ' - 'Default: %default'), + 'Default: %(default)s'), StrOpt('log-date-format', default=DEFAULT_LOG_DATE_FORMAT, metavar='DATE_FORMAT', - help='Format string for %(asctime)s in log records. ' - 'Default: %default'), + help='Format string for %%(asctime)s in log records. ' + 'Default: %(default)s'), StrOpt('log-file', metavar='PATH', + deprecated_name='logfile', help='(Optional) Name of log file to output to. ' 'If not set, logging will go to stdout.'), StrOpt('log-dir', + deprecated_name='logdir', help='(Optional) The directory to keep log files in ' - '(will be prepended to --logfile)'), + '(will be prepended to --log-file)'), BoolOpt('use-syslog', default=False, help='Use syslog for logging.'), diff --git a/quantum/plugins/linuxbridge/agent/linuxbridge_quantum_agent.py b/quantum/plugins/linuxbridge/agent/linuxbridge_quantum_agent.py index f5da45fd19..8f855b2eb6 100755 --- a/quantum/plugins/linuxbridge/agent/linuxbridge_quantum_agent.py +++ b/quantum/plugins/linuxbridge/agent/linuxbridge_quantum_agent.py @@ -612,7 +612,8 @@ class LinuxBridgeQuantumAgentRPC(sg_rpc.SecurityGroupAgentRpcMixin): def main(): eventlet.monkey_patch() - cfg.CONF(args=sys.argv, project='quantum') + cfg.CONF(project='quantum') + logging_config.setup_logging(cfg.CONF) try: interface_mappings = q_utils.parse_mappings( diff --git a/quantum/plugins/nec/agent/nec_quantum_agent.py b/quantum/plugins/nec/agent/nec_quantum_agent.py index b018b979e4..7e557fd4e7 100755 --- a/quantum/plugins/nec/agent/nec_quantum_agent.py +++ b/quantum/plugins/nec/agent/nec_quantum_agent.py @@ -109,7 +109,7 @@ class NECQuantumAgent(object): def main(): - config.CONF(args=sys.argv, project='quantum') + config.CONF(project='quantum') logging_config.setup_logging(config.CONF) diff --git a/quantum/plugins/openvswitch/agent/ovs_quantum_agent.py b/quantum/plugins/openvswitch/agent/ovs_quantum_agent.py index 57c59de345..233d6d3491 100755 --- a/quantum/plugins/openvswitch/agent/ovs_quantum_agent.py +++ b/quantum/plugins/openvswitch/agent/ovs_quantum_agent.py @@ -686,7 +686,7 @@ def create_agent_config_map(config): def main(): eventlet.monkey_patch() - cfg.CONF(args=sys.argv, project='quantum') + cfg.CONF(project='quantum') logging_config.setup_logging(cfg.CONF) try: diff --git a/quantum/plugins/ryu/agent/ryu_quantum_agent.py b/quantum/plugins/ryu/agent/ryu_quantum_agent.py index 5fd9bea2db..a5d9afe1d2 100755 --- a/quantum/plugins/ryu/agent/ryu_quantum_agent.py +++ b/quantum/plugins/ryu/agent/ryu_quantum_agent.py @@ -205,9 +205,8 @@ def check_ofp_rest_api_addr(db): def main(): - cfg.CONF(args=sys.argv, project='quantum') + cfg.CONF(project='quantum') - # (TODO) gary - swap with common logging logging_config.setup_logging(cfg.CONF) integ_br = cfg.CONF.OVS.integration_bridge diff --git a/quantum/server/__init__.py b/quantum/server/__init__.py index b1f47a677c..c9769f0c25 100755 --- a/quantum/server/__init__.py +++ b/quantum/server/__init__.py @@ -28,7 +28,7 @@ from quantum import service def main(): # the configuration will be read into the cfg.CONF global data structure - config.parse(sys.argv) + config.parse(sys.argv[1:]) if not cfg.CONF.config_file: sys.exit("ERROR: Unable to find configuration file via the default" " search paths (~/.quantum/, ~/, /etc/quantum/, /etc/) and" diff --git a/quantum/tests/unit/test_db_migration.py b/quantum/tests/unit/test_db_migration.py index 9c54115b2c..1667631c28 100644 --- a/quantum/tests/unit/test_db_migration.py +++ b/quantum/tests/unit/test_db_migration.py @@ -35,83 +35,87 @@ class TestDbMigration(unittest.TestCase): self.assertTrue(migration.should_run('foo', ['*'])) -class TestMain(unittest.TestCase): +class TestCli(unittest.TestCase): def setUp(self): - self.process_argv_p = mock.patch.object(cli, 'process_argv') - self.process_argv = self.process_argv_p.start() - - self.alembic_cmd_p = mock.patch.object(cli, 'alembic_command') - self.alembic_cmd = self.alembic_cmd_p.start() + self.do_alembic_cmd_p = mock.patch.object(cli, 'do_alembic_command') + self.do_alembic_cmd = self.do_alembic_cmd_p.start() def tearDown(self): - self.alembic_cmd_p.stop() - self.process_argv_p.stop() + self.do_alembic_cmd_p.stop() + cli.CONF.reset() - def test_main(self): - self.process_argv.return_value = ('foo', ('bar', ), {'baz': 1}) - cli.main() + def _main_test_helper(self, argv, func_name, exp_args=(), exp_kwargs={}): + with mock.patch.object(sys, 'argv', argv): + cli.main() + self.do_alembic_cmd.assert_has_calls( + [mock.call(mock.ANY, func_name, *exp_args, **exp_kwargs)] + ) - self.process_argv.assert_called_once_with(sys.argv) - self.alembic_cmd.foo.assert_called_once_with(mock.ANY, 'bar', baz=1) + def test_stamp(self): + self._main_test_helper( + ['prog', 'stamp', 'foo'], + 'stamp', + ('foo',), + {'sql': False} + ) + self._main_test_helper( + ['prog', 'stamp', 'foo', '--sql'], + 'stamp', + ('foo',), + {'sql': True} + ) -class TestDatabaseSync(unittest.TestCase): - def test_process_argv_stamp(self): - self.assertEqual( - ('stamp', ('foo',), {'sql': False}), - cli.process_argv(['prog', 'stamp', 'foo'])) + def test_current(self): + self._main_test_helper(['prog', 'current'], 'current') - self.assertEqual( - ('stamp', ('foo',), {'sql': True}), - cli.process_argv(['prog', 'stamp', '--sql', 'foo'])) + def test_history(self): + self._main_test_helper(['prog', 'history'], 'history') - def test_process_argv_current(self): - self.assertEqual( - ('current', (), {}), - cli.process_argv(['prog', 'current'])) - - def test_process_argv_history(self): - self.assertEqual( - ('history', (), {}), - cli.process_argv(['prog', 'history'])) - - def test_process_argv_check_migration(self): - self.assertEqual( - ('branches', (), {}), - cli.process_argv(['prog', 'check_migration'])) + def test_check_migration(self): + self._main_test_helper(['prog', 'check_migration'], 'branches') def test_database_sync_revision(self): - expected = ( + self._main_test_helper( + ['prog', 'revision', '--autogenerate', '-m', 'message'], 'revision', (), {'message': 'message', 'sql': False, 'autogenerate': True} ) - self.assertEqual( - cli.process_argv( - ['prog', 'revision', '-m', 'message', '--autogenerate'] - ), - expected + self._main_test_helper( + ['prog', 'revision', '--sql', '-m', 'message'], + 'revision', + (), + {'message': 'message', 'sql': True, 'autogenerate': False} ) - def test_database_sync_upgrade(self): - self.assertEqual( - cli.process_argv(['prog', 'upgrade', 'head']), - ('upgrade', ('head', ), {'sql': False}) + def test_upgrade(self): + self._main_test_helper( + ['prog', 'upgrade', '--sql', 'head'], + 'upgrade', + ('head',), + {'sql': True} ) - self.assertEqual( - cli.process_argv(['prog', 'upgrade', '--delta', '3']), - ('upgrade', ('+3', ), {'sql': False}) + self._main_test_helper( + ['prog', 'upgrade', '--delta', '3'], + 'upgrade', + ('+3',), + {'sql': False} ) - def test_database_sync_downgrade(self): - self.assertEqual( - cli.process_argv(['prog', 'downgrade', 'folsom']), - ('downgrade', ('folsom', ), {'sql': False}) + def test_downgrade(self): + self._main_test_helper( + ['prog', 'downgrade', '--sql', 'folsom'], + 'downgrade', + ('folsom',), + {'sql': True} ) - self.assertEqual( - cli.process_argv(['prog', 'downgrade', '--delta', '2']), - ('downgrade', ('-2', ), {'sql': False}) + self._main_test_helper( + ['prog', 'downgrade', '--delta', '2'], + 'downgrade', + ('-2',), + {'sql': False} ) diff --git a/quantum/tests/unit/test_debug_commands.py b/quantum/tests/unit/test_debug_commands.py index 2927662351..8fa505a68e 100644 --- a/quantum/tests/unit/test_debug_commands.py +++ b/quantum/tests/unit/test_debug_commands.py @@ -40,7 +40,7 @@ class TestDebugCommands(unittest.TestCase): def setUp(self): cfg.CONF.register_opts(interface.OPTS) cfg.CONF.register_opts(QuantumDebugAgent.OPTS) - cfg.CONF(args=['quantum-debug'], project='quantum') + cfg.CONF(args=[], project='quantum') cfg.CONF.set_override('use_namespaces', True) cfg.CONF.root_helper = 'sudo' diff --git a/quantum/tests/unit/test_dhcp_agent.py b/quantum/tests/unit/test_dhcp_agent.py index 9ace0ed155..8674b2ad6b 100644 --- a/quantum/tests/unit/test_dhcp_agent.py +++ b/quantum/tests/unit/test_dhcp_agent.py @@ -15,7 +15,9 @@ # License for the specific language governing permissions and limitations # under the License. +import os import socket +import sys import uuid import mock @@ -29,6 +31,14 @@ from quantum.openstack.common import cfg from quantum.openstack.common import jsonutils +ROOTDIR = os.path.dirname(os.path.dirname(__file__)) +ETCDIR = os.path.join(ROOTDIR, 'etc') + + +def etcdir(*p): + return os.path.join(ETCDIR, *p) + + class FakeModel: def __init__(self, id_, **kwargs): self.id = id_ @@ -95,12 +105,12 @@ class TestDhcpAgent(unittest.TestCase): logging_str = 'quantum.agent.common.config.setup_logging' manager_str = 'quantum.agent.dhcp_agent.DeviceManager' agent_str = 'quantum.agent.dhcp_agent.DhcpAgent' - agent_sys_str = 'quantum.agent.dhcp_agent.sys' with mock.patch(logging_str): with mock.patch(manager_str) as dev_mgr: with mock.patch(agent_str) as dhcp: - with mock.patch(agent_sys_str) as mock_sys: - mock_sys.argv = [] + with mock.patch.object(sys, 'argv') as sys_argv: + sys_argv.return_value = ['dhcp', '--config-file', + etcdir('quantum.conf.test')] dhcp_agent.main() dev_mgr.assert_called_once(mock.ANY, 'sudo') dhcp.assert_has_calls([ diff --git a/tools/pip-requires b/tools/pip-requires index 604a65ae5b..ff31a51410 100644 --- a/tools/pip-requires +++ b/tools/pip-requires @@ -3,6 +3,7 @@ PasteDeploy==1.5.0 Routes>=1.12.3 amqplib==0.6.1 anyjson>=0.2.4 +argparse eventlet>=0.9.17 greenlet>=0.3.1 httplib2