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

[Martin Babinsky]

On 01/23/2015 05:56 PM, Martin Basti wrote:
> 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
>
Thanks for suggestions, attaching updated patch

Martin^3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0003-4-use-remove-ds.pl-to-remove-DS-instance.patch
Type: text/x-patch
Size: 8866 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150126/0a804e13/attachment.bin>