replica-conncheck: improve error msg + logging #276
Conversation
| print_info("\nThe following list of ports use UDP protocol and would need to be") | ||
| print_info("checked manually:") | ||
| root_logger.info(("\nThe following list of ports use UDP protocol" | ||
| "and would need to be")) |
There was a problem hiding this comment.
I guess the double parentheses are just a typo, right?
| print_info("checked manually:") | ||
| root_logger.info(("\nThe following list of ports use UDP protocol" | ||
| "and would need to be")) | ||
| root_logger.info("checked manually:") |
There was a problem hiding this comment.
Also, you can squash this into previous info() call.
|
|
||
| # create listeners | ||
| print_info("Start listening on required ports for remote master check") | ||
| root_logger.info(("Start listening on required ports for remote " | ||
| "master check")) |
There was a problem hiding this comment.
Double parentheses again? Is there really some reason to it?
There was a problem hiding this comment.
To join the multiline string.
There was a problem hiding this comment.
And it turns out they are not needed when the string is an argument for a function call :) I'll remove them.
| raise RuntimeError(( | ||
| 'Could not SSH to remote host.\n' | ||
| 'See /var/log/ipareplica-conncheck.log for more ' | ||
| 'information.')) |
| raise RuntimeError("Remote master check failed with following error message(s):\n%s" % stderr) | ||
| raise RuntimeError( | ||
| ("Remote master check failed with following " | ||
| "error message(s):\n%s" % stderr)) |
| "the listening part after the test.")) | ||
| root_logger.info("") | ||
| root_logger.info(("Please run the following command on " | ||
| "remote master:")) |
tkrizek
force-pushed
the
t6497
branch
2 times, most recently
from
9ddf77f
to
916ea2d
Compare
|
|
||
| print_info("/usr/sbin/ipa-replica-conncheck " + " ".join(remote_check_opts)) | ||
| root_logger.info("/usr/sbin/ipa-replica-conncheck " + | ||
| " ".join(remote_check_opts)) |
There was a problem hiding this comment.
This could be merged into the previous message.
There was a problem hiding this comment.
Also, use str.format() here instead of '+'.
| ("The following UDP ports could not be verified as open: %s\n" | ||
| "This can happen if they are already bound to an application\n" | ||
| "and ipa-replica-conncheck cannot attach own UDP responder.") | ||
| % ", ".join(str(port.port) for port in ports_udp_warning)) |
There was a problem hiding this comment.
NOTE: using str.format() would help you remove the double parentheses here as well but you don't have to do it as the old '%' way is used in most of the other messages here.
| for line in result.error_output.splitlines(): | ||
| print(' %s' % line) | ||
| raise RuntimeError('Could not SSH to remote host.') | ||
| root_logger.debug(result.error_output) |
There was a problem hiding this comment.
Is there a reason not to print the output to stdout as it used to be?
There was a problem hiding this comment.
It is purely a debug information (~50-100 lines), so I don't see the reason why it should be displayed to the user. Since previously the function lacked any reasonable logging, I think it was decided to simply print this debug info out, as the rest of the messages.
Since this patch adds logging to the function, I think the proper place for this information is the log. If the command fails, the user is prompted to check the log (if it fails during replica installation) where he can find this info.
There was a problem hiding this comment.
Right, that's the explanation I was looking for, thanks.
| sys.exit(1) | ||
| except RuntimeError as e: | ||
| sys.exit(e) | ||
| root_logger.error('ERROR: ' + str(e)) |
There was a problem hiding this comment.
Don't use '+' in strings, use str.format().
|
Needs rebase. |
Replica conncheck may fail for other reasons then network misconfiguration. For example, an incorrect admin password might be provided. Since conncheck is ran as a separate script in quiet mode, no insightful error message can be displayed. https://fedorahosted.org/freeipa/ticket/6497
Make sure all messages displayed on screen to the user can be found in the log as well. The messages are also logged if the script is ran in quiet mode. https://fedorahosted.org/freeipa/ticket/6497
| sys.exit(1) | ||
| except RuntimeError as e: | ||
| sys.exit(e) | ||
| root_logger.error('ERROR: {ex}'.format(ex=e)) |
There was a problem hiding this comment.
NOTE: The 'ERROR' prefix was not here before, however it increases awareness that there was an error so I'm fine with keeping it.
|
Seems to work fine, ACK. |
Replica conncheck may fail for other reasons then network
misconfiguration. For example, an incorrect admin password might be
provided. Since conncheck is ran as a separate script in quiet mode,
no insightful error message can be displayed.
User is instead pointed to the logs, which were also improved to contain
the usual on screen messages.
https://fedorahosted.org/freeipa/ticket/6497