Add max connection limit

[daimaxiaxie]

Please provide a description of this PR:

Control Plane load imbalance, such as instance failure, network disconnection. Both may cause a single instance burst and overload. Final avalanche(All instances start and then crash). #50412

Therefore there is an upper limit on connections for a single control plane instance to prevent overload.

env PILOT_MAX_CONNECTION = 0, Limits the number of incoming ADS connection. If set to 0 or unset, disabled the feature.

[istio-policy-bot]

😊 Welcome @daimaxiaxie! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[istio-testing]

Hi @daimaxiaxie. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[zirain]

/ok-to-test

[wulianglongrd]

I don't think there is much value in limiting the total number of connections. If there are too many connections, it will only affect the speed of configuration push.

If you just want to prevent the impact of a sudden increase in traffic, you can use PILOT_MAX_REQUESTS_PER_SECOND to limit the request.

[wulianglongrd]

Also, can we use DiscoveryServer.adsClients to count the number of connections?

[wulianglongrd]

duplicate with

istio/pilot/pkg/xds/monitoring.go

Line 75 in 3e427fc

xdsClients = monitoring.NewGauge(

[daimaxiaxie]

I think their meanings are different. xdsClients represents how many xds streams. connectionTotal represents the number of grpc sessions.

What do you think?

[hzxuzhonghu]

It makes some sense, the imbalanced connections could cause one pilot overloaded, costing too much more mem/cpu than the others.

[hzxuzhonghu]

use that DiscoveryServer.adsClientCount

[daimaxiaxie]

DiscoveryServer.adsClients change in stream handler(initConnection). It is not synchronized with grpc's dial. I think this will cause some problems.

[wulianglongrd]

Do we need such precise limits? The grpc's dial time should be very short, and PILOT_MAX_REQUESTS_PER_SECOND can limit burst connections. This PR is just to limit the continued increase in connections, right?

[daimaxiaxie]

This PR is just to limit the continued increase in connections, right?

Yes.
There is a gap between initConnection and grpc.dial. If use DiscoveryServer.adsClients, ,this feature will invalid in extreme cases. I feel it's better to be more precise. What do you think about that? Thanks.

[daimaxiaxie]

I don't think there is much value in limiting the total number of connections. If there are too many connections, it will only affect the speed of configuration push.

Too many connections lead to a heavy memory load, even OOM. @wulianglongrd

And this sudden increase in traffic is different. It sets a overall service cap for a control plane instance.

[wulianglongrd]

Makes sense. Addendum: There is also an existing counter here, which is used by the pilot_xds indicator.

istio/pilot/pkg/xds/monitoring.go

Line 80 in 3e427fc

xdsClientTracker = make(map[string]float64)

[daimaxiaxie]

Also, can we use DiscoveryServer.adsClients to count the number of connections?

No, DiscoveryServer.adsClients change in stream handler. It is not synchronized with grpc's dial time.

[howardjohn]

This is incredibly unsafe. Unless you have a fixed number of istiod pods and clients, having hard limits around connections without having autoscaling tied to this limit is a recipe to trigger an outage where all connections are denied. It also makes a DOS attack trivial

[ramaraochavali]

+1. Curious how much memory you have allocated for Istiod and how many total clients you have + and how many clients connected to single Istiod caused Istiod to get OOMKilled?

[daimaxiaxie]

In our test cluster, each instance is limited to 16c40g.（replicas 10+, fixed number） Each instance usually maintains more than 700 connections, and the memory usage is 22g-26g. When the cluster is thrashing (some failures), it is easy for a single instance to reach the memory limit. Our production cluster has many more clients.

So I think: when the resources of an instance are fixed, the upper limit of clients it can serve is also fixed.
This feature(pr) is disabled by default

[istio-policy-bot]

🧭 This issue or pull request has been automatically marked as stale because it has not had activity from an Istio team member since 2024-04-12. It will be closed on 2024-05-27 unless an Istio team member takes action. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.