[Freeipa-devel] [PATCH 0153-0158] move ipa-replica-manage del functionality into server-del

[Martin Basti]

On 17.06.2016 16:18, Martin Babinsky wrote:
> On 06/17/2016 03:56 PM, Martin Babinsky wrote:
>> On 06/16/2016 12:45 PM, Martin Basti wrote:
>>>
>>>
>>> On 15.06.2016 15:29, Martin Babinsky wrote:
>>>> On 06/15/2016 10:30 AM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> On 12.6.2016 17:31, Martin Babinsky wrote:
>>>>>> On 06/09/2016 08:12 PM, Martin Babinsky wrote:
>>>>>>> These patches expand `server_del` to a full fledged IPA master
>>>>>>> killer in
>>>>>>> domain level 1.
>>>>>>>
>>>>>>> Due to 'server uninstallation removed master from topology' use 
>>>>>>> case,
>>>>>>> the individual steps are not in the same order as in the original
>>>>>>> code
>>>>>>> to facilitate self-removal from topology without introducing an
>>>>>>> array of
>>>>>>> permissions for master to remove itself.
>>>>>>>
>>>>>>> I had no opportunity to test out the CI test suite because of
>>>>>>> technical
>>>>>>> problems so it would be nice if our upstream QE could give it a
>>>>>>> spin and
>>>>>>> report errors.
>>>>>>>
>>>>>>> http://www.freeipa.org/page/V4/Manage_replication_topology_4_4
>>>>>>> https://fedorahosted.org/freeipa/ticket/5181
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Attaching rebased patches and bumping for review.
>>>>>>
>>>>>> Please note that they depend on 'Server Roles v2' patchset.
>>>>>
>>>>> Patch 0153:
>>>>>
>>>>> Should be an ipaserver module, unless it is required on clients as
>>>>> well,
>>>>> in which case it should be an ipalib module.
>>>>>
>>>>>
>>>>> Patch 0154: LGTM
>>>>>
>>>>>
>>>>> Patch 0155:
>>>>>
>>>>> In LDAPDelete subclasses, the primary key argument is multivalue, so
>>>>> I'm
>>>>> guessing your post_callback won't work correctly.
>>>>>
>>>>> Also, since this is *server*-del, s/master/server/ where applicable.
>>>>>
>>>>>
>>>>> Patch 0156: LGTM
>>>>>
>>>>>
>>>>> Patch 0157:
>>>>>
>>>>> This looks suspicious:
>>>>>
>>>>> +    result = server_del_cmd(hostname, version=api_version, 
>>>>> **options)
>>>>>
>>>>> Version is automatically filled in in Command.__call__(), why do you
>>>>> add
>>>>> it manually here?
>>>>>
>>>>>
>>>>> Patch 0158: LGTM
>>>>>
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> Attaching updated patches.
>>>>
>>>>
>>>>
>>>
>>> Hello, I have a few comments:
>>>
>>> 1)
>>> you reused ID numbers for the your messages
>>>
>>> +class ServerRemovalInfo(PublicMessage):
>>> ...
>>> +    errno = 13020
>>>
>>> +class ServerRemovalWarning(PublicMessage):
>>> ...
>>> +    errno = 13021
>>>
>>>
>>>  class FailedToRemoveHostDNSRecords(PublicMessage):
>>> ...
>>>      errno = 13020
>>>
>>>
>>>  class DNSForwardPolicyConflictWithEmptyZone(PublicMessage):
>>> ...
>>>      errno = 13021
>>>
>>> 2)
>>> +    def _check_topology_connectivity(self, topology_connectivity,
>>> master_cn):
>>> +        try:
>>> +            topology_connectivity.check_current_state()
>>> +        except ValueError as e:
>>> +            raise errors.ServerRemovalError(reason=_(str(e)))
>>> +
>>> +        try:
>>> + topology_connectivity.check_state_after_removal(master_cn)
>>> +        except ValueError as e:
>>> +            raise errors.ServerRemovalError(reason=_(str(e)))
>>>
>>> * _(str(e)): gettext cannot be used by this way
>>> * str(e): you dont need to convert exception to string, this is done
>>> automatically in exception
>>>
>>>
>>> 3) gettext again
>>> +                self.add_message(
>>> +                    messages.ServerRemovalWarning(
>>> +                        message=_(msg)
>>> +                    )
>>> +                )
>>>
>>>
>>> 4)
>>> +                    messages.ServerRemovalWarning(
>>> +                        message=_("Failed to clean memberPrincipal
>>> {principal}"
>>> +                                  " from s4u2proxy entry {dn}:
>>> {err}".format(
>>> + principal=member_principal,
>>> +                                      dn=dn,
>>> +                                      err=e))))
>>>
>>> +                messages.ServerRemovalWarning(
>>> +                    message=_("Failed to clean up DNA hostname entries
>>> for "
>>> +                              "{master}: {err}".format(
>>> +                                  master=master, err=e))))
>>>
>>> several more times
>>>
>>> I'm not sure if this will work, for safety I would prefer to change it
>>> to dictionary substitution
>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=668226
>>> It looks like it was fixed in gettext 18.3, some distributions still
>>> have the older one
>>>
>>> I have to test more how gettext works with the new python format 
>>> strings
>>>
>>> 5)
>>> +    def interactive_prompt_callback(self, kw):
>>> +        self.api.Backend.textui.print_plain(
>>> +            _("Removing {server} from replication topology, "
>>> +              "please wait...".format(server=', '.join(kw['cn']))))
>>>
>>> Will this work? IMO this should be on client side
>>>
>>>
>>>
>>>
>> Updated and rebased patches attached.
>>
>>
>>
> I made an error during rebase of patch 153. Re-sending the whole batch 
> with the correct one.
>
ACK, rebased and pushed
master:
* d8ae2b4055284de8c1baf76819d6611978f83cc6 ipaserver module for working 
with managed topology
* db882ae8d6eba768e08be9317e386f8ab3c8fcf7 delegate removal of master 
DNS record and replica keys to separate functions
* a6eb87bd68295e15ea19f5cb274cffbef5954d04 server-del: perform full 
master removal in managed topology
* 081941a5b9c9ac8832c465b857032e474bb9b09f CI test suite for `server-del`
* 47decc9b843b1b1d292511bcc8a24f8ac85745c0 ipa-replica-manage: use 
`server_del` when removing domain level 1 replica
* 31ffe1a12922b5118c847cbd6ac1ca9ea232ef94 remove the master from 
managed topology during uninstallation