From 63049a81dc0f9cbfa7fe2fca6ca875f96aad359d Mon Sep 17 00:00:00 2001 From: Michael Fenn Date: Mon, 7 Jul 2014 11:41:17 -0400 Subject: Change fam_blocking default to True Based on discussion in #bcfg2, the consensus seems to be that the behavior provided by fam_blocking = True is the least surprising of the two options (i.e. the server should not process data until it is ready). 1.4 seems like a good time to make this change. --- doc/man/bcfg2.conf.txt | 2 +- man/bcfg2.conf.5 | 2 +- src/lib/Bcfg2/Server/Core.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/man/bcfg2.conf.txt b/doc/man/bcfg2.conf.txt index 36776b5cb..9e5da3eb9 100644 --- a/doc/man/bcfg2.conf.txt +++ b/doc/man/bcfg2.conf.txt @@ -49,7 +49,7 @@ fam_blocking Whether the server should block at startup until the file monitor backend has processed all events. This can cause a slower startup, but ensure that all files are recognized before the first client - is handled. + is handled. Defaults to True. ignore_files A comma-separated list of globs that should be ignored by the file diff --git a/man/bcfg2.conf.5 b/man/bcfg2.conf.5 index b9642b334..851f5527d 100644 --- a/man/bcfg2.conf.5 +++ b/man/bcfg2.conf.5 @@ -79,7 +79,7 @@ pseudo Whether the server should block at startup until the file monitor backend has processed all events. This can cause a slower startup, but ensure that all files are recognized before the first client -is handled. +is handled. Defaults to True. .TP .B ignore_files A comma\-separated list of globs that should be ignored by the file diff --git a/src/lib/Bcfg2/Server/Core.py b/src/lib/Bcfg2/Server/Core.py index 544f417b4..4b2f5da0e 100644 --- a/src/lib/Bcfg2/Server/Core.py +++ b/src/lib/Bcfg2/Server/Core.py @@ -131,7 +131,7 @@ class Core(object): Bcfg2.Options.Common.repository, Bcfg2.Options.Common.filemonitor, Bcfg2.Options.BooleanOption( - cf=('server', 'fam_blocking'), default=False, + cf=('server', 'fam_blocking'), default=True, help='FAM blocks on startup until all events are processed'), Bcfg2.Options.BooleanOption( cf=('logging', 'performance'), dest="perflog", -- cgit v1.2.3-1-g7c22 From 439ef74041e22c7ecfdbd15a41fb7336089bb0d2 Mon Sep 17 00:00:00 2001 From: Michael Fenn Date: Mon, 7 Jul 2014 15:46:55 -0400 Subject: Add a --no-fam-blocking command-line argument to bcfg2-server There is a quirk in the configuration parser where a BooleanOption with default=True will always return false __unless_ a command-line option is provided. Not sure why that's the case, but this is a work- around --- doc/man/bcfg2-server.txt | 5 +++-- man/bcfg2-server.8 | 7 +++++-- src/lib/Bcfg2/Server/Core.py | 3 ++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/doc/man/bcfg2-server.txt b/doc/man/bcfg2-server.txt index 3f8f3ea21..f85964ae7 100644 --- a/doc/man/bcfg2-server.txt +++ b/doc/man/bcfg2-server.txt @@ -11,7 +11,7 @@ Synopsis **bcfg2-server** [-d] [-v] [-C *configfile*] [-D *pidfile*] [-E *encoding*] [-Q *repo path*] [-S *server url*] [-o *logfile*] [-x -*password*] [--ssl-key=\ *ssl key*] +*password*] [--ssl-key=\ *ssl key*] [--no-fam-blocking] Description ----------- @@ -33,8 +33,9 @@ Options -v Run in verbose mode. -h Print usage information. --ssl-key=key Specify the path to the SSL key. +--no-fam-blocking Synonym for fam_blocking = False in bcfg2.conf See Also -------- -:manpage:`bcfg2(1)`, :manpage:`bcfg2-lint(8)` +:manpage:`bcfg2(1)`, :manpage:`bcfg2-lint(8)`, :manpage:`bcfg2.conf(5)` diff --git a/man/bcfg2-server.8 b/man/bcfg2-server.8 index dcec03252..60fe58a30 100644 --- a/man/bcfg2-server.8 +++ b/man/bcfg2-server.8 @@ -34,7 +34,7 @@ level margin: \\n[rst2man-indent\\n[rst2man-indent-level]] .sp \fBbcfg2\-server\fP [\-d] [\-v] [\-C \fIconfigfile\fP] [\-D \fIpidfile\fP] [\-E \fIencoding\fP] [\-Q \fIrepo path\fP] [\-S \fIserver url\fP] [\-o \fIlogfile\fP] [\-x -\fIpassword\fP] [\-\-ssl\-key=\fIssl key\fP] +\fIpassword\fP] [\-\-ssl\-key=\fIssl key\fP] [\-\-no\-fam\-blocking] .SH DESCRIPTION .sp \fBbcfg2\-server\fP is the daemon component of Bcfg2 which serves @@ -70,9 +70,12 @@ Print usage information. .TP .BI \-\-ssl\-key\fB= key Specify the path to the SSL key. +.TP +.BI \-\-no\-fam\-blocking +Synonym for fam_blocking = False in bcfg2.conf .UNINDENT .SH SEE ALSO .sp -\fIbcfg2(1)\fP, \fIbcfg2\-lint(8)\fP +\fIbcfg2(1)\fP, \fIbcfg2\-lint(8)\fP, \fIbcfg2.conf(5)\fP .\" Generated by docutils manpage writer. . diff --git a/src/lib/Bcfg2/Server/Core.py b/src/lib/Bcfg2/Server/Core.py index 4b2f5da0e..a2f9499f2 100644 --- a/src/lib/Bcfg2/Server/Core.py +++ b/src/lib/Bcfg2/Server/Core.py @@ -131,7 +131,8 @@ class Core(object): Bcfg2.Options.Common.repository, Bcfg2.Options.Common.filemonitor, Bcfg2.Options.BooleanOption( - cf=('server', 'fam_blocking'), default=True, + "--no-fam-blocking", cf=('server', 'fam_blocking'), + dest="fam_blocking", default=True, help='FAM blocks on startup until all events are processed'), Bcfg2.Options.BooleanOption( cf=('logging', 'performance'), dest="perflog", -- cgit v1.2.3-1-g7c22 From c0a68b9262e32fd4c881d4634c6dc3d9a9a61121 Mon Sep 17 00:00:00 2001 From: Michael Fenn Date: Mon, 14 Jul 2014 16:15:41 -0400 Subject: Correctly handle config-file-only BooleanOptions which default to True In Bcfg2.Options.Parser._parse_config_options, if the "default" of the option (perhaps set by the config file) evaluates to True, then it runs the actions associated with the option. Otherwise, it just sets the option to the default directly with setattr. Since the default of a store_false action is True, the code will run argparse's _StoreFalseAction function. Well, _StoreFalseAction, is a subclass of _StoreConstAction where const=False. _StoreConstAction has a call method which looks like this: def __call__(self, parser, namespace, values, option_string=None): setattr(namespace, self.dest, self.const) So it completely ignores the value passed in and just sets the value of the option to const (i.e. False). Looking at fam_blocking {None: _StoreFalseAction(option_strings='fam_blocking', dest='fam_blocking', nargs=0, const=False, default=True, type=None, choices=None, help='FAM blocks on startup until all events are processed', metavar=None)} start phase 3 fam_blocking config val is True start phase 3 _parse_config_options: fam_blocking default is True _parse_config_options: calling argparse. with value True on fam_blocking _parse_config_options: after calling argparse. fam_blocking is False after _parse_config_options fam_blocking is False after phase 3 fam_blocking is False after phase 4 fam_blocking is False end of parser fam_blocking is False CLI init fam_blocking is False So how to fix it? Define a new Action? That seems like the most direct approach since the problem really is that _StoreFalseAction does what it says on the tin, it stores false no matter what. The new action BooleanOptionAction works like store_true and store_false, except that it stores the value that was passed in, or the default if there was no value passed in. --- src/lib/Bcfg2/Options/Options.py | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/lib/Bcfg2/Options/Options.py b/src/lib/Bcfg2/Options/Options.py index 11388fb4d..fcc76dca5 100644 --- a/src/lib/Bcfg2/Options/Options.py +++ b/src/lib/Bcfg2/Options/Options.py @@ -306,6 +306,26 @@ class PathOption(Option): Option.__init__(self, *args, **kwargs) +class _BooleanOptionAction(argparse.Action): + """ BooleanOptionAction sets a boolean value in the following ways: + - if None is passed, store the default + - if the option_string is not None, then the option was passed on the + command line, thus store the opposite of the default (this is the + argparse store_true and store_false behavior) + - if a boolean value is passed, use that + + Defined here instead of :mod:`Bcfg2.Options.Actions` because otherwise + there is a circular import Options -> Actions -> Parser -> Options """ + + def __call__(self, parser, namespace, values, option_string=None): + if values is None: + setattr(namespace, self.dest, self.default) + elif option_string is not None: + setattr(namespace, self.dest, not self.default) + else: + setattr(namespace, self.dest, bool(values)) + + class BooleanOption(Option): """ Shortcut for boolean options. The default is False, but this can easily be overridden: @@ -317,11 +337,12 @@ class BooleanOption(Option): "--dwim", default=True, help="Do What I Mean")] """ def __init__(self, *args, **kwargs): - if 'default' in kwargs and kwargs['default']: - kwargs.setdefault('action', 'store_false') - else: - kwargs.setdefault('action', 'store_true') + kwargs.setdefault('action', _BooleanOptionAction) + kwargs.setdefault('nargs', 0) + + if 'default' not in kwargs: kwargs.setdefault('default', False) + Option.__init__(self, *args, **kwargs) -- cgit v1.2.3-1-g7c22 From c76fb188215208fcfc5af9027f16447855a6f064 Mon Sep 17 00:00:00 2001 From: Michael Fenn Date: Tue, 15 Jul 2014 10:45:07 -0400 Subject: Use setdefault instead of a conditional --- src/lib/Bcfg2/Options/Options.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/lib/Bcfg2/Options/Options.py b/src/lib/Bcfg2/Options/Options.py index fcc76dca5..3874f810d 100644 --- a/src/lib/Bcfg2/Options/Options.py +++ b/src/lib/Bcfg2/Options/Options.py @@ -339,9 +339,7 @@ class BooleanOption(Option): def __init__(self, *args, **kwargs): kwargs.setdefault('action', _BooleanOptionAction) kwargs.setdefault('nargs', 0) - - if 'default' not in kwargs: - kwargs.setdefault('default', False) + kwargs.setdefault('default', False) Option.__init__(self, *args, **kwargs) -- cgit v1.2.3-1-g7c22