[Freeipa-devel] [PATCH 0153-0158] move ipa-replica-manage del functionality into server-del
Martin Babinsky
mbabinsk at redhat.com
Fri Jun 17 14:18:36 UTC 2016
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.
--
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0153-5-ipaserver-module-for-working-with-managed-topology.patch
Type: text/x-patch
Size: 10033 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160617/2252c6c3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0154-4-delegate-removal-of-master-DNS-record-and-replica-ke.patch
Type: text/x-patch
Size: 2983 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160617/2252c6c3/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0155-4-server-del-perform-full-master-removal-in-managed-to.patch
Type: text/x-patch
Size: 22324 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160617/2252c6c3/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0156-4-CI-test-suite-for-server-del.patch
Type: text/x-patch
Size: 16669 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160617/2252c6c3/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0157-4-ipa-replica-manage-use-server_del-when-removing-doma.patch
Type: text/x-patch
Size: 12944 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160617/2252c6c3/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0158-4-remove-the-master-from-managed-topology-during-unins.patch
Type: text/x-patch
Size: 10643 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160617/2252c6c3/attachment-0005.bin>
More information about the Freeipa-devel
mailing list