HWKMETRICS-330 Update filter to be async #481
Conversation
|
I tested the changes with a mock Kubernetes master server. Here are the results. Before patch: After patch: The throughput is nearly twice as greater than before, the response time 75th percentile is more than ten times better and the response time distribution is narrowed down. The tests ran on my laptop, with 1 Metrics server, 1 C* node. |
|
AFAICT LGTM (enough acronyms ?) ;) |
|
2016-04-15 22:56 GMT+02:00 Michael Burman notifications@github.com:
|
|
Still reviewing the PR. |
|
This PR does not work for me and it fails to function properly. I just deployed the war into the deployment directory of WildFly, so if there is any other steps I need to do, please let me know. |
| if (SECURITY_OPTION.contains(OPENSHIFT_OAUTH)) { | ||
| authenticator = new TokenAuthenticator(containerHandler); | ||
| } else if (SECURITY_OPTION.contains(HTPASSWD)) { | ||
| authenticator = new BasicAuthenticator(containerHandler); |
There was a problem hiding this comment.
We can have both basic auth and/or token auth enabled at the same time. Its should not be considered one or the other.
There was a problem hiding this comment.
Looking at https://git.io/vwwlB, my understanding is that SECURITY_OPTION
allows just one mechanism at a time. Or am I missing something?
2016-04-26 21:49 GMT+02:00 Matthew Wringe notifications@github.com:
In
containers/hawkular-metrics-openshift-integration/src/main/java/org/hawkular/openshift/auth/OpenshiftAuthHandler.java
#481 (comment)
:+public class OpenshiftAuthHandler implements HttpHandler {
- private static final String OPENSHIFT_OAUTH = "openshift-oauth";
- private static final String HTPASSWD = "htpasswd";
- private static final String DISABLED = "disabled";
- private static final String SECURITY_OPTION_SYSPROP = "hawkular-metrics.openshift.auth-methods";
- private static final String SECURITY_OPTION = System.getProperty(SECURITY_OPTION_SYSPROP, OPENSHIFT_OAUTH);
- private final HttpHandler containerHandler;
- private final Authenticator authenticator;
- public OpenshiftAuthHandler(HttpHandler containerHandler) {
this.containerHandler = containerHandler;if (SECURITY_OPTION.contains(OPENSHIFT_OAUTH)) {authenticator = new TokenAuthenticator(containerHandler);} else if (SECURITY_OPTION.contains(HTPASSWD)) {authenticator = new BasicAuthenticator(containerHandler);We can have both basic auth and/or token auth enabled at the same time.
Its should not be considered one or the other.—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/hawkular/hawkular-metrics/pull/481/files/6f1f68379c3a457ff34180821423e68be5469a2b#r61152239
Thomas Segismont
JBoss ON Engineering Team
There was a problem hiding this comment.
Not exactly, that code selects which one is based on what the auth header is for the request and if the corresponding SECURITY_OPTION is specified. There is nothing in there that prevents multiple SECURITY_OPTION from being available.
We need to support multiple SECURITY_OPTIONS, and we do use both in our current implementation: https://github.com/openshift/origin-metrics/blob/master/deployer/templates/hawkular-metrics.yaml#L75
There was a problem hiding this comment.
Ooops, right. I overlooked the "contains" method. Fixing it now.
2016-04-27 16:49 GMT+02:00 Matthew Wringe notifications@github.com:
In
containers/hawkular-metrics-openshift-integration/src/main/java/org/hawkular/openshift/auth/OpenshiftAuthHandler.java
#481 (comment)
:+public class OpenshiftAuthHandler implements HttpHandler {
- private static final String OPENSHIFT_OAUTH = "openshift-oauth";
- private static final String HTPASSWD = "htpasswd";
- private static final String DISABLED = "disabled";
- private static final String SECURITY_OPTION_SYSPROP = "hawkular-metrics.openshift.auth-methods";
- private static final String SECURITY_OPTION = System.getProperty(SECURITY_OPTION_SYSPROP, OPENSHIFT_OAUTH);
- private final HttpHandler containerHandler;
- private final Authenticator authenticator;
- public OpenshiftAuthHandler(HttpHandler containerHandler) {
this.containerHandler = containerHandler;if (SECURITY_OPTION.contains(OPENSHIFT_OAUTH)) {authenticator = new TokenAuthenticator(containerHandler);} else if (SECURITY_OPTION.contains(HTPASSWD)) {authenticator = new BasicAuthenticator(containerHandler);Not exactly, that code selects which one is based on what the auth header
is for the request and if the corresponding SECURITY_OPTION is specified.
There is nothing in there that prevents multiple SECURITY_OPTION from being
available.We need to support multiple SECURITY_OPTIONS, and we do use both in our
current implementation:
https://github.com/openshift/origin-metrics/blob/master/deployer/templates/hawkular-metrics.yaml#L75—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/hawkular/hawkular-metrics/pull/481/files/6f1f68379c3a457ff34180821423e68be5469a2b#r61271587
Thomas Segismont
JBoss ON Engineering Team
|
@mwringe I have no problems deploying it. |
|
@burmanm Did you update our containers to use it and then run it on OpenShift? |
|
Looks like something I missed as I was working with a mock server. I will 2016-04-26 21:37 GMT+02:00 Matthew Wringe notifications@github.com:
Thomas Segismont |
|
Actually it is a communication level problem. Is there any way for me to I would also like to understand the difference between your environment and 2016-04-27 15:16 GMT+02:00 Thomas Segismont tsegismo@redhat.com:
Thomas Segismont |
me too, I am wondering if I made a mistake with my test containers. @burmanm what was your setup for your test environment? |
| // com.codahale.metrics.Reporter instances. | ||
| metricsService.startUp(session, keyspace, false, false, new MetricRegistry()); | ||
| MetricRegistry metricRegistry = MetricRegistryProvider.INSTANCE.getMetricRegistry(); | ||
| jmxReporter = JmxReporter.forRegistry(metricRegistry).inDomain("hawkular.metrics").build(); |
There was a problem hiding this comment.
Can you make JmxReporter configurable? So that it does not start unless it is explicitly configured to start?
There was a problem hiding this comment.
I'd prefer not to add another config var unless it can be useful. Why would we want to have the Metrics collected but not accessible? It is very convenient to start jconsole or Mission control and have a a view of what's going on (threads, MBeans, memory, ... etc).
There was a problem hiding this comment.
I'm late to the game on this PR, but I tend to agree with Stefan. Unless we need to always have JMXReporter running, I would prefer to keep it off or at least be able to turn it off. This made me think of a bug with an early version of the DataStax driver that involved JMXReporter that we hit in RHQ. It may have been a thread leak. I do not recall the specifics. What I do recall though is that disabling the reporter worked around the issue.
There was a problem hiding this comment.
There's a new config flag in the last commit.
|
Based on your comments @mwringe and @tsegismont I became suspicious and rebuilt everything.. and still can't get those errors? Newest origin (pulled yesterday), modified the Dockerfile of origin-metrics to use "COPY" instead of fetching from repository (to be able to use locally built .war of PR481). openjdk version "1.8.0_77" |
tsegismont
force-pushed
the
jira/HWKMETRICS-330
branch
2 times, most recently
from
08ca1fa
to
3d3c5f5
Compare
|
@burmanm good news |
|
@burmanm did you verify that the metrics were showing in the console at all? The components should not have worked since before it was only using one type of authentication, and the system requires 2 |
|
@mwringe the fix for this is in the PR since this morning 2016-04-28 13:20 GMT+02:00 Matthew Wringe notifications@github.com:
Thomas Segismont |
|
@mwringe No, I did not test console |
A bit of context first. Hawkular Metrics is a reactive application and calls to a blocking API should be avoided as much as possible. The Openshift integration library had a servlet filter mechanism to authenticate users. The filter code used the JDK's HTTP client to call Kubernetes' master server. While this was alright has a first implementation (in order to provide the feature as quickly as possible), it is a serious limitation to reach our performance goals. There were two problems to tackle: 1. choose an async HTTP client API 2. make the filter code asynchronous For the HTTP client, I considered a few options: Netty, vert.x, Undertow. I picked undertow because it does not add a new dependency, we can reuse the io threads (instead of creating yet another thread pool). The downside is that the Undertow client API is low-level (no pool implementation) and not well documented. For the filter code, my first try was to use servlet async API from the filter. But RestEasy throws an exception because it wants start the async exchange itself. I wrote to the resteasy-user list but got no reply. And even if we had a fix, we would have to wait for a new RestEasy and Wildfly release. So I wrote an Undertow extension which is executed in io threads *before* the servlet handler is involved. The implementation consists in pooling Undertow HTTP client connections and filering the Metrics client requests (only dispatch to the servlet handler if the user is authenticated). While working on the implementation I had to find a way to share the MetricsRegistry between the webapp code and the authenticator code. So there's a MetricsRegistry provider class in core-util now. Also, I enhanced the Gatling scenario file to support multiple authentication mechanisms (none, Hawkular integration, Openshift htpasswd file, Openshift token). I took the opportunity to document the scenario options in the project README. Note that performance enhancements will be more visible in environments where Kubernetes reponse time is minimal.
Multiple authenticators can be active at the same time
tsegismont
force-pushed
the
jira/HWKMETRICS-330
branch
from
c1db087
to
c635826
Compare
Disables reporting for both the driver's registry and our registry
|
With the newest version I can see metrics in the console. |
|
Sorry I'm lost. So do you see the same exceptions as @mwringe or not? 2016-04-29 12:11 GMT+02:00 Michael Burman notifications@github.com:
Thomas Segismont |
|
As said on the IRC, no I did not see any exceptions. I see the metrics charts being drawn correctly on the console. |
|
Thank @tsegismont! |
HWKMETRICS-330 Update filter to be async
A bit of context first. Hawkular Metrics is a reactive application and
calls to a blocking API should be avoided as much as possible.
The Openshift integration library had a servlet filter mechanism to
authenticate users. The filter code used the JDK's HTTP client to call
Kubernetes' master server. While this was alright has a first
implementation (in order to provide the feature as quickly as possible),
it is a serious limitation to reach our performance goals.
There were two problems to tackle:
For the HTTP client, I considered a few options: Netty, vert.x,
Undertow. I picked undertow because it does not add a new dependency,
we can reuse the io threads (instead of creating yet another thread
pool). The downside is that the Undertow client API is low-level (no pool
implementation) and not well documented.
For the filter code, my first try was to use servlet async API from the
filter. But RestEasy throws an exception because it wants start the
async exchange itself. I wrote to the resteasy-user list but got no
reply. And even if we had a fix, we would have to wait for a new
RestEasy and Wildfly release. So I wrote an Undertow extension which is
executed in io threads before the servlet handler is involved.
The implementation consists in pooling Undertow HTTP client connections and
filering the Metrics client requests (only dispatch to the servlet
handler if the user is authenticated).
While working on the implementation I had to find a way to share the
MetricsRegistry between the webapp code and the authenticator code. So
there's a MetricsRegistry provider class in core-util now.
Also, I enhanced the Gatling scenario file to support multiple
authentication mechanisms (none, Hawkular integration, Openshift
htpasswd file, Openshift token). I took the opportunity to document the
scenario options in the project README.
Note that performance enhancements will be more visible in environments
where Kubernetes reponse time is minimal.