[Freeipa-devel] [PATCH 003] Use 'remove-ds.pl' to remove DS instance during server uninstall

[Martin Basti]

On 22/01/15 15:03, Martin Babinsky wrote:
> On 01/22/2015 12:38 PM, Martin Babinsky wrote:
>> On 01/22/2015 12:19 PM, Martin Kosek wrote:
>>> On 01/22/2015 11:58 AM, Martin Babinsky wrote:
>>>> On 01/22/2015 11:04 AM, Martin Babinsky wrote:
>>>>> The attached patch addresses
>>>>> https://fedorahosted.org/freeipa/ticket/4487.
>>>>>
>>>>> Using 'remove-ds.pl' script from 389-ds during server/replica
>>>>> uninstallation should allow for cleaner removal of DS instance 
>>>>> with no
>>>>> leftovers (opened ports etc).
>>>>>
>>>>> Martin^3
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Freeipa-devel mailing list
>>>>> Freeipa-devel at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>>
>>>> Thanks to Martin^2 for explaining the rules governing the placement
>>>> of new
>>>> attributes into BasePathNamespace class.
>>>>
>>>> Attaching updated patch.
>>>>
>>>> Martin^3
>>>
>>> I see you kept erase_ds_instance_data still in the dsinstance. What is
>>> the
>>> motivation? I thought that just like with PKI, with DS we will also 
>>> have
>>> uninstall based solely only on the DS uninstall script and remove our
>>> custom
>>> removal.
>>>
>>> We can keep the manual removal somewhere in wiki, but I would
>>> personally not
>>> keep/maintain it in FreeIPA code.
>>>
>> I originally thought that I will keep erase_ds_instance-data as a
>> failsafe method to clean up the dirsrv data in th case that remove-ds.pl
>> fails.
>>
>> But I will remove the method altogether and change the code so that it
>> prints out some warning about manual removal of DS data when
>> remove-ds.pl fails.
>>
>> Martin^2
>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
> Updated patch attached
>
> Martin^3
>

Hi,

thank you for patch, but I have a few nitpicks:

1) Extra parenthesis
+                root_logger.error(("Failed to remove CA DS instance. 
You may "
+                                   "need to remove instance data 
manually"))

and (twice)

+                root_logger.error(("Failed to remove DS instance. You may "
+                                   "need to remove instance data 
manually"))

and

+        root_logger.debug(("'%s' failed. "
+                          "Attempting to force removal" % 
paths.REMOVE_DS_PL))

2) Remove unused paths:
USR_LIB_DIRSRV_SLAPD_INSTANCE_DIR_TEMPLATE
SLAPD_INSTANCE_LOCK_TEMPLATE
USR_LIB_SLAPD_INSTANCE_TEMPLATE

Please do double check.

3) IMO we should avoid hardcoded value 'slapd'
instance_name = '-'.join(['slapd', serverid])

you may create module variable DS_INSTANCE_PREFIX and use it. Dont 
forget to change it in following place as well.
ipaserver/install/dsinstance.py:117:    instance_prefix = 'slapd-'

4)
DS keytab hasn't been removed, it is okay?
It is created by IPA (krbinstance), not by DS install script.


Martin^2

-- 
Martin Basti