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

Mon Apr 4 05:58:49 UTC 2016

On 30.3.2016 14:32, 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).

Actually I would like if *all* del commands had --force, which, if used, 
would make the commands idempotent - if the target object does not 
exist, do not fail - just like rm and virtually every other rm-like command.

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

-- 
Jan Cholasta