On 01/04/2013 02:43 PM, Petr Viktorin wrote:
On 01/03/2013 02:56 PM, John Dennis wrote:
On 01/03/2013 08:00 AM, Petr Viktorin wrote:
Hello,
The first patch implements logging-related changes to the admintool
framework and ipa-ldap-updater (the only thing ported to it so far).
The design document is at http://freeipa.org/page/V3/Logging_and_output
John, I decided to go ahead and put an explicit "logger" attribute on
the tool class rather than adding debug, info, warn. etc methods
dynamically using log_mgr.get_logger. I believe it's the cleanest
solution.
We had a discussion about this in this thread:
https://www.redhat.com/archives/freeipa-devel/2012-July/msg00223.html; I
didn't get a reaction to my conclusion so I'm letting you know in case
you have more to say.
I'm fine with not directly adding the debug, info, warn etc. methods,
that practice was historical dating back to the days of Jason. However I
do think it's useful to use a named logger and not the global
root_logger. I'd prefer we got away from using the root_logger, it's
continued existence is historical as well and the idea was over time we
would slowly eliminate it's usage. FWIW the log_mgr.get_logger() is
still useful for what you want to do.
def get_logger(self, who, bind_logger_names=False)
If you don't set bind_logger_names to True (and pass the class instance
as who) you won't get the offensive debug, info, etc. methods added to
the class instance. But it still does all the other bookeeping.
The 'who' in this instance could be either the name of the admin tool or
the class instance.
Also I'd prefer using the attribute 'log' rather than 'logger'. That
would make it consistent with code which does already use get_logger()
passing a class instance because it's adds a 'log' attribute which is
the logger. Also 'log' is twice as succinct than 'logger' (shorter line
lengths).
Thus if you do:
log_mgr.get_logger(self)
I think you'll get exactly what you want. A logger named for the class
and being able to say
self.log.debug()
self.log.error()
inside the class.
In summary, just drop the True from the get_logger() call.
Thanks! Yes, this works better. Updated patches attached.
Here is patch 117 rebased to current master.
--
Petr³
From 764e2c2f295f056570cf30ce2553024e5d5e2a1a Mon Sep 17 00:00:00 2001
From: Petr Viktorin <[email protected]>
Date: Tue, 18 Dec 2012 06:24:04 -0500
Subject: [PATCH] Better logging for AdminTool and ipa-ldap-updater
- Automatically add a "Logging and output options" group with the --quiet,
--verbose, --log-file options.
- Set up logging based on these options; details are in the setup_logging
docstring and in the design document.
- Don't bind log methods as individual methods of the class. This means one
less linter exception.
- Make the help for command line options consistent with optparse's --help and
--version options.
Design document: http://freeipa.org/page/V3/Logging_and_output
---
freeipa.spec.in | 5 +-
ipapython/admintool.py | 120 ++++++++++++++++++++++++--------
ipaserver/install/ipa_ldap_updater.py | 48 ++++++-------
make-lint | 1 -
4 files changed, 116 insertions(+), 58 deletions(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 189c7b9220ee1b3f47bc37078f7c80922922a8ad..e17a626b86962a88f1561ef64f667af579cc2430 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -438,7 +438,7 @@ fi
%posttrans server
# This must be run in posttrans so that updates from previous
# execution that may no longer be shipped are not applied.
-/usr/sbin/ipa-ldap-updater --upgrade >/dev/null || :
+/usr/sbin/ipa-ldap-updater --upgrade --quiet || :
%preun server
if [ $1 = 0 ]; then
@@ -769,6 +769,9 @@ fi
%ghost %attr(0644,root,apache) %config(noreplace) %{_sysconfdir}/ipa/ca.crt
%changelog
+* Thu Jan 24 2013 Petr Viktorin <[email protected]> - 3.0.99-13
+- Use ipa-ldap-updater --quiet instead of redirecting to /dev/null
+
* Thu Jan 24 2013 Rob Crittenden <[email protected]> - 3.0.99-12
- Add certmonger condrestart to server post scriptlet
- Make certmonger a (pre) Requires on the server subpackage
diff --git a/ipapython/admintool.py b/ipapython/admintool.py
index b644516dd2afd0d373734767feee4d65b3fbfd5b..5bc21516562ac2a8c6790ea3421628a4c7db9797 100644
--- a/ipapython/admintool.py
+++ b/ipapython/admintool.py
@@ -76,14 +76,15 @@ class AdminTool(object):
Class attributes to define in subclasses:
command_name - shown in logs
log_file_name - if None, logging is to stderr only
- needs_root - if true, non-root users can't run the tool
usage - text shown in help
+
+ See the setup_logging method for more info on logging.
"""
command_name = None
log_file_name = None
- needs_root = False
usage = None
+ log = None
_option_parsers = dict()
@classmethod
@@ -95,10 +96,23 @@ class AdminTool(object):
cls.add_options(parser)
@classmethod
- def add_options(cls, parser):
- """Add command-specific options to the option parser"""
- parser.add_option("-d", "--debug", dest="debug", default=False,
+ def add_options(cls, parser, debug_option=False):
+ """Add command-specific options to the option parser
+
+ :param parser: The parser to add options to
+ :param debug_option: Add a --debug option as an alias to --verbose
+ """
+ group = OptionGroup(parser, "Logging and output options")
+ group.add_option("-v", "--verbose", dest="verbose", default=False,
action="store_true", help="print debugging information")
+ if debug_option:
+ group.add_option("-d", "--debug", dest="verbose", default=False,
+ action="store_true", help="alias for --verbose (deprecated)")
+ group.add_option("-q", "--quiet", dest="quiet", default=False,
+ action="store_true", help="output only errors")
+ group.add_option("--log-file", dest="log_file", default=None,
+ metavar="FILE", help="log to the given file")
+ parser.add_option_group(group)
@classmethod
def run_cli(cls):
@@ -145,6 +159,8 @@ class AdminTool(object):
This includes validating options, setting up logging, doing the
actual work, and handling the result.
"""
+ self._setup_logging(no_file=True)
+ return_value = 1
try:
self.validate_options()
self.ask_for_options()
@@ -160,29 +176,85 @@ class AdminTool(object):
self.log_success()
return return_value
- def validate_options(self):
+ def validate_options(self, needs_root=False):
"""Validate self.options
It's also possible to compute and store information that will be
useful later, but no changes to the system should be made here.
"""
- if self.needs_root and os.getegid() != 0:
+ if needs_root and os.getegid() != 0:
raise ScriptError('Must be root to run %s' % self.command_name, 1)
+ if self.options.verbose and self.options.quiet:
+ raise ScriptError(
+ 'The --quiet and --verbose options are mutually exclusive')
def ask_for_options(self):
"""Ask for missing options interactively
Similar to validate_options. This is separate method because we want
any validation errors to abort the script before bothering the user
with prompts.
+
+ Any options that might be asked for should also be validated here.
"""
pass
- def setup_logging(self):
- """Set up logging"""
+ def setup_logging(self, log_file_mode='w'):
+ """Set up logging
+
+ :param _to_file: Setting this to false will disable logging to file.
+ For internal use.
+
+ If the --log-file option was given or if a filename is in
+ self.log_file_name, the tool will log to that file. In this case,
+ all messages are logged.
+
+ What is logged to the console depends on command-line options:
+ the default is INFO; --quiet sets ERROR; --verbose sets DEBUG.
+
+ Rules of thumb for logging levels:
+ - CRITICAL for fatal errors
+ - ERROR for critical things that the admin must see, even with --quiet
+ - WARNING for things that need to stand out in the log
+ - INFO to display normal messages
+ - DEBUG to spam about everything the program does
+ - a plain print for things that should not be log (for example,
+ interactive prompting)
+
+ To log, use `self.log.info()`, `self.log.warning()`, etc.
+
+ Logging to file is only set up after option validation and prompting;
+ before that, all output will go to the console only.
+ """
+ self._setup_logging(log_file_mode=log_file_mode)
+
+ def _setup_logging(self, log_file_mode='w', no_file=False):
+ if no_file:
+ log_file_name = None
+ elif self.options.log_file:
+ log_file_name = self.options.log_file
+ else:
+ log_file_name = self.log_file_name
+ if self.options.verbose:
+ console_format = '%(name)s: %(levelname)s: %(message)s'
+ verbose = True
+ debug = True
+ else:
+ console_format = '%(message)s'
+ debug = False
+ if self.options.quiet:
+ verbose = False
+ else:
+ verbose = True
ipa_log_manager.standard_logging_setup(
- self.log_file_name, debug=self.options.debug)
- ipa_log_manager.log_mgr.get_logger(self, True)
+ log_file_name, console_format=console_format,
+ filemode=log_file_mode, debug=debug, verbose=verbose)
+ self.log = ipa_log_manager.log_mgr.get_logger(self)
+ if log_file_name:
+ self.log.debug('Logging to %s' % log_file_name)
+ elif not no_file:
+ self.log.debug('Not logging to a file')
+
def handle_error(self, exception):
"""Given an exception, return a message (or None) and process exit code
@@ -206,27 +278,15 @@ class AdminTool(object):
assumed to have run successfully, and the return value is used as the
SystemExit code.
"""
- self.debug('%s was invoked with arguments %s and options: %s',
+ self.log.debug('%s was invoked with arguments %s and options: %s',
self.command_name, self.args, self.safe_options)
def log_failure(self, error_message, return_value, exception, backtrace):
- try:
- self.log
- except AttributeError:
- # Logging was not set up yet
- if error_message:
- print >> sys.stderr, '\n', error_message
- else:
- self.info(''.join(traceback.format_tb(backtrace)))
- self.info('The %s command failed, exception: %s: %s',
- self.command_name, type(exception).__name__, exception)
- if error_message:
- self.error(error_message)
+ self.log.debug(''.join(traceback.format_tb(backtrace)))
+ self.log.debug('The %s command failed, exception: %s: %s',
+ self.command_name, type(exception).__name__, exception)
+ if error_message:
+ self.log.error(error_message)
def log_success(self):
- try:
- self.log
- except AttributeError:
- pass
- else:
- self.info('The %s command was successful', self.command_name)
+ self.log.info('The %s command was successful', self.command_name)
diff --git a/ipaserver/install/ipa_ldap_updater.py b/ipaserver/install/ipa_ldap_updater.py
index d9f68092752debcd4e5a3a70878aceb7215aae30..df409ebb6cc796569333406331bd66dc688ed64f 100644
--- a/ipaserver/install/ipa_ldap_updater.py
+++ b/ipaserver/install/ipa_ldap_updater.py
@@ -34,7 +34,6 @@ from ipapython import ipautil, admintool
from ipaserver.install import installutils
from ipaserver.install.ldapupdate import LDAPUpdate, UPDATES_DIR
from ipaserver.install.upgradeinstance import IPAUpgrade
-from ipapython import ipa_log_manager
class LDAPUpdater(admintool.AdminTool):
@@ -45,37 +44,37 @@ class LDAPUpdater(admintool.AdminTool):
@classmethod
def add_options(cls, parser):
- super(LDAPUpdater, cls).add_options(parser)
+ super(LDAPUpdater, cls).add_options(parser, debug_option=True)
parser.add_option("-t", "--test", action="store_true", dest="test",
default=False,
- help="Run through the update without changing anything")
+ help="run through the update without changing anything")
parser.add_option("-y", dest="password",
- help="File containing the Directory Manager password")
+ help="file containing the Directory Manager password")
parser.add_option("-l", '--ldapi', action="store_true", dest="ldapi",
default=False,
- help="Connect to the LDAP server using the ldapi socket")
+ help="connect to the LDAP server using the ldapi socket")
parser.add_option("-u", '--upgrade', action="store_true",
dest="upgrade", default=False,
- help="Upgrade an installed server in offline mode")
+ help="upgrade an installed server in offline mode")
parser.add_option("-p", '--plugins', action="store_true",
dest="plugins", default=False,
- help="Execute update plugins. " +
- "Always true when applying all update files.")
+ help="execute update plugins " +
+ "(implied when no input files are given)")
parser.add_option("-W", '--password', action="store_true",
dest="ask_password",
- help="Prompt for the Directory Manager password")
+ help="prompt for the Directory Manager password")
@classmethod
def get_command_class(cls, options, args):
if options.upgrade:
return LDAPUpdater_Upgrade
else:
return LDAPUpdater_NonUpgrade
- def validate_options(self):
+ def validate_options(self, **kwargs):
options = self.options
- super(LDAPUpdater, self).validate_options()
+ super(LDAPUpdater, self).validate_options(**kwargs)
self.files = self.args
@@ -100,34 +99,26 @@ class LDAPUpdater(admintool.AdminTool):
self.dirman_password = None
def setup_logging(self):
- ipa_log_manager.standard_logging_setup(self.log_file_name,
- console_format='%(levelname)s: %(message)s',
- debug=self.options.debug, filemode='a')
- ipa_log_manager.log_mgr.get_logger(self, True)
+ super(LDAPUpdater, self).setup_logging(log_file_mode='a')
def run(self):
super(LDAPUpdater, self).run()
- api.bootstrap(
- in_server=True,
- context='updates',
- debug=self.options.debug,
- )
+ api.bootstrap(in_server=True, context='updates')
api.finalize()
def handle_error(self, exception):
return installutils.handle_error(exception, self.log_file_name)
class LDAPUpdater_Upgrade(LDAPUpdater):
- needs_root = True
log_file_name = '/var/log/ipaupgrade.log'
def validate_options(self):
if os.getegid() != 0:
raise admintool.ScriptError('Must be root to do an upgrade.', 1)
- super(LDAPUpdater_Upgrade, self).validate_options()
+ super(LDAPUpdater_Upgrade, self).validate_options(needs_root=True)
def run(self):
super(LDAPUpdater_Upgrade, self).run()
@@ -145,7 +136,7 @@ class LDAPUpdater_Upgrade(LDAPUpdater):
elif upgrade.upgradefailed:
raise admintool.ScriptError('IPA upgrade failed.', 1)
elif upgrade.modified and options.test:
- self.info('Update complete, changes to be made, test mode')
+ self.log.info('Update complete, changes to be made, test mode')
return 2
@@ -160,8 +151,13 @@ class LDAPUpdater_NonUpgrade(LDAPUpdater):
self.run_plugins = not self.files or options.plugins
# Need root for running plugins
- if self.run_plugins and os.getegid() != 0:
- raise admintool.ScriptError('Plugins can only be run as root.', 1)
+ if os.getegid() != 0:
+ if self.run_plugins:
+ raise admintool.ScriptError(
+ 'Plugins can only be run as root.', 1)
+ else:
+ # Can't log to the default file as non-root
+ self.log_file_name = None
def ask_for_options(self):
super(LDAPUpdater_NonUpgrade, self).ask_for_options()
@@ -192,5 +188,5 @@ class LDAPUpdater_NonUpgrade(LDAPUpdater):
modified = ld.update(self.files)
if modified and options.test:
- self.info('Update complete, changes to be made, test mode')
+ self.log.info('Update complete, changes to be made, test mode')
return 2
diff --git a/make-lint b/make-lint
index ae09e2a1b2a6b75ef18c8a8375651dcaa9292225..61be8ed5b0d342a8beba4b2c0bc71ceff747b5f0 100755
--- a/make-lint
+++ b/make-lint
@@ -78,7 +78,6 @@ class IPATypeChecker(TypeChecker):
'ipalib.session.SessionManager' : ['log', 'debug', 'info', 'warning', 'error', 'critical', 'exception'],
'ipalib.session.SessionCCache' : ['log', 'debug', 'info', 'warning', 'error', 'critical', 'exception'],
'ipalib.session.MemcacheSessionManager' : ['log', 'debug', 'info', 'warning', 'error', 'critical', 'exception'],
- 'ipapython.admintool.AdminTool' : ['log', 'debug', 'info', 'warning', 'error', 'critical', 'exception'],
'ipapython.cookie.Cookie' : ['log', 'debug', 'info', 'warning', 'error', 'critical', 'exception'],
}
--
1.7.7.6
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
