[PATCH] Use lvconvert --repair as a dmeventd mirror failure handler

[PATCH] Use lvconvert --repair as a dmeventd mirror failure handler

[Petr Rockai]

Hi,

this one-liner enables the dmeventd mirror monitoring DSO to use lvconvert
--repair to fix any broken mirrors. This means two things:

1) Non-mirrored LVs will be no longer affected by mirror monitoring. (Before,
if you had a LV that went partially missing on a VG where a mirror leg failed,
this LV would be removed automatically by dmeventd... Probably not an
unrecoverable dataloss bug, but still quite unpleasant.)

2) If enough parallel PV space is available at the time of the mirror failure,
the failed devices will be automatically replaced using this spare space. Which
(and whether) free space may be used is still not configurable, but is a
planned feature. Since it is relatively easy to undo the action by converting
the mirror manually, I don't consider this to be a showstopper. In fact, I
think the compromise is much better than what we have now.

I have tested the patch and mirrors get repaired for me automatically when
things fail. This does not equate thorough testing, but lvconvert --repair is
already covered by the testsuite for a few weeks now and so far no obvious
issues appeared.

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmevent-lvconvert-mirrors.diff
Type: text/x-diff
Size: 956 bytes
Desc: dmeventd-lvconvert-mirrors.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090519/dbd7f5d5/attachment.bin>
-------------- next part --------------

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH] Use lvconvert --repair as a dmeventd mirror failure handler

[Milan Broz]

Petr Rockai wrote:
> this one-liner enables the dmeventd mirror monitoring DSO to use lvconvert
> --repair to fix any broken mirrors. This means two things:

ack,

it is just finalisation of previous work.

Milan

[PATCH] Use lvconvert --repair as a dmeventd mirror failure handler

[Jonathan Brassow]

On May 19, 2009, at 1:01 AM, Petr Rockai wrote:

> 2) If enough parallel PV space is available at the time of the  
> mirror failure,
> the failed devices will be automatically replaced using this spare  
> space. Which
> (and whether) free space may be used is still not configurable, but  
> is a
> planned feature. Since it is relatively easy to undo the action by  
> converting
> the mirror manually, I don't consider this to be a showstopper. In  
> fact, I
> think the compromise is much better than what we have now.

Wait, what?  Are you saying that with this change, it /will/ find  
space for a new mirror leg?  It doesn't do that now.  I also don't  
think lvrepair is the right place to have the allocation take place.   
'lvrepair' should do exactly that - repair.  From there, you could do  
an lvconvert.  The mirror DSO should do the 'lvconvert' portion based  
on user set policy found in /etc/lvm/lvm.conf -- see  
'mirror_log_fault_policy' and 'mirror device_fault_policy'.

Am I missing something?

  brassow

[PATCH] Use lvconvert --repair as a dmeventd mirror failure handler

[Petr Rockai]

Jonathan Brassow <jbrassow@redhat.com> writes:
> Wait, what?  Are you saying that with this change, it /will/ find space for a
> new mirror leg?  It doesn't do that now.  I also don't  think lvrepair is the
> right place to have the allocation take place.   'lvrepair' should do exactly
> that - repair.  From there, you could do  an lvconvert.  The mirror DSO should
> do the 'lvconvert' portion based  on user set policy found in /etc/lvm/lvm.conf
> -- see  'mirror_log_fault_policy' and 'mirror device_fault_policy'.
>
> Am I missing something?

Yes, the current policy is completely bogus. It will kill LVs that are
completely unrelated to the mirror in question (if they happen to have any
extents allocated in any currently missing PV, which may be even unrelated to
the one that caused the mirror failure). Moreover, I don't know what you mean
with "lvrepair" since there's no such thing. And lvconvert --repair does
exactly repair, by either removing (no parallel space available) or replacing
(free parallel space available) missing devices.

