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