Mailing List Archive: reset in proxy_balancer

reset in proxy_balancer_method

Feb 9, 2024, 2:59 AM

Post #1 of 7 (511 views)

Hi,

I have noted to the reset() clean up too much in the balancers:
mod_lbmethod_bybusyness.c for example does:
+++
for (i = 0; i < balancer->workers->nelts; i++, worker++) {
(*worker)->s->lbstatus = 0;
ap_proxy_set_busy_count(*worker, 0); /* BAD */
}
+++
In fact reset() is called by ap_proxy_initialize_balancer() when a child
process is created... Resetting the counters messes up the logic.

Does it make sense to stop calling reset() from
ap_proxy_initialize_balancer() or is it better to fix all reset()?

There is another thing adding a new worker via /balancer-manager/
probably requires some kind of reset() otherwise all the load moves to
the new worker which is not the best.... May be calling age() or
triggering calls to age() can help.

--
Cheers

Jean-Frederic

Re: reset in proxy_balancer_method [ In reply to ]

rpluem at apache

Feb 13, 2024, 11:19 PM

Post #2 of 7 (493 views)

Permalink

On 2/9/24 11:59 AM, jean-frederic clere wrote:
> Hi,
>
> I have noted to the reset() clean up too much in the balancers:
> mod_lbmethod_bybusyness.c for example does:
> +++
>     for (i = 0; i < balancer->workers->nelts; i++, worker++) {
>         (*worker)->s->lbstatus = 0;
>         ap_proxy_set_busy_count(*worker, 0); /* BAD */
>     }
> +++
> In fact reset() is called by ap_proxy_initialize_balancer() when a child process is created... Resetting the counters messes up
> the logic.
>
> Does it make sense to stop calling reset() from ap_proxy_initialize_balancer() or is it better to fix all reset()?

I am not sure what the original idea / intention of reset was. Until this is clarified I would not remove the call to reset in
ap_proxy_initialize_balancer(). In ap_proxy_sync_balancer the call to it is guarded by a check for the need_reset flag. Maybe this
gives a hint.

>
>
> There is another thing adding a new worker via /balancer-manager/ probably requires some kind of reset() otherwise all the load
> moves to the new worker which is not the best.... May be calling age() or triggering calls to age() can help.

Doesn't ap_proxy_sync_balancer take care of this which is called before processing a request?

Regards

Rüdiger

Re: reset in proxy_balancer_method [ In reply to ]

jfclere at gmail

Feb 14, 2024, 6:45 AM

Post #3 of 7 (490 views)

Permalink

On 2/14/24 08:19, Ruediger Pluem wrote:
>
>
> On 2/9/24 11:59 AM, jean-frederic clere wrote:
>> Hi,
>>
>> I have noted to the reset() clean up too much in the balancers:
>> mod_lbmethod_bybusyness.c for example does:
>> +++
>>     for (i = 0; i < balancer->workers->nelts; i++, worker++) {
>>         (*worker)->s->lbstatus = 0;
>>         ap_proxy_set_busy_count(*worker, 0); /* BAD */
>>     }
>> +++
>> In fact reset() is called by ap_proxy_initialize_balancer() when a child process is created... Resetting the counters messes up
>> the logic.
>>
>> Does it make sense to stop calling reset() from ap_proxy_initialize_balancer() or is it better to fix all reset()?
>
> I am not sure what the original idea / intention of reset was. Until this is clarified I would not remove the call to reset in
> ap_proxy_initialize_balancer(). In ap_proxy_sync_balancer the call to it is guarded by a check for the need_reset flag. Maybe this
> gives a hint.

The need_reset is set when a worker is enabled or when the load
balancing method is changed.

>
>>
>>
>> There is another thing adding a new worker via /balancer-manager/ probably requires some kind of reset() otherwise all the load
>> moves to the new worker which is not the best.... May be calling age() or triggering calls to age() can help.
>
> Doesn't ap_proxy_sync_balancer take care of this which is called before processing a request?
>
> Regards
>
> Rüdiger
>

--
Cheers

Jean-Frederic

Re: reset in proxy_balancer_method [ In reply to ]

rpluem at apache

Feb 14, 2024, 11:54 AM

Post #4 of 7 (489 views)

Permalink

