On Tue, Jul 10, 2012 at 4:33 PM, Iustin Pop <[email protected]> wrote:
> On Tue, Jul 10, 2012 at 12:53:46PM +0200, Agata Murawska wrote:
>> When we delete DRBD disks from some instance, we do not want to get
>> errors due to nodes other than that instance's primary being offline.
>>
>> Signed-off-by: Agata Murawska <[email protected]>
>> ---
>> lib/cmdlib.py | 11 +++++++----
>> 1 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/cmdlib.py b/lib/cmdlib.py
>> index dde940d..d4915a2 100644
>> --- a/lib/cmdlib.py
>> +++ b/lib/cmdlib.py
>> @@ -9015,11 +9015,14 @@ def _RemoveDisks(lu, instance, target_node=None,
>> ignore_failures=False):
>> edata = device.ComputeNodeTree(instance.primary_node)
>> for node, disk in edata:
>> lu.cfg.SetDiskID(disk, node)
>> - msg = lu.rpc.call_blockdev_remove(node, disk).fail_msg
>> - if msg:
>> + result = lu.rpc.call_blockdev_remove(node, disk)
>> + if result.fail_msg:
>> lu.LogWarning("Could not remove disk %s on node %s,"
>> - " continuing anyway: %s", idx, node, msg)
>> - all_result = False
>> + " continuing anyway: %s", idx, node, result.fail_msg)
>> + if result.offline and node != instance.primary_node:
>> + continue
>> + else:
>> + all_result = False
>
> This seems wrong (but might not be; the commit message is not clear
> enough, only talks about messages but not change behaviour).
Sorry, I do not understand the part about the commit message talking
about messages only. I can try to reword it, but for me an error = failed
removal, so it was clear that this ignores offline secondaries (as in
"does not produce an error").
> You don't
> want to "continue", but simply not set all_result to false; as you still
> want to do other cleanup, like return the disk to the pool, etc.
I'm confused - I thought that we do, no? The continue is just for the inner
loop. I can make the `if` test negative, I just found
"result.offline and node != instance.primary_node"
an easier logic explanation (as in "this is a special case, we ignore this
error") than the
"not result.offline or node == instance.primary_node" ("these are the
cases in which error should be produced").
>
> iustin
Agata
