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

[Martin Babinsky]

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.
>
I will move the required code from ipaserver/install/replication anyway 
since it needs to be usable also for clients, and it also needs 
substantial modification so that it can fit into API context (prune all 
prints, etc.).

I can't think about any use case so there is probably no point in 
'commandifying' it:).
>>
>>>> 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
>>>>>>
>>
>>


-- 
Martin^3 Babinsky