[Bug 64338] New: Expose balancer member state to origin server

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 64338] New: Expose balancer member state to origin server

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

            Bug ID: 64338
           Summary: Expose balancer member state to origin server
           Product: Apache httpd-2
           Version: 2.5-HEAD
          Hardware: All
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: mod_proxy_balancer
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

In preparation to migrate from mod_jk to mod_proxy_http, I looked for a way to
detect worker state in order to detect whether a node had been disabled and
should be draining.

mod_jk does this by sending a value which can be checked by the Java
application in the request's "JK_LB_ACTIVATION" attribute.

I don't see anything like this in mod_proxy_balancer, and it would be of great
help to have it.

From [1] it looks like mod_proxy_ajp will forward any environment variable
called AJP_* through the connection as a request attribute (slight different
than a request parameter or request a header), but I'd prefer to use
mod_proxy_http so that leaves basically headers as an option for transport.

I could use "Header set X-lb-activation-state ${state}e", but the worker state
isn't available for this kind of thing.

It sounds "simple" to me to add this, but I have no idea what I'm talking
about. If this value were available via environment variable, I'd be able to
build similar capabilities with the other existing building-blocks to achieve
feature-parity with mod_jk.


[1]
https://lists.apache.org/thread.html/rfa741b0b2d0a53a9a4ae4bca26eaaccb57280392224348381e080376%40%3Cusers.tomcat.apache.org%3E

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin server

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

--- Comment #1 from Christopher Schultz <[hidden email]> ---
Created attachment 37337
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37337&action=edit
Add BALANCER_WORKER_STATUS environment variable

Patch is against 2.4 trunk.

I decided to simply dump the decimal-int value of the status into the
environment variable rather than trying to render it into a human-readable
status value like the (confusingly-named) ap_proxy_parse_wstatus() function
does.

I believe I have chosen the correct APR routines for proper
resource-management.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

Christopher Schultz <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #37337|0                           |1
        is obsolete|                            |

--- Comment #2 from Christopher Schultz <[hidden email]> ---
Created attachment 37338
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37338&action=edit
Add BALANCER_WORKER_STATUS environment variable

Fix typo -- how'd THAT get in there?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

--- Comment #3 from Ruediger Pluem <[hidden email]> ---
Thanks for the patch. Some points:

1. Why not setting this environment variable in the same block as
BALANCER_WORKER_ROUTE (line 622) but only when a session route was supplied?
2. I haven't checked it we could run into issues as apr_itoa takes an int while
worker->s->status is an unsigned int. At least this could cause compiler
warnings.
3. Please add a documentation for your new variable to
docs/manual/mod/mod_proxy_balancer.xml at the section environment starting at
about line 130.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

Christopher Schultz <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #37338|0                           |1
           is patch|                            |

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

--- Comment #4 from Christopher Schultz <[hidden email]> ---
(In reply to Ruediger Pluem from comment #3)
> Thanks for the patch.

Long-time listener. First-time patcher. :)

> Some points:
>
> 1. Why not setting this environment variable in the same block as
> BALANCER_WORKER_ROUTE (line 622) but only when a session route was supplied?

