[Freeipa-devel] [WIP PATCH] server-del: perform full master removal in managed topology

[Martin Babinsky]

On 04/14/2016 11:46 AM, Ludwig Krispenz wrote:
>
> On 04/14/2016 10:59 AM, Martin Babinsky wrote:
>> On 04/14/2016 08:24 AM, Jan Cholasta wrote:
>>> On 13.4.2016 17:10, Rob Crittenden wrote:
>>>> Martin Babinsky wrote:
>>>>> This is a WIP patch which moves the `ipa-replica-manage del`
>>>>> subcommand
>>>>> to the 'server-del' API method and exposes it as CLI command[1]. A CI
>>>>> test suite is also included.
>>>>>
>>>>> There are some issues with the patch I would like to discuss in more
>>>>> detail on the list:
>>>>>
>>>>> 1.) In the original subcommand there was a lot of output (mostly print
>>>>> statements) during all stages of master removal. I have tried to port
>>>>> these as messages to the command which results in quite voluminous
>>>>> response sent back to the frontend. Should we try to reduce the
>>>>> output?
>>>>
>>>> I don't think it applies anymore. These messages were there so the user
>>>> would know something was happening. If it is an API command there isn't
>>>> much we can do other than add something to the cli to print "This could
>>>> take a bit" before making the call.
>>>
>>> +1
>>>
>> This is already implemented in PoC. So I guess we may reduce the
>> output only to the following:
>>
>>
>> In CLI print:
>> "Removing {server} from replication topology, "
>> "please wait...
>>
>> The adding info messages:
>>
>> "checking topology connectivity" | "skipping topology connectivity check"
>> "checking remaining services" | "skipping check for remaining services"
>> "performing cleanup"
>> "Deleted server {server}"
>>
>>
>>>>
>>>>> 2.) In the original discussion[2] we assumed that the cleanup part
>>>>> would
>>>>> me a separate API method called during server_del postcallback.
>>>>> However
>>>>> since the two objects ended up sharing a lot of state (e.g. topology
>>>>> state from pre-callback, messages) i have merged it to server-del.
>>>>> That
>>>>> makes the code rather unwieldy but I found it difficult to keep the
>>>>> two
>>>>> entities separate without some hacking around framework capabilities
>>>>
>>>> I haven't looked at the code but as a general principal having separate
>>>> operations has saved our bacon on more than one occasion.
>>>
>>> The patch adds a force option, which allows you to re-run server-del
>>> even if the master entry does not exist anymore, so I think we are safe.
>>>
>>>>
>>>>> 3.) since actions in post-callback require a knowledge about topology
>>>>> state gathered in pre-callback, I had to store some information in the
>>>>> command's context. Sorry about that, if you know about some way to
>>>>> circumvent me, let me know.
>>>>
>>>> Looks like it is the only way since you are extending server_del.
>>>> Another option would be to drop pre/post and add all this topology
>>>> stuff
>>>> directly to server_del execute.
>>>>
>>>>> 4.) The master can not remove itself. I have implemented an ad-hoc
>>>>> forwarding of the request to other master that can do the job. Is this
>>>>> okay?
>>>
>>> Why can't the master remove itself?
>>>
>> Because it removes its own replication agreements hence any changes in
>> DIT (like removed principals, s4u2 proxy targets etc.) won't replicate
>> to other masters.
> It shouldn't remove replication agreements, in fact this should be
> prevented by the topology plugin.
> The removal of the agreements will be triggered by removing the master
> entry

That is true, but there is a plenty of cleanup code that is executed 
*after* the master entry is removed and these changes would not 
replicate if the agreements were removed by topology plugin in the 
meanwhile.

>>>>
>>>> Assisted suicide?
>>>>
>>>> What does this effectively do? Potentially disconnect it from the
>>>> topology, perhaps to run as standalone for upgrade, integration or
>>>> other
>>>> testing (e.g. there might be valid reasons to do this)? If so that
>>>> seems
>>>> ok to me, or if too hacky you could just spit out a message directing
>>>> them to disconnect from another host, but that would be strange in the
>>>> UI I think.
>>>>
>>>>> 5.) Since the original behavior of 'chekc_deleted_segments' was kept,
>>>>> the code can sometimes hang waiting for removal of some segments,
>>>>> especially during forced removal in wonky topologies. This can cause
>>>>> gateway timeout in JSON-RPC call and report false positive error
>>>>> back to
>>>>> the user. This makes a large part of 'TestServerDel' suite to fail.
>>>>> How
>>>>> should we handle this? Should we keep the original behavior?
>>>>
>>>> Oh to have async calls...
>>>>
>>>> I wonder if "I'm still here" messages could be sent. I'm not entirely
>>>> sure this is possible with the framework but it might be one way to
>>>> keep
>>>> long-lived connections alive.
>>>
>>> Impossible, everything is synchronous.
>>>
>>>>
>>>>> 6.) There are some in-place imports of server-side code in some
>>>>> places.
>>>>> I'm not very happy about it and would be glad if we can agree on a way
>>>>> to fix this.
>>>>
>>>> Is this to prevent imports on non-servers? You might look at trust for
>>>> inspiration, it does this.
>>>
>>> I wouldn't worry about this, I'm going to move most plugin code to
>>> ipaserver as part of the thin client feature anyway ;-)
>>>
>>> Honza
>>>
>>
>>
>


-- 
Martin^3 Babinsky