+1
> On Aug 25, 2015, at 8:57 AM, Plüm, Rüdiger, Vodafone Group
> <[email protected]> wrote:
>
> Now the more complete patch (including bump):
>
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c (revision 1697578)
> +++ modules/proxy/proxy_util.c (working copy)
> @@ -1460,6 +1460,7 @@
> (*worker)->flush_packets = flush_off;
> (*worker)->flush_wait = PROXY_FLUSH_WAIT;
> (*worker)->smax = -1;
> + (*worker)->server = conf->s;
> /* Increase the total worker count */
> proxy_lb_workers++;
> init_conn_pool(p, *worker);
> @@ -1807,6 +1808,7 @@
> server_rec *server)
> {
> if (ap_scoreboard_image && !worker->s) {
> + server_rec *id_server;
> int i = 0;
> proxy_worker_stat *free_slot = NULL;
> proxy_worker_stat *s;
> @@ -1824,14 +1826,20 @@
> apr_md5_update(&ctx, (unsigned char *)balancer->name,
> strlen(balancer->name));
> }
> - if (server) {
> + if (worker->server) {
> + id_server = worker->server;
> + }
> + else {
> + id_server = server;
> + }
> + if (id_server) {
> server_addr_rec *addr;
> /* Assumes the unique identifier of a vhost is its address(es)
> * plus the ServerName:Port. Should two or more vhosts have this
> * same identifier, the first one would always be elected to
> * handle the requests, so this shouldn't be an issue...
> */
> - for (addr = server->addrs; addr; addr = addr->next) {
> + for (addr = id_server->addrs; addr; addr = addr->next) {
> char host_ip[64]; /* for any IPv[46] string */
> apr_sockaddr_ip_getbuf(host_ip, sizeof host_ip,
> addr->host_addr);
> @@ -1840,10 +1848,10 @@
> apr_md5_update(&ctx, (unsigned char *)&addr->host_port,
> sizeof(addr->host_port));
> }
> - apr_md5_update(&ctx, (unsigned char *)server->server_hostname,
> - strlen(server->server_hostname));
> - apr_md5_update(&ctx, (unsigned char *)&server->port,
> - sizeof(server->port));
> + apr_md5_update(&ctx, (unsigned char *)id_server->server_hostname,
> + strlen(id_server->server_hostname));
> + apr_md5_update(&ctx, (unsigned char *)&id_server->port,
> + sizeof(id_server->port));
> }
> apr_md5_final(digest, &ctx);
>
> Index: modules/proxy/mod_proxy.c
> ===================================================================
> --- modules/proxy/mod_proxy.c (revision 1697578)
> +++ modules/proxy/mod_proxy.c (working copy)
> @@ -1129,6 +1129,7 @@
> ps->badopt = bad_error;
> ps->badopt_set = 0;
> ps->pool = p;
> + ps->s = s;
>
> return ps;
> }
> @@ -1172,6 +1173,7 @@
> ps->proxy_status = (overrides->proxy_status_set == 0) ?
> base->proxy_status : overrides->proxy_status;
> ps->proxy_status_set = overrides->proxy_status_set ||
> base->proxy_status_set;
> ps->pool = p;
> + ps->s = overrides->s;
> return ps;
> }
>
> Index: modules/proxy/mod_proxy.h
> ===================================================================
> --- modules/proxy/mod_proxy.h (revision 1697578)
> +++ modules/proxy/mod_proxy.h (working copy)
> @@ -193,6 +193,7 @@
> } proxy_status; /* Status display options */
> char proxy_status_set;
> apr_pool_t *pool; /* Pool used for allocating this struct */
> + server_rec *s; /* The server_rec where this configuration
> was created in */
> } proxy_server_conf;
>
>
> @@ -369,6 +370,7 @@
> char disablereuse_set;
> apr_interval_time_t conn_timeout;
> char conn_timeout_set;
> + server_rec *server; /* The server_rec where this configuration
> was created in */
> };
>
> /*
> Index: include/ap_mmn.h
> ===================================================================
> --- include/ap_mmn.h (revision 1697578)
> +++ include/ap_mmn.h (working copy)
> @@ -158,6 +158,8 @@
> * 20051115.38 (2.2.30) Add ap_proxy_set_scoreboard_lb() in mod_proxy.h
> * 20051115.39 (2.2.30) Add ap_proxy_connection_reusable()
> * 20051115.40 (2.2.30) Add ap_map_http_request_error()
> + * 20051115.41 (2.2.32) Add s member to proxy_server_conf struct and server
> + * member to proxy_worker struct.
> */
>
> #define MODULE_MAGIC_COOKIE 0x41503232UL /* "AP22" */
> @@ -165,7 +167,7 @@
> #ifndef MODULE_MAGIC_NUMBER_MAJOR
> #define MODULE_MAGIC_NUMBER_MAJOR 20051115
> #endif
> -#define MODULE_MAGIC_NUMBER_MINOR 40 /* 0...n */
> +#define MODULE_MAGIC_NUMBER_MINOR 41 /* 0...n */
>
> /**
> * Determine if the server's current MODULE_MAGIC_NUMBER is at least a
>
>
> Regards
>
> Rüdiger
>
>> -----Original Message-----
>> From: Plüm, Rüdiger, Vodafone Group
>> Sent: Dienstag, 25. August 2015 14:41
>> To: [email protected]
>> Subject: RE: PR 58267: Regression in 2.2.31 caused by r1680920
>>
>> Thanks for the pointer. It is missing because I removed it by accident
>> when excluding some debug code I setup locally for analysing the issue
>> from the patch I posted. I will post a proper version and if you agree put
>> it in STATUS for 2.2.x. As far as I can tell this change only applies to
>> 2.2.x. So it would be fine to propose it directly in STATUS without any
>> trunk commit.
>>
>> Regards
>>
>> Rüdiger
>>
>>> -----Original Message-----
>>> From: Jan Kaluža [mailto:[email protected]]
>>> Sent: Dienstag, 25. August 2015 14:15
>>> To: [email protected]
>>> Subject: Re: PR 58267: Regression in 2.2.31 caused by r1680920
>>>
>>> On 08/25/2015 11:39 AM, Plüm, Rüdiger, Vodafone Group wrote:
>>>> How about the following patch? It uses the server_rec of the server
>> that
>>> originally created the configuration item.
>>>
>>> This looks like good strategy. I've verified that the patch fixes this
>>> issue and does not break anything when testing briefly.
>>>
>>> What do you think, Yann?
>>>
>>> Note that "id_server" declaration is missing in the patch. Otherwise
>>> it's OK.
>>>
>>> Regards,
>>> Jan Kaluza
>>>
>>>> Index: modules/proxy/proxy_util.c
>>>> ===================================================================
>>>> --- modules/proxy/proxy_util.c (revision 1697578)
>>>> +++ modules/proxy/proxy_util.c (working copy)
>>>> @@ -1460,6 +1460,7 @@
>>>> (*worker)->flush_packets = flush_off;
>>>> (*worker)->flush_wait = PROXY_FLUSH_WAIT;
>>>> (*worker)->smax = -1;
>>>> + (*worker)->server = conf->s;
>>>> /* Increase the total worker count */
>>>> proxy_lb_workers++;
>>>> init_conn_pool(p, *worker);
>>>> @@ -1824,14 +1829,20 @@
>>>> apr_md5_update(&ctx, (unsigned char *)balancer->name,
>>>> strlen(balancer->name));
>>>> }
>>>> - if (server) {
>>>> + if (worker->server) {
>>>> + id_server = worker->server;
>>>> + }
>>>> + else {
>>>> + id_server = server;
>>>> + }
>>>> + if (id_server) {
>>>> server_addr_rec *addr;
>>>> /* Assumes the unique identifier of a vhost is its
>>> address(es)
>>>> * plus the ServerName:Port. Should two or more vhosts
>>> have this
>>>> * same identifier, the first one would always be
>> elected
>>> to
>>>> * handle the requests, so this shouldn't be an issue...
>>>> */
>>>> - for (addr = server->addrs; addr; addr = addr->next) {
>>>> + for (addr = id_server->addrs; addr; addr = addr->next) {
>>>> char host_ip[64]; /* for any IPv[46] string */
>>>> apr_sockaddr_ip_getbuf(host_ip, sizeof host_ip,
>>>> addr->host_addr);
>>>> @@ -1840,10 +1851,10 @@
>>>> apr_md5_update(&ctx, (unsigned char *)&addr-
>>>> host_port,
>>>> sizeof(addr->host_port));
>>>> }
>>>> - apr_md5_update(&ctx, (unsigned char *)server-
>>>> server_hostname,
>>>> - strlen(server->server_hostname));
>>>> - apr_md5_update(&ctx, (unsigned char *)&server->port,
>>>> - sizeof(server->port));
>>>> + apr_md5_update(&ctx, (unsigned char *)id_server-
>>>> server_hostname,
>>>> + strlen(id_server->server_hostname));
>>>> + apr_md5_update(&ctx, (unsigned char *)&id_server->port,
>>>> + sizeof(id_server->port));
>>>> }
>>>> apr_md5_final(digest, &ctx);
>>>>
>>>> Index: modules/proxy/mod_proxy.c
>>>> ===================================================================
>>>> --- modules/proxy/mod_proxy.c (revision 1697578)
>>>> +++ modules/proxy/mod_proxy.c (working copy)
>>>> @@ -1129,6 +1129,7 @@
>>>> ps->badopt = bad_error;
>>>> ps->badopt_set = 0;
>>>> ps->pool = p;
>>>> + ps->s = s;
>>>>
>>>> return ps;
>>>> }
>>>> @@ -1172,6 +1173,7 @@
>>>> ps->proxy_status = (overrides->proxy_status_set == 0) ? base-
>>>> proxy_status : overrides->proxy_status;
>>>> ps->proxy_status_set = overrides->proxy_status_set || base-
>>>> proxy_status_set;
>>>> ps->pool = p;
>>>> + ps->s = overrides->s;
>>>> return ps;
>>>> }
>>>>
>>>> Index: modules/proxy/mod_proxy.h
>>>> ===================================================================
>>>> --- modules/proxy/mod_proxy.h (revision 1697578)
>>>> +++ modules/proxy/mod_proxy.h (working copy)
>>>> @@ -193,6 +193,7 @@
>>>> } proxy_status; /* Status display options */
>>>> char proxy_status_set;
>>>> apr_pool_t *pool; /* Pool used for allocating this
>>> struct */
>>>> + server_rec *s; /* The server_rec where this
>>> configuration was created in */
>>>> } proxy_server_conf;
>>>>
>>>>
>>>> @@ -369,6 +370,7 @@
>>>> char disablereuse_set;
>>>> apr_interval_time_t conn_timeout;
>>>> char conn_timeout_set;
>>>> + server_rec *server; /* The server_rec where this
>>> configuration was created in */
>>>> };
>>>>
>>>> /*
>>>>
>>>>
>>>> Regards
>>>>
>>>> Rüdiger
>>>>
>>>>> -----Original Message-----
>>>>> From: Plüm, Rüdiger, Vodafone Group
>>>>> Sent: Dienstag, 25. August 2015 10:48
>>>>> To: [email protected]
>>>>> Subject: RE: PR 58267: Regression in 2.2.31 caused by r1680920
>>>>>
>>>>> I think the current state of 2.2.31 breaks existing 2.2.x
>> configuration
>>>>> prior to 2.2.31.
>>>>> Prior to 2.2.31 you could do the following:
>>>>>
>>>>> <Proxy Balancer://proxy1>
>>>>> BalancerMember ajp://127.0.0.1:7001
>>>>> BalancerMember ajp://127.0.0.2:7002
>>>>> </Proxy>
>>>>>
>>>>> <virtualhost *:80>
>>>>> ProxyPass / balancer://proxy1/
>>>>> </virtualhost>
>>>>>
>>>>> <virtualhost 127.0.0.1:8080>
>>>>>
>>>>> <Location /bmanager>
>>>>> sethandler balancer-manager
>>>>> </Location>
>>>>>
>>>>> </virtualhost>
>>>>>
>>>>> With this configuration you could provide your reverse proxy to the
>>>>> outside world while having the
>>>>> balancermanager managing the balancer only listening to localhost.
>> This
>>> is
>>>>> not possible any longer with 2.2.31
>>>>> as the worker->s is now different in each virtualhost whereas before
>> it
>>>>> was the same as the worker->id was used
>>>>> to identify the scoreboard slot.
>>>>> The patches proposed so far would not change this. I think in order
>> to
>>>>> revert to the old behaviour we need to
>>>>> store with each worker in which server_rec context it was created.
>> e.g.
>>>>> adding a const char * field to the worker that would be filled with
>>>>> server->server_hostname. Then we could use this value for creating
>> the
>>>>> md5.
>>>>>
>>>>> Regards
>>>>>
>>>>> Rüdiger
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jan Kaluža [mailto:[email protected]]
>>>>>> Sent: Dienstag, 25. August 2015 10:23
>>>>>> To: [email protected]
>>>>>> Subject: Re: PR 58267: Regression in 2.2.31 caused by r1680920
>>>>>>
>>>>>> On 08/24/2015 11:12 PM, Yann Ylavic wrote:
>>>>>>> On Mon, Aug 24, 2015 at 5:51 PM, Yann Ylavic <[email protected]>
>>>>>> wrote:
>>>>>>>>
>>>>>>>> On Mon, Aug 24, 2015 at 4:47 PM, Jan Kaluža <[email protected]>
>>>>> wrote:
>>>>>>>>>
>>>>>>>>> 2) Increment proxy_lb_workers according to number of workers in
>>>>>> balancer
>>>>>>>>> when using "ProxyPass /foobar/ Balancer://foobar/" in the
>>>>> VirtualHost.
>>>>>> The
>>>>>>>>> scoreboard would have right size and ap_proxy_set_scoreboard_lb
>>>>> would
>>>>>> not
>>>>>>>>> fail then.
>>>>>>>>
>>>>>>>> I think we can do this quite easily in merge_proxy_config(), by
>>>>>>>> incrementing proxy_lb_workers for each base->balancers->workers. I
>>>>> did
>>>>>>>> not test it yet though.
>>>>>>>
>>>>>>> I tested the below which seems to work.
>>>>>>
>>>>>> Hm, this reserves the slots in scoreboard even when the balancers
>> are
>>>>>> not used in the virtualhost, or am I wrong?
>>>>>>
>>>>>> I originally thought about increasing proxy_lb_workers only when
>> they
>>>>>> are used in the ProxyPass* in the vhost.
>>>>>>
>>>>>>> Index: modules/proxy/mod_proxy.c
>>>>>>> ===================================================================
>>>>>>> --- modules/proxy/mod_proxy.c (revision 1697358)
>>>>>>> +++ modules/proxy/mod_proxy.c (working copy)
>>>>>>> @@ -1135,6 +1135,7 @@ static void * create_proxy_config(apr_pool_t
>>> *p,
>>>>> s
>>>>>>>
>>>>>>> static void * merge_proxy_config(apr_pool_t *p, void *basev,
>> void
>>>>>> *overridesv)
>>>>>>> {
>>>>>>> + int i;
>>>>>>> proxy_server_conf *ps = apr_pcalloc(p,
>>>>> sizeof(proxy_server_conf));
>>>>>>> proxy_server_conf *base = (proxy_server_conf *) basev;
>>>>>>> proxy_server_conf *overrides = (proxy_server_conf *)
>>> overridesv;
>>>>>>> @@ -1147,6 +1148,13 @@ static void * merge_proxy_config(apr_pool_t
>>> *p,
>>>>>> vo
>>>>>>> ps->allowed_connect_ports = apr_array_append(p,
>>>>>>> base->allowed_connect_ports, overrides->allowed_connect_ports);
>>>>>>> ps->workers = apr_array_append(p, base->workers, overrides-
>>>>>>> workers);
>>>>>>> ps->balancers = apr_array_append(p, base->balancers,
>>> overrides-
>>>>>>> balancers);
>>>>>>> + /* The balancers inherited from base don't have their members
>>>>>> reserved on
>>>>>>> + * the scorebord_lb for this server, account for them now.
>>>>>>> + */
>>>>>>> + for (i = 0; i < base->balancers->nelts; ++i) {
>>>>>>> + proxy_balancer *balancer = (proxy_balancer *)base-
>>>> balancers-
>>>>>>> elts + i;
>>>>>>> + proxy_lb_workers += balancer->workers->nelts;
>>>>>>> + }
>>>>>>> ps->forward = overrides->forward ? overrides->forward :
>> base-
>>>>>>> forward;
>>>>>>> ps->reverse = overrides->reverse ? overrides->reverse :
>> base-
>>>>>>> reverse;
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Please note that since all the workers would really be accounted in
>>>>>>> the scoreboard, configurations like the one of PR 58267 (with
>>>>>>> inherited balancers) would also need bigger SHMs (but no more) than
>>>>>>> currently...
>>>>>>>
>>>>>>> WDYT?
>>>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Jan Kaluza
>>>>>>
>>>>
>
> <pr58267.diff>