This is my ignorance showing. I figured that if there was no route, then the
request wouldn't actually be delivered anywhere and it would be wasted effort.
What does the condition on line 632 actually check?

    /* Rewrite the url from 'balancer://url'
     * to the 'worker_scheme://worker_hostname[:worker_port]/url'
     * This replaces the balancers fictional name with the
     * real hostname of the elected worker.
     */
    access_status = rewrite_url(r, *worker, url);
    /* Add the session route to request notes if present */
    if (route) {                                 // <=================== line
632
        apr_table_setn(r->notes, "session-sticky", sticky);
        apr_table_setn(r->notes, "session-route", route);

        /* Add session info to env. */
        apr_table_setn(r->subprocess_env,
                       "BALANCER_SESSION_STICKY", sticky);
        apr_table_setn(r->subprocess_env,
                       "BALANCER_SESSION_ROUTE", route);
        apr_table_setn(r->subprocess_env,
                       "BALANCER_WORKER_STATUS",
                       apr_itoa(r->pool, (*worker)->s->status));

I'm happy to move this to a more appropriate location.

> 2. I haven't checked it we could run into issues as apr_itoa takes an int
> while worker->s->status is an unsigned int. At least this could cause
> compiler warnings.

I got no compiler warnings using gcc, though I didn't specifically enable
anything during the build. I just assumed warnings were enabled during compile.

But I do take your point: status is unsigned int and I'm using atoi. All the
current statuses are non-negative and don't run higher than 0x800 so we're safe
for now, but it's definitely not future-proof.

I didn't see apr_uitoa, but of course there is still apr_ltoa. I just wanted to
be as minimally impactful on performance as possible. I suppose I could check
the magnitude of 'status' and use apr_ltoa if necessary after a cast.

What is your recommendation, here?

> 3. Please add a documentation for your new variable to
> docs/manual/mod/mod_proxy_balancer.xml at the section environment starting
> at about line 130.

Aha! I can certainly do that.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

--- Comment #5 from Ruediger Pluem <[hidden email]> ---
(In reply to Christopher Schultz from comment #4)
> (In reply to Ruediger Pluem from comment #3)

> >
> > 1. Why not setting this environment variable in the same block as
> > BALANCER_WORKER_ROUTE (line 622) but only when a session route was supplied?
>
> This is my ignorance showing. I figured that if there was no route, then the
> request wouldn't actually be delivered anywhere and it would be wasted
> effort. What does the condition on line 632 actually check?

It checks if a route was supplied by the client, but I think the worker state
might be interesting even in the case that the client did not supply a route,
e.g. because it is a freshly opened browser which has no session cookie yet.
A worker will be chosen according to the LB policy then.

>
>     /* Rewrite the url from 'balancer://url'
>      * to the 'worker_scheme://worker_hostname[:worker_port]/url'
>      * This replaces the balancers fictional name with the
>      * real hostname of the elected worker.
>      */
>     access_status = rewrite_url(r, *worker, url);
>     /* Add the session route to request notes if present */
>     if (route) {                                 // <===================
> line 632
>         apr_table_setn(r->notes, "session-sticky", sticky);
>         apr_table_setn(r->notes, "session-route", route);
>
>         /* Add session info to env. */
>         apr_table_setn(r->subprocess_env,
>                        "BALANCER_SESSION_STICKY", sticky);
>         apr_table_setn(r->subprocess_env,
>                        "BALANCER_SESSION_ROUTE", route);
>         apr_table_setn(r->subprocess_env,
>                        "BALANCER_WORKER_STATUS",
>                        apr_itoa(r->pool, (*worker)->s->status));
>
> I'm happy to move this to a more appropriate location.
>
> > 2. I haven't checked it we could run into issues as apr_itoa takes an int
> > while worker->s->status is an unsigned int. At least this could cause
> > compiler warnings.
>
> I got no compiler warnings using gcc, though I didn't specifically enable
> anything during the build. I just assumed warnings were enabled during
> compile.
>
> But I do take your point: status is unsigned int and I'm using atoi. All the
> current statuses are non-negative and don't run higher than 0x800 so we're
> safe for now, but it's definitely not future-proof.
>
> I didn't see apr_uitoa, but of course there is still apr_ltoa. I just wanted
> to be as minimally impactful on performance as possible. I suppose I could
> check the magnitude of 'status' and use apr_ltoa if necessary after a cast.
>
> What is your recommendation, here?

So far I have none. I am waiting for my fellow developers :-).
If no one speaks up here, we probably just commit and discuss later on the
development list.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

--- Comment #6 from Christopher Schultz <[hidden email]> ---
(In reply to Christopher Schultz from comment #4)
> (In reply to Ruediger Pluem from comment #3)
> > 3. Please add a documentation for your new variable to
> > docs/manual/mod/mod_proxy_balancer.xml at the section environment starting
> > at about line 130.
>
> Aha! I can certainly do that.

Are the individual status bits for a proxy worker documented anywhere other
than in the header file? Since the value of BALANCER_WORKER_STATUS is an int
value, the bits need to be documented to be useful, and it seems like this
isn't the right place to do it.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

--- Comment #7 from Christopher Schultz <[hidden email]> ---
It seems that this needs to be more complicated to be truly useful.

The use-case is to provide the worker status to the upstream server, and merely
making an environment variable available doesn't really accomplish much. See
https://lists.apache.org/thread.html/r5bbb2421edcdca810f70185417a7e7d82e13910008b4b7e7e69e64ef%40%3Cusers.httpd.apache.org%3E
for more background.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin / upstream server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

Christopher Schultz <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Expose balancer member      |Expose balancer member
                   |state to origin server      |state to origin / upstream
                   |                            |server

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin / upstream server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

Christopher Schultz <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #37338|0                           |1
        is obsolete|                            |

--- Comment #8 from Christopher Schultz <[hidden email]> ---
Created attachment 37339
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37339&action=edit
Add BALANCER_WORKER_STATUS environment variable, pass to upstream in http/ajp
modules

A new proposal where mod_proxy_balancer sets the environment variable and the
individual protocol-handlers decide what to do. mod_proxy_http uses
X-Worker-Status header and mod_proxy_ajp uses BALANCER_WORKER_STATUS request
attribute.

No documentation yet.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64338] Expose balancer member state to origin / upstream server

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64338

--- Comment #9 from Yann Ylavic <[hidden email]> ---
I'd suggest using the PROXY_WORKER_*_STAT characters or proxy_wstat_tbl strings
rather than the numeric/mask value. Something like this maybe:

static char *worker_status_str(apr_pool_t *p, const proxy_worker *worker)
{
    apr_array_header_t *str = apr_array_make(p, 13, sizeof(char));
    proxy_wstat_t *wstat = proxy_wstat_tbl;
    while (wstat->flag) {
        if (worker->s->status & wstat->bit) {
            APR_ARRAY_PUSH(str, char) = wstat->flag;
        }
        wstat++;
    }
    APR_ARRAY_PUSH(str, char) = '\0';
    return str->elts;
}

Regarding the X-Worker-Status header, possibly it could be conditional (opt-in)
to "ProxyAddWorkerHeaders on" or alike (similar to ProxyAddHeaders)? One does
not necessarily want to expose this information to the backend.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]