[Freeipa-devel] [PATCH 0153-0158] move ipa-replica-manage del functionality into server-del
Martin Babinsky
mbabinsk at redhat.com
Fri Jun 17 13:56:05 UTC 2016
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.
--
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0153-4-ipaserver-module-for-working-with-managed-topology.patch
Type: text/x-patch
Size: 9954 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160617/1d5f3332/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/1d5f3332/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/1d5f3332/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/1d5f3332/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/1d5f3332/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/1d5f3332/attachment-0005.bin>
More information about the Freeipa-devel
mailing list