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
Created attachment 37337 [details] 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.
Created attachment 37338 [details] Add BALANCER_WORKER_STATUS environment variable Fix typo -- how'd THAT get in there?
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.
(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.
(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.
(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.
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.
Created attachment 37339 [details] 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.
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.
Coming back to this after lo these many years. I've added a string-based status reporting to the modules and it seems to work. What doesn't seem to work is ... reporting the status as expected. So, here is what balancer-status says about the balancer worker's status (yeah, I lit it up): Init Ign Drn Dis Stop Stby Spar But here is what comes across the wire to the app server: BALANCER_WORKER_STATUS 1 BALANCER_WORKER_STATUS_STR O At least 1 and O (oh) are consistent, but I don't think that's the actual status of the worker. Am I missing something? The status is being obtained using (*worker)->s->status