httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Jagielski <...@jaguNET.com>
Subject Re: svn commit: r1696960 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/proxy/mod_proxy_balancer.c
Date Tue, 25 Aug 2015 16:16:21 GMT
I see that, but the assumption is that the reason why the size
would have changed is that someone edited the httpd.conf file and
added another balancer. At that point, the more logical thing
would be to wipe the slate clean (as we do in other places
where certain values in the slotmem != what we expect)...
Also, as mentioned, we DO want to fail if the size isn't
the same, and the only way the size can be different is if
someone changed the file config. So the underlying assumption
behind the patch is invalid, imo.

Also, slotmem->create already does an attach when it should; the
below "breaks" the protections provided when using create as the
primary method to "obtain" the slotmem. "Exposing" the attach here
leads to confusion ("why isn't this done elsewhere").

> On Aug 25, 2015, at 9:26 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
> 
> With this commit the overall size of the storage slot does not change,
> the slot is attached and if it has enough room (growth margin) for the
> new configuration, it is used (like with the balancer-manager),
> otherwise the restart fails.
> 
> On Tue, Aug 25, 2015 at 3:15 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>> Well, if one adds a new balancer before restarting, at post_config
>> time conf->balancers->nelts is +1 wrt the previous load...
>> 
>> On Tue, Aug 25, 2015 at 2:46 PM, Jim Jagielski <jim@jagunet.com> wrote:
>>> But the number of balancers does not change yet... And when it
>>> is implemented the overall size of the storage slot should *never*
>>> change. It must be constant, ala the scoreboard.
>>> 
>>>> On Aug 25, 2015, at 8:39 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>>>> 
>>>> At every startup, in balancer_post_config:
>>>>   conf->max_balancers = conf->balancers->nelts + conf->bgrowth;
>>>> 
>>>> which means that if you add a balancer and restart, storage->create()
>>>> (eg. slotmem_create) will try to find an existing slot with this ID
>>>> and the exact ALIGNED_PROXY_BALANCER_SHARED_SIZE * conf->max_balancers
>>>> size, and fail:
>>>>   if (apr_shm_size_get(shm) != size) {
>>>>       apr_shm_detach(shm);
>>>>       ap_log_error(APLOG_MARK, APLOG_ERR, 0, ap_server_conf, APLOGNO(02599)
>>>>                    "existing shared memory for %s could not be used
>>>> (failed size check)",
>>>>                    fname);
>>>>       return APR_EINVAL;
>>>>   }
>>>> 
>>>> By using storage->attach() first (which is based on the ID only and
>>>> returns the existing item_size and item_num), we can do the check
>>>> ourselves and fail only if conf->max_balancers > item_num, allowing
>>>> for balancers to be added thanks to growth margin used at startup.
>>>> 
>>>> On Tue, Aug 25, 2015 at 2:12 PM, Jim Jagielski <jim@jagunet.com> wrote:
>>>>> How and when do ALIGNED_PROXY_BALANCER_SHARED_SIZE and conf->max_balancers
>>>>> change???
>>>>> 
>>>>>> On Aug 21, 2015, at 8:34 AM, ylavic@apache.org wrote:
>>>>>> 
>>>>>> Author: ylavic
>>>>>> Date: Fri Aug 21 12:34:02 2015
>>>>>> New Revision: 1696960
>>>>>> 
>>>>>> URL: http://svn.apache.org/r1696960
>>>>>> Log:
>>>>>> mod_proxy_balancer: Fix balancers and balancer members reuse on
>>>>>> restart when new ones are added.  PR 58024.
>>>>>> 
>>>>>> Since slotmem_create() issues a strict check on the size of an existing
>>>>>> slot before reusing it, it won't reuse existing balancers and members
when
>>>>>> new ones are added during restart (whereas growth margins would allow
it).
>>>>>> Fix this by using slotmem_attach() first and if it succeeds do the
checks
>>>>>> based on the returned previous number of existing entries.
>>>>>> 
>>>>>> Modified:
>>>>>>  httpd/httpd/trunk/CHANGES
>>>>>>  httpd/httpd/trunk/docs/log-message-tags/next-number
>>>>>>  httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>>>>>> 
>>>>>> Modified: httpd/httpd/trunk/CHANGES
>>>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1696960&r1=1696959&r2=1696960&view=diff
>>>>>> ==============================================================================
>>>>>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>>>>>> +++ httpd/httpd/trunk/CHANGES [utf-8] Fri Aug 21 12:34:02 2015
>>>>>> @@ -1,6 +1,9 @@
>>>>>>                                                        -*- coding:
utf-8 -*-
>>>>>> Changes with Apache 2.5.0
>>>>>> 
>>>>>> +  *) mod_proxy_balancer: Fix balancers and balancer members reuse
on
>>>>>> +     restart when new ones are added.  PR 58024.  [Yann Ylavic]
>>>>>> +
>>>>>> *) mod_socache_memcache: Add the 'MemcacheConnTTL' directive to control
how
>>>>>>    long to keep idle connections with the memcache server(s).
>>>>>>    Change default value from 600 usec (!) to 15 sec. PR 58091
>>>>>> 
>>>>>> Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
>>>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1696960&r1=1696959&r2=1696960&view=diff
>>>>>> ==============================================================================
>>>>>> --- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
>>>>>> +++ httpd/httpd/trunk/docs/log-message-tags/next-number Fri Aug 21
12:34:02 2015
>>>>>> @@ -1 +1 @@
>>>>>> -2964
>>>>>> +2966
>>>>>> 
>>>>>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>>>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c?rev=1696960&r1=1696959&r2=1696960&view=diff
>>>>>> ==============================================================================
>>>>>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
>>>>>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Fri Aug
21 12:34:02 2015
>>>>>> @@ -761,8 +761,11 @@ static int balancer_post_config(apr_pool
>>>>>>       char *id;
>>>>>>       proxy_balancer *balancer;
>>>>>>       ap_slotmem_type_t type;
>>>>>> +        apr_size_t attached_size;
>>>>>> +        unsigned int attached_num;
>>>>>>       void *sconf = s->module_config;
>>>>>>       conf = (proxy_server_conf *)ap_get_module_config(sconf, &proxy_module);
>>>>>> +
>>>>>>       /*
>>>>>>        * During create_proxy_config() we created a dummy id. Now
that
>>>>>>        * we have identifying info, we can create the real id
>>>>>> @@ -794,11 +797,39 @@ static int balancer_post_config(apr_pool
>>>>>>                        (int)ALIGNED_PROXY_BALANCER_SHARED_SIZE,
>>>>>>                        (int)conf->balancers->nelts, conf->max_balancers);
>>>>>> 
>>>>>> -            rv = storage->create(&new, conf->id,
>>>>>> -                                 ALIGNED_PROXY_BALANCER_SHARED_SIZE,
>>>>>> -                                 conf->max_balancers, type, pconf);
>>>>>> +            /* First try to attach() since the number of configured
balancers
>>>>>> +             * may have changed during restart, and we don't want
create() to
>>>>>> +             * fail because the overall size * number of entries
is not stricly
>>>>>> +             * identical to the previous run.  There may still be
enough room
>>>>>> +             * for this new run thanks to bgrowth margin, so if
attach()
>>>>>> +             * succeeds we can only check for the number of available
entries
>>>>>> +             * to be *greater or* equal to what we need now.  If
attach() fails
>>>>>> +             * we simply fall back to create().
>>>>>> +             */
>>>>>> +            rv = storage->attach(&new, conf->id,
>>>>>> +                                 &attached_size, &attached_num,
>>>>>> +                                 pconf);
>>>>>> +            if (rv != APR_SUCCESS) {
>>>>>> +                rv = storage->create(&new, conf->id,
>>>>>> +                                     ALIGNED_PROXY_BALANCER_SHARED_SIZE,
>>>>>> +                                     conf->max_balancers, type,
pconf);
>>>>>> +            }
>>>>>> +            else {
>>>>>> +                ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02964)
>>>>>> +                             "Balancers attached: %d, %d (%d)",
>>>>>> +                             (int)ALIGNED_PROXY_BALANCER_SHARED_SIZE,
>>>>>> +                             (int)attached_num, conf->max_balancers);
>>>>>> +                if (attached_size == ALIGNED_PROXY_BALANCER_SHARED_SIZE
>>>>>> +                        && attached_num >= conf->balancers->nelts)
{
>>>>>> +                    conf->max_balancers = attached_num;
>>>>>> +                }
>>>>>> +                else {
>>>>>> +                    rv = APR_ENOSPC;
>>>>>> +                }
>>>>>> +            }
>>>>>>           if (rv != APR_SUCCESS) {
>>>>>> -                ap_log_error(APLOG_MARK, APLOG_EMERG, rv, s, APLOGNO(01179)
"balancer slotmem_create failed");
>>>>>> +                ap_log_error(APLOG_MARK, APLOG_EMERG, rv, s, APLOGNO(01179)
>>>>>> +                             "balancer slotmem create or attach
failed");
>>>>>>               return !OK;
>>>>>>           }
>>>>>>           conf->bslot = new;
>>>>>> @@ -864,11 +895,32 @@ static int balancer_post_config(apr_pool
>>>>>>                        (int)ALIGNED_PROXY_WORKER_SHARED_SIZE,
>>>>>>                        (int)balancer->max_workers, i);
>>>>>> 
>>>>>> -            rv = storage->create(&new, balancer->s->sname,
>>>>>> -                                 ALIGNED_PROXY_WORKER_SHARED_SIZE,
>>>>>> -                                 balancer->max_workers, type,
pconf);
>>>>>> +            /* try to attach first (see rationale from balancers
above) */
>>>>>> +            rv = storage->attach(&new, balancer->s->sname,
>>>>>> +                                 &attached_size, &attached_num,
>>>>>> +                                 pconf);
>>>>>> +            if (rv != APR_SUCCESS) {
>>>>>> +                rv = storage->create(&new, balancer->s->sname,
>>>>>> +                                     ALIGNED_PROXY_WORKER_SHARED_SIZE,
>>>>>> +                                     balancer->max_workers, type,
pconf);
>>>>>> +            }
>>>>>> +            else {
>>>>>> +                ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02965)
>>>>>> +                             "Workers attached: %s (%s), %d, %d
(%d) [%u]",
>>>>>> +                             balancer->s->name, balancer->s->sname,
>>>>>> +                             (int)ALIGNED_PROXY_WORKER_SHARED_SIZE,
>>>>>> +                             (int)attached_num, balancer->max_workers,
i);
>>>>>> +                if (attached_size == ALIGNED_PROXY_WORKER_SHARED_SIZE
>>>>>> +                        && attached_num >= balancer->workers->nelts)
{
>>>>>> +                    balancer->max_workers = attached_num;
>>>>>> +                }
>>>>>> +                else {
>>>>>> +                    rv = APR_ENOSPC;
>>>>>> +                }
>>>>>> +            }
>>>>>>           if (rv != APR_SUCCESS) {
>>>>>> -                ap_log_error(APLOG_MARK, APLOG_EMERG, rv, s, APLOGNO(01185)
"worker slotmem_create failed");
>>>>>> +                ap_log_error(APLOG_MARK, APLOG_EMERG, rv, s, APLOGNO(01185)
>>>>>> +                             "worker slotmem create or attach failed");
>>>>>>               return !OK;
>>>>>>           }
>>>>>>           balancer->wslot = new;
>>>>>> 
>>>>>> 
>>>>> 
>>> 


Mime
View raw message