[Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater

Wed Jul 18 11:43:24 UTC 2012

On 07/17/2012 10:41 PM, Rob Crittenden wrote:
> Petr Viktorin wrote:
>> On 06/29/2012 11:28 PM, Rob Crittenden wrote:
>>> Petr Viktorin wrote:
>>>> On 06/25/2012 03:00 PM, Petr Viktorin wrote:
>>>>> On 06/20/2012 06:15 PM, Rob Crittenden wrote:
>>>>>> Petr Viktorin wrote:
>>>>>>> On 06/04/2012 04:56 PM, Petr Viktorin wrote:
>>>>>>>> Currently, FreeIPA's install/admin scripts are long pieces of code
>>>>>>>> that aren't very reusable, importable, or testable.
>>>>>>>> They have been extended over time with features such as logging and
>>>>>>>> error handling, but since each tool was extended individually,
>>>>>>>> there
>>>>>>>> is much inconsistency and code duplication.
>>>>>>>> This patch starts a framework which the admin tools can use, and
>>>>>>>> converts ipa-ldap-updater to use the framework.
>>>>>>>>
>>>>>>>> In an earlier patch I found that improving a particular
>>>>>>>> functionality in
>>>>>>>> all the commands is not workable, so I want to tackle this one tool
>>>>>>>> at a
>>>>>>>> time.
>>>>>>>> I'm starting with ipa-ldap-updater, because it's pretty small,
>>>>>>>> doesn't
>>>>>>>> use DNs (I don't want conflicts with John's work), and has the
>>>>>>>> interesting --upgrade option.
>>>>>>>>
>>>>>>>>
>>>>>>>> The framework does these tasks:
>>>>>>>> - Parse options
>>>>>>>> - Select tool to run (see below)
>>>>>>>> - Validate options
>>>>>>>> - Set up logging
>>>>>>>> - Run the tool code
>>>>>>>> - Handle any errors
>>>>>>>> - Log success/failure
>>>>>>>>
>>>>>>>> The base class has some defaults for these that the tools can
>>>>>>>> extend/override.
>>>>>>>>
>>>>>>>>
>>>>>>>> To handle the case where one script does two different things
>>>>>>>> (ipa-ldap-updater with/without --upgrade, or ipa-server-install
>>>>>>>> with/without --uninstall), I want to split the tool in two classes
>>>>>>>> rather than have repeated ifs in the code.
>>>>>>>> This meant that option parsing (and initializing the parser) has
>>>>>>>> to be
>>>>>>>> done before creating an instance of the tool. I use a factory
>>>>>>>> classmethod.
>>>>>>>>
>>>>>>>>
>>>>>>>> I put the admintool base class in ipapython/ as it should be useful
>>>>>>>> for
>>>>>>>> ipa-client-install as well.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> First part of the work for:
>>>>>>>> https://fedorahosted.org/freeipa/ticket/2652
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Attaching rebased patch.
>>>>>>
>>>>>> I gather you want people to be calling run_cli() in their admin
>>>>>> tools.
>>>>>> Should main() be made private then? I could see someone getting
>>>>>> confused
>>>>>> and using main instead, which would work, but then the return value
>>>>>> might not do the right thing.
>>>>>>
>>>>>> Or maybe just drop run_cli and have main exit with sys.exit()?
>>>>>
>>>>> I don't see why running a command as a Python function should be
>>>>> discouraged. In fact it could even help -- for example logging could
>>>>> only be set up once, so if we call, say, ipa-ldap-updater from
>>>>> ipa-server-install, all related logs would go to a single file.
>>>>> A C-style main (taking a list of arguments and returning the exit
>>>>> status) is a good thing for modularity and testability.
>>>>> The `run_cli` method is just a convenient shortcut for the usual
>>>>> usage,
>>>>> so the calling modules can be as small as possible.
>>>>>
>>>>> If people get confused and call main instead of run_cli, they need to
>>>>> manually pass in sys.argv. I think this is enough of a warning that
>>>>> their assumptions aren't right.
>>>>> To make it even clearer I've removed the possibility to pass None as
>>>>> argv to main() and have it auto-filled.
>>>>>
>>>>> Some relevant reading:
>>>>> http://www.artima.com/weblogs/viewpost.jsp?thread=4829 (old but still
>>>>> valid)
>>>>> http://en.wikipedia.org/wiki/Main_function#Python
>>>>>
>>>>>> It isn't correctly handling the case of an update not found:
>>>>>>
>>>>>> ipa : INFO Parsing file ad
>>>>>> [Errno 2] No such file or directory: 'ad'
>>>>>> ipa : INFO File
>>>>>> "/usr/lib/python2.7/site-packages/ipapython/admintool.py", line
>>>>>> 151, in
>>>>>> execute
>>>>>> self.run()
>>>>>> File
>>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/ipa_ldap_updater.py",
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> line
>>>>>> 180, in run
>>>>>> modified = ld.update(self.files)
>>>>>> File
>>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py",
>>>>>> line 828, in update
>>>>>> sys.exit(1)
>>>>>>
>>>>>> ipa : INFO The ipa-ldap-updater command failed, exception:
>>>>>> SystemExit: 1
>>>>>
>>>>> I've added validation for missing files, and improved the error
>>>>> message
>>>>> ldapupdate raises (for cases the validation doesn't catch, like
>>>>> passing
>>>>> directories or unreadable files).
>>>>> Ideally ldapupdate would not try to handle the error itself, but that
>>>>> code is used in more places that I don't want to break, so I'm leaving
>>>>> the extraneous print in.
>>>>>
>>>>>> Running in test mode with the attached update doesn't seem to work
>>>>>> either. There is nothing special about this file, just something I
>>>>>> had
>>>>>> lying around:
>>>>>>
>>>>>> ipa : INFO File
>>>>>> "/usr/lib/python2.7/site-packages/ipapython/admintool.py", line
>>>>>> 151, in
>>>>>> execute
>>>>>> self.run()
>>>>>> File
>>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/ipa_ldap_updater.py",
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> line
>>>>>> 184, in run
>>>>>> 'Update complete, changes to be made, test mode', 2)
>>>>>>
>>>>>> ipa : INFO The ipa-ldap-updater command failed, exception:
>>>>>> ScriptError:
>>>>>> Update complete, changes to be made, test mode
>>>>>> ipa : ERROR Update complete, changes to be made, test mode
>>>>>>
>>>>>> ipa : ERROR None
>>>>>
>>>>> Fixed.
>>>>>
>>>>>> The unit tests still pass which is good.
>>>>>>
>>>>>> With ipa-ldap-updater the return value is a bit strange. All the
>>>>>> updates
>>>>>> themselves can fail for one reason or another and the command can
>>>>>> still
>>>>>> consider this a success (it may fail because a feature is not
>>>>>> enabled,
>>>>>> for example). Still, the success message displayed at the end is a
>>>>>> bit
>>>>>> jarring when the updates themselves aren't applied. Here is a snippet
>>>>>> when running ad.update live:
>>>>>>
>>>>>> ipa : INFO New entry:
>>>>>> uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
>>>>>> ipa : DEBUG ---------------------------------------------
>>>>>> ipa : DEBUG dn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
>>>>>> ipa : DEBUG add: 'account' to objectClass, current value []
>>>>>> ipa : DEBUG add: updated value [u'account']
>>>>>> ipa : DEBUG ---------------------------------------------
>>>>>> ipa : DEBUG dn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
>>>>>> ipa : DEBUG objectClass:
>>>>>> ipa : DEBUG account
>>>>>> ipa : DEBUG add: 'adtrust' to uid, current value []
>>>>>> ipa : DEBUG add: updated value [u'adtrust']
>>>>>> ipa : DEBUG ---------------------------------------------
>>>>>> ipa : DEBUG dn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
>>>>>> ipa : DEBUG objectClass:
>>>>>> ipa : DEBUG account
>>>>>> ipa : DEBUG uid:
>>>>>> ipa : DEBUG adtrust
>>>>>> ipa : DEBUG ---------------------------------------------
>>>>>> ipa : DEBUG Final value
>>>>>> ipa : DEBUG dn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
>>>>>> ipa : DEBUG objectClass:
>>>>>> ipa : DEBUG account
>>>>>> ipa : DEBUG uid:
>>>>>> ipa : DEBUG adtrust
>>>>>> ipa : INFO Parent DN of
>>>>>> uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
>>>>>> may not exist, cannot create the entry
>>>>>> ipa : INFO The ipa-ldap-updater command was successful
>>>>>> [root at pinto freeipa]# echo $?
>>>>>> 0
>>>>>>
>>>>>> This may be contrasting just because it is a contrived case. The
>>>>>> command
>>>>>> rval is separate from whether the updates all applied, so maybe this
>>>>>> is ok.
>>>>>
>>>>> The current ipa-ldap-updater also works this way, so this should go
>>>>> in a
>>>>> separate ticket.
>>>>> I worry that changing the return value could make installations fail,
>>>>> for example.
>>>>>
>>>>>> rob
>>>>>
>>>>>
>>>>> Thanks for the review!
>>>>>
>>>>
>>>> Once again, this time with the patch.
>>>
>>> Almost there. When running in test mode and an update that would be
>>> applied should return 2.
>>>
>>> rob
>>>
>>>
>>
>> Fixed
>>
>>
>
> Ok, things seem to work. One last question.
>
> This puts all the guts of the work into a file in ipaserver/install/.
>
> Is there any benefit to that over putting the class into the tool
> itself? I'm trying to think ahead as more commands get migrated, we're
> just going to be adding more files to ipaserver/install and making
> shells out of the tools in install/tools.
>
> rob

Code in a package is importable and thus testable.
It could go somewhere like ipaserver/install/tools, but I figured the 
ipa_ prefix makes it stand out enough so I left it in ipaserver/install.

Attaching rebased patch.

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0056-05-Framework-for-admin-install-tools-with-ipa-ldap-upda.patch
Type: text/x-patch
Size: 31163 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120718/39e0e659/attachment.bin>