> On Aug 25, 2015, at 10:41 AM, 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,
As it should, and as I designed it, yeah. The whole assumption
is that restarts DO NOT CHANGE THE REVERSE PROXY CONFIG.
> even if the settings changed (but the size of the slot prior to this
> commit), so your proposed changes may break compatibility too...
>
Please note how the runtime state from a persisted slotmem
is re-used and how/when/if it is over-written or over-writes.
Please don't change the behavior of what it is currently doing
based on the feedback and insights and discussions of years
to what you think it should be doing, or what you want it to
do. If it's broke, fix it. But don't just change it. :)
Again, if the slotmem exists and is persisted, the assumption
is that THAT is what the admin wants, and when Apache restarts,
THAT is the running config they desire. If there are changes
to the reverse proxy setup, the assumption must be we are
starting from scratch; There are, iirc, a number of places where
these kinds of checks are done. Trying to somehow "merge" a just-changed
file config and a running config (based on an older file config
with dynamic changes) is nasty and tough to do correctly.
>
> 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;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
