Mailing List Archive: Linux Virtual Server: Users

[lvs-users] Patches for ldirectord

Index | Next | Previous | View Threaded

sean at bruenor

Apr 21, 2009, 10:51 AM

Post #1 of 3 (778 views)
Permalink

[lvs-users] Patches for ldirectord

I have some relatively minor patches for ldirectord to address some
issues we had in our environment. The patches are attached to this
email. All are developed against the mercurial checkout at
http://hg.linux-ha.org/dev/

They are relatively independent in concept, but I developed them
cumulatively, so if they weren't all applied in order I would expect
some issues.

ldirectord-01-cleanup-forked-children.patch:

I observed that when ldirectord exits while using fork=yes the children
forked to perform the checks are not killed or otherwise cleaned up and
continue running. This patch sends the children SIGTERM when ldirectord
exits.

ldirectord-02-dont-disable-realservers-in-forking.patch:

When using fork=no there is a sanity check on the state of the LVS
pools, and ldirectord modifies them as necessary. If there are
realservers already present in existing pools ldirectord doesn't disable
them until checks can complete. However, when using fork=yes this same
sanity check is performed, but all realservers are set down until one
check passes successfully. This patch fixes this behavior so that the
realservers are not set down initially when fork=yes

ldirectord-03-prefix-forked-processes-with-ldirectord.patch:

It is very nice how ldirectord changes the process name of the forked
children to be more informative, but it makes it difficult to easily
grep for all ldirectord related processes and non-obvious for people
less familiar with ldirectord what those processes are. This patch just
prefixes the names of the forked children with "ldirectord", otherwise
keeping the current names.

ldirectord-04-add-cleanstop-option.patch:

We would like the ability to stop ldirectord without it clearing out our
pools. The new "cleanstop" option allows global or per virtual service
ability to disable this cleanup. The default remains to perform the
cleanup.

ldirectord-05-add-per-virtual-service-checkinterval.patch:

The current checkinterval option is global only. However, when using
fork=yes it is possible and sensible to want to adjust the checkinterval
on a per-service basis. This patch allows you to do that, expands on
the documentation for the option, and warns you if you try to set
checkinterval in a virtual service definition when fork=no.

Feedback welcome.

Thanks!

Sean

Attachments:

ldirectord-01-cleanup-forked-children.patch (0.78 KB)

ldirectord-02-dont-disable-realservers-in-forking.patch (0.81 KB)

ldirectord-03-prefix-forked-processes-with-ldirectord.patch (1.06 KB)

ldirectord-04-add-cleanstop-option.patch (2.64 KB)

ldirectord-05-add-per-virtual-service-checkinterval.patch (1.97 KB)

horms at verge

Apr 21, 2009, 5:36 PM

Post #2 of 3 (720 views)
Permalink

Re: [lvs-users] Patches for ldirectord [In reply to]

Hi Sean,

Thanks for all your changes. I have a couple of comments, but they look
good to me. I have pushed them into http://hg.vergenet.net/linux-ha/dev/
and will push them up to http://hg.linux-ha.org/dev/ once my access
to that tree is fixed (sorry for the delay there, I only noticed
it was broken this morning).

One or two things about the patch posting, as I hope you send some
more patches some day :-)

1. I'd prefer one patch per email, as that makes it
easier to discuss individual patches. But its not a big deal.
2. I know the commit logs on linux-ha.org are rather brief,
but I'm happty if you want to include the longer descriptions
you put in the cover portion of your email.
3. Again this isn't really common practice on linux-ha.org,
but if you want to add a Signed-off-by line, that is also fine by me.
I tend to do that myself - except when I forget.

On Tue, Apr 21, 2009 at 01:51:41PM -0400, Sean E. Millichamp wrote:
> I have some relatively minor patches for ldirectord to address some
> issues we had in our environment. The patches are attached to this
> email. All are developed against the mercurial checkout at
> http://hg.linux-ha.org/dev/
>
> They are relatively independent in concept, but I developed them
> cumulatively, so if they weren't all applied in order I would expect
> some issues.
>
> ldirectord-01-cleanup-forked-children.patch:
>
> I observed that when ldirectord exits while using fork=yes the children
> forked to perform the checks are not killed or otherwise cleaned up and
> continue running. This patch sends the children SIGTERM when ldirectord
> exits.

I'm surprised that this is needed. But it isn't the first time this
has come up, so I'm happy top put the change in.

> ldirectord-02-dont-disable-realservers-in-forking.patch:
>
> When using fork=no there is a sanity check on the state of the LVS
> pools, and ldirectord modifies them as necessary. If there are
> realservers already present in existing pools ldirectord doesn't disable
> them until checks can complete. However, when using fork=yes this same
> sanity check is performed, but all realservers are set down until one
> check passes successfully. This patch fixes this behavior so that the
> realservers are not set down initially when fork=yes

