Conncetiion, ConcurrentRequests, PendingRequest limits#23396
Conversation
LakshmiNarayananDesikan
added
the
pr/no-changelog
github-actions
Bot
added
theme/api
LakshmiNarayananDesikan
force-pushed
the
api-gateway/connection-concurrentRequest-pendingRequestLimits
branch
from
74ab68a to
229fd3d
Compare
| upstreams := make(map[APIGatewayListenerKey]structs.Upstreams) | ||
|
|
||
| var defaultLimits *structs.UpstreamLimits | ||
| if snap.APIGateway.GatewayConfig != nil { |
There was a problem hiding this comment.
Snap is initialized with defaults, It can not be nil
There was a problem hiding this comment.
In the scope of this method, it can be nil. If we call this method somewhere else and that passes nil, there will be a panic.
There was a problem hiding this comment.
it will not be nil, all handling is initialized after snapshot is initialized in the lifecycle handled by a proxycfg , manager. This comment can be ignored.
| upstreams := make(map[APIGatewayListenerKey]structs.Upstreams) | ||
|
|
||
| var defaultLimits *structs.UpstreamLimits | ||
| if snap.APIGateway.GatewayConfig != nil { |
There was a problem hiding this comment.
In the scope of this method, it can be nil. If we call this method somewhere else and that passes nil, there will be a panic.
| var clusters []proto.Message | ||
| createdClusters := make(map[proxycfg.UpstreamID]bool) | ||
| readyListeners := getReadyListeners(cfgSnap) | ||
| mergedLimitsByUID := make(map[proxycfg.UpstreamID]*structs.UpstreamLimits) |
There was a problem hiding this comment.
why do we need 2 loops one for merging limits . another for config . can we have duplicate upstreams which are already generated in api_gateway.go .
readyListeners is prepared/populated in the API gateway proxycfg pipeline (proxycfg/api_gateway.go) and then consumed later in cluster construction.
So in the clusters.go loop:
- it is iterating over a structure already assembled upstream in api_gateway.go,
- each readyListener carries the resolved/attached upstreams for that listener,
- cluster generation is using that prepared state rather than discovering listeners/upstreams itself.
my intuition says: if limits were already fully merged into the listener/upstream state in api_gateway.go, then clusters.go should only need to read/apply them (unless it has extra cluster-stage merge precedence).
upstream is created at service name level and uid here is created for cluster naming. Can you check the difference and reduce the merge here .
There was a problem hiding this comment.
readyListeners is intentionally route/listener is built for listener-level xDS assembly, not for unique upstream identity, so duplicates by upstream UID are expected.
cluster stage must remove duplicates and keep one final item per UID and apply merged limit precedence.
| upstreams := make(map[APIGatewayListenerKey]structs.Upstreams) | ||
|
|
||
| var defaultLimits *structs.UpstreamLimits | ||
| if snap.APIGateway.GatewayConfig != nil { |
There was a problem hiding this comment.
it will not be nil, all handling is initialized after snapshot is initialized in the lifecycle handled by a proxycfg , manager. This comment can be ignored.
| var clusters []proto.Message | ||
| createdClusters := make(map[proxycfg.UpstreamID]bool) | ||
| readyListeners := getReadyListeners(cfgSnap) | ||
| mergedLimitsByUID := make(map[proxycfg.UpstreamID]*structs.UpstreamLimits) |
There was a problem hiding this comment.
why do we need 2 loops one for merging limits . another for config . can we have duplicate upstreams which are already generated in api_gateway.go .
readyListeners is prepared/populated in the API gateway proxycfg pipeline (proxycfg/api_gateway.go) and then consumed later in cluster construction.
So in the clusters.go loop:
- it is iterating over a structure already assembled upstream in api_gateway.go,
- each readyListener carries the resolved/attached upstreams for that listener,
- cluster generation is using that prepared state rather than discovering listeners/upstreams itself.
my intuition says: if limits were already fully merged into the listener/upstream state in api_gateway.go, then clusters.go should only need to read/apply them (unless it has extra cluster-stage merge precedence).
upstream is created at service name level and uid here is created for cluster naming. Can you check the difference and reduce the merge here .
LakshmiNarayananDesikan
dismissed stale reviews from anandmukul93 and sreeram77
via
2b4a037
LakshmiNarayananDesikan
force-pushed
the
api-gateway/connection-concurrentRequest-pendingRequestLimits
branch
from
26cc7a5 to
2b4a037
Compare
LakshmiNarayananDesikan
removed
the
pr/no-changelog
LakshmiNarayananDesikan
added
backport/all
LakshmiNarayananDesikan
removed
backport/ent/1.18
3 participants
Description
Testing & Reproduction steps
Links
PR Checklist
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.