Re: [PATCH] Balancers, VirtualHost and ProxyPass

2015-03-19 Thread Jan Kaluža 

On 12/12/2014 02:23 PM, Jan Kaluža wrote:

On 12/12/2014 02:17 PM, Yann Ylavic wrote:

On Fri, Dec 12, 2014 at 2:09 PM, Plüm, Rüdiger, Vodafone Group
ruediger.pl...@vodafone.com wrote:



-Ursprüngliche Nachricht-
Von: Jan Kaluža [mailto:jkal...@redhat.com]
Gesendet: Freitag, 12. Dezember 2014 14:00
An: dev@httpd.apache.org
Betreff: Re: [PATCH] Balancers, VirtualHost and ProxyPass

You are right :(. Fix attached. Order of errstatuses should not be a
problem. Its values are checked against r-status to determine status
code on which is balancer worker marked as in error.


Guess it is fine now :-)


+1


I forgot to commit this one. Done in trunk in r1667707 now.


Thanks you both, that one was painful :(.

Jan Kaluza


Regards,
Yann.





Regards,
Jan Kaluza

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-17 Thread Jim Jagielski 
As also noted, the whole aspect of using the Balancer Manager
factors into this as well...

IMO, the whole merging stuff is getting more and more
fragile, and adding more to it makes it even worse.

 On Dec 17, 2014, at 2:35 AM, Jan Kaluža jkal...@redhat.com wrote:
 
 On 12/16/2014 01:57 PM, Jim Jagielski wrote:
 Isn't this already addressed/handled with the BalancerInherit directive??
 
 No, it isn't. The BalancerInherit only says that the balancers from the main 
 config will be copied to vhost context *after* the config_tree is processed. 
 And the word *after* is the problem here. When you try to use the inherited 
 balancer in the vhost config, you can't, because vhost does not know about 
 the inherited balancers in the time of config processing.
 
 Every time you try to use (with ProxyPass for example) inherited balancer in 
 the vhost, mod_proxy creates brand new balancer in the vhost context. This 
 new balancer is completely ignored later and replaced by the original 
 balancer you wanted to use when BalancerInherit is used.
 
 If we think httpd should not allow ProxyPass in the VirtualHost with 
 balancers defined outside of VirtualHost context, we should disable that and 
 show warning that httpd doesn't support this configuration.
 
 Otherwise we need to add balancer merging as I did in the patch.
 
 Regards,
 Jan Kaluza
 
 On Dec 10, 2014, at 7:25 AM, Jan Kaluža jkal...@redhat.com wrote:
 
 Hi,
 
 I've found out that following configuration does not work as expected:
 
 Proxy balancer://a
   ...
 /Proxy
 VirtualHost *:80
ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
 /VirtualHost
 
 In this case, two proxy_balancers are created. The first one in Proxy 
 section in the main config without stickysession and the second one in the 
 vhost section with stickysession set.
 
 Because of merge_proxy_config behaviour, the one from the main config is 
 always preferred and therefore you cannot set stickysession (and other 
 options) this way.
 
 Attached patch fixes that by changing the merge strategy for balancers 
 array to merge options set by ProxyPass.
 
 I think we would need the same for proxy_worker too, but before I spent 
 afternoon working on it, I wanted to ask, do you think this is the right 
 way how to fix this?
 
 Regards,
 Jan Kaluza
 httpd-trunk-balancer-vhost.patch

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-16 Thread Jim Jagielski 
Isn't this already addressed/handled with the BalancerInherit directive??

 On Dec 10, 2014, at 7:25 AM, Jan Kaluža jkal...@redhat.com wrote:
 
 Hi,
 
 I've found out that following configuration does not work as expected:
 
 Proxy balancer://a
   ...
 /Proxy
 VirtualHost *:80
ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
 /VirtualHost
 
 In this case, two proxy_balancers are created. The first one in Proxy section 
 in the main config without stickysession and the second one in the vhost 
 section with stickysession set.
 
 Because of merge_proxy_config behaviour, the one from the main config is 
 always preferred and therefore you cannot set stickysession (and other 
 options) this way.
 
 Attached patch fixes that by changing the merge strategy for balancers array 
 to merge options set by ProxyPass.
 
 I think we would need the same for proxy_worker too, but before I spent 
 afternoon working on it, I wanted to ask, do you think this is the right way 
 how to fix this?
 
 Regards,
 Jan Kaluza
 httpd-trunk-balancer-vhost.patch

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-16 Thread Jim Jagielski 
Why? If a balancer is used just in a Vhost, then it should be
defined just in that Vhost. I can't see adding complexity and
workarounds to hack around what is a simple config error.

 On Dec 10, 2014, at 7:52 AM, Yann Ylavic ylavic@gmail.com wrote:
 
 Hi,
 
 didn't look at the patch yet, but the workaround for this is usually
 to use ProxySet in the Proxy block.
 I agree that it would be nice to have these parameters merged, though.
 
 Regards,
 Yann.
 
 
 On Wed, Dec 10, 2014 at 1:25 PM, Jan Kaluža jkal...@redhat.com wrote:
 Hi,
 
 I've found out that following configuration does not work as expected:
 
 Proxy balancer://a
   ...
 /Proxy
 VirtualHost *:80
ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
 /VirtualHost
 
 In this case, two proxy_balancers are created. The first one in Proxy
 section in the main config without stickysession and the second one in the
 vhost section with stickysession set.
 
 Because of merge_proxy_config behaviour, the one from the main config is
 always preferred and therefore you cannot set stickysession (and other
 options) this way.
 
 Attached patch fixes that by changing the merge strategy for balancers array
 to merge options set by ProxyPass.
 
 I think we would need the same for proxy_worker too, but before I spent
 afternoon working on it, I wanted to ask, do you think this is the right way
 how to fix this?
 
 Regards,
 Jan Kaluza

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-16 Thread Jan Kaluža 

On 12/16/2014 01:57 PM, Jim Jagielski wrote:

Isn't this already addressed/handled with the BalancerInherit directive??


No, it isn't. The BalancerInherit only says that the balancers from the 
main config will be copied to vhost context *after* the config_tree is 
processed. And the word *after* is the problem here. When you try to use 
the inherited balancer in the vhost config, you can't, because vhost 
does not know about the inherited balancers in the time of config 
processing.


Every time you try to use (with ProxyPass for example) inherited 
balancer in the vhost, mod_proxy creates brand new balancer in the vhost 
context. This new balancer is completely ignored later and replaced by 
the original balancer you wanted to use when BalancerInherit is used.


If we think httpd should not allow ProxyPass in the VirtualHost with 
balancers defined outside of VirtualHost context, we should disable that 
and show warning that httpd doesn't support this configuration.


Otherwise we need to add balancer merging as I did in the patch.

Regards,
Jan Kaluza


On Dec 10, 2014, at 7:25 AM, Jan Kaluža jkal...@redhat.com wrote:

Hi,

I've found out that following configuration does not work as expected:

Proxy balancer://a
   ...
/Proxy
VirtualHost *:80
ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
/VirtualHost

In this case, two proxy_balancers are created. The first one in Proxy section 
in the main config without stickysession and the second one in the vhost 
section with stickysession set.

Because of merge_proxy_config behaviour, the one from the main config is always 
preferred and therefore you cannot set stickysession (and other options) this 
way.

Attached patch fixes that by changing the merge strategy for balancers array to 
merge options set by ProxyPass.

I think we would need the same for proxy_worker too, but before I spent 
afternoon working on it, I wanted to ask, do you think this is the right way 
how to fix this?

Regards,
Jan Kaluza
httpd-trunk-balancer-vhost.patch

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Jan Kaluža 

On 12/11/2014 03:05 PM, Plüm, Rüdiger, Vodafone Group wrote:




-Original Message-
From: Jan Kaluža [mailto:jkal...@redhat.com]
Sent: Donnerstag, 11. Dezember 2014 14:40
To: dev@httpd.apache.org
Subject: Re: [PATCH] Balancers, VirtualHost and ProxyPass

On 12/11/2014 08:47 AM, Jan Kaluža wrote:

On 12/10/2014 08:21 PM, Ruediger Pluem wrote:



On 12/10/2014 02:21 PM, Jan Kaluža wrote:

On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:

But this way we lose the base ones that are not touched in the
virtual host and e.g. are only used by rewriterules.
So we should transfer the base ones to the merged array in any case
and update them where needed.


Hm, you are right. Check the new version attached to this email.


But this one changes the parent configuration. So if you have two
virtual hosts and in each you change a different
parameter of the parent the later one has *both* changes, not only
one. So you would need to do a copy of these
balancers first and adjust the copy. Then only add the adjusted copy
to the result. For the base balancers not matching
the override balancers just add them to the result untouched. Same for
the override ones, as you already do


That makes sense, thanks. I will work on it and send updated patch.


I have to admit that this feature is starting to be more time-consuming
than I thought :). The attached patch should address all the issues
pointed by Yann and Rüdiger.


Looks fine in general. Details:


I hope I've finally fixed everything now :), see the attached patch 
please. I really appreciate your reviews here!


Regards,
Jan Kaluza


+if (!tmp.lbmethod_set  b1-lbmethod_set) {
+b2-lbmethod_set = b1-lbmethod_set;
+b2-lbmethod = b1-lbmethod;

This is already the case because of the

  +*b2 = *b1;

above. So if lbmethod is set differently in overrides you lose it in favour of 
the base setting.


+PROXY_STRNCPY(tmp.s-lbpname, b1-s-lbpname);
+}
+if (!tmp.growth_set  b1-growth_set) {
+b2-growth_set = b1-growth_set;
+b2-growth = b1-growth;

Same as above

+}
+if (!tmp.failontimeout_set  b1-failontimeout_set) {
+b2-failontimeout_set = b1-failontimeout_set;
+b2-failontimeout = b1-failontimeout;
+}

Same as above.

  if (apr_is_empty_array(tmp.errstatuses)
+ !apr_is_empty_array(b1-errstatuses)) {
+apr_array_cat(b2-errstatuses, b1-errstatuses);
+}

b1 and b2 point to the same array. So the result will be a doubled b1 if 
apr_array_cat doesn't fail on getting supplied the same pointer twice :-)


Regards

Rüdiger



Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c	(revision 1644346)
+++ modules/proxy/mod_proxy.c	(working copy)
@@ -306,6 +306,7 @@
 }
 else
 balancer-s-sticky_separator = *val;
+balancer-s-sticky_separator_set = 1;
 }
 else if (!strcasecmp(key, nofailover)) {
 /* If set to 'on' the session will break
@@ -318,6 +319,7 @@
 balancer-s-sticky_force = 0;
 else
 return failover must be On|Off;
+balancer-s-sticky_force_set = 1;
 }
 else if (!strcasecmp(key, timeout)) {
 /* Balancer timeout in seconds.
@@ -348,6 +350,7 @@
 if (provider) {
 balancer-lbmethod = provider;
 if (PROXY_STRNCPY(balancer-s-lbpname, val) == APR_SUCCESS) {
+balancer-lbmethod_set = 1;
 return NULL;
 }
 else {
@@ -367,6 +370,7 @@
 balancer-s-scolonsep = 0;
 else
 return scolonpathdelim must be On|Off;
+balancer-s-scolonsep_set = 1;
 }
 else if (!strcasecmp(key, failonstatus)) {
 char *val_split;
@@ -397,6 +401,7 @@
 balancer-failontimeout = 0;
 else
 return failontimeout must be On|Off;
+balancer-failontimeout_set = 1;
 }
 else if (!strcasecmp(key, nonce)) {
 if (!strcasecmp(val, None)) {
@@ -407,6 +412,7 @@
 return Provided nonce is too large;
 }
 }
+balancer-s-nonce_set = 1;
 }
 else if (!strcasecmp(key, growth)) {
 ival = atoi(val);
@@ -413,6 +419,7 @@
 if (ival  1 || ival  100)   /* arbitrary limit here */
 return growth must be between 1 and 100;
 balancer-growth = ival;
+balancer-growth_set = 1;
 }
 else if (!strcasecmp(key, forcerecovery)) {
 if (!strcasecmp(val, on))
@@ -421,6 +428,7 @@
 balancer-s-forcerecovery = 0;
 else
 return forcerecovery must be On|Off;
+balancer-s-forcerecovery_set = 1;
 }
 else {
 return unknown

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Ruediger Pluem 


On 12/12/2014 09:44 AM, Jan Kaluža wrote:
 On 12/11/2014 03:05 PM, Plüm, Rüdiger, Vodafone Group wrote:


 -Original Message-
 From: Jan Kaluža [mailto:jkal...@redhat.com]
 Sent: Donnerstag, 11. Dezember 2014 14:40
 To: dev@httpd.apache.org
 Subject: Re: [PATCH] Balancers, VirtualHost and ProxyPass

 On 12/11/2014 08:47 AM, Jan Kaluža wrote:
 On 12/10/2014 08:21 PM, Ruediger Pluem wrote:


 On 12/10/2014 02:21 PM, Jan Kaluža wrote:
 On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:
 But this way we lose the base ones that are not touched in the
 virtual host and e.g. are only used by rewriterules.
 So we should transfer the base ones to the merged array in any case
 and update them where needed.

 Hm, you are right. Check the new version attached to this email.

 But this one changes the parent configuration. So if you have two
 virtual hosts and in each you change a different
 parameter of the parent the later one has *both* changes, not only
 one. So you would need to do a copy of these
 balancers first and adjust the copy. Then only add the adjusted copy
 to the result. For the base balancers not matching
 the override balancers just add them to the result untouched. Same for
 the override ones, as you already do

 That makes sense, thanks. I will work on it and send updated patch.

 I have to admit that this feature is starting to be more time-consuming
 than I thought :). The attached patch should address all the issues
 pointed by Yann and Rüdiger.

 Looks fine in general. Details:
 
 I hope I've finally fixed everything now :), see the attached patch please. I 
 really appreciate your reviews here!

Looks good. One thing though: I think apr_array_cat is still wrong since we 
only copied a *pointer* to the array with
*b2 = *b1. So apr_array_cat adjusts the original array from the base. I guess 
we should do:

apr_array_cat(tmp.errstatuses, b2-errstatuses);
b2-errstatuses = tmp.errstatuses;

Of course this is only fine when the order of the errstatuses array doesn't 
matter. Otherwise:

 b2-errstatuses = apr_array_append(b2-errstatuses, tmp.errstatuses);

And from order perspective, maybe:

return apr_array_append(p, tocopy, overrides);

If order doesn't matter I guess

apr_array_cat(overrides, tocopy);
return overrides;

would be also fine.

Regards

Rüdiger

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Yann Ylavic 
Hi Jan,

On Fri, Dec 12, 2014 at 9:44 AM, Jan Kaluža jkal...@redhat.com wrote:
 On 12/11/2014 03:05 PM, Plüm, Rüdiger, Vodafone Group wrote:

 Looks fine in general. Details:

 I hope I've finally fixed everything now :), see the attached patch please.

Aren't the following parameters missing for the merge:
unsigned intmax_attempts_set:1;
unsigned intvhosted:1;
unsigned intinactive:1;
vhosted does not seem to be used, but seems a good candidate to
differentiate base vs vhost balancer.

Detail, sticky can't be NULL here:
+if ((!b2-s-sticky || *b2-s-sticky == 0)
+ b1-s-sticky  *b1-s-sticky) {
+PROXY_STRNCPY(b2-s-sticky_path, b1-s-sticky_path);
+PROXY_STRNCPY(b2-s-sticky, b1-s-sticky);
+}
+

I am (still) confused about the shallow copy:

   +*b2 = *b1;

IIUC, Rüdiger's point is that base balancers' parameters shouldn't be
modified by vhosts specifics, because some vhosts (or RewriteRules)
may use the default ones.
But then why would they share (at runtime) the same lbmethod, members
list (dynamic with balancer-manager), backend connections (reslist,
same timeouts, reusability, states, any worker parameter).

IMHO there should be a deep copy here, that's another balancer, with
its own mutexes, own members having their own parameters, own
sockets...

Regards,
Yann.

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Yann Ylavic 
On Fri, Dec 12, 2014 at 12:08 PM, Yann Ylavic ylavic@gmail.com wrote:

 IMHO there should be a deep copy here, that's another balancer, with
 its own mutexes, own members having their own parameters, own
 sockets...

Oh, got it now...
If this is expected, the admin can still declare a new balancer in the vost :)

Please ignore this overly complicating remark.


 Regards,
 Yann.

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Jan Kaluža 

On 12/12/2014 12:08 PM, Yann Ylavic wrote:

Hi Jan,

On Fri, Dec 12, 2014 at 9:44 AM, Jan Kaluža jkal...@redhat.com wrote:

On 12/11/2014 03:05 PM, Plüm, Rüdiger, Vodafone Group wrote:


Looks fine in general. Details:


I hope I've finally fixed everything now :), see the attached patch please.


Aren't the following parameters missing for the merge:
 unsigned intmax_attempts_set:1;
 unsigned intvhosted:1;
 unsigned intinactive:1;
vhosted does not seem to be used, but seems a good candidate to
differentiate base vs vhost balancer.


max_attempts_set is merged. I've been merging only things which are set 
by set_balancer_param(). vhosted and inactive seems both to be unused. 
Without the semantics for those, I have no idea how to merge them 
(should I add vhosted_set/inactive_set or just merge them as they are...).


I think vhosted has been committed by a mistake in r1206286. I can't 
find a place where inactive is set (was just grepping for inactive).




Detail, sticky can't be NULL here:
+if ((!b2-s-sticky || *b2-s-sticky == 0)
+ b1-s-sticky  *b1-s-sticky) {
+PROXY_STRNCPY(b2-s-sticky_path, b1-s-sticky_path);
+PROXY_STRNCPY(b2-s-sticky, b1-s-sticky);
+}
+


Ok, it looks like the rest of mod_proxy does not check for s-sticky == 
NULL too, so I will remove it from this method too.


Regards,
Jan Kaluza


I am (still) confused about the shallow copy:


   +*b2 = *b1;


IIUC, Rüdiger's point is that base balancers' parameters shouldn't be
modified by vhosts specifics, because some vhosts (or RewriteRules)
may use the default ones.
But then why would they share (at runtime) the same lbmethod, members
list (dynamic with balancer-manager), backend connections (reslist,
same timeouts, reusability, states, any worker parameter).

IMHO there should be a deep copy here, that's another balancer, with
its own mutexes, own members having their own parameters, own
sockets...



Regards,
Yann.

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Jan Kaluža 

On 12/12/2014 11:56 AM, Ruediger Pluem wrote:



On 12/12/2014 09:44 AM, Jan Kaluža wrote:

On 12/11/2014 03:05 PM, Plüm, Rüdiger, Vodafone Group wrote:




-Original Message-
From: Jan Kaluža [mailto:jkal...@redhat.com]
Sent: Donnerstag, 11. Dezember 2014 14:40
To: dev@httpd.apache.org
Subject: Re: [PATCH] Balancers, VirtualHost and ProxyPass

On 12/11/2014 08:47 AM, Jan Kaluža wrote:

On 12/10/2014 08:21 PM, Ruediger Pluem wrote:



On 12/10/2014 02:21 PM, Jan Kaluža wrote:

On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:

But this way we lose the base ones that are not touched in the
virtual host and e.g. are only used by rewriterules.
So we should transfer the base ones to the merged array in any case
and update them where needed.


Hm, you are right. Check the new version attached to this email.


But this one changes the parent configuration. So if you have two
virtual hosts and in each you change a different
parameter of the parent the later one has *both* changes, not only
one. So you would need to do a copy of these
balancers first and adjust the copy. Then only add the adjusted copy
to the result. For the base balancers not matching
the override balancers just add them to the result untouched. Same for
the override ones, as you already do


That makes sense, thanks. I will work on it and send updated patch.


I have to admit that this feature is starting to be more time-consuming
than I thought :). The attached patch should address all the issues
pointed by Yann and Rüdiger.


Looks fine in general. Details:


I hope I've finally fixed everything now :), see the attached patch please. I 
really appreciate your reviews here!


Looks good. One thing though: I think apr_array_cat is still wrong since we 
only copied a *pointer* to the array with
*b2 = *b1. So apr_array_cat adjusts the original array from the base. I guess 
we should do:

apr_array_cat(tmp.errstatuses, b2-errstatuses);
b2-errstatuses = tmp.errstatuses;

Of course this is only fine when the order of the errstatuses array doesn't 
matter. Otherwise:

  b2-errstatuses = apr_array_append(b2-errstatuses, tmp.errstatuses);

And from order perspective, maybe:

return apr_array_append(p, tocopy, overrides);

If order doesn't matter I guess

apr_array_cat(overrides, tocopy);
return overrides;

would be also fine.


You are right :(. Fix attached. Order of errstatuses should not be a 
problem. Its values are checked against r-status to determine status 
code on which is balancer worker marked as in error.


Jan Kaluza


Regards

Rüdiger



Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c	(revision 1644346)
+++ modules/proxy/mod_proxy.c	(working copy)
@@ -306,6 +306,7 @@
 }
 else
 balancer-s-sticky_separator = *val;
+balancer-s-sticky_separator_set = 1;
 }
 else if (!strcasecmp(key, nofailover)) {
 /* If set to 'on' the session will break
@@ -318,6 +319,7 @@
 balancer-s-sticky_force = 0;
 else
 return failover must be On|Off;
+balancer-s-sticky_force_set = 1;
 }
 else if (!strcasecmp(key, timeout)) {
 /* Balancer timeout in seconds.
@@ -348,6 +350,7 @@
 if (provider) {
 balancer-lbmethod = provider;
 if (PROXY_STRNCPY(balancer-s-lbpname, val) == APR_SUCCESS) {
+balancer-lbmethod_set = 1;
 return NULL;
 }
 else {
@@ -367,6 +370,7 @@
 balancer-s-scolonsep = 0;
 else
 return scolonpathdelim must be On|Off;
+balancer-s-scolonsep_set = 1;
 }
 else if (!strcasecmp(key, failonstatus)) {
 char *val_split;
@@ -397,6 +401,7 @@
 balancer-failontimeout = 0;
 else
 return failontimeout must be On|Off;
+balancer-failontimeout_set = 1;
 }
 else if (!strcasecmp(key, nonce)) {
 if (!strcasecmp(val, None)) {
@@ -407,6 +412,7 @@
 return Provided nonce is too large;
 }
 }
+balancer-s-nonce_set = 1;
 }
 else if (!strcasecmp(key, growth)) {
 ival = atoi(val);
@@ -413,6 +419,7 @@
 if (ival  1 || ival  100)   /* arbitrary limit here */
 return growth must be between 1 and 100;
 balancer-growth = ival;
+balancer-growth_set = 1;
 }
 else if (!strcasecmp(key, forcerecovery)) {
 if (!strcasecmp(val, on))
@@ -421,6 +428,7 @@
 balancer-s-forcerecovery = 0;
 else
 return forcerecovery must be On|Off;
+balancer-s-forcerecovery_set = 1;
 }
 else {
 return unknown Balancer parameter;
@@ -1272,6 +1280,99 @@
 return ps;
 }
 
+static apr_array_header_t *merge_balancers(apr_pool_t *p,
+   apr_array_header_t *base

AW: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Plüm , Rüdiger , Vodafone Group 


 -Ursprüngliche Nachricht-
 Von: Jan Kaluža [mailto:jkal...@redhat.com]
 Gesendet: Freitag, 12. Dezember 2014 14:00
 An: dev@httpd.apache.org
 Betreff: Re: [PATCH] Balancers, VirtualHost and ProxyPass
 
 You are right :(. Fix attached. Order of errstatuses should not be a
 problem. Its values are checked against r-status to determine status
 code on which is balancer worker marked as in error.

Guess it is fine now :-)

Regards

Rüdiger

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Yann Ylavic 
On Fri, Dec 12, 2014 at 2:09 PM, Plüm, Rüdiger, Vodafone Group
ruediger.pl...@vodafone.com wrote:

 -Ursprüngliche Nachricht-
 Von: Jan Kaluža [mailto:jkal...@redhat.com]
 Gesendet: Freitag, 12. Dezember 2014 14:00
 An: dev@httpd.apache.org
 Betreff: Re: [PATCH] Balancers, VirtualHost and ProxyPass

 You are right :(. Fix attached. Order of errstatuses should not be a
 problem. Its values are checked against r-status to determine status
 code on which is balancer worker marked as in error.

 Guess it is fine now :-)

+1

Regards,
Yann.

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Jan Kaluža 

On 12/12/2014 02:17 PM, Yann Ylavic wrote:

On Fri, Dec 12, 2014 at 2:09 PM, Plüm, Rüdiger, Vodafone Group
ruediger.pl...@vodafone.com wrote:



-Ursprüngliche Nachricht-
Von: Jan Kaluža [mailto:jkal...@redhat.com]
Gesendet: Freitag, 12. Dezember 2014 14:00
An: dev@httpd.apache.org
Betreff: Re: [PATCH] Balancers, VirtualHost and ProxyPass

You are right :(. Fix attached. Order of errstatuses should not be a
problem. Its values are checked against r-status to determine status
code on which is balancer worker marked as in error.


Guess it is fine now :-)


+1


Thanks you both, that one was painful :(.

Jan Kaluza


Regards,
Yann.

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-12 Thread Yann Ylavic 
On Fri, Dec 12, 2014 at 2:23 PM, Jan Kaluža jkal...@redhat.com wrote:

 Thanks you both, that one was painful :(.

Thank *you* for the improvement!
Most of my remarks were noise, sorry if you wasted time with them...

Yann.

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-11 Thread Jan Kaluža 

On 12/10/2014 08:21 PM, Ruediger Pluem wrote:



On 12/10/2014 02:21 PM, Jan Kaluža wrote:

On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:

But this way we lose the base ones that are not touched in the virtual host and 
e.g. are only used by rewriterules.
So we should transfer the base ones to the merged array in any case and update 
them where needed.


Hm, you are right. Check the new version attached to this email.


But this one changes the parent configuration. So if you have two virtual hosts 
and in each you change a different
parameter of the parent the later one has *both* changes, not only one. So you 
would need to do a copy of these
balancers first and adjust the copy. Then only add the adjusted copy to the 
result. For the base balancers not matching
the override balancers just add them to the result untouched. Same for the 
override ones, as you already do


That makes sense, thanks. I will work on it and send updated patch.

Jan Kaluza


Regards

Rüdiger

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-11 Thread Jan Kaluža 

On 12/11/2014 08:47 AM, Jan Kaluža wrote:

On 12/10/2014 08:21 PM, Ruediger Pluem wrote:



On 12/10/2014 02:21 PM, Jan Kaluža wrote:

On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:

But this way we lose the base ones that are not touched in the
virtual host and e.g. are only used by rewriterules.
So we should transfer the base ones to the merged array in any case
and update them where needed.


Hm, you are right. Check the new version attached to this email.


But this one changes the parent configuration. So if you have two
virtual hosts and in each you change a different
parameter of the parent the later one has *both* changes, not only
one. So you would need to do a copy of these
balancers first and adjust the copy. Then only add the adjusted copy
to the result. For the base balancers not matching
the override balancers just add them to the result untouched. Same for
the override ones, as you already do


That makes sense, thanks. I will work on it and send updated patch.


I have to admit that this feature is starting to be more time-consuming 
than I thought :). The attached patch should address all the issues 
pointed by Yann and Rüdiger.


When balancer is found in both overrides and base arrays, the copy of 
the base balancer replaces the one in override array, but the original 
override's options, which can be set by ProxyPass, are kept.


If the balancer exists only in the overrides array, it is kept untouched.

If the balancer exists only in the base array, it is appended to the 
override array and returned untouched.



Jan Kaluza


Regards

Rüdiger





Regards,
Jan Kaluza

Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c	(revision 1644346)
+++ modules/proxy/mod_proxy.c	(working copy)
@@ -306,6 +306,7 @@
 }
 else
 balancer-s-sticky_separator = *val;
+balancer-s-sticky_separator_set = 1;
 }
 else if (!strcasecmp(key, nofailover)) {
 /* If set to 'on' the session will break
@@ -318,6 +319,7 @@
 balancer-s-sticky_force = 0;
 else
 return failover must be On|Off;
+balancer-s-sticky_force_set = 1;
 }
 else if (!strcasecmp(key, timeout)) {
 /* Balancer timeout in seconds.
@@ -348,6 +350,7 @@
 if (provider) {
 balancer-lbmethod = provider;
 if (PROXY_STRNCPY(balancer-s-lbpname, val) == APR_SUCCESS) {
+balancer-lbmethod_set = 1;
 return NULL;
 }
 else {
@@ -367,6 +370,7 @@
 balancer-s-scolonsep = 0;
 else
 return scolonpathdelim must be On|Off;
+balancer-s-scolonsep_set = 1;
 }
 else if (!strcasecmp(key, failonstatus)) {
 char *val_split;
@@ -397,6 +401,7 @@
 balancer-failontimeout = 0;
 else
 return failontimeout must be On|Off;
+balancer-failontimeout_set = 1;
 }
 else if (!strcasecmp(key, nonce)) {
 if (!strcasecmp(val, None)) {
@@ -407,6 +412,7 @@
 return Provided nonce is too large;
 }
 }
+balancer-s-nonce_set = 1;
 }
 else if (!strcasecmp(key, growth)) {
 ival = atoi(val);
@@ -413,6 +419,7 @@
 if (ival  1 || ival  100)   /* arbitrary limit here */
 return growth must be between 1 and 100;
 balancer-growth = ival;
+balancer-growth_set = 1;
 }
 else if (!strcasecmp(key, forcerecovery)) {
 if (!strcasecmp(val, on))
@@ -421,6 +428,7 @@
 balancer-s-forcerecovery = 0;
 else
 return forcerecovery must be On|Off;
+balancer-s-forcerecovery_set = 1;
 }
 else {
 return unknown Balancer parameter;
@@ -1272,6 +1280,94 @@
 return ps;
 }
 
+static apr_array_header_t *merge_balancers(apr_pool_t *p,
+   apr_array_header_t *base,
+   apr_array_header_t *overrides)
+{
+proxy_balancer *b1;
+proxy_balancer *b2;
+proxy_balancer tmp;
+int x, y, found;
+apr_array_header_t *tocopy = apr_array_make(p, 1, sizeof(proxy_balancer));
+
+/* Check if the balancer is defined in both override and base configs:
+ * a) If it is, Create copy of base balancer and change the configuration
+ *which can be changed by ProxyPass.
+ * b) Otherwise, copy the balancer to tocopy array and merge it later.
+ */
+b1 = (proxy_balancer *) base-elts;
+for (y = 0; y  base-nelts; y++) {
+b2 = (proxy_balancer *) overrides-elts;
+for (x = 0, found = 0; x  overrides-nelts; x++) {
+if (b1-hash.def == b2-hash.def  b1-hash.fnv == b2-hash.fnv) {
+tmp = *b2;
+*b2 = *b1;
+b2-s = tmp.s;
+if ((!tmp.s-sticky || *tmp.s-sticky == 0)
+ b1-s-sticky

RE: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-11 Thread Plüm , Rüdiger , Vodafone Group 


 -Original Message-
 From: Jan Kaluža [mailto:jkal...@redhat.com]
 Sent: Donnerstag, 11. Dezember 2014 14:40
 To: dev@httpd.apache.org
 Subject: Re: [PATCH] Balancers, VirtualHost and ProxyPass
 
 On 12/11/2014 08:47 AM, Jan Kaluža wrote:
  On 12/10/2014 08:21 PM, Ruediger Pluem wrote:
 
 
  On 12/10/2014 02:21 PM, Jan Kaluža wrote:
  On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:
  But this way we lose the base ones that are not touched in the
  virtual host and e.g. are only used by rewriterules.
  So we should transfer the base ones to the merged array in any case
  and update them where needed.
 
  Hm, you are right. Check the new version attached to this email.
 
  But this one changes the parent configuration. So if you have two
  virtual hosts and in each you change a different
  parameter of the parent the later one has *both* changes, not only
  one. So you would need to do a copy of these
  balancers first and adjust the copy. Then only add the adjusted copy
  to the result. For the base balancers not matching
  the override balancers just add them to the result untouched. Same for
  the override ones, as you already do
 
  That makes sense, thanks. I will work on it and send updated patch.
 
 I have to admit that this feature is starting to be more time-consuming
 than I thought :). The attached patch should address all the issues
 pointed by Yann and Rüdiger.

Looks fine in general. Details:

+if (!tmp.lbmethod_set  b1-lbmethod_set) {
+b2-lbmethod_set = b1-lbmethod_set;
+b2-lbmethod = b1-lbmethod;

This is already the case because of the

 +*b2 = *b1;

above. So if lbmethod is set differently in overrides you lose it in favour of 
the base setting.


+PROXY_STRNCPY(tmp.s-lbpname, b1-s-lbpname);
+}
+if (!tmp.growth_set  b1-growth_set) {
+b2-growth_set = b1-growth_set;
+b2-growth = b1-growth;

Same as above

+}
+if (!tmp.failontimeout_set  b1-failontimeout_set) {
+b2-failontimeout_set = b1-failontimeout_set;
+b2-failontimeout = b1-failontimeout;
+}

Same as above.

 if (apr_is_empty_array(tmp.errstatuses)
+ !apr_is_empty_array(b1-errstatuses)) {
+apr_array_cat(b2-errstatuses, b1-errstatuses);
+}

b1 and b2 point to the same array. So the result will be a doubled b1 if 
apr_array_cat doesn't fail on getting supplied the same pointer twice :-)


Regards

Rüdiger

[PATCH] Balancers, VirtualHost and ProxyPass

2014-12-10 Thread Jan Kaluža 

Hi,

I've found out that following configuration does not work as expected:

Proxy balancer://a
   ...
/Proxy
VirtualHost *:80
ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
/VirtualHost

In this case, two proxy_balancers are created. The first one in Proxy 
section in the main config without stickysession and the second one in 
the vhost section with stickysession set.


Because of merge_proxy_config behaviour, the one from the main config is 
always preferred and therefore you cannot set stickysession (and other 
options) this way.


Attached patch fixes that by changing the merge strategy for balancers 
array to merge options set by ProxyPass.


I think we would need the same for proxy_worker too, but before I spent 
afternoon working on it, I wanted to ask, do you think this is the right 
way how to fix this?


Regards,
Jan Kaluza
Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c	(revision 1644346)
+++ modules/proxy/mod_proxy.c	(working copy)
@@ -306,6 +306,7 @@
 }
 else
 balancer-s-sticky_separator = *val;
+balancer-s-sticky_separator_set = 1;
 }
 else if (!strcasecmp(key, nofailover)) {
 /* If set to 'on' the session will break
@@ -318,6 +319,7 @@
 balancer-s-sticky_force = 0;
 else
 return failover must be On|Off;
+balancer-s-sticky_force_set = 1;
 }
 else if (!strcasecmp(key, timeout)) {
 /* Balancer timeout in seconds.
@@ -348,6 +350,7 @@
 if (provider) {
 balancer-lbmethod = provider;
 if (PROXY_STRNCPY(balancer-s-lbpname, val) == APR_SUCCESS) {
+balancer-lbmethod_set = 1;
 return NULL;
 }
 else {
@@ -367,6 +370,7 @@
 balancer-s-scolonsep = 0;
 else
 return scolonpathdelim must be On|Off;
+balancer-s-scolonsep_set = 1;
 }
 else if (!strcasecmp(key, failonstatus)) {
 char *val_split;
@@ -397,6 +401,7 @@
 balancer-failontimeout = 0;
 else
 return failontimeout must be On|Off;
+balancer-failontimeout_set = 1;
 }
 else if (!strcasecmp(key, nonce)) {
 if (!strcasecmp(val, None)) {
@@ -407,6 +412,7 @@
 return Provided nonce is too large;
 }
 }
+balancer-s-nonce_set = 1;
 }
 else if (!strcasecmp(key, growth)) {
 ival = atoi(val);
@@ -413,6 +419,7 @@
 if (ival  1 || ival  100)   /* arbitrary limit here */
 return growth must be between 1 and 100;
 balancer-growth = ival;
+balancer-growth_set = 1;
 }
 else if (!strcasecmp(key, forcerecovery)) {
 if (!strcasecmp(val, on))
@@ -421,6 +428,7 @@
 balancer-s-forcerecovery = 0;
 else
 return forcerecovery must be On|Off;
+balancer-s-forcerecovery_set = 1;
 }
 else {
 return unknown Balancer parameter;
@@ -1272,6 +1280,89 @@
 return ps;
 }
 
+static apr_array_header_t *merge_balancers(apr_pool_t *p,
+   apr_array_header_t *base,
+   apr_array_header_t *overrides)
+{
+proxy_balancer *b1;
+proxy_balancer *b2;
+proxy_balancer *copy;
+int x, y, found;
+apr_array_header_t *merged = apr_array_make(p, base-nelts,
+sizeof(proxy_balancer));
+
+/* Check if the balancer is defined in both override and base configs:
+ * a) If it is, merge the changes which can be done by ProxyPass.
+ * b) Otherwise, copy the balancer to merged array.
+ */
+b2 = (proxy_balancer *) overrides-elts;
+for (x = 0; x  overrides-nelts; x++) {
+b1 = (proxy_balancer *) base-elts;
+for (y = 0, found = 0; y  base-nelts; y++) {
+if (b1-hash.def == b2-hash.def  b1-hash.fnv == b2-hash.fnv) {
+if (b2-s-sticky  *b2-s-sticky) {
+PROXY_STRNCPY(b1-s-sticky_path, b2-s-sticky_path);
+PROXY_STRNCPY(b1-s-sticky, b2-s-sticky);
+}
+if (b2-s-sticky_separator_set) {
+b1-s-sticky_separator_set = b2-s-sticky_separator_set;
+b1-s-sticky_separator = b2-s-sticky_separator;
+}
+if (b2-s-timeout) {
+b1-s-timeout = b2-s-timeout;
+}
+if (b2-s-max_attempts_set) {
+b1-s-max_attempts_set = b2-s-max_attempts_set;
+b1-s-max_attempts = b2-s-max_attempts;
+}
+if (b2-lbmethod_set) {
+b1-lbmethod_set = b2-lbmethod_set;
+b1-lbmethod = b2-lbmethod;
+PROXY_STRNCPY(b1-s-lbpname, b2-s-lbpname);
+}
+if

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-10 Thread Yann Ylavic 
Hi,

didn't look at the patch yet, but the workaround for this is usually
to use ProxySet in the Proxy block.
I agree that it would be nice to have these parameters merged, though.

Regards,
Yann.


On Wed, Dec 10, 2014 at 1:25 PM, Jan Kaluža jkal...@redhat.com wrote:
 Hi,

 I've found out that following configuration does not work as expected:

 Proxy balancer://a
...
 /Proxy
 VirtualHost *:80
 ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
 /VirtualHost

 In this case, two proxy_balancers are created. The first one in Proxy
 section in the main config without stickysession and the second one in the
 vhost section with stickysession set.

 Because of merge_proxy_config behaviour, the one from the main config is
 always preferred and therefore you cannot set stickysession (and other
 options) this way.

 Attached patch fixes that by changing the merge strategy for balancers array
 to merge options set by ProxyPass.

 I think we would need the same for proxy_worker too, but before I spent
 afternoon working on it, I wanted to ask, do you think this is the right way
 how to fix this?

 Regards,
 Jan Kaluza

RE: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-10 Thread Plüm , Rüdiger , Vodafone Group 
But this way we lose the base ones that are not touched in the virtual host and 
e.g. are only used by rewriterules.
So we should transfer the base ones to the merged array in any case and update 
them where needed.
Isn't the config merge on a critical path with every request? So double for 
loops always worry me a little bit from performance point of view.

Regards

Rüdiger

 -Original Message-
 From: Jan Kaluža [mailto:jkal...@redhat.com]
 Sent: Mittwoch, 10. Dezember 2014 13:26
 To: httpd
 Subject: [PATCH] Balancers, VirtualHost and ProxyPass
 
 Hi,
 
 I've found out that following configuration does not work as expected:
 
 Proxy balancer://a
 ...
 /Proxy
 VirtualHost *:80
  ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
 /VirtualHost
 
 In this case, two proxy_balancers are created. The first one in Proxy
 section in the main config without stickysession and the second one in
 the vhost section with stickysession set.
 
 Because of merge_proxy_config behaviour, the one from the main config is
 always preferred and therefore you cannot set stickysession (and other
 options) this way.
 
 Attached patch fixes that by changing the merge strategy for balancers
 array to merge options set by ProxyPass.
 
 I think we would need the same for proxy_worker too, but before I spent
 afternoon working on it, I wanted to ask, do you think this is the right
 way how to fix this?
 
 Regards,
 Jan Kaluza

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-10 Thread Yann Ylavic 
On Wed, Dec 10, 2014 at 1:49 PM, Plüm, Rüdiger, Vodafone Group
ruediger.pl...@vodafone.com wrote:
 Isn't the config merge on a critical path with every request?

I don't think so, my understanding is that the *server* config merge
is before post_config.

Regards,
Yann.

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-10 Thread Yann Ylavic 
On Wed, Dec 10, 2014 at 1:52 PM, Yann Ylavic ylavic@gmail.com wrote:
 didn't look at the patch yet, but the workaround for this is usually
 to use ProxySet in the Proxy block.

This is not (at all) equivalent to a merge, please ignore this...

 Regards,
 Yann.

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-10 Thread Jan Kaluža 

On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:

But this way we lose the base ones that are not touched in the virtual host and 
e.g. are only used by rewriterules.
So we should transfer the base ones to the merged array in any case and update 
them where needed.


Hm, you are right. Check the new version attached to this email.


Isn't the config merge on a critical path with every request? So double for 
loops always worry me a little bit from performance point of view.


I think that happens only in ap_fixup_virtual_hosts call, which is 
executed only during the httpd start. From this point of view, double 
for loop should be OK.


Regards,
Jan Kaluza


Regards

Rüdiger


-Original Message-
From: Jan Kaluža [mailto:jkal...@redhat.com]
Sent: Mittwoch, 10. Dezember 2014 13:26
To: httpd
Subject: [PATCH] Balancers, VirtualHost and ProxyPass

Hi,

I've found out that following configuration does not work as expected:

Proxy balancer://a
 ...
/Proxy
VirtualHost *:80
  ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
/VirtualHost

In this case, two proxy_balancers are created. The first one in Proxy
section in the main config without stickysession and the second one in
the vhost section with stickysession set.

Because of merge_proxy_config behaviour, the one from the main config is
always preferred and therefore you cannot set stickysession (and other
options) this way.

Attached patch fixes that by changing the merge strategy for balancers
array to merge options set by ProxyPass.

I think we would need the same for proxy_worker too, but before I spent
afternoon working on it, I wanted to ask, do you think this is the right
way how to fix this?

Regards,
Jan Kaluza


Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c	(revision 1644346)
+++ modules/proxy/mod_proxy.c	(working copy)
@@ -306,6 +306,7 @@
 }
 else
 balancer-s-sticky_separator = *val;
+balancer-s-sticky_separator_set = 1;
 }
 else if (!strcasecmp(key, nofailover)) {
 /* If set to 'on' the session will break
@@ -318,6 +319,7 @@
 balancer-s-sticky_force = 0;
 else
 return failover must be On|Off;
+balancer-s-sticky_force_set = 1;
 }
 else if (!strcasecmp(key, timeout)) {
 /* Balancer timeout in seconds.
@@ -348,6 +350,7 @@
 if (provider) {
 balancer-lbmethod = provider;
 if (PROXY_STRNCPY(balancer-s-lbpname, val) == APR_SUCCESS) {
+balancer-lbmethod_set = 1;
 return NULL;
 }
 else {
@@ -367,6 +370,7 @@
 balancer-s-scolonsep = 0;
 else
 return scolonpathdelim must be On|Off;
+balancer-s-scolonsep_set = 1;
 }
 else if (!strcasecmp(key, failonstatus)) {
 char *val_split;
@@ -397,6 +401,7 @@
 balancer-failontimeout = 0;
 else
 return failontimeout must be On|Off;
+balancer-failontimeout_set = 1;
 }
 else if (!strcasecmp(key, nonce)) {
 if (!strcasecmp(val, None)) {
@@ -407,6 +412,7 @@
 return Provided nonce is too large;
 }
 }
+balancer-s-nonce_set = 1;
 }
 else if (!strcasecmp(key, growth)) {
 ival = atoi(val);
@@ -413,6 +419,7 @@
 if (ival  1 || ival  100)   /* arbitrary limit here */
 return growth must be between 1 and 100;
 balancer-growth = ival;
+balancer-growth_set = 1;
 }
 else if (!strcasecmp(key, forcerecovery)) {
 if (!strcasecmp(val, on))
@@ -421,6 +428,7 @@
 balancer-s-forcerecovery = 0;
 else
 return forcerecovery must be On|Off;
+balancer-s-forcerecovery_set = 1;
 }
 else {
 return unknown Balancer parameter;
@@ -1272,6 +1280,86 @@
 return ps;
 }
 
+static apr_array_header_t *merge_balancers(apr_pool_t *p,
+   apr_array_header_t *base,
+   apr_array_header_t *overrides)
+{
+proxy_balancer *b1;
+proxy_balancer *b2;
+int x, y, found;
+apr_array_header_t *tocopy = apr_array_make(p, 1, sizeof(proxy_balancer));
+
+/* Check if the balancer is defined in both override and base configs:
+ * a) If it is, merge the changes which can be done by ProxyPass.
+ * b) Otherwise, copy the balancer to merged array.
+ */
+b2 = (proxy_balancer *) overrides-elts;
+for (x = 0; x  overrides-nelts; x++) {
+b1 = (proxy_balancer *) base-elts;
+for (y = 0, found = 0; y  base-nelts; y++) {
+if (b1-hash.def == b2-hash.def  b1-hash.fnv == b2-hash.fnv) {
+if (b2-s-sticky  *b2-s-sticky) {
+PROXY_STRNCPY(b1-s-sticky_path, b2-s-sticky_path);
+PROXY_STRNCPY(b1-s-sticky, b2-s-sticky

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-10 Thread Yann Ylavic 
On Wed, Dec 10, 2014 at 2:21 PM, Jan Kaluža jkal...@redhat.com wrote:
 On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:
 Isn't the config merge on a critical path with every request? So double
 for loops always worry me a little bit from performance point of view.

 I think that happens only in ap_fixup_virtual_hosts call, which is executed
 only during the httpd start. From this point of view, double for loop should
 be OK.

Right, but since there will possibly more balancers/workers (main and
vhosts ones), the child_init() of mod_proxy and mod_proxy_balancer may
take longer to initialize all the balancers/workers.
This is not at request time still.

Also, what will the balancer_handler() do now, manage main
workers/balancers only?

Regards,
Yann.

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-10 Thread Jan Kaluža 

On 12/10/2014 02:50 PM, Yann Ylavic wrote:

On Wed, Dec 10, 2014 at 2:21 PM, Jan Kaluža jkal...@redhat.com wrote:

On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:

Isn't the config merge on a critical path with every request? So double
for loops always worry me a little bit from performance point of view.


I think that happens only in ap_fixup_virtual_hosts call, which is executed
only during the httpd start. From this point of view, double for loop should
be OK.


Right, but since there will possibly more balancers/workers (main and
vhosts ones), the child_init() of mod_proxy and mod_proxy_balancer may
take longer to initialize all the balancers/workers.
This is not at request time still.


That's true, but I'm not sure I see different way how to do it without 
changing the way how the balancers are stored.


I could for example create temporary apr_hash which would allow 
iterating override balancers just once, but I'm not sure it's so much 
better than the current way. (Btw, is there a reason, why are we storing 
balancers in array?)



Also, what will the balancer_handler() do now, manage main
workers/balancers only?


Hm, this should not change anything done by balancer_handler().

The patch only sets the balancer-s before the post_config is called. 
The vhost has still its own proxy_balancer instance as well as the main 
config. The only thing changed by the patch is that if you set some 
option in vhost config section, it will get set in proxy_balancer too.


The question here is if it is a valid use-case to use balancer with the 
same name in the main config and also in the vhost, but treat them as 
two independent balancers. If that's valid use-case, then the patch is 
wrong, because it changes the main balancer according to things done in 
vhost.


If you use BalancerPersist, it (imho) does not matter what you do with 
balancer-s before post_config, because after the post_config, it will 
get replaced by the stored shared memory.



Regards,
Yann.




Regards,
Jan Kaluza

Re: [PATCH] Balancers, VirtualHost and ProxyPass

2014-12-10 Thread Ruediger Pluem 


On 12/10/2014 02:21 PM, Jan Kaluža wrote:
 On 12/10/2014 01:49 PM, Plüm, Rüdiger, Vodafone Group wrote:
 But this way we lose the base ones that are not touched in the virtual host 
 and e.g. are only used by rewriterules.
 So we should transfer the base ones to the merged array in any case and 
 update them where needed.
 
 Hm, you are right. Check the new version attached to this email.

But this one changes the parent configuration. So if you have two virtual hosts 
and in each you change a different
parameter of the parent the later one has *both* changes, not only one. So you 
would need to do a copy of these
balancers first and adjust the copy. Then only add the adjusted copy to the 
result. For the base balancers not matching
the override balancers just add them to the result untouched. Same for the 
override ones, as you already do

Regards

Rüdiger