Should scalers reuse opened connection/clients?

[zroubalik]

I have been thinking about this for some time. Currently we are creating a new Scaler (and creating a client and opening a connection) in every cycle after in the Scale Loop (after pollingInterval has passed) and we are doing the same in the Metrics Server when HPA is requesting the metrics.

This has some benefits, eg. if the pollingInterval is long, we don't need to keep the opened conection, use the memory resources etc. And if I am not mistaken, then ENV variables on the Target Deployment are evaluated in each cycle.

On the other hand, if the pollingInterval is not that long, we keep opening and closing the clients very often. This is not ideal from the performance perspective and we can as well flood the Event Source (eg. Kafka Broker) with many opened and closed connections.

What if we try to create the Scaler just once, and keep the reference on the Scaler (and if there is an error while accessing this scaler, we can recreate again). We should do it on both sides (in the Operator's Scale Loop and in the Metrics server).

Are there any downsides or problems? Or any other ideas?

[zroubalik]

CC @ahmelsayed @anirudhgarg

[arschles]

Would #1133 be a step toward this? in the PR for this #1251 creates a long-lived HTTP client that has an internal connection pool

[zroubalik]

@arschles yeah, that's definititely a step forward 👍

[ahmelsayed]

Sorry for not replying to this earlier @zroubalik, I know I promised I'll but wanted to take a look at all the scalers, and how they implement connections to get a better feel for what would be the best option here.

The following scalers are the ones that create a client/connection on New*() and implement a Close() method.

scaler
azure_eventhub scaler
azure_log_analytics scaler
gcp_pub_sub scaler
influxdb scaler
kafka scaler
liiklus scaler
mongo scaler
mysql scaler
postgresql scaler
rabbitmq scaler
redis scaler
redis_streams scaler

All other scalers don't use connections, or use HTTP style clients that don't need to open/close connections.

Initially I thought a Scaler lifetime could be long, and thought the Scaler interface should make it easier for Scaler authors to handle that scenario, hence the Close() method on the interface. However, as you mentioned it's actually on every pollingInterval now, and we always create and close scalers right away.

The reason to recreate the scalers on every pollingInterval I think was to make sure to always use the latest connection secrets/auth parameters since we don't get notified on Secret or other changes. I think we can hash those values though and only recreate the scaler if those have changed. I did something similar for the external streaming GRPC scaler to make it so that if there are 1000 ScaledObjects pointing to the same GRPC server, they can all reuse the same connection instead of opening 1000 connections for no reason.

We have had few memory leaks (1, 2, 3) when those scalers are not closed, there is one code path now actually also missing to close those scalers (#1608), so I'd also like to improve that to make sure the interface of buildScalers() forces the caller to deal with closing scalers once they are done.

It'd be a slightly large change, but I'll work on it

[mboutet]

@ahmelsayed, you can also add 1543642 to your memory leaks list

[tomkerkhove]

@ahmelsayed is working on this one and will aim for v2.5 🎉