I'm not sure that I completely understand the difference in desired
behaviour between forking and non-forking.

> ldirectord-03-prefix-forked-processes-with-ldirectord.patch:
>
> It is very nice how ldirectord changes the process name of the forked
> children to be more informative, but it makes it difficult to easily
> grep for all ldirectord related processes and non-obvious for people
> less familiar with ldirectord what those processes are. This patch just
> prefixes the names of the forked children with "ldirectord", otherwise
> keeping the current names.

Agreed

> ldirectord-04-add-cleanstop-option.patch:
>
> We would like the ability to stop ldirectord without it clearing out our
> pools. The new "cleanstop" option allows global or per virtual service
> ability to disable this cleanup. The default remains to perform the
> cleanup.

Good idea

> ldirectord-05-add-per-virtual-service-checkinterval.patch:
>
> The current checkinterval option is global only. However, when using
> fork=yes it is possible and sensible to want to adjust the checkinterval
> on a per-service basis. This patch allows you to do that, expands on
> the documentation for the option, and warns you if you try to set
> checkinterval in a virtual service definition when fork=no.

Also a good idea.

--
Simon Horman
VA Linux Systems Japan K.K. Satellite Lab in Sydney, Australia
H: www.vergenet.net/~horms/ W: www.valinux.co.jp/en

_______________________________________________
Please read the documentation before posting - it's available at:
http://www.linuxvirtualserver.org/

LinuxVirtualServer.org mailing list - lvs-users [at] LinuxVirtualServer
Send requests to lvs-users-request [at] LinuxVirtualServer
or go to http://lists.graemef.net/mailman/listinfo/lvs-users

sean at bruenor

Apr 22, 2009, 6:07 AM

Post #3 of 3 (710 views)
Permalink

Re: [lvs-users] Patches for ldirectord [In reply to]

Simon Horman wrote:
> One or two things about the patch posting, as I hope you send some
> more patches some day :-)
>
Hi Simon,

Thanks for the feedback. I wouldn't be surprised if you get at least a
couple of more patches from me.

> On Tue, Apr 21, 2009 at 01:51:41PM -0400, Sean E. Millichamp wrote:
>
>> ldirectord-01-cleanup-forked-children.patch:
>>
>> I observed that when ldirectord exits while using fork=yes the children
>> forked to perform the checks are not killed or otherwise cleaned up and
>> continue running. This patch sends the children SIGTERM when ldirectord
>> exits.
>>
>
> I'm surprised that this is needed. But it isn't the first time this
> has come up, so I'm happy top put the change in.
>
The children were left behind on every test in my testing environment.
It appears to have been a prevalent problem in our production
environment as well. I agree that it looks like it shouldn't be needed,
but it does fix the problem for us.
>> ldirectord-02-dont-disable-realservers-in-forking.patch:
>>
>> When using fork=no there is a sanity check on the state of the LVS
>> pools, and ldirectord modifies them as necessary. If there are
>> realservers already present in existing pools ldirectord doesn't disable
>> them until checks can complete. However, when using fork=yes this same
>> sanity check is performed, but all realservers are set down until one
>> check passes successfully. This patch fixes this behavior so that the
>> realservers are not set down initially when fork=yes
>>
>
> I'm not sure that I completely understand the difference in desired
> behaviour between forking and non-forking.
>
Well, it isn't so much as a desired difference, as a desired similarity
to fix an existing difference. In both the forking and non-forking code
paths ldirectord starts by, among other things, calling ld_start(). In
ld_start it goes to apparently great lengths to scan the pre-existing
kernel LVS tables (if any) and remove any realservers listed in the
table that aren't in the config, add any realservers with an initial
weight of 0 to the server_down state, and leave the other realservers at
whatever weight they initially started set at. However, if in forking
mode the run_child function put all realservers into a forced down mode,
setting their weights to 0. This had the effect of interrupting the
virtual services until the first check could complete on each system.

By applying this patch it merely removes that initial force-down setting
and causes it to actually behave similarly to non-forking mode. At
least, that was the intent behind the patch. :)

Thanks!

Sean

_______________________________________________
Please read the documentation before posting - it's available at:
http://www.linuxvirtualserver.org/

LinuxVirtualServer.org mailing list - lvs-users [at] LinuxVirtualServer
Send requests to lvs-users-request [at] LinuxVirtualServer
or go to http://lists.graemef.net/mailman/listinfo/lvs-users

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads