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

[Petr Spacek]

On 30.3.2016 15:13, Martin Babinsky wrote:
> On 03/30/2016 02:32 PM, Petr Vobornik wrote:
>> 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.
>>
> well in both ways the move requires a conditional import of server-side
> modules like this so that it does not break clients:
> """
> if api.env.in_server:
>     from ipaserver.install import bindinstance, dnskeysyncinstance
>     # do magic
> """
> 
> which will make people like Jan Cholasta cringe mightily.
> 
> I will try to move the code around so that we can avoid these hacks.

BTW I will have to rewrite most of the DNS record management for DNS locations
anyway so you might want to let it be as it for now and I will do it at once
together with rest of the codebase.

Petr^2 Spacek

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