Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 04/18/2012 12:38 AM, Dmitri Pal wrote: On 04/17/2012 01:13 PM, Petr Viktorin wrote: On 04/17/2012 06:46 PM, John Dennis wrote: Thank you for the explanation Petr, it's very much appreciated. I do have a problem with this patch and I'm inclined to NAK it, but I'm open to discussion. Here's my thoughts, if I've made mistakes in my reasoning please point them out. The fundamental problem is many of our command line utilities do not do logging correctly. Fixing the logging is not terribly difficult but it raises concerns over invasive changes at the last minute. To address the problem we're going to introduce what can only be called a hack to compensate for the original deficiency. The hack is a bit obscure and convoluted (and I'm not sure entirely robust). It introduces enough complexity it's not obvious or easy to see what is going on. Code that obscures should be treated with skepticism and be justified by important needs. I'm also afraid what should really be a short term work-around will get ensconced in the code and never cleaned up, it will be duplicated, and used as an example of how things are supposed to work. So my question is, Is the output of the command line utilities so broken that it justifies introducing this? and Is this any less invasive than simply fixing the messages in the utilities cleanly and not introducing a hack? Yes, it's a hack, because it needs to be non-invasive. It does that by not modifying the scripts themselves, but just wrapping them in the context handler. So no functionality is broken, there are just problems with extra messages printed or not printed. I think that's the least invasive thing to do. So the answer to your second question is yes. I'm not experienced enough to answer the first one. I opened https://fedorahosted.org/freeipa/ticket/2652 to track the larger issue that needs solving: removing the code duplication in these tools. This includes a common way to configure logging and report errors. Let us do the hack and pick the cleanup task in 3.0 so that we have things done correctly for future. If this task is not enough let us open other tasks to make sure that we track correctly the need to the right fix. We can even revert the change in 3.0 and go a different path. NACK We have agreed outside this list to drop the issue for 2.2, and fix it properly for 3.0. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 04/17/2012 12:12 AM, Rob Crittenden wrote: John Dennis wrote: On 04/16/2012 04:15 PM, Rob Crittenden wrote: John Dennis wrote: On 04/16/2012 01:31 PM, Rob Crittenden wrote: John Dennis wrote: On 04/13/2012 06:25 AM, Petr Viktorin wrote: When the utility sets logging to console, the extra log message gets printed out there. I agree this isn't optimal. Attached patch removes the console log handler before logging the result. I read through log_manager, and found that I can do this more cleanly with remove_handler. John, is this a good use of the API? The problem you're trying to correct is that under some circumstances a log message is emitted twice to the console, right? Removing the console handler fees like the wrong solution, it's a pretty big hammer, you have to know when to remove it and once removed any expectation that messages will appear on the console will be untrue. Can you give me a brief explanation as to why you're getting duplicate messages on the console. I wonder if there isn't a better way to handle the problem which isn't so invasive and potentially hidden. The patch adds a final log message that says if the command ultimately succeeded or not (https://fedorahosted.org/freeipa/ticket/2071). Some of the commands print an error message to the console independently. When they've configured logging to console, the added message appears as well. The right thing to do is to modify the utility just log once. But this patch treats the utilities as black boxes, otherwise it's too invasive. Or is the issue you don't want a console handler at all? If that's the case then maybe we should provide a configuration that does not create one, that way it will be explicit and obvious there is no console handler. This would mean changing how the commands set up logging. I plan to do that, but it's too invasive to do now. FWIW, the current configuration of logging is historical dating back to the beginning of the project. When I added the log manager the intent was to fix deficiencies in logging, not to modify the existing behavior. I wasn't clear to me the existing configuration was ideal. If you're hitting problems because of the existing configuration perhaps we should look at the historical configuration and ask if it needs to be modified. This patch is standardizing logging the final disposition of a number of commands. Currently is must be inferred. The problem is that some commands have had this disposition added but open no log files so some error messages are being displayed twice and in log format. I think the easiest solution would simply be to scale this back to only those commands that open a log file at all. You say command but command is a loaded word :-) I presume you mean command line utility and not an IPA Command object. The reason I'm drawing this distinction is because the environment Commands execute in are not supposed to change once api.bootstrap() has completed. Who or what is aggregating the final disposition of a number of commands? The reason I ask is because the log_mgr object is global and deleting a handler from a global resource to satisfy a local need may have unintentional global side effects. How is the aggregation accomplished? This is a wrapper around scripts. The final log message is the last thing done before exiting Python. This patch is in the context of the command-line utilities like ipa-server-install, ipa-client-install, etc. and not the ipa tool. Some of them initialize the api, some do not. Also there's ipa-ldap-updater, which sometimes opens a log file and sometimes not. This is exactly the type of problem I'm concerned about, a conflict over who owns the logging configuration. These multiple potential owners of a global configuration is what lead me to introduce the configure_state attribute of the log manager, to disambiguate who is in control and who initialized the configuration. In fact bootstrap() in plugable.py explicitly examines the configure_state attribute of the log_manager and if it's been previously configured (for example because the api was loaded by a command line utility that has configured it's own logging) it then defers to the logging configuration established by the environment it's being run in (in other words if a command line utility calls standard_logging_setup() before loading the api it wins and the logging configuration belongs to the utility, not the api). All of this is to say that isolated code that reaches into a global configuration and takes an axe to it and chops out a significant part of the configuration that someone else may have established without them knowing about it feels wrong. I suspect there is some deeper problem afoot and unilaterally chopping out the console handler is not addressing the fundamental problem and likely introduces a new one. But I really need to look at the specific instances of the problem, maybe if I dig through the history of this patch
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
There have been so many versions of the patch and various comments attached to it I'm afraid I'm still trying to wrap my head around what the actual problem is we're trying to solve, until I have that understanding I can't evaluate the proposed solution. Could you please state simply what the fundamental problem is the patch is trying to correct for and how it accomplishes it? When I tried to reach this understanding myself about the only thing I figured out was that some of our command line utilities pass a string to the exit() command whose output is out of band with respect to logging resulting in a duplicate message (however before application of the patch I don't see why logging would re-emit the message). -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 04/17/2012 04:12 PM, John Dennis wrote: There have been so many versions of the patch and various comments attached to it I'm afraid I'm still trying to wrap my head around what the actual problem is we're trying to solve, until I have that understanding I can't evaluate the proposed solution. Could you please state simply what the fundamental problem is the patch is trying to correct for and how it accomplishes it? The fundamental problem is in https://fedorahosted.org/freeipa/ticket/2071: the install tools need to put a final message in their logs, which states whether the command succeeded or not, and if not, what the error was. The patch needs to do this with minimal modifications to the scripts, so that it doesn't break any functionality. It accomplishes this by wrapping the scripts' top-level code in a context manager. When the script is done, the context manager looks whether an exception was raised and logs an appropriate message. The current problem is that some tools configure logging to the console, so this extra message ends up both in both the log file and on the console. The tools themselves also write their errors to the console (via print, SystemExit, or traceback), so the error appears twice. Since I do not want to add an extra message to the console, but only want it in a log file (if the tool uses one), I remove the console log handler before logging the message. When I tried to reach this understanding myself about the only thing I figured out was that some of our command line utilities pass a string to the exit() command whose output is out of band with respect to logging resulting in a duplicate message (however before application of the patch I don't see why logging would re-emit the message). Yes, that's exactly right. The output to console was fine, it should stay as it was before the patch. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
Thank you for the explanation Petr, it's very much appreciated. I do have a problem with this patch and I'm inclined to NAK it, but I'm open to discussion. Here's my thoughts, if I've made mistakes in my reasoning please point them out. The fundamental problem is many of our command line utilities do not do logging correctly. Fixing the logging is not terribly difficult but it raises concerns over invasive changes at the last minute. To address the problem we're going to introduce what can only be called a hack to compensate for the original deficiency. The hack is a bit obscure and convoluted (and I'm not sure entirely robust). It introduces enough complexity it's not obvious or easy to see what is going on. Code that obscures should be treated with skepticism and be justified by important needs. I'm also afraid what should really be a short term work-around will get ensconced in the code and never cleaned up, it will be duplicated, and used as an example of how things are supposed to work. So my question is, Is the output of the command line utilities so broken that it justifies introducing this? and Is this any less invasive than simply fixing the messages in the utilities cleanly and not introducing a hack? -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 04/17/2012 06:46 PM, John Dennis wrote: Thank you for the explanation Petr, it's very much appreciated. I do have a problem with this patch and I'm inclined to NAK it, but I'm open to discussion. Here's my thoughts, if I've made mistakes in my reasoning please point them out. The fundamental problem is many of our command line utilities do not do logging correctly. Fixing the logging is not terribly difficult but it raises concerns over invasive changes at the last minute. To address the problem we're going to introduce what can only be called a hack to compensate for the original deficiency. The hack is a bit obscure and convoluted (and I'm not sure entirely robust). It introduces enough complexity it's not obvious or easy to see what is going on. Code that obscures should be treated with skepticism and be justified by important needs. I'm also afraid what should really be a short term work-around will get ensconced in the code and never cleaned up, it will be duplicated, and used as an example of how things are supposed to work. So my question is, Is the output of the command line utilities so broken that it justifies introducing this? and Is this any less invasive than simply fixing the messages in the utilities cleanly and not introducing a hack? Yes, it's a hack, because it needs to be non-invasive. It does that by not modifying the scripts themselves, but just wrapping them in the context handler. So no functionality is broken, there are just problems with extra messages printed or not printed. I think that's the least invasive thing to do. So the answer to your second question is yes. I'm not experienced enough to answer the first one. I opened https://fedorahosted.org/freeipa/ticket/2652 to track the larger issue that needs solving: removing the code duplication in these tools. This includes a common way to configure logging and report errors. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 04/17/2012 01:13 PM, Petr Viktorin wrote: On 04/17/2012 06:46 PM, John Dennis wrote: Thank you for the explanation Petr, it's very much appreciated. I do have a problem with this patch and I'm inclined to NAK it, but I'm open to discussion. Here's my thoughts, if I've made mistakes in my reasoning please point them out. The fundamental problem is many of our command line utilities do not do logging correctly. Fixing the logging is not terribly difficult but it raises concerns over invasive changes at the last minute. To address the problem we're going to introduce what can only be called a hack to compensate for the original deficiency. The hack is a bit obscure and convoluted (and I'm not sure entirely robust). It introduces enough complexity it's not obvious or easy to see what is going on. Code that obscures should be treated with skepticism and be justified by important needs. I'm also afraid what should really be a short term work-around will get ensconced in the code and never cleaned up, it will be duplicated, and used as an example of how things are supposed to work. So my question is, Is the output of the command line utilities so broken that it justifies introducing this? and Is this any less invasive than simply fixing the messages in the utilities cleanly and not introducing a hack? Yes, it's a hack, because it needs to be non-invasive. It does that by not modifying the scripts themselves, but just wrapping them in the context handler. So no functionality is broken, there are just problems with extra messages printed or not printed. I think that's the least invasive thing to do. So the answer to your second question is yes. I'm not experienced enough to answer the first one. I opened https://fedorahosted.org/freeipa/ticket/2652 to track the larger issue that needs solving: removing the code duplication in these tools. This includes a common way to configure logging and report errors. Let us do the hack and pick the cleanup task in 3.0 so that we have things done correctly for future. If this task is not enough let us open other tasks to make sure that we track correctly the need to the right fix. We can even revert the change in 3.0 and go a different path. -- Thank you, Dmitri Pal Sr. Engineering Manager IPA project, Red Hat Inc. --- Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 04/13/2012 06:25 AM, Petr Viktorin wrote: When the utility sets logging to console, the extra log message gets printed out there. I agree this isn't optimal. Attached patch removes the console log handler before logging the result. I read through log_manager, and found that I can do this more cleanly with remove_handler. John, is this a good use of the API? The problem you're trying to correct is that under some circumstances a log message is emitted twice to the console, right? Removing the console handler fees like the wrong solution, it's a pretty big hammer, you have to know when to remove it and once removed any expectation that messages will appear on the console will be untrue. Can you give me a brief explanation as to why you're getting duplicate messages on the console. I wonder if there isn't a better way to handle the problem which isn't so invasive and potentially hidden. Or is the issue you don't want a console handler at all? If that's the case then maybe we should provide a configuration that does not create one, that way it will be explicit and obvious there is no console handler. FWIW, the current configuration of logging is historical dating back to the beginning of the project. When I added the log manager the intent was to fix deficiencies in logging, not to modify the existing behavior. I wasn't clear to me the existing configuration was ideal. If you're hitting problems because of the existing configuration perhaps we should look at the historical configuration and ask if it needs to be modified. -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
John Dennis wrote: On 04/13/2012 06:25 AM, Petr Viktorin wrote: When the utility sets logging to console, the extra log message gets printed out there. I agree this isn't optimal. Attached patch removes the console log handler before logging the result. I read through log_manager, and found that I can do this more cleanly with remove_handler. John, is this a good use of the API? The problem you're trying to correct is that under some circumstances a log message is emitted twice to the console, right? Removing the console handler fees like the wrong solution, it's a pretty big hammer, you have to know when to remove it and once removed any expectation that messages will appear on the console will be untrue. Can you give me a brief explanation as to why you're getting duplicate messages on the console. I wonder if there isn't a better way to handle the problem which isn't so invasive and potentially hidden. Or is the issue you don't want a console handler at all? If that's the case then maybe we should provide a configuration that does not create one, that way it will be explicit and obvious there is no console handler. FWIW, the current configuration of logging is historical dating back to the beginning of the project. When I added the log manager the intent was to fix deficiencies in logging, not to modify the existing behavior. I wasn't clear to me the existing configuration was ideal. If you're hitting problems because of the existing configuration perhaps we should look at the historical configuration and ask if it needs to be modified. This patch is standardizing logging the final disposition of a number of commands. Currently is must be inferred. The problem is that some commands have had this disposition added but open no log files so some error messages are being displayed twice and in log format. I think the easiest solution would simply be to scale this back to only those commands that open a log file at all. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
John Dennis wrote: On 04/16/2012 01:31 PM, Rob Crittenden wrote: John Dennis wrote: On 04/13/2012 06:25 AM, Petr Viktorin wrote: When the utility sets logging to console, the extra log message gets printed out there. I agree this isn't optimal. Attached patch removes the console log handler before logging the result. I read through log_manager, and found that I can do this more cleanly with remove_handler. John, is this a good use of the API? The problem you're trying to correct is that under some circumstances a log message is emitted twice to the console, right? Removing the console handler fees like the wrong solution, it's a pretty big hammer, you have to know when to remove it and once removed any expectation that messages will appear on the console will be untrue. Can you give me a brief explanation as to why you're getting duplicate messages on the console. I wonder if there isn't a better way to handle the problem which isn't so invasive and potentially hidden. Or is the issue you don't want a console handler at all? If that's the case then maybe we should provide a configuration that does not create one, that way it will be explicit and obvious there is no console handler. FWIW, the current configuration of logging is historical dating back to the beginning of the project. When I added the log manager the intent was to fix deficiencies in logging, not to modify the existing behavior. I wasn't clear to me the existing configuration was ideal. If you're hitting problems because of the existing configuration perhaps we should look at the historical configuration and ask if it needs to be modified. This patch is standardizing logging the final disposition of a number of commands. Currently is must be inferred. The problem is that some commands have had this disposition added but open no log files so some error messages are being displayed twice and in log format. I think the easiest solution would simply be to scale this back to only those commands that open a log file at all. You say command but command is a loaded word :-) I presume you mean command line utility and not an IPA Command object. The reason I'm drawing this distinction is because the environment Commands execute in are not supposed to change once api.bootstrap() has completed. Who or what is aggregating the final disposition of a number of commands? The reason I ask is because the log_mgr object is global and deleting a handler from a global resource to satisfy a local need may have unintentional global side effects. How is the aggregation accomplished? This patch is in the context of the command-line utilities like ipa-server-install, ipa-client-install, etc. and not the ipa tool. Some of them initialize the api, some do not. As I said, the intention of my original request was to log the disposition of an install command. I think this patch goes too far and updates all admin utilities. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 04/16/2012 04:15 PM, Rob Crittenden wrote: John Dennis wrote: On 04/16/2012 01:31 PM, Rob Crittenden wrote: John Dennis wrote: On 04/13/2012 06:25 AM, Petr Viktorin wrote: When the utility sets logging to console, the extra log message gets printed out there. I agree this isn't optimal. Attached patch removes the console log handler before logging the result. I read through log_manager, and found that I can do this more cleanly with remove_handler. John, is this a good use of the API? The problem you're trying to correct is that under some circumstances a log message is emitted twice to the console, right? Removing the console handler fees like the wrong solution, it's a pretty big hammer, you have to know when to remove it and once removed any expectation that messages will appear on the console will be untrue. Can you give me a brief explanation as to why you're getting duplicate messages on the console. I wonder if there isn't a better way to handle the problem which isn't so invasive and potentially hidden. Or is the issue you don't want a console handler at all? If that's the case then maybe we should provide a configuration that does not create one, that way it will be explicit and obvious there is no console handler. FWIW, the current configuration of logging is historical dating back to the beginning of the project. When I added the log manager the intent was to fix deficiencies in logging, not to modify the existing behavior. I wasn't clear to me the existing configuration was ideal. If you're hitting problems because of the existing configuration perhaps we should look at the historical configuration and ask if it needs to be modified. This patch is standardizing logging the final disposition of a number of commands. Currently is must be inferred. The problem is that some commands have had this disposition added but open no log files so some error messages are being displayed twice and in log format. I think the easiest solution would simply be to scale this back to only those commands that open a log file at all. You say command but command is a loaded word :-) I presume you mean command line utility and not an IPA Command object. The reason I'm drawing this distinction is because the environment Commands execute in are not supposed to change once api.bootstrap() has completed. Who or what is aggregating the final disposition of a number of commands? The reason I ask is because the log_mgr object is global and deleting a handler from a global resource to satisfy a local need may have unintentional global side effects. How is the aggregation accomplished? This patch is in the context of the command-line utilities like ipa-server-install, ipa-client-install, etc. and not the ipa tool. Some of them initialize the api, some do not. This is exactly the type of problem I'm concerned about, a conflict over who owns the logging configuration. These multiple potential owners of a global configuration is what lead me to introduce the configure_state attribute of the log manager, to disambiguate who is in control and who initialized the configuration. In fact bootstrap() in plugable.py explicitly examines the configure_state attribute of the log_manager and if it's been previously configured (for example because the api was loaded by a command line utility that has configured it's own logging) it then defers to the logging configuration established by the environment it's being run in (in other words if a command line utility calls standard_logging_setup() before loading the api it wins and the logging configuration belongs to the utility, not the api). All of this is to say that isolated code that reaches into a global configuration and takes an axe to it and chops out a significant part of the configuration that someone else may have established without them knowing about it feels wrong. I suspect there is some deeper problem afoot and unilaterally chopping out the console handler is not addressing the fundamental problem and likely introduces a new one. But I really need to look at the specific instances of the problem, maybe if I dig through the history of this patch review more carefully I'll discover it, but if someone wants to chime in an point me at the exact issue that would be great :-) As I said, the intention of my original request was to log the disposition of an install command. I think this patch goes too far and updates all admin utilities. rob -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 04/12/2012 01:30 PM, Petr Viktorin wrote:
On 04/10/2012 10:41 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/30/2012 11:00 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/26/2012 05:35 PM, Petr Viktorin wrote:
On 03/26/2012 04:54 PM, Rob Crittenden wrote:
Some minor compliants.
Ideally, there would be a routine that sets up the logging and
handles
command-line arguments in some uniform way (which is also needed
before
logging setup to detect ipa-server-install --uninstall).
The original patch did the common logging setup, and I hacked around
the
install/uninstall problem too.
I guess I overdid it when I simplified the patch.
I'm somewhat confused about the scope, so bear with me as I clarify
what
you mean.
If you abort the installation you get this somewhat unnerving error:
Continue to configure the system with these values? [no]:
ipa : ERROR ipa-server-install failed, SystemExit: Installation
aborted
Installation aborted
ipa-ldap-updater is the same:
# ipa-ldap-updater
[2012-03-26T14:53:41Z ipa] ERROR: ipa-ldap-updater failed,
SystemExit:
IPA is not configured on this system.
IPA is not configured on this system.
and ipa-upgradeconfig
$ ipa-upgradeconfig
[2012-03-26T14:54:05Z ipa] ERROR: ipa-upgradeconfig failed,
SystemExit:
You must be root to run this script.
You must be root to run this script.
I'm guessing that the issue is that the log file isn't opened yet.
It would be nice if the logging would be confined to just the log.
If I understand you correctly, the code should check if logging has
been
configured already, and if not, skip displaying the message?
When uninstalling you get the message 'ipa-server-install
successful'.
This is a little odd as well.
ipa-server-install is the name of the command. Wontfix for now,
unless
you disagree strongly.
Updated patch: only log if logging has been configured (detected by
looking at the root logger's handlers), and changed the message to
“The
ipa-server-install command has succeeded/failed”.
Works much better thanks. Just one request. When you created
final_log()
you show less information than you did in earlier patches. It is nice
seeing the SystemExit failure. Can you do something like this
(basically
cut-n-pasted from v05)?
diff --git a/ipaserver/install/installutils.py
b/ipaserver/install/installutils.
py
index 851b58d..ca82a1b 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -721,15 +721,15 @@ def script_context(operation_name):
# Only log if logging was already configured
# TODO: Do this properly (e.g. configure logging before the try/except)
if log_mgr.handlers.keys() != ['console']:
- root_logger.info(template, operation_name)
+ root_logger.info(template)
try:
yield
except BaseException, e:
if isinstance(e, SystemExit) and (e.code is None or e.code == 0):
# Not an error after all
- final_log('The %s command was successful')
+ final_log('The %s command was successful' % operation_name)
else:
- final_log('The %s command failed')
+ final_log('%s failed, %s: %s' % (operation_name, type(e).__name__,
e))
raise
else:
final_log('The %s command was successful')
This looks like:
2012-03-30T20:56:53Z INFO ipa-dns-install failed, SystemExit:
DNS is already configured in this IPA server.
rob
Fixed.
Hate to do this to you but I've found a few more issues. I basically
went down the list and ran all the commands in various conditions.
Some don't open any logs at all so the output gets written twice, like
ipa-replica-prepare and ipa-replica-manage:
# ipa-replica-manage del foo
Directory Manager password:
ipa: INFO: The ipa-replica-manage command failed, SystemExit:
'pony.greyoak.com' has no replication agreement for 'foo'
'pony.greyoak.com' has no replication agreement for 'foo'
Same with ipa-csreplica-manage.
# ipa-replica-prepare foo
Directory Manager (existing master) password:
ipa: INFO: The ipa-replica-prepare command failed, SystemExit:
The password provided is incorrect for LDAP server pony.greyoak.com
The password provided is incorrect for LDAP server pony.greyoak.com
rob
When the utility sets logging to console, the extra log message gets
printed out there. I agree this isn't optimal.
Attached patch removes the console log handler before logging the result.
I read through log_manager, and found that I can do this more cleanly
with remove_handler.
John, is this a good use of the API?
--
Petr³
From b7fd14e300a055c59250a4c3717ff144b386d782 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 1 Mar 2012 05:29:52 -0500
Subject: [PATCH] Add final debug message in installers
Invocation of install tools is wrapped in a context manager that logs
the final success or failure.
https://fedorahosted.org/freeipa/ticket/2071
---
install/tools/ipa-adtrust-install| 47 +++---
install/tools/ipa-ca-install | 71 +-
install/tools/ipa-compat-manage | 43
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 04/10/2012 10:41 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/30/2012 11:00 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/26/2012 05:35 PM, Petr Viktorin wrote:
On 03/26/2012 04:54 PM, Rob Crittenden wrote:
Some minor compliants.
Ideally, there would be a routine that sets up the logging and handles
command-line arguments in some uniform way (which is also needed
before
logging setup to detect ipa-server-install --uninstall).
The original patch did the common logging setup, and I hacked around
the
install/uninstall problem too.
I guess I overdid it when I simplified the patch.
I'm somewhat confused about the scope, so bear with me as I clarify
what
you mean.
If you abort the installation you get this somewhat unnerving error:
Continue to configure the system with these values? [no]:
ipa : ERROR ipa-server-install failed, SystemExit: Installation
aborted
Installation aborted
ipa-ldap-updater is the same:
# ipa-ldap-updater
[2012-03-26T14:53:41Z ipa] ERROR: ipa-ldap-updater failed,
SystemExit:
IPA is not configured on this system.
IPA is not configured on this system.
and ipa-upgradeconfig
$ ipa-upgradeconfig
[2012-03-26T14:54:05Z ipa] ERROR: ipa-upgradeconfig failed,
SystemExit:
You must be root to run this script.
You must be root to run this script.
I'm guessing that the issue is that the log file isn't opened yet.
It would be nice if the logging would be confined to just the log.
If I understand you correctly, the code should check if logging has
been
configured already, and if not, skip displaying the message?
When uninstalling you get the message 'ipa-server-install
successful'.
This is a little odd as well.
ipa-server-install is the name of the command. Wontfix for now, unless
you disagree strongly.
Updated patch: only log if logging has been configured (detected by
looking at the root logger's handlers), and changed the message to “The
ipa-server-install command has succeeded/failed”.
Works much better thanks. Just one request. When you created final_log()
you show less information than you did in earlier patches. It is nice
seeing the SystemExit failure. Can you do something like this (basically
cut-n-pasted from v05)?
diff --git a/ipaserver/install/installutils.py
b/ipaserver/install/installutils.
py
index 851b58d..ca82a1b 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -721,15 +721,15 @@ def script_context(operation_name):
# Only log if logging was already configured
# TODO: Do this properly (e.g. configure logging before the try/except)
if log_mgr.handlers.keys() != ['console']:
- root_logger.info(template, operation_name)
+ root_logger.info(template)
try:
yield
except BaseException, e:
if isinstance(e, SystemExit) and (e.code is None or e.code == 0):
# Not an error after all
- final_log('The %s command was successful')
+ final_log('The %s command was successful' % operation_name)
else:
- final_log('The %s command failed')
+ final_log('%s failed, %s: %s' % (operation_name, type(e).__name__,
e))
raise
else:
final_log('The %s command was successful')
This looks like:
2012-03-30T20:56:53Z INFO ipa-dns-install failed, SystemExit:
DNS is already configured in this IPA server.
rob
Fixed.
Hate to do this to you but I've found a few more issues. I basically
went down the list and ran all the commands in various conditions.
Some don't open any logs at all so the output gets written twice, like
ipa-replica-prepare and ipa-replica-manage:
# ipa-replica-manage del foo
Directory Manager password:
ipa: INFO: The ipa-replica-manage command failed, SystemExit:
'pony.greyoak.com' has no replication agreement for 'foo'
'pony.greyoak.com' has no replication agreement for 'foo'
Same with ipa-csreplica-manage.
# ipa-replica-prepare foo
Directory Manager (existing master) password:
ipa: INFO: The ipa-replica-prepare command failed, SystemExit:
The password provided is incorrect for LDAP server pony.greyoak.com
The password provided is incorrect for LDAP server pony.greyoak.com
rob
When the utility sets logging to console, the extra log message gets
printed out there. I agree this isn't optimal.
Attached patch removes the console log handler before logging the result.
--
Petr³
From 92c2ec49e9be9b7f12ab0427d0bf803b582df21c Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 1 Mar 2012 05:29:52 -0500
Subject: [PATCH] Add final debug message in installers
Invocation of install tools is wrapped in a context manager that logs
the final success or failure.
https://fedorahosted.org/freeipa/ticket/2071
---
install/tools/ipa-adtrust-install| 47 +++---
install/tools/ipa-ca-install | 71 +-
install/tools/ipa-compat-manage | 43 ++--
install/tools/ipa-compliance | 17
install/tools/ipa-csreplica-manage | 33
install/tools/ipa-dns-install| 47
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 03/30/2012 06:21 AM, Petr Viktorin wrote: Updated patch: only log if logging has been configured (detected by looking at the root logger's handlers), and changed the message to “The ipa-server-install command has succeeded/failed”. Actually the log_manager has an attribute called configure_state whose purpose is to tell you if the log manager has been configured (not None) and if so how it was configured. When it's not None it's a string whose value is by programmer convention (there are no restrictions). Currently the 3 values in use are: 'standard' The log_mgr was initialized by standard_logging_setup() 'default' The log_mgr was initialized by virtue of the ipa_log_manager being loaded. 'api' The log_mgr was initialized by the plugin bootstrap process (i.e. the api is initialized and has the log manager bound to it). It better to reference log_mgr.configure_state than to make assumptions about the internals of the class. -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
Petr Viktorin wrote:
On 03/30/2012 11:00 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/26/2012 05:35 PM, Petr Viktorin wrote:
On 03/26/2012 04:54 PM, Rob Crittenden wrote:
Some minor compliants.
Ideally, there would be a routine that sets up the logging and handles
command-line arguments in some uniform way (which is also needed before
logging setup to detect ipa-server-install --uninstall).
The original patch did the common logging setup, and I hacked around
the
install/uninstall problem too.
I guess I overdid it when I simplified the patch.
I'm somewhat confused about the scope, so bear with me as I clarify
what
you mean.
If you abort the installation you get this somewhat unnerving error:
Continue to configure the system with these values? [no]:
ipa : ERROR ipa-server-install failed, SystemExit: Installation
aborted
Installation aborted
ipa-ldap-updater is the same:
# ipa-ldap-updater
[2012-03-26T14:53:41Z ipa] ERROR: ipa-ldap-updater failed,
SystemExit:
IPA is not configured on this system.
IPA is not configured on this system.
and ipa-upgradeconfig
$ ipa-upgradeconfig
[2012-03-26T14:54:05Z ipa] ERROR: ipa-upgradeconfig failed,
SystemExit:
You must be root to run this script.
You must be root to run this script.
I'm guessing that the issue is that the log file isn't opened yet.
It would be nice if the logging would be confined to just the log.
If I understand you correctly, the code should check if logging has
been
configured already, and if not, skip displaying the message?
When uninstalling you get the message 'ipa-server-install successful'.
This is a little odd as well.
ipa-server-install is the name of the command. Wontfix for now, unless
you disagree strongly.
Updated patch: only log if logging has been configured (detected by
looking at the root logger's handlers), and changed the message to “The
ipa-server-install command has succeeded/failed”.
Works much better thanks. Just one request. When you created final_log()
you show less information than you did in earlier patches. It is nice
seeing the SystemExit failure. Can you do something like this (basically
cut-n-pasted from v05)?
diff --git a/ipaserver/install/installutils.py
b/ipaserver/install/installutils.
py
index 851b58d..ca82a1b 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -721,15 +721,15 @@ def script_context(operation_name):
# Only log if logging was already configured
# TODO: Do this properly (e.g. configure logging before the try/except)
if log_mgr.handlers.keys() != ['console']:
- root_logger.info(template, operation_name)
+ root_logger.info(template)
try:
yield
except BaseException, e:
if isinstance(e, SystemExit) and (e.code is None or e.code == 0):
# Not an error after all
- final_log('The %s command was successful')
+ final_log('The %s command was successful' % operation_name)
else:
- final_log('The %s command failed')
+ final_log('%s failed, %s: %s' % (operation_name, type(e).__name__,
e))
raise
else:
final_log('The %s command was successful')
This looks like:
2012-03-30T20:56:53Z INFO ipa-dns-install failed, SystemExit:
DNS is already configured in this IPA server.
rob
Fixed.
Hate to do this to you but I've found a few more issues. I basically
went down the list and ran all the commands in various conditions.
Some don't open any logs at all so the output gets written twice, like
ipa-replica-prepare and ipa-replica-manage:
# ipa-replica-manage del foo
Directory Manager password:
ipa: INFO: The ipa-replica-manage command failed, SystemExit:
'pony.greyoak.com' has no replication agreement for 'foo'
'pony.greyoak.com' has no replication agreement for 'foo'
Same with ipa-csreplica-manage.
# ipa-replica-prepare foo
Directory Manager (existing master) password:
ipa: INFO: The ipa-replica-prepare command failed, SystemExit:
The password provided is incorrect for LDAP server pony.greyoak.com
The password provided is incorrect for LDAP server pony.greyoak.com
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 03/26/2012 05:35 PM, Petr Viktorin wrote:
On 03/26/2012 04:54 PM, Rob Crittenden wrote:
Some minor compliants.
Ideally, there would be a routine that sets up the logging and handles
command-line arguments in some uniform way (which is also needed before
logging setup to detect ipa-server-install --uninstall).
The original patch did the common logging setup, and I hacked around the
install/uninstall problem too.
I guess I overdid it when I simplified the patch.
I'm somewhat confused about the scope, so bear with me as I clarify what
you mean.
If you abort the installation you get this somewhat unnerving error:
Continue to configure the system with these values? [no]:
ipa : ERROR ipa-server-install failed, SystemExit: Installation aborted
Installation aborted
ipa-ldap-updater is the same:
# ipa-ldap-updater
[2012-03-26T14:53:41Z ipa] ERROR: ipa-ldap-updater failed, SystemExit:
IPA is not configured on this system.
IPA is not configured on this system.
and ipa-upgradeconfig
$ ipa-upgradeconfig
[2012-03-26T14:54:05Z ipa] ERROR: ipa-upgradeconfig failed, SystemExit:
You must be root to run this script.
You must be root to run this script.
I'm guessing that the issue is that the log file isn't opened yet.
It would be nice if the logging would be confined to just the log.
If I understand you correctly, the code should check if logging has been
configured already, and if not, skip displaying the message?
When uninstalling you get the message 'ipa-server-install successful'.
This is a little odd as well.
ipa-server-install is the name of the command. Wontfix for now, unless
you disagree strongly.
Updated patch: only log if logging has been configured (detected by
looking at the root logger's handlers), and changed the message to “The
ipa-server-install command has succeeded/failed”.
--
Petr³
From 248744253385dab1b0747c38f37287842f00aa62 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 1 Mar 2012 05:29:52 -0500
Subject: [PATCH] Add final debug message in installers
Invocation of install tools is wrapped in a context manager that logs
the final success or failure.
https://fedorahosted.org/freeipa/ticket/2071
---
install/tools/ipa-adtrust-install| 47 +++---
install/tools/ipa-ca-install | 71 +-
install/tools/ipa-compat-manage | 43 ++--
install/tools/ipa-compliance | 17
install/tools/ipa-csreplica-manage | 33
install/tools/ipa-dns-install| 47 +++---
install/tools/ipa-ldap-updater | 27 +++--
install/tools/ipa-managed-entries| 35 +
install/tools/ipa-nis-manage | 43 ++--
install/tools/ipa-replica-conncheck | 35 +
install/tools/ipa-replica-install| 71 +-
install/tools/ipa-replica-manage | 45 +++--
install/tools/ipa-replica-prepare| 33
install/tools/ipa-server-certinstall | 17
install/tools/ipa-server-install | 66 ---
install/tools/ipa-upgradeconfig | 16 ---
ipaserver/install/installutils.py| 21 ++
17 files changed, 353 insertions(+), 314 deletions(-)
diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 248ea35eaa86dd59ebbc871b86df780cfd71ccf6..c3558fce0cceae879f83b61e0057d14fe42811de 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -227,26 +227,27 @@ def main():
return 0
-try:
-sys.exit(main())
-except SystemExit, e:
-sys.exit(e)
-except KeyboardInterrupt:
-print Installation cancelled.
-except RuntimeError, e:
-print str(e)
-except HostnameLocalhost:
-print The hostname resolves to the localhost address (127.0.0.1/::1)
-print Please change your /etc/hosts file so that the hostname
-print resolves to the ip address of your network interface.
-print The KDC service does not listen on localhost
-print
-print Please fix your /etc/hosts file and restart the setup program
-except Exception, e:
-message = Unexpected error - see ipaserver-install.log for details:\n %s % str(e)
-print message
-message = str(e)
-for str in traceback.format_tb(sys.exc_info()[2]):
-message = message + \n + str
-root_logger.debug(message)
-sys.exit(1)
+with installutils.script_context('ipa-adtrust-install'):
+try:
+sys.exit(main())
+except SystemExit, e:
+sys.exit(e)
+except KeyboardInterrupt:
+print Installation cancelled.
+except RuntimeError, e:
+print str(e)
+except HostnameLocalhost:
+print The hostname resolves to the localhost address (127.0.0.1/::1)
+print Please change your /etc/hosts file so that the hostname
+print resolves to the ip address
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 03/15/2012 11:30 AM, Petr Viktorin wrote:
On 03/01/2012 11:45 AM, Petr Viktorin wrote:
On 02/29/2012 07:46 PM, Rob Crittenden wrote:
Martin Kosek wrote:
On Mon, 2012-02-27 at 17:51 +0100, Petr Viktorin wrote:
On 02/22/2012 10:41 AM, Petr Viktorin wrote:
This fixes https://fedorahosted.org/freeipa/ticket/2071 (Add final
debug
message in installers). The try/except blocks at the end of
installers/management scripts are replaced by a call to a common
function, which includes the final message.
Obviously the installers still need some more love. This is as far
as I
got before Martin stopped me, saying I shouldn't change too much
before
a release :)
If it's still too many changes to test, I could just wrap the
blocks in
some `with add_final_message` block for now, and resubmit this patch
after the release.
Yeah, this is exactly the kind of changes that can have yet-unseen
consequences and I don't like pushing this close to the release.
The original ticket just asks for a debug message when the install
script ends. If possible, I would really prefer to have some low-risk
patch adding just those and leave install script refactoring for next
big release, like 3.x. Rob, what's your opinion on this?
Martin
Yes, I agree. Simpler is better at this point.
rob
This patch simply wraps the try blocks in a context that logs the final
result.
Most of the changes are indentation; diff with -w to see the additions.
Not sure if this would count as an update or a new patch...
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Rebased against current master.
Rebased again.
--
Petr³
From 444c07382eb330650106eb8e77b401e0b41aca58 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 1 Mar 2012 05:29:52 -0500
Subject: [PATCH] Add final debug message in installers
Invocation of install tools is wrapped in a context manager that logs
the final success or failure.
https://fedorahosted.org/freeipa/ticket/2071
---
install/tools/ipa-adtrust-install| 47 +++---
install/tools/ipa-ca-install | 71 +-
install/tools/ipa-compat-manage | 43 ++--
install/tools/ipa-compliance | 17
install/tools/ipa-csreplica-manage | 33
install/tools/ipa-dns-install| 47 +++---
install/tools/ipa-ldap-updater | 27 +++--
install/tools/ipa-managed-entries| 35 +
install/tools/ipa-nis-manage | 43 ++--
install/tools/ipa-replica-conncheck | 35 +
install/tools/ipa-replica-install| 71 +-
install/tools/ipa-replica-manage | 45 +++--
install/tools/ipa-replica-prepare| 33
install/tools/ipa-server-certinstall | 17
install/tools/ipa-server-install | 66 ---
install/tools/ipa-upgradeconfig | 16 ---
ipaserver/install/installutils.py| 16
17 files changed, 348 insertions(+), 314 deletions(-)
diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 248ea35eaa86dd59ebbc871b86df780cfd71ccf6..c3558fce0cceae879f83b61e0057d14fe42811de 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -227,26 +227,27 @@ def main():
return 0
-try:
-sys.exit(main())
-except SystemExit, e:
-sys.exit(e)
-except KeyboardInterrupt:
-print Installation cancelled.
-except RuntimeError, e:
-print str(e)
-except HostnameLocalhost:
-print The hostname resolves to the localhost address (127.0.0.1/::1)
-print Please change your /etc/hosts file so that the hostname
-print resolves to the ip address of your network interface.
-print The KDC service does not listen on localhost
-print
-print Please fix your /etc/hosts file and restart the setup program
-except Exception, e:
-message = Unexpected error - see ipaserver-install.log for details:\n %s % str(e)
-print message
-message = str(e)
-for str in traceback.format_tb(sys.exc_info()[2]):
-message = message + \n + str
-root_logger.debug(message)
-sys.exit(1)
+with installutils.script_context('ipa-adtrust-install'):
+try:
+sys.exit(main())
+except SystemExit, e:
+sys.exit(e)
+except KeyboardInterrupt:
+print Installation cancelled.
+except RuntimeError, e:
+print str(e)
+except HostnameLocalhost:
+print The hostname resolves to the localhost address (127.0.0.1/::1)
+print Please change your /etc/hosts file so that the hostname
+print resolves to the ip address of your network interface.
+print The KDC service does not listen on localhost
+print
+print Please fix
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 03/01/2012 11:45 AM, Petr Viktorin wrote:
On 02/29/2012 07:46 PM, Rob Crittenden wrote:
Martin Kosek wrote:
On Mon, 2012-02-27 at 17:51 +0100, Petr Viktorin wrote:
On 02/22/2012 10:41 AM, Petr Viktorin wrote:
This fixes https://fedorahosted.org/freeipa/ticket/2071 (Add final
debug
message in installers). The try/except blocks at the end of
installers/management scripts are replaced by a call to a common
function, which includes the final message.
Obviously the installers still need some more love. This is as far
as I
got before Martin stopped me, saying I shouldn't change too much
before
a release :)
If it's still too many changes to test, I could just wrap the
blocks in
some `with add_final_message` block for now, and resubmit this patch
after the release.
Yeah, this is exactly the kind of changes that can have yet-unseen
consequences and I don't like pushing this close to the release.
The original ticket just asks for a debug message when the install
script ends. If possible, I would really prefer to have some low-risk
patch adding just those and leave install script refactoring for next
big release, like 3.x. Rob, what's your opinion on this?
Martin
Yes, I agree. Simpler is better at this point.
rob
This patch simply wraps the try blocks in a context that logs the final
result.
Most of the changes are indentation; diff with -w to see the additions.
Not sure if this would count as an update or a new patch...
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Rebased against current master.
--
Petr³
From b4d10086b34b63215af1437c138d8857b2355962 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 1 Mar 2012 05:29:52 -0500
Subject: [PATCH] Add final debug message in installers
Invocation of install tools is wrapped in a context manager that logs
the final success or failure.
https://fedorahosted.org/freeipa/ticket/2071
---
install/tools/ipa-adtrust-install| 47 +++---
install/tools/ipa-ca-install | 71 +-
install/tools/ipa-compat-manage | 43 ++--
install/tools/ipa-compliance | 17
install/tools/ipa-csreplica-manage | 33
install/tools/ipa-dns-install| 47 +++---
install/tools/ipa-ldap-updater | 27 +++--
install/tools/ipa-managed-entries| 35 +
install/tools/ipa-nis-manage | 43 ++--
install/tools/ipa-replica-conncheck | 33
install/tools/ipa-replica-install| 71 +-
install/tools/ipa-replica-manage | 45 +++--
install/tools/ipa-replica-prepare| 33
install/tools/ipa-server-certinstall | 17
install/tools/ipa-server-install | 66 ---
install/tools/ipa-upgradeconfig | 16 ---
ipaserver/install/installutils.py| 16
17 files changed, 347 insertions(+), 313 deletions(-)
diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 248ea35eaa86dd59ebbc871b86df780cfd71ccf6..c3558fce0cceae879f83b61e0057d14fe42811de 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -227,26 +227,27 @@ def main():
return 0
-try:
-sys.exit(main())
-except SystemExit, e:
-sys.exit(e)
-except KeyboardInterrupt:
-print Installation cancelled.
-except RuntimeError, e:
-print str(e)
-except HostnameLocalhost:
-print The hostname resolves to the localhost address (127.0.0.1/::1)
-print Please change your /etc/hosts file so that the hostname
-print resolves to the ip address of your network interface.
-print The KDC service does not listen on localhost
-print
-print Please fix your /etc/hosts file and restart the setup program
-except Exception, e:
-message = Unexpected error - see ipaserver-install.log for details:\n %s % str(e)
-print message
-message = str(e)
-for str in traceback.format_tb(sys.exc_info()[2]):
-message = message + \n + str
-root_logger.debug(message)
-sys.exit(1)
+with installutils.script_context('ipa-adtrust-install'):
+try:
+sys.exit(main())
+except SystemExit, e:
+sys.exit(e)
+except KeyboardInterrupt:
+print Installation cancelled.
+except RuntimeError, e:
+print str(e)
+except HostnameLocalhost:
+print The hostname resolves to the localhost address (127.0.0.1/::1)
+print Please change your /etc/hosts file so that the hostname
+print resolves to the ip address of your network interface.
+print The KDC service does not listen on localhost
+print
+print Please fix your /etc/hosts file and restart the setup program
+except
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
On 02/29/2012 07:46 PM, Rob Crittenden wrote:
Martin Kosek wrote:
On Mon, 2012-02-27 at 17:51 +0100, Petr Viktorin wrote:
On 02/22/2012 10:41 AM, Petr Viktorin wrote:
This fixes https://fedorahosted.org/freeipa/ticket/2071 (Add final
debug
message in installers). The try/except blocks at the end of
installers/management scripts are replaced by a call to a common
function, which includes the final message.
Obviously the installers still need some more love. This is as far as I
got before Martin stopped me, saying I shouldn't change too much before
a release :)
If it's still too many changes to test, I could just wrap the blocks in
some `with add_final_message` block for now, and resubmit this patch
after the release.
Yeah, this is exactly the kind of changes that can have yet-unseen
consequences and I don't like pushing this close to the release.
The original ticket just asks for a debug message when the install
script ends. If possible, I would really prefer to have some low-risk
patch adding just those and leave install script refactoring for next
big release, like 3.x. Rob, what's your opinion on this?
Martin
Yes, I agree. Simpler is better at this point.
rob
This patch simply wraps the try blocks in a context that logs the final
result.
Most of the changes are indentation; diff with -w to see the additions.
Not sure if this would count as an update or a new patch...
--
Petr³
From 2c05094a2b7e272bb08dc2ab24f4b9b97b00b1b7 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 1 Mar 2012 05:29:52 -0500
Subject: [PATCH] Add final debug message in installers
Invocation of install tools is wrapped in a context manager that logs
the final success or failure.
https://fedorahosted.org/freeipa/ticket/2071
---
install/tools/ipa-adtrust-install| 47 +++---
install/tools/ipa-ca-install | 71 +-
install/tools/ipa-compat-manage | 43 ++--
install/tools/ipa-compliance | 17
install/tools/ipa-csreplica-manage | 33
install/tools/ipa-dns-install| 47 +++---
install/tools/ipa-ldap-updater | 27 +++--
install/tools/ipa-managed-entries| 35 +
install/tools/ipa-nis-manage | 43 ++--
install/tools/ipa-replica-conncheck | 33
install/tools/ipa-replica-install| 71 +-
install/tools/ipa-replica-manage | 45 +++--
install/tools/ipa-replica-prepare| 33
install/tools/ipa-server-certinstall | 17
install/tools/ipa-server-install | 66 ---
install/tools/ipa-upgradeconfig | 17 +---
ipaserver/install/installutils.py| 16
17 files changed, 348 insertions(+), 313 deletions(-)
diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 248ea35eaa86dd59ebbc871b86df780cfd71ccf6..c3558fce0cceae879f83b61e0057d14fe42811de 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -227,26 +227,27 @@ def main():
return 0
-try:
-sys.exit(main())
-except SystemExit, e:
-sys.exit(e)
-except KeyboardInterrupt:
-print Installation cancelled.
-except RuntimeError, e:
-print str(e)
-except HostnameLocalhost:
-print The hostname resolves to the localhost address (127.0.0.1/::1)
-print Please change your /etc/hosts file so that the hostname
-print resolves to the ip address of your network interface.
-print The KDC service does not listen on localhost
-print
-print Please fix your /etc/hosts file and restart the setup program
-except Exception, e:
-message = Unexpected error - see ipaserver-install.log for details:\n %s % str(e)
-print message
-message = str(e)
-for str in traceback.format_tb(sys.exc_info()[2]):
-message = message + \n + str
-root_logger.debug(message)
-sys.exit(1)
+with installutils.script_context('ipa-adtrust-install'):
+try:
+sys.exit(main())
+except SystemExit, e:
+sys.exit(e)
+except KeyboardInterrupt:
+print Installation cancelled.
+except RuntimeError, e:
+print str(e)
+except HostnameLocalhost:
+print The hostname resolves to the localhost address (127.0.0.1/::1)
+print Please change your /etc/hosts file so that the hostname
+print resolves to the ip address of your network interface.
+print The KDC service does not listen on localhost
+print
+print Please fix your /etc/hosts file and restart the setup program
+except Exception, e:
+message = Unexpected error - see ipaserver-install.log for details:\n %s % str(e)
+print message
+message = str(e)
+for str in traceback.format_tb(sys.exc_info()[2]):
+message =