On 2/14/24 3:45 PM, jean-frederic clere wrote:
> On 2/14/24 08:19, Ruediger Pluem wrote:
>>
>>
>> On 2/9/24 11:59 AM, jean-frederic clere wrote:
>>> Hi,
>>>
>>> I have noted to the reset() clean up too much in the balancers:
>>> mod_lbmethod_bybusyness.c for example does:
>>> +++
>>>      for (i = 0; i < balancer->workers->nelts; i++, worker++) {
>>>          (*worker)->s->lbstatus = 0;
>>>          ap_proxy_set_busy_count(*worker, 0); /* BAD */
>>>      }
>>> +++
>>> In fact reset() is called by ap_proxy_initialize_balancer() when a child process is created... Resetting the counters messes up
>>> the logic.
>>>
>>> Does it make sense to stop calling reset() from ap_proxy_initialize_balancer() or is it better to fix all reset()?
>>
>> I am not sure what the original idea / intention of reset was. Until this is clarified I would not remove the call to reset in
>> ap_proxy_initialize_balancer(). In ap_proxy_sync_balancer the call to it is guarded by a check for the need_reset flag. Maybe this
>> gives a hint.

Rethinking this. I guess we lack a clear API definition of the reset method of a balancer provider and it does not really seem
sensible to reset the stats in case a new child is created. Hence I guess the regression risk is rather low when just removing
this call to reset.

>
> The need_reset is set when a worker is enabled or when the load balancing method is changed.

Isn't that a similar situation to when a worker is added (in respect to the point below)?

>
>>
>>>
>>>
>>> There is another thing adding a new worker via /balancer-manager/ probably requires some kind of reset() otherwise all the load
>>> moves to the new worker which is not the best.... May be calling age() or triggering calls to age() can help.
>>
>> Doesn't ap_proxy_sync_balancer take care of this which is called before processing a request?
>>

Regards

Rüdiger

Re: reset in proxy_balancer_method [ In reply to ]

jfclere at gmail

Feb 15, 2024, 12:28 AM

Post #5 of 7 (487 views)

Permalink

On 2/14/24 20:54, Ruediger Pluem wrote:
>
>
> On 2/14/24 3:45 PM, jean-frederic clere wrote:
>> On 2/14/24 08:19, Ruediger Pluem wrote:
>>>
>>>
>>> On 2/9/24 11:59 AM, jean-frederic clere wrote:
>>>> Hi,
>>>>
>>>> I have noted to the reset() clean up too much in the balancers:
>>>> mod_lbmethod_bybusyness.c for example does:
>>>> +++
>>>>      for (i = 0; i < balancer->workers->nelts; i++, worker++) {
>>>>          (*worker)->s->lbstatus = 0;
>>>>          ap_proxy_set_busy_count(*worker, 0); /* BAD */
>>>>      }
>>>> +++
>>>> In fact reset() is called by ap_proxy_initialize_balancer() when a child process is created... Resetting the counters messes up
>>>> the logic.
>>>>
>>>> Does it make sense to stop calling reset() from ap_proxy_initialize_balancer() or is it better to fix all reset()?
>>>
>>> I am not sure what the original idea / intention of reset was. Until this is clarified I would not remove the call to reset in
>>> ap_proxy_initialize_balancer(). In ap_proxy_sync_balancer the call to it is guarded by a check for the need_reset flag. Maybe this
>>> gives a hint.
>
> Rethinking this. I guess we lack a clear API definition of the reset method of a balancer provider and it does not really seem
> sensible to reset the stats in case a new child is created. Hence I guess the regression risk is rather low when just removing
> this call to reset.
>
>>
>> The need_reset is set when a worker is enabled or when the load balancing method is changed.
>
> Isn't that a similar situation to when a worker is added (in respect to the point below)?

The worker are added disabled.

>
>>
>>>
>>>>
>>>>
>>>> There is another thing adding a new worker via /balancer-manager/ probably requires some kind of reset() otherwise all the load
>>>> moves to the new worker which is not the best.... May be calling age() or triggering calls to age() can help.
>>>
>>> Doesn't ap_proxy_sync_balancer take care of this which is called before processing a request?
>>>
>
> Regards
>
> Rüdiger

--
Cheers

Jean-Frederic

Re: reset in proxy_balancer_method [ In reply to ]

rpluem at apache

Feb 15, 2024, 4:04 AM

Post #6 of 7 (486 views)

Permalink

