[Freeipa-devel] [PATCH] 1050 prevent replica orphans

[Martin Kosek]

On 09/14/2012 09:16 PM, Rob Crittenden wrote:
> Martin Kosek wrote:
>> On 09/10/2012 08:34 PM, Rob Crittenden wrote:
>>> Martin Kosek wrote:
>>>> On Thu, 2012-09-06 at 17:22 -0400, Rob Crittenden wrote:
>>>>> Martin Kosek wrote:
>>>>>> On 08/31/2012 07:40 PM, Rob Crittenden wrote:
>>>>>>> Rob Crittenden wrote:
>>>>>>>> It was possible use ipa-replica-manage connect/disconnect/del to end up
>>>>>>>> orphaning or or more IPA masters. This is an attempt to catch and
>>>>>>>> prevent that case.
>>>>>>>>
>>>>>>>> I tested with this topology, trying to delete B.
>>>>>>>>
>>>>>>>> A <-> B <-> C
>>>>>>>>
>>>>>>>> I got here by creating B and C from A, connecting B to C then deleting
>>>>>>>> the link from A to B, so it went from A -> B and A -> C to the above.
>>>>>>>>
>>>>>>>> What I do is look up the servers that the delete candidate host has
>>>>>>>> connections to and see if we're the last link.
>>>>>>>>
>>>>>>>> I added an escape clause if there are only two masters.
>>>>>>>>
>>>>>>>> rob
>>>>>>>
>>>>>>> Oh, this relies on my cleanruv patch 1031.
>>>>>>>
>>>>>>> rob
>>>>>>>
>>>>>>
>>>>>> 1) When I run ipa-replica-manage del --force to an already uninstalled host,
>>>>>> the new code will prevent me the deletation because it cannot connect to
>>>>>> it. It
>>>>>> also crashes with UnboundLocalError:
>>>>>>
>>>>>> # ipa-replica-manage del vm-055.idm.lab.bos.redhat.com --force
>>>>>>
>>>>>> Unable to connect to replica vm-055.idm.lab.bos.redhat.com, forcing removal
>>>>>> Traceback (most recent call last):
>>>>>>      File "/sbin/ipa-replica-manage", line 708, in <module>
>>>>>>        main()
>>>>>>      File "/sbin/ipa-replica-manage", line 677, in main
>>>>>>        del_master(realm, args[1], options)
>>>>>>      File "/sbin/ipa-replica-manage", line 476, in del_master
>>>>>>        sys.exit("Failed read master data from '%s': %s" % (delrepl.hostname,
>>>>>> str(e)))
>>>>>> UnboundLocalError: local variable 'delrepl' referenced before assignment
>>>>>
>>>>> Fixed.
>>>>>
>>>>>>
>>>>>>
>>>>>> I also hit this error when removing a winsync replica.
>>>>>
>>>>> Fixed.
>>>>>
>>>>>>
>>>>>>
>>>>>> 2) As I wrote before, I think having --force option override the user
>>>>>> inquiries
>>>>>> would benefit test automation:
>>>>>>
>>>>>> +            if not ipautil.user_input("Continue to delete?", False):
>>>>>> +                sys.exit("Aborted")
>>>>>
>>>>> Fixed.
>>>>>
>>>>>>
>>>>>>
>>>>>> 3) I don't think this code won't cover this topology:
>>>>>>
>>>>>> A - B - C - D - E
>>>>>>
>>>>>> It would allow you deleting a replica C even though it would separate A-B
>>>>>> and
>>>>>> D-E. Though we may not want to cover this situation now, what you got is
>>>>>> definitely helping.
>>>>>
>>>>> I think you may be right. I only tested with 4 servers. With this B and
>>>>> D would both still have 2 agreements so wouldn't be covered by the last
>>>>> link test.
>>>>
>>>> Everything looks good now, so ACK. We just need to push it along with
>>>> CLEANALLRUV patch.
>>>>
>>>> Martin
>>>>
>>>
>>> Not to look a gift ACK In the mouth but here is a revised patch. I've added a
>>> cleanup routine to remove an orphaned master properly. If you had tried the
>>> mechanism I outlined in the man page it would have worked but was
>>> less-than-complete. This way is better, just don't try it on a live master.
>>>
>>> I also added a cleanruv abort command, in case you want to kill an existing
>>> task.
>>>
>>> rob
>>>
>>
>> 1) CLEANRUV stuff should be in your patch 1031 and not here (but I will comment
>> on the code in this mail since it is in this patch)
>>
>>
>> 2) In new command defitinion:
>>
>> +    "abort-clean-ruv":(1, 1, "Replica ID of to abort cleaning", ""),
>>
>> I miss error message in case REPLICA_ID is not passed, this way error message
>> when I omit the parameter is confusing:
>>
>> # ipa-replica-manage abort-clean-ruv
>> Usage: ipa-replica-manage [options]
>>
>> ipa-replica-manage: error: must provide a command [force-sync | clean-ruv |
>> disconnect | connect | list-ruv | del | re-initialize | list | abort-clean-ruv]
>>
>> On another note, I am thinking about the new command(s). Since we now have
>> abort-clean-ruv command, we may want to also implement list-clean-ruv commands
>> in the future to see all CLEANALLRUV commands in process... Or we may enhance
>> list-ruv to write a flag like [CLEANALLRUV in process] for RUV's for which
>> CLEANALLRUV task is in process.
>>
>>
>> 3) Will clean-ruv - abort-clean-ruv - clean-ruv sequence work? If the aborted
>> CLEANALLRUV task stays in DIT, we may not be able to enter new CLEANALLRUV task
>> because we always use the same DN...
>>
>> Btw. did abort CLEANALLRUV command worked for you? Mine seemed to be stuck on
>> replicas that are down just like CLEANALLRUV command. I had then paralelly
>> running CLEANALLRUV and ABORT CLEANALLRUV running for the same RUV ID. Then, it
>> is unclear to me what this command is actually good for.
>>
>>
>> 4) Man page now contains non-existent command:
>>
>> --- a/install/tools/man/ipa-replica-manage.1
>> +++ b/install/tools/man/ipa-replica-manage.1
>> @@ -42,12 +42,18 @@ Manages the replication agreements of an IPA server.
>>   \fBforce\-sync\fR
>>   \- Immediately flush any data to be replicated from a server specified with
>> the \-\-from option
>>   .TP
>> +\fBcleanup\fR
>> +\- Remove leftover references to a deleted master.
>> +.TP
>>
>>
>> The cleanup procedure was implemented rather as an option to the del command
>> than a separate command.
>>
>>
>> 5) My favorite - new cleanup procedure user prompt miss the --force option
>> useful for test automation
>>
>> +            if not ipautil.user_input("Continue to clean master?", False):
>> +                sys.exit("Cleanup aborted")
>>
>>
>> Otherwise the patch looks good.
>>
>> Martin
>>
> 
> I pulled the abort-ruv stuff out. It was just easier to stuff it in here than
> rebasing back, but yeah, its cleaner this way.
> 
> No need to check force for clean because it is already required (can't contact
> the deleted master, it's gone).
> 
> rob

The patch works fine now. So ACK when the dependent patch 1031 is pushed.

Martin