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: [email protected]
> 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 *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
> > > >>>
> > > >
