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,
+ 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;
+
+ /* For shared memory entries, b2->s belongs to override
+ * balancer, so if some entry is not set there, we have to
+ * update it according to the base balancer. */
+ if (*b2->s->sticky == 0 && *b1->s->sticky) {
+ PROXY_STRNCPY(b2->s->sticky_path, b1->s->sticky_path);
+ PROXY_STRNCPY(b2->s->sticky, b1->s->sticky);
+ }
+ if (!b2->s->sticky_separator_set
+ && b1->s->sticky_separator_set) {
+ b2->s->sticky_separator_set = b1->s->sticky_separator_set;
+ b2->s->sticky_separator = b1->s->sticky_separator;
+ }
+ if (!b2->s->timeout && b1->s->timeout) {
+ b2->s->timeout = b1->s->timeout;
+ }
+ if (!b2->s->max_attempts_set && b1->s->max_attempts_set) {
+ b2->s->max_attempts_set = b1->s->max_attempts_set;
+ b2->s->max_attempts = b1->s->max_attempts;
+ }
+ if (!b2->s->nonce_set && b1->s->nonce_set) {
+ b2->s->nonce_set = b1->s->nonce_set;
+ PROXY_STRNCPY(b2->s->nonce, b1->s->nonce);
+ }
+ if (!b2->s->sticky_force_set && b1->s->sticky_force_set) {
+ b2->s->sticky_force_set = b1->s->sticky_force_set;
+ b2->s->sticky_force = b1->s->sticky_force;
+ }
+ if (!b2->s->scolonsep_set && b1->s->scolonsep_set) {
+ b2->s->scolonsep_set = b1->s->scolonsep_set;
+ b2->s->scolonsep = b1->s->scolonsep;
+ }
+ if (!b2->s->forcerecovery_set && b1->s->forcerecovery_set) {
+ b2->s->forcerecovery_set = b1->s->forcerecovery_set;
+ b2->s->forcerecovery = b1->s->forcerecovery;
+ }
+
+ /* For non-shared memory entries, b2 is copy of b1, so we have
+ * to use tmp copy of b1 to detect changes done in override. */
+ if (tmp.lbmethod_set) {
+ b2->lbmethod_set = tmp.lbmethod_set;
+ b2->lbmethod = tmp.lbmethod;
+ }
+ if (tmp.growth_set) {
+ b2->growth_set = tmp.growth_set;
+ b2->growth = tmp.growth;
+ }
+ if (tmp.failontimeout_set) {
+ b2->failontimeout_set = tmp.failontimeout_set;
+ b2->failontimeout = tmp.failontimeout;
+ }
+ if (!apr_is_empty_array(tmp.errstatuses)) {
+ apr_array_cat(tmp.errstatuses, b2->errstatuses);
+ b2->errstatuses = tmp.errstatuses;
+ }
+
+ found = 1;
+ break;
+ }
+ b2++;
+ }
+ if (!found) {
+ *(proxy_balancer *)apr_array_push(tocopy) = *b1;
+ }
+ b1++;
+ }
+
+ return apr_array_append(p, tocopy, overrides);
+}
+
static void * merge_proxy_config(apr_pool_t *p, void *basev, void *overridesv)
{
proxy_server_conf *ps = apr_pcalloc(p, sizeof(proxy_server_conf));
@@ -1296,7 +1397,7 @@
ps->dirconn = apr_array_append(p, base->dirconn, overrides->dirconn);
if (ps->inherit || ps->ppinherit) {
ps->workers = apr_array_append(p, base->workers, overrides->workers);
- ps->balancers = apr_array_append(p, base->balancers, overrides->balancers);
+ ps->balancers = merge_balancers(p, base->balancers, overrides->balancers);
}
else {
ps->workers = overrides->workers;
Index: modules/proxy/mod_proxy.h
===================================================================
--- modules/proxy/mod_proxy.h (revision 1644346)
+++ modules/proxy/mod_proxy.h (working copy)
@@ -443,6 +443,11 @@
unsigned int inactive:1;
unsigned int forcerecovery:1;
char sticky_separator; /* separator for sessionid/route */
+ unsigned int forcerecovery_set:1;
+ unsigned int scolonsep_set:1;
+ unsigned int sticky_force_set:1;
+ unsigned int nonce_set:1;
+ unsigned int sticky_separator_set:1;
} proxy_balancer_shared;
#define ALIGNED_PROXY_BALANCER_SHARED_SIZE (APR_ALIGN_DEFAULT(sizeof(proxy_balancer_shared)))
@@ -463,6 +468,9 @@
void *context; /* general purpose storage */
proxy_balancer_shared *s; /* Shared data */
int failontimeout; /* Whether to mark a member in Err if IO timeout occurs */
+ unsigned int failontimeout_set:1;
+ unsigned int growth_set:1;
+ unsigned int lbmethod_set:1;
};
struct proxy_balancer_method {