On 2/15/24 9:28 AM, jean-frederic clere wrote:
> On 2/14/24 20:54, Ruediger Pluem wrote:
>>
>>
>> On 2/14/24 3:45 PM, jean-frederic clere wrote:
>>> On 2/14/24 08:19, Ruediger Pluem wrote:
>>>>
>>>>
>>>> On 2/9/24 11:59 AM, jean-frederic clere wrote:
>>>>> Hi,
>>>>>
>>>>> I have noted to the reset() clean up too much in the balancers:
>>>>> mod_lbmethod_bybusyness.c for example does:
>>>>> +++
>>>>>       for (i = 0; i < balancer->workers->nelts; i++, worker++) {
>>>>>           (*worker)->s->lbstatus = 0;
>>>>>           ap_proxy_set_busy_count(*worker, 0); /* BAD */
>>>>>       }
>>>>> +++
>>>>> In fact reset() is called by ap_proxy_initialize_balancer() when a child process is created... Resetting the counters messes up
>>>>> the logic.
>>>>>
>>>>> Does it make sense to stop calling reset() from ap_proxy_initialize_balancer() or is it better to fix all reset()?
>>>>
>>>> I am not sure what the original idea / intention of reset was. Until this is clarified I would not remove the call to reset in
>>>> ap_proxy_initialize_balancer(). In ap_proxy_sync_balancer the call to it is guarded by a check for the need_reset flag. Maybe
>>>> this
>>>> gives a hint.
>>
>> Rethinking this. I guess we lack a clear API definition of the reset method of a balancer provider and it does not really seem
>> sensible to reset the stats in case a new child is created. Hence I guess the regression risk is rather low when just removing
>> this call to reset.
>>
>>>
>>> The need_reset is set when a worker is enabled or when the load balancing method is changed.
>>
>> Isn't that a similar situation to when a worker is added (in respect to the point below)?
>
> The worker are added disabled.

Which means stuff gets reset once they get enabled?

Regards

Rüdiger

Re: reset in proxy_balancer_method [ In reply to ]

jfclere at gmail

Feb 15, 2024, 4:44 AM

Post #7 of 7 (485 views)

Permalink

On 2/15/24 13:04, Ruediger Pluem wrote:
>
>
> On 2/15/24 9:28 AM, jean-frederic clere wrote:
>> On 2/14/24 20:54, Ruediger Pluem wrote:
>>>
>>>
>>> On 2/14/24 3:45 PM, jean-frederic clere wrote:
>>>> On 2/14/24 08:19, Ruediger Pluem wrote:
>>>>>
>>>>>
>>>>> On 2/9/24 11:59 AM, jean-frederic clere wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I have noted to the reset() clean up too much in the balancers:
>>>>>> mod_lbmethod_bybusyness.c for example does:
>>>>>> +++
>>>>>>       for (i = 0; i < balancer->workers->nelts; i++, worker++) {
>>>>>>           (*worker)->s->lbstatus = 0;
>>>>>>           ap_proxy_set_busy_count(*worker, 0); /* BAD */
>>>>>>       }
>>>>>> +++
>>>>>> In fact reset() is called by ap_proxy_initialize_balancer() when a child process is created... Resetting the counters messes up
>>>>>> the logic.
>>>>>>
>>>>>> Does it make sense to stop calling reset() from ap_proxy_initialize_balancer() or is it better to fix all reset()?
>>>>>
>>>>> I am not sure what the original idea / intention of reset was. Until this is clarified I would not remove the call to reset in
>>>>> ap_proxy_initialize_balancer(). In ap_proxy_sync_balancer the call to it is guarded by a check for the need_reset flag. Maybe
>>>>> this
>>>>> gives a hint.
>>>
>>> Rethinking this. I guess we lack a clear API definition of the reset method of a balancer provider and it does not really seem
>>> sensible to reset the stats in case a new child is created. Hence I guess the regression risk is rather low when just removing
>>> this call to reset.
>>>
>>>>
>>>> The need_reset is set when a worker is enabled or when the load balancing method is changed.
>>>
>>> Isn't that a similar situation to when a worker is added (in respect to the point below)?
>>
>> The worker are added disabled.
>
> Which means stuff gets reset once they get enabled?

Correct ;-)

>
> Regards
>
> Rüdiger

--
Cheers

Jean-Frederic