Skip to content

[ML] Inference API add configurable connection pool TTL #127585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 7, 2025
Merged

[ML] Inference API add configurable connection pool TTL #127585

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6ae06f
Adding ttl
jonathan-buttner Apr 30, 2025
265eac9
Merge branch 'main' into ml-pool-ttl
jonathan-buttner May 1, 2025
6b6d138
Using 60 seconds as default
jonathan-buttner May 6, 2025
3087ee1
Merge branch 'main' into ml-pool-ttl
jonathan-buttner May 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferencePlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,13 +270,17 @@ public Collection<?> createComponents(PluginServices services) {
var inferenceServices = new ArrayList<>(inferenceServiceExtensions);
inferenceServices.add(this::getInferenceServiceFactories);

var inferenceServiceSettings = new ElasticInferenceServiceSettings(settings);
inferenceServiceSettings.init(services.clusterService());

// Create a separate instance of HTTPClientManager with its own SSL configuration (`xpack.inference.elastic.http.ssl.*`).
var elasticInferenceServiceHttpClientManager = HttpClientManager.create(
settings,
services.threadPool(),
services.clusterService(),
throttlerManager,
getSslService()
getSslService(),
inferenceServiceSettings.getConnectionTtl()
);

var elasticInferenceServiceRequestSenderFactory = new HttpRequestSender.Factory(
Expand All @@ -286,9 +290,6 @@ public Collection<?> createComponents(PluginServices services) {
);
elasicInferenceServiceFactory.set(elasticInferenceServiceRequestSenderFactory);

var inferenceServiceSettings = new ElasticInferenceServiceSettings(settings);
inferenceServiceSettings.init(services.clusterService());

var authorizationHandler = new ElasticInferenceServiceAuthorizationRequestHandler(
inferenceServiceSettings.getElasticInferenceServiceUrl(),
services.threadPool()
Expand Down
18 changes: 14 additions & 4 deletions ...ence/src/main/java/org/elasticsearch/xpack/inference/external/http/HttpClientManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.io.Closeable;
import java.io.IOException;
import java.util.List;
import java.util.concurrent.TimeUnit;

import static org.elasticsearch.core.Strings.format;
import static org.elasticsearch.xpack.inference.services.elastic.ElasticInferenceServiceSettings.ELASTIC_INFERENCE_SERVICE_SSL_CONFIGURATION_PREFIX;
Expand Down Expand Up @@ -112,14 +113,15 @@ public static HttpClientManager create(
ThreadPool threadPool,
ClusterService clusterService,
ThrottlerManager throttlerManager,
SSLService sslService
SSLService sslService,
TimeValue connectionTtl
) {
// Set the sslStrategy to ensure an encrypted connection, as Elastic Inference Service requires it.
SSLIOSessionStrategy sslioSessionStrategy = sslService.sslIOSessionStrategy(
sslService.getSSLConfiguration(ELASTIC_INFERENCE_SERVICE_SSL_CONFIGURATION_PREFIX)
);

PoolingNHttpClientConnectionManager connectionManager = createConnectionManager(sslioSessionStrategy);
PoolingNHttpClientConnectionManager connectionManager = createConnectionManager(sslioSessionStrategy, connectionTtl);
return new HttpClientManager(settings, connectionManager, threadPool, clusterService, throttlerManager);
}

Expand All @@ -146,7 +148,7 @@ public static HttpClientManager create(
this.addSettingsUpdateConsumers(clusterService);
}

private static PoolingNHttpClientConnectionManager createConnectionManager(SSLIOSessionStrategy sslStrategy) {
private static PoolingNHttpClientConnectionManager createConnectionManager(SSLIOSessionStrategy sslStrategy, TimeValue connectionTtl) {
ConnectingIOReactor ioReactor;
try {
var configBuilder = IOReactorConfig.custom().setSoKeepAlive(true);
Expand All @@ -162,7 +164,15 @@ private static PoolingNHttpClientConnectionManager createConnectionManager(SSLIO
.register("https", sslStrategy)
.build();

return new PoolingNHttpClientConnectionManager(ioReactor, registry);
return new PoolingNHttpClientConnectionManager(
ioReactor,
null,
registry,
null,
null,
Math.toIntExact(connectionTtl.getMillis()),
TimeUnit.MILLISECONDS
);
}

private static PoolingNHttpClientConnectionManager createConnectionManager() {
Expand Down
18 changes: 18 additions & 0 deletions ...a/org/elasticsearch/xpack/inference/services/elastic/ElasticInferenceServiceSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,20 +70,33 @@ public class ElasticInferenceServiceSettings {
Setting.Property.NodeScope
);

/**
* Total time to live (TTL) defines maximum life span of persistent connections regardless of their
* expiration setting. No persistent connection will be re-used past its TTL value.
*/
public static final Setting<TimeValue> CONNECTION_TTL_SETTING = Setting.timeSetting(
"xpack.inference.elastic.http.connection_ttl",
// -1 indicates that the TTL never expires
Copy link
Contributor Author

@jonathan-buttner jonathan-buttner Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the constructor for the pooling connection manager

    public PoolingNHttpClientConnectionManager(ConnectingIOReactor ioReactor, NHttpConnectionFactory<ManagedNHttpClientConnection> connFactory, Registry<SchemeIOSessionStrategy> ioSessionFactoryRegistry, SocketAddressResolver<HttpRoute> socketAddressResolver) {
        this(ioReactor, connFactory, ioSessionFactoryRegistry, socketAddressResolver, -1L, TimeUnit.MILLISECONDS);
    }

TimeValue.MINUS_ONE,
Copy link
Contributor Author

@jonathan-buttner jonathan-buttner Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxjakob @prwhelan @dtuck9 any thoughts on a default value here?

Copy link

@dtuck9 dtuck9 Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm not entirely familiar with the use case to give specific guidance. But some questions to consider:

  1. Is this a high request rate client?
  2. Would having to re-perform the TLS handshake more frequently cause any notable latency/performance issues?
  3. Do you have metrics in place that could monitor any potential negative consequences of setting this?

And to confirm, this setting wouldn't abruptly close connections with in-flight requests, correct?

Copy link
Contributor Author

@jonathan-buttner jonathan-buttner May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Diana

Is this a high request rate client?

Yeah it could be, at the moment likely not but once we support sparse embedding I can see it having a higher request rate.

Would having to re-perform the TLS handshake more frequently cause any notable latency/performance issues?

I'm not sure

Do you have metrics in place that could monitor any potential negative consequences of setting this?

Not at the moment but we should probably add some 😅

And to confirm, this setting wouldn't abruptly close connections with in-flight requests, correct?

My understanding of the docs is that if the TTL is reached, the pool won't reuse an existing connection. I haven't seen anything about it terminating active connections.

Copy link
Member

@prwhelan prwhelan May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is for closing idle connections, so it should be safe to set this to some value so that the connection gets released from the NLB/EIS.

My understanding is it just needs to be lower than the NLB's connection timeout, which is ~350s, otherwise the client tries to reuse the connection and the NLB sends TCP RST which I think surfaces as another connection timeout? The setting would depend on the user's traffic patterns, so I feel like it's hard for us to pick a good value for everyone. Making it configurable is a good idea, then maybe we have the default set to 60s (I'm picking that number randomly)

Copy link
Member

@davidkyle davidkyle May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value should be set so the connection does expire, this will refresh the connection pool and possibly help overcome bugs with broken connections. The setting limits the lifespan of the connection in the pool, 24 hours seems like a reasonable value. If all the connections in the pool are created at the same time maybe add some jitter (+/- [1-60]) mins so they don't all EOL at the same time

Copy link
Contributor Author

@jonathan-buttner jonathan-buttner May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback.

24 hours seems like a reasonable value

One thing to note is that we have an idle connection evictor that runs every minute and checks to see if any connection in the pool has been idle for over at least a minute. If it has, the connection is closed and removed from the pool. I'm not saying that a 1 minute idle time is the correct value, but if we leave that as it is I suspect that a 24 hour value for the TTL won't have much affect (if the connection is idle it will have been closed long before the 24 hours is up). Unless a user is constantly sending requests for a 24 hour period. Maybe that'd happen more often when we go to multi-tenant (the connect is associated with a URL not the user) or in a reindex use case.

Copy link
Contributor

@demjened demjened May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note is that we have an idle connection evictor that runs every minute and checks to see if any connection in the pool has been idle for over at least a minute.

Does this mean that if we set the TTL to anything above 60s, it won't have an effect as the eviction will happen earlier?
And for my understanding, what we're trying to do by setting the right TTL is to find a balance between avoiding unnecessarily frequent TLS handshakes with new connections, vs avoiding unnecessarily long-lived connections that consume resources?

Copy link
Contributor Author

@jonathan-buttner jonathan-buttner May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that if we set the TTL to anything above 60s, it won't have an effect as the eviction will happen earlier?

I believe the connection will be evicted if it is idle for 60 seconds or more. If we set the TTL to 2 minutes, after the connection has existed for 2 minutes or more (even if it hasn't been idle for the 60 seconds), when a request is sent , the pool will not use that connection. It's basically an upper bound limit that takes priority over other settings. So if a connection is viable for leasing in each other way (hasn't been idle for a long period of time, etc) if it has reached the TTL it won't be used.

So it still has an effect if we set it above 60 seconds.

Copy link
Contributor

@demjened demjened May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion, 60s sounds like a good initial setting, then we can revisit this if the workload changes (e.g. ELSER inference bursts at ingest). Some metrics/logs would help to keep an eye on this.

Setting.Property.NodeScope
);

@Deprecated
private final String eisGatewayUrl;

private final String elasticInferenceServiceUrl;
private final boolean periodicAuthorizationEnabled;
private volatile TimeValue authRequestInterval;
private volatile TimeValue maxAuthorizationRequestJitter;
private final TimeValue connectionTtl;

public ElasticInferenceServiceSettings(Settings settings) {
eisGatewayUrl = EIS_GATEWAY_URL.get(settings);
elasticInferenceServiceUrl = ELASTIC_INFERENCE_SERVICE_URL.get(settings);
periodicAuthorizationEnabled = PERIODIC_AUTHORIZATION_ENABLED.get(settings);
authRequestInterval = AUTHORIZATION_REQUEST_INTERVAL.get(settings);
maxAuthorizationRequestJitter = MAX_AUTHORIZATION_REQUEST_JITTER.get(settings);
connectionTtl = CONNECTION_TTL_SETTING.get(settings);
}

/**
Expand Down Expand Up @@ -115,6 +128,10 @@ public TimeValue getMaxAuthorizationRequestJitter() {
return maxAuthorizationRequestJitter;
}

public TimeValue getConnectionTtl() {
return connectionTtl;
}

public static List<Setting<?>> getSettingsDefinitions() {
ArrayList<Setting<?>> settings = new ArrayList<>();
settings.add(EIS_GATEWAY_URL);
Expand All @@ -124,6 +141,7 @@ public static List<Setting<?>> getSettingsDefinitions() {
settings.add(PERIODIC_AUTHORIZATION_ENABLED);
settings.add(AUTHORIZATION_REQUEST_INTERVAL);
settings.add(MAX_AUTHORIZATION_REQUEST_JITTER);
settings.add(CONNECTION_TTL_SETTING);
return settings;
}

Expand Down