The mirror_log_fault_policy and device_fault_policy is all cool, but it's not
implemented (even the fact it's in lvm.conf while there's no code to handle the
options is quite silly). So I expect that an --auto switch will need to be
added to lvconvert --repair, that will honour those two configuration options.

Please also note that this could have been fixed months ago (the patches have
been in review since last July at least -- see eg. message-id
<87tze8244p.fsf@eriador.mornfall.net>). Please next time, if you know all along
that a policy change is not acceptable, take a few minutes and reply to the
proposed patch. Thanks!

(Just as a rationale, I did not implement the current lvm.conf options simply
because it has been suggested, that a much more complex configuration using
tags is planned. It just seemed redundant to implement options that would
become deprecated in the next release. Unfortunately, no-one has pointed out
that they need to be, or why.)

As for doing things directly in the mirror DSO (as compared to lvconvert
--repair), it would get much more complicated, implementation-wise. It would
also lead to lots of code duplication (and the theoretical advantage of having
the code separated is dubious, too).

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH] Use lvconvert --repair as a dmeventd mirror failure handler

[Jonathan Brassow]

On May 21, 2009, at 4:15 AM, Petr Rockai wrote:

> Jonathan Brassow <jbrassow@redhat.com> writes:
>> Wait, what?  Are you saying that with this change, it /will/ find  
>> space for a
>> new mirror leg?  It doesn't do that now.  I also don't  think  
>> lvrepair is the
>> right place to have the allocation take place.   'lvrepair' should  
>> do exactly
>> that - repair.  From there, you could do  an lvconvert.  The mirror  
>> DSO should
>> do the 'lvconvert' portion based  on user set policy found in /etc/ 
>> lvm/lvm.conf
>> -- see  'mirror_log_fault_policy' and 'mirror device_fault_policy'.
>>
>> Am I missing something?
>
> Yes, the current policy is completely bogus. It will kill LVs that are
> completely unrelated to the mirror in question (if they happen to  
> have any
> extents allocated in any currently missing PV, which may be even  
> unrelated to
> the one that caused the mirror failure). Moreover, I don't know what  
> you mean
> with "lvrepair" since there's no such thing. And lvconvert --repair  
> does
> exactly repair, by either removing (no parallel space available) or  
> replacing
> (free parallel space available) missing devices.
>
> The mirror_log_fault_policy and device_fault_policy is all cool, but  
> it's not
> implemented (even the fact it's in lvm.conf while there's no code to  
> handle the
> options is quite silly). So I expect that an --auto switch will need  
> to be
> added to lvconvert --repair, that will honour those two  
> configuration options.
>
> Please also note that this could have been fixed months ago (the  
> patches have
> been in review since last July at least -- see eg. message-id
> <87tze8244p.fsf@eriador.mornfall.net>). Please next time, if you  
> know all along
> that a policy change is not acceptable, take a few minutes and reply  
> to the
> proposed patch. Thanks!
>
> (Just as a rationale, I did not implement the current lvm.conf  
> options simply
> because it has been suggested, that a much more complex  
> configuration using
> tags is planned. It just seemed redundant to implement options that  
> would
> become deprecated in the next release. Unfortunately, no-one has  
> pointed out
> that they need to be, or why.)
>
> As for doing things directly in the mirror DSO (as compared to  
> lvconvert
> --repair), it would get much more complicated, implementation-wise.  
> It would
> also lead to lots of code duplication (and the theoretical advantage  
> of having
> the code separated is dubious, too).

Of course the current policy is bogus.  Forgive my typo, s/lvrepair/ 
lvconvert --repair/.

I guess the thing I care most about is that we are switching from a  
policy of "just remove the failed device" to "just replace the device  
(if possible)".  The methods you are using to repair are far superior  
to what we had (addresses the "bogus" part in the current  
implementation).  However, I don't understand why we are now  
automatically taking the next step.  If we allow for user  
specification of policy - then why not wait for that before taking  
action to replace the device?  Otherwise we could be flip-flopping on  
the (default) policy.

  brassow
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090521/e6b228e6/attachment.htm>