httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Jagielski <...@jaguNET.com>
Subject Re: PR 58267: Regression in 2.2.31 caused by r1680920
Date Tue, 25 Aug 2015 14:34:43 GMT
+1
> On Aug 25, 2015, at 8:57 AM, Plüm, Rüdiger, Vodafone Group <ruediger.pluem@vodafone.com>
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: 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:jkaluza@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 @@
>>>>      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: 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:
>>>>> 
>>>>> <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:jkaluza@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 <ylavic.dev@gmail.com>
>>>>>> wrote:
>>>>>>>> 
>>>>>>>> On Mon, Aug 24, 2015 at 4:47 PM, Jan Kaluža <jkaluza@redhat.com>
>>>>> 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>


Mime
View raw message