On Tue, 2012-05-22 at 15:45 +0200, Petr Viktorin wrote:
> On 2012-04-23 17:05, John Dennis wrote:
> > On 04/23/2012 05:19 AM, Petr Viktorin wrote:
> >> This fixes https://fedorahosted.org/freeipa/ticket/2071 (Add final debug
> >> message in installers).
> >>
> >> I submitted an earlier version of this patch before (0014), but it was
> >> too much to include in 2.2. Hopefully now there's more space for
> >> restructuring. I think it's better to start a new thread with this
> >> approach.
> >>
> >> 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.
> >> For each specific error, the error handlers in all scripts was almost
> >> the same, but each script handled a different selection of errors.
> >> Instead of having this copy/pasted code (with subtle differences
> >> creeping in over time), this patch consolidates it all in one place.
> >
> > I like this approach much better than the earlier patch, great, thanks.
> > I'm a big fan of calling into common code instead of copying code to my
> > mind the refactoring to utilize common code is great approach. I also
> > like the fact the logging configuration is not modified after it's
> > established.
> >
> > At some point we may want to revist how the log messages are generated.
> > For example should all communication to the console pass through the
> > console handler? Is there a logger established for the script? Should
> > the format of messages emitted to the console be altered? Should all
> > command line utilities accept the both the verbose and debug flag? Etc.
> > But for now this is fantastic start in the right direction.
> >
> > I have not installed and exercised the patch so I can't comment on any
> > runtime time issues that might be present, but from code inspection only
> > it has my ACK.
> >
> >
> Thanks John!
> Yes, this is just a start.
>
>
> Patch rebased to curent master
>
The patch needs another rebase.
Besides that, the approach is fine (and removes a lot of redundant code)
but I found several issues with the patches:
1) ipa-server-install: Fails when option parser error is raised:
# ipa-server-install -p foo
Usage: ipa-server-install [options]
ipa-server-install: error: DS admin password: Password must be at least
8 characters long
Traceback (most recent call last):
File "/sbin/ipa-server-install", line 1131, in <module>
if not success and installation_cleanup:
NameError: name 'success' is not defined
2) BadSyntax error is not caught properly:
# ipa-ldap-updater
Directory Manager password:
ipa : INFO PRE_UPDATE
ipa : INFO Parsing
file /usr/share/ipa/updates/10-60basev2.update
ipa : INFO Parsing
file /usr/share/ipa/updates/10-60basev3.update
ipa : INFO Parsing
file /usr/share/ipa/updates/10-RFC2307bis.update
ipa : INFO Parsing
file /usr/share/ipa/updates/10-RFC4876.update
ipa : INFO Parsing
file /usr/share/ipa/updates/10-bind-schema.update
ipa : INFO Parsing
file /usr/share/ipa/updates/10-config.update
ipa : INFO Parsing
file /usr/share/ipa/updates/10-schema_compat.update
ipa : INFO Parsing
file /usr/share/ipa/updates/10-selinuxusermap.update
ipa : INFO Parsing file /usr/share/ipa/updates/10-ssh.update
ipa : INFO File
"/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
line 695, in run_script
return_value = main_function()
File "/sbin/ipa-ldap-updater", line 142, in main
modified = ld.update(files)
File
"/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py", line
830, in update
(all_updates, dn_list) = self.parse_update_file(data, all_updates,
dn_list)
File
"/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py", line
284, in parse_update_file
raise BadSyntax, "dn is not defined in the update"
ipa : INFO The ipa-ldap-updater command failed, exception:
BadSyntax: 'dn is not defined in the update'
There is a syntax error in this update file:
dn is not defined in the update
3) Inappropriate debug message in ipa-replica-manage:
# ipa-replica-manage list
Directory Manager password:
vm-125.idm.lab.bos.redhat.com: master
ipa: INFO: The ipa-replica-manage command was successful <<<
4) ipa-ca-install emits failed_message when option error is detected:
# ipa-ca-install
Usage: ipa-ca-install [options] REPLICA_FILE
ipa-ca-install: error: you must provide a file generated by
ipa-replica-prepare
>>> Your system may be partly configured.
>>> Run /usr/sbin/ipa-server-install --uninstall to clean up.
5) ipa-cs-replica-manage - the same issue as with 3)
6) ipa-replica-prepare - the same issue as with 3)
7) ipa-replica-prepare - the same issue as with 4)
8) ldap.SERVER_DOWN is not caught:
# ipactl stop
Stopping HTTP Service
Stopping MEMCACHE Service
Stopping KPASSWD Service
Stopping KDC Service
Stopping Directory Service
# ipa-replica-manage list
ipa: INFO: File
"/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
line 695, in run_script
return_value = main_function()
...
ipa: INFO: The ipa-replica-manage command failed, exception:
SERVER_DOWN: {'desc': "Can't contact LDAP server"}
Can't contact LDAP server
IMO we should add a handler both for ldap.SERVER_DOWN and also a general
exception ldap.LDAPError to catch other potential LDAP errors.
9) KeyboardInterrupt is not processed properly in ipa-replica-manage:
# ipa-replica-manage list
^Cipa: INFO: File
"/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
line 695, in run_script
return_value = main_function()
File "/sbin/ipa-replica-manage", line 477, in main
api.finalize()
...
File "/usr/lib/python2.7/site-packages/ipalib/parameters.py", line
467, in __init__
type(kind) is tuple and not isinstance(value, kind)
ipa: INFO: The ipa-replica-manage command failed, exception:
KeyboardInterrupt:
Cancelled.
I think it would be nicer if such traceback would only be printed either
to log (when the command has a log file) or only when --debug option is
passed (some of our commands miss that option).
10) Traceback in ipa-csreplica-manage:
# ipa-csreplica-manage list
Directory Manager password:
ipa: INFO: File
"/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
line 695, in run_script
return_value = main_function()
File "/sbin/ipa-csreplica-manage", line 415, in main
list_replicas(realm, host, replica, dirman_passwd, options.verbose)
File "/sbin/ipa-csreplica-manage", line 180, in list_replicas
sys.exit("Failed to get data from '%s': %s" % (host,
convert_error(e)))
ipa: INFO: The ipa-replica-manage command failed, exception: SystemExit:
Failed to get data from 'vm-057.idm.lab.bos.redhat.com': Can't contact
LDAP server
Failed to get data from 'vm-057.idm.lab.bos.redhat.com': Can't contact
LDAP server
The command also has a wrong operation_name. This is what happens when a
script name is hard-coded and it is not detected automatically, e.g.
from __file__.
Martin
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
