The whole intent of the dynamic capability of balancer manage,
plus the persistence of it, is to allow changes to be done
to the reverse proxy setup w/o requiring any changes to
the config at all. It allows it to be reconfigured dynamically,
either via the manager or some other mechanism. It's to
*avoid* changing the file config. It's been a long-requested
change and something which *completely* distinguishes httpd
from others. I am very -1 on any changes that reduce that
capability or any that somehow "encourages" going back to
a preference for changes to the config file.

> On Aug 25, 2015, at 10:43 AM, Yann Ylavic <ylavic....@gmail.com> wrote:
> 
> The advantage of using the balancer-manager to add a member is that
> failures won't kill httpd, whereas failing to add member or balancer
> via the configuration file (no space left) stops httpd (now
> documented).
> 
> To avoid that, maybe could we use non-shared (per child) memory in
> this case, like in 2.2.x, so that httpd can continue to ~work~?
> 
> 
> On Tue, Aug 25, 2015 at 4:41 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
>> I don't see the difference between the configuration via
>> balancer-manager or file+graceful-restart, both can change the number
>> of balancers/members AND the settings while children are running.
>> I'd kind of agree if we were talking about non-graceful restarts or
>> first startup, we could add a new storage->destroy() method to do the
>> job depending on the slotmem provider (though what about remote
>> storage shared between multiple httpds?).
>> Currently any restart (graceful or not) reuses the existing slots,
>> even if the settings changed (but the size of the slot prior to this
>> commit), so your proposed changes may break compatibility too...
>> 
>> 
>> On Tue, Aug 25, 2015 at 3:59 PM, Jim Jagielski <j...@jagunet.com> wrote:
>>> When 1st configured, Apache sees how many balancers there are
>>> set in the config file. It then pads that amount by growth to
>>> allow for the to-be-added dynamic addition of new balancers via
>>> the balancer manager (ala adding members).
>>> 
>>> The assumption is that if the actual config file is changed,
>>> then the runtime configuration of balancers and members
>>> is no longer valid; after all, if someone is changing the
>>> config file, they could have done the same to add/change
>>> bal/mem settings. So a config file change means something
>>> more "serious" at which point the dynamic config should be
>>> considered void and Apache starts with a clean (dynamic) slate
>>> and just uses what's in the config only.
>>> 
>>>> On Aug 25, 2015, at 9:48 AM, Yann Ylavic <ylavic....@gmail.com> wrote:
>>>> 
>>>> Hm, what's the point of the balancers and balancer->workers growth margin 
>>>> then?
>>>> The first time the configration is loaded we create enough room for
>>>> adding new balancers/workers, and on (graceful) restart we wouldn't
>>>> use this space?
>>>> That's also how the balancer-manager works when adding a new worker to
>>>> a balancer, AFAICT.
>>>> 
>>>> 
>>>> On Tue, Aug 25, 2015 at 3:43 PM, Jim Jagielski <j...@jagunet.com> wrote:
>>>>> You mean via changing the config file? If so, then all bets
>>>>> are off. You change the config file and restart then Apache
>>>>> should start off w/ a clean slate. Even so, you need to handle
>>>>> freeing the old shared mem segment and creating a brand new one
>>>>> with the "correct" size... Trying to "use" the old one is
>>>>> just fraught with issues.
>>>>> 
>>>>> 
>>>>>> On Aug 25, 2015, at 9:15 AM, Yann Ylavic <ylavic....@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 <j...@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....@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 <j...@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, yla...@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;
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 

Reply via email to