Re: PR 58267: Regression in 2.2.31 caused by r1680920
)
> * 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: dev@httpd.apache.org
>> 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:jkal...@redhat.com]
>>> Sent: Dienstag, 25. August 2015 14:15
>>> To: dev@httpd.apache.org
>>> 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,
>>>> -
Re: PR 58267: Regression in 2.2.31 caused by r1680920
On Tue, Aug 25, 2015 at 10:48 AM, Plüm, Rüdiger, Vodafone Group wrote: > 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: > > > BalancerMember ajp://127.0.0.1:7001 > BalancerMember ajp://127.0.0.2:7002 > > > > ProxyPass / balancer://proxy1/ > > > > > >sethandler balancer-manager > > > > > 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. You are right, the old behaviour was to share the score of the main server for all the vhosts that use it. I thought that since the worker's parameters are per vhost, each vhost's worker had its own score so that e.g. an error on one of them would not affect the error state of the others. I can (and indeed always unconsciously did) live with the fact that the scored parameters are ignored in the vhost if they are also defined in the main server (i.e. it is useless to define the same with different parameters both in the main server and a vhost :) > The patches proposed so far would not change this. Yes, it implements a score per vhost... On Tue, Aug 25, 2015 at 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. LGTM, thanks Rüdiger for helping on this.
RE: PR 58267: Regression in 2.2.31 caused by r1680920
Plus CHANGES:
Index: CHANGES
===
--- CHANGES (revision 1697578)
+++ CHANGES (working copy)
@@ -1,8 +1,10 @@
-*- coding: utf-8 -*-
Changes with Apache 2.2.32
+ *) mod_proxy: Fix a regression with 2.2.31 that caused inherited workers to
+ use a different scoreboard slot then the original one. PR 58267.
+[Ruediger Pluem]
-
Changes with Apache 2.2.31
*) Correct win32 build issues for mod_proxy exports, OpenSSL 1.0.x headers.
Regards
Rüdiger
> -Original Message-
> From: Plüm, Rüdiger, Vodafone Group
> Sent: Dienstag, 25. August 2015 14:58
> To: dev@httpd.apache.org
> Subject: RE: PR 58267: Regression in 2.2.31 caused by r1680920
>
> 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 *po
RE: PR 58267: Regression in 2.2.31 caused by r1680920
least a
Regards
Rüdiger
> -Original Message-
> From: Plüm, Rüdiger, Vodafone Group
> Sent: Dienstag, 25. August 2015 14:41
> To: dev@httpd.apache.org
> 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:jkal...@redhat.com]
> > Sent: Dienstag, 25. August 2015 14:15
> > To: dev@httpd.apache.org
> > 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)
> > > +++ module
Re: PR 58267: Regression in 2.2.31 caused by r1680920
Looks good to me... Even though this is 2.2.x I did some quick
and dirty testing :)
> On Aug 25, 2015, at 5:44 AM, Plüm, Rüdiger, Vodafone Group
> wrote:
>
> Of course it requires a minor bump.
>
> Regards
>
> Rüdiger
>
>> -Original Message-
>> From: Plüm, Rüdiger, Vodafone Group
>> Sent: Dienstag, 25. August 2015 11:39
>> To: dev@httpd.apache.org
>> Subject: RE: PR 58267: Regression in 2.2.31 caused by r1680920
>>
>> How about the following patch? It uses the server_rec of the server that
>> originally created the configuration item.
>>
>> 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,
Re: PR 58267: Regression in 2.2.31 caused by r1680920
On 08/25/2015 02:41 PM, Plüm, Rüdiger, Vodafone Group wrote:
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.
I agree.
Jan Kaluza
Regards
Rüdiger
-Original Message-
From: Jan Kaluža [mailto:jkal...@redhat.com]
Sent: Dienstag, 25. August 2015 14:15
To: dev@httpd.apache.org
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 @@
chardisablereuse_set;
apr_interval_time_t conn_timeout;
charconn_timeout_set;
+server_rec *server;/* The server_rec where this
configuration was created in */
};
/*
Regards
Rüdiger
-
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:jkal...@redhat.com]
> Sent: Dienstag, 25. August 2015 14:15
> To: dev@httpd.apache.org
> 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
Re: PR 58267: Regression in 2.2.31 caused by r1680920
On Tue, Aug 25, 2015 at 2:15 PM, Jan Kaluža wrote: > 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? OK for me too.
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 @@
chardisablereuse_set;
apr_interval_time_t conn_timeout;
charconn_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: dev@httpd.apache.org
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:
BalancerMember ajp://127.0.0.1:7001
BalancerMember ajp://127.0.0.2:7002
ProxyPass / balancer://proxy1/
sethandler balancer-manager
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
RE: PR 58267: Regression in 2.2.31 caused by r1680920
Of course it requires a minor bump.
Regards
Rüdiger
> -Original Message-
> From: Plüm, Rüdiger, Vodafone Group
> Sent: Dienstag, 25. August 2015 11:39
> To: dev@httpd.apache.org
> Subject: RE: PR 58267: Regression in 2.2.31 caused by r1680920
>
> How about the following patch? It uses the server_rec of the server that
> originally created the configuration item.
>
> 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;
> charconn_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: dev@httpd.apache.org
> > Subject: RE: PR 58267: Regression in 2.2.31 caused by r1680920
> >
> > I think the current state o
RE: PR 58267: Regression in 2.2.31 caused by r1680920
How about the following patch? It uses the server_rec of the server that
originally created the configuration item.
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 @@
chardisablereuse_set;
apr_interval_time_t conn_timeout;
charconn_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: dev@httpd.apache.org
> 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:
>
>
> BalancerMember ajp://127.0.0.1:7001
> BalancerMember ajp://127.0.0.2:7002
>
>
>
> ProxyPass / balancer://proxy1/
>
>
>
>
>
>sethandler balancer-manager
>
>
>
>
> 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 o
Re: PR 58267: Regression in 2.2.31 caused by r1680920
On Tue, Aug 25, 2015 at 10:22 AM, Jan Kaluža wrote: > > On 08/24/2015 11:12 PM, Yann Ylavic wrote: >> >> 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? Correct, but there may still be RewriteRule(s) (with [P] flag) that use the balancer. > > I originally thought about increasing proxy_lb_workers only when they are > used in the ProxyPass* in the vhost. I think this would be a regression, RewriteRules using declared balancers/workers do also reuse their parameters/connections (not the "proxy:reverse" worker), some third-party modules may too.
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:
BalancerMember ajp://127.0.0.1:7001
BalancerMember ajp://127.0.0.2:7002
ProxyPass / balancer://proxy1/
sethandler balancer-manager
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:jkal...@redhat.com]
> Sent: Dienstag, 25. August 2015 10:23
> To: dev@httpd.apache.org
> 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
> wrote:
> >>
> >> On Mon, Aug 24, 2015 at 4:47 PM, Jan Kaluža 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
>
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 wrote:
On Mon, Aug 24, 2015 at 4:47 PM, Jan Kaluža 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
Re: PR 58267: Regression in 2.2.31 caused by r1680920
On Mon, Aug 24, 2015 at 5:51 PM, Yann Ylavic wrote:
>
> On Mon, Aug 24, 2015 at 4:47 PM, Jan Kaluža 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.
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?
Re: PR 58267: Regression in 2.2.31 caused by r1680920
Hi Jan, I was working on the same issue... and was going to implement 2) :) On Mon, Aug 24, 2015 at 4:47 PM, Jan Kaluža wrote: > > Now, the root of the error is that the scoreboard size is static (set to > proxy_lb_workers + PROXY_DYNAMIC_BALANCER_LIMIT), but it is not incremented > when ProxyPass with balancer is used in the virtualhost. This leads to lack > of space in scoreboard when Balancers are used in multiple virtualhosts. I came to the same conclusion. > > I think there are two possible fixes: > > 1) Do not use server->server_hostname when computing hash which is used to > determine right scoreboard field. I think this would fix this bug, but I'm > not sure what would happen in situations when you define 2 balancers with > the same name in different virtualhosts... They should be different balancers... > > On the other-side, when there is global Proxy balancer, it make sense to use > the same worker->s for all the ProxyPass in virtualhosts. This would break compatibility. > > 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. Regards, Yann.
Re: PR 58267: Regression in 2.2.31 caused by r1680920
On 08/24/2015 04:47 PM, Jan Kaluža wrote: Hi, unfortunately, the r1680920 brought undesired behavior described in PR 58267 to 2.2.x. The bug is well described in the PR, so I won't describe it in this email. I have tried to debug it and I think the problem is that we use also server->server_hostname to compute the hash in the ap_proxy_set_scoreboard_lb. This hash is used to find out proper ap_scoreboard field. It all happens in mod_proxy.c:child_init's scope. If the "" has been defined, all the BalancerMembers are initialized with the hash computed with usage of global server->server_hostname. Later, if the "ProxyPass /foobar/ Balancer://foobar/" has been used in the VirtualHost, ap_proxy_initialize_worker_share is called again with server->server_hostname set to the VirtualHost's one. Now, the root of the error is that the scoreboard size is static (set to proxy_lb_workers + PROXY_DYNAMIC_BALANCER_LIMIT), but it is not incremented when ProxyPass with balancer is used in the virtualhost. This leads to lack of space in scoreboard when Balancers are used in multiple virtualhosts. I think there are two possible fixes: 1) Do not use server->server_hostname when computing hash which is used to determine right scoreboard field. I think this would fix this bug, but I'm not sure what would happen in situations when you define 2 balancers with the same name in different virtualhosts... On the other-side, when there is global Proxy balancer, it make sense to use the same worker->s for all the ProxyPass in virtualhosts. It seems that 2.4.x uses just the name of the worker to determine slot for shared memory, so maybe it wouldn't be a problem for 2.2.x too... Jan Kaluza 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. Since it's 2.2.x which should be probably stable without big changes, I'm asking the list for more opinions... I will try to implement patch for option 2) tomorrow and see if this really fixes the issue. Regards, Jan Kaluza
PR 58267: Regression in 2.2.31 caused by r1680920
Hi, unfortunately, the r1680920 brought undesired behavior described in PR 58267 to 2.2.x. The bug is well described in the PR, so I won't describe it in this email. I have tried to debug it and I think the problem is that we use also server->server_hostname to compute the hash in the ap_proxy_set_scoreboard_lb. This hash is used to find out proper ap_scoreboard field. It all happens in mod_proxy.c:child_init's scope. If the "" has been defined, all the BalancerMembers are initialized with the hash computed with usage of global server->server_hostname. Later, if the "ProxyPass /foobar/ Balancer://foobar/" has been used in the VirtualHost, ap_proxy_initialize_worker_share is called again with server->server_hostname set to the VirtualHost's one. Now, the root of the error is that the scoreboard size is static (set to proxy_lb_workers + PROXY_DYNAMIC_BALANCER_LIMIT), but it is not incremented when ProxyPass with balancer is used in the virtualhost. This leads to lack of space in scoreboard when Balancers are used in multiple virtualhosts. I think there are two possible fixes: 1) Do not use server->server_hostname when computing hash which is used to determine right scoreboard field. I think this would fix this bug, but I'm not sure what would happen in situations when you define 2 balancers with the same name in different virtualhosts... On the other-side, when there is global Proxy balancer, it make sense to use the same worker->s for all the ProxyPass in virtualhosts. 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. Since it's 2.2.x which should be probably stable without big changes, I'm asking the list for more opinions... I will try to implement patch for option 2) tomorrow and see if this really fixes the issue. Regards, Jan Kaluza
