[Freeipa-devel] server_del (re)implementation in domain level 1 topology management

Wed Mar 30 12:04:36 UTC 2016

On 03/17/2016 04:15 PM, Petr Vobornik wrote:
> On 03/17/2016 04:10 PM, Martin Babinsky wrote:
>> On 03/17/2016 03:37 PM, Petr Vobornik wrote:
>>> On 03/17/2016 03:17 PM, Martin Babinsky wrote:
>>>> On 03/17/2016 02:55 PM, Petr Vobornik wrote:
>>>>> On 03/17/2016 02:39 PM, Martin Babinsky wrote:
>>>>>> Hi list,
>>>>>>
>>>>>> I would like to discuss the merge of `del_master_managed()` function
>>>>>> from `ipa-replica-manage` command into the server_del API call that
>>>>>> is a
>>>>>> part of the managed replication topology design update[1] (see also
>>>>>> the
>>>>>> corresponding upstream ticket [2]).
>>>>>>
>>>>>> Before I head down into coding I want to be sure that everyone is one
>>>>>> the same page regarding the expected use-cases which govern the API
>>>>>> design.
>>>>>>
>>>>>> IIUC, there are two main uses of the new functionality according to
>>>>>> design document:
>>>>>>
>>>>>> 1.) run 'server_del' when 'ipa-replica-manage del' is run in
>>>>>> domain-level 1
>>>>>
>>>>> Right, this is for backwards compatibility(BC).
>>>>>
>>>>>>
>>>>>> 2.) during 'ipa-server-install --uninstall', 'server_del' should be
>>>>>> called on one of remote masters to remove the uninstalled server from
>>>>>> the managed topology
>>>>>>
>>>>>> What I didn't get from the design document is whether the method
>>>>>> should
>>>>>> have some kind of 'force' option which should bypass all topology
>>>>>> connectivity checks. Currently both `ipa-replica-manage del` and
>>>>>> server
>>>>>> uninstaller have options which will force the removal even if it
>>>>>> disconnects the topology ('--force' in the former,
>>>>>> '--ignore-disconnected-topology' in the latter).
>>>>>
>>>>> I would say that uninstaller should do checks in validate method
>>>>> therefore the subsequent `server-del` doesn't need to do it again
>>>>> but it
>>>>> shouldn't harm. I.e. it should follow what the user specified. If user
>>>>> wants to skip (--ignore-d..-t..) then skip. If not then it will
>>>>> fail in
>>>>> validate method.
>>>>>
>>>>> Only issue might be error state where servers have different
>>>>> picture of
>>>>> the topology.
>>>>>
>>>> If the view of the topology is not self-consistent then you have plenty
>>>> of other issues to take care of and that may include some forced
>>>> removal
>>>> and recreation of nodes.
>>>>
>>>>>>
>>>>>> I guess the 'server_del' method should inherit this flag so that we
>>>>>> retain the original functionality (for better or worse). I propose to
>>>>>> name this option 'ignore_topology_disconnect' because it is more
>>>>>> descriptive than plain 'force'.
>>>>>
>>>>> +1
>>>>>
>>>>> And in BC case, `ipa-replica-manage --force` would call `server-del
>>>>> --ig..-d..-t...`
>>>>>
>>>> Yes.
>>>>>>
>>>>>> I would also like to ask whether 'server_del' (which is currently
>>>>>> NO_CLI) should be usable also from command line.
>>>>>
>>>>> IMO yes, it should mostly as a couterpart of `ipa-replica-manage
>>>>> --force
>>>>> --clean`
>>>>>
>>>>> Which bring us to --clean option and what it should do...
>>>>>
>>>> According to the design, '--clean' should be used as a cleanup of
>>>> leftovers after deleted servers. How I image it from the implementation
>>>> point of view is that when '--clean' is specified and the server was
>>>> already deleted, the NotFound error raised from the framework should be
>>>> ignored and the code should continue in clean up. (I assume that
>>>> segment/service/dns cleanup will be done in post_callback portion and
>>>> the topology connectivity/sanity checks in the pre_callback).
>>>
>>> When thinking about it, clean could be a separate command which would be
>>> called internally in post callback of server-del. It would reduce the
>>> number of ifs in server-del and simplify it in general. It would work
>>> only if server entry doesn't exists.
>>>
>> That was my original idea. I also thought that 'check_last_link_managed'
>> could be a separate command, but it is probably not a very good idea to
>> add the overhead of calling two separate commands to a single API call.
>> OTOH it would improve the code organization IMHO.
>
> Not sure if check_last_link_managed should be an API command. It is
> already a separate function. What would be the use case for it as a
> command?
>
> Maybe the function should be moved from ipaserver/install/replication to
> a more suitable place given that it's used from API - it's up to you.
>
>>
>>>> That means that '--clean' has no additional effect when the server
>>>> exists.
>>>
>>> Right
>>>
>>>>>>
>>>>>>
>>>>>> [1] http://www.freeipa.org/page/V4/Manage_replication_topology_4_4
>>>>>> [2] https://fedorahosted.org/freeipa/ticket/5588
>>>>>>
>>
>>

There are two more things I would like to clarify before I can finish 
the patch:

1.) We have discussed offline that the ensure_last_services function[1] 
shall be reimplemented in the API command using server roles (I will 
open a ticket for this when time comes). Currently the code works 
interactively, prompting the user about removing last DNS/CA/etc server. 
The behavior is overriden by --force option.

I guess we do not want to have interactive prompting in the API command, 
so we will have to handle this somehow. A proposal would be to by 
default abort removal of last DNS/CA server and add a '--force' option 
which will override this check and also override the disconnected 
topology check (IMHO it is not of much use to keep both options in this 
case). What do you think?

2.) Removal of DNS entries is handled by directly calling 
bindinstance/dnskeysyncinstance code[2]. Obviously this is not very 
desirable in the context of API code since we would then need to 
conditionally import these modules on server side (one option but not 
very nice IMHO). Should I reimplement this code using API commands or 
move the relevant bits into ipapython/ipalib?

[1] 
https://git.fedorahosted.org/cgit/freeipa.git/tree/install/tools/ipa-replica-manage?h=ipa-4-3#n753

[2] 
https://git.fedorahosted.org/cgit/freeipa.git/tree/install/tools/ipa-replica-manage?h=ipa-4-3#n810

-- 
Martin^3 Babinsky