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

Wed Mar 30 12:32:54 UTC 2016

On 03/30/2016 02:04 PM, Martin Babinsky wrote:
> 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?

In general +1.

* move services automatically but fail if it is the last
* have override param to force it. It can be mapped to `ipa server-del 
--force`. Or maybe other name than --force (too easy to use, very general).

>
> 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?

Both methods (ensure_last_services, removal of DNS records) should be 
moved. There is no need for separate API calls.

But we might want to consider other option if simple move is not so simple.

>
>
> [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
>
>

-- 
Petr Vobornik