Re: [PATCH] Balancers, VirtualHost and ProxyPass
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 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
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 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 wrote: >>> >>> Hi, >>> >>> I've found out that following configuration does not work as expected: >>> >>> >>> ... >>> >>> >>>ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid >>> >>> >>> 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
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 wrote: Hi, I've found out that following configuration does not work as expected: ... ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid 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
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 wrote: > > Hi, > > didn't look at the patch yet, but the workaround for this is usually > to use ProxySet in the 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 wrote: >> Hi, >> >> I've found out that following configuration does not work as expected: >> >> >> ... >> >> >>ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid >> >> >> 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
Isn't this already addressed/handled with the BalancerInherit directive?? > On Dec 10, 2014, at 7:25 AM, Jan Kaluža wrote: > > Hi, > > I've found out that following configuration does not work as expected: > > > ... > > >ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid > > > 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
On Fri, Dec 12, 2014 at 2:23 PM, Jan Kaluža 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
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 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
On Fri, Dec 12, 2014 at 2:09 PM, Plüm, Rüdiger, Vodafone Group 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.
AW: [PATCH] Balancers, VirtualHost and ProxyPass
> -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
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
Re: [PATCH] Balancers, VirtualHost and ProxyPass
On 12/12/2014 12:08 PM, Yann Ylavic wrote:
Hi Jan,
On Fri, Dec 12, 2014 at 9:44 AM, Jan Kaluža 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
On Fri, Dec 12, 2014 at 12:08 PM, Yann Ylavic 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
Hi Jan,
On Fri, Dec 12, 2014 at 9:44 AM, Jan Kaluža 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
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
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";
RE: [PATCH] Balancers, VirtualHost and ProxyPass
> -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
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.
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->sti
Re: [PATCH] Balancers, VirtualHost and ProxyPass
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
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
Re: [PATCH] Balancers, VirtualHost and ProxyPass
On 12/10/2014 02:50 PM, Yann Ylavic wrote: On Wed, Dec 10, 2014 at 2:21 PM, Jan Kaluža 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
On Wed, Dec 10, 2014 at 2:21 PM, Jan Kaluža 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
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:
...
ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
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;
Re: [PATCH] Balancers, VirtualHost and ProxyPass
On Wed, Dec 10, 2014 at 1:52 PM, Yann Ylavic wrote: > didn't look at the patch yet, but the workaround for this is usually > to use ProxySet in the block. This is not (at all) equivalent to a merge, please ignore this... > Regards, > Yann.
Re: [PATCH] Balancers, VirtualHost and ProxyPass
On Wed, Dec 10, 2014 at 1:49 PM, Plüm, Rüdiger, Vodafone Group 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
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: > > > ... > > > ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid > > > 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
Hi, didn't look at the patch yet, but the workaround for this is usually to use ProxySet in the 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 wrote: > Hi, > > I've found out that following configuration does not work as expected: > > >... > > > ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid > > > 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
[PATCH] Balancers, VirtualHost and ProxyPass
Hi,
I've found out that following configuration does not work as expected:
...
ProxyPass / balancer://a stickysession=JSESSIONID|jsessionid
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->lbp
