odcds: add inactivity timeout for removal of discovered clusters#46069
odcds: add inactivity timeout for removal of discovered clusters#46069akonradi wants to merge 7 commits into
Conversation
Signed-off-by: Alex Bakon <abakon@netflix.com>
Signed-off-by: Alex Bakon <abakon@netflix.com>
Signed-off-by: Alex Bakon <abakon@netflix.com>
Signed-off-by: Alex Bakon <abakon@netflix.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Alex Bakon <abakon@netflix.com>
Signed-off-by: Alex Bakon <abakon@netflix.com>
| // A cluster that has been idle for this long will be subject to automatic removal. | ||
| // An idle cluster is one that has no streams to any of its endpoints. | ||
| // Inactivity detection is disabled if this value is zero or unset. | ||
| google.protobuf.Duration cluster_inactivity_timeout = 4 [(validate.rules).duration = {gte {}}]; |
There was a problem hiding this comment.
After a rethink, may be call the cluster_idle_timeout to consistency with our lots of other _idle_timeout.
And I am think should this API be part of OnDemand or this OnDemandCds
There was a problem hiding this comment.
Renamed to use "idle" vs "inactivity".
Moving out of the shared OnDemandCds into the filter-specific OnDemand would mean that the route version wouldn't have the same behavior. What would be the benefit of that?
| // Reads a cluster's live active-stream and cumulative-request counts. Returns nullopt if the | ||
| // cluster is no longer an active cluster in the manager. | ||
| std::optional<OdCdsClusterInactivityTimeout::ClusterActivityStats> | ||
| sampleClusterActivity(const ClusterManager& cm, absl::string_view cluster_name) { | ||
| const OptRef<const Cluster> cluster = cm.getActiveCluster(cluster_name); | ||
| if (!cluster.has_value()) { | ||
| return std::nullopt; | ||
| } | ||
| const ClusterInfoConstSharedPtr info = cluster->info(); | ||
| return OdCdsClusterInactivityTimeout::ClusterActivityStats{ | ||
| .active_rq = info->trafficStats()->upstream_rq_active_.value(), | ||
| .total_rq = info->trafficStats()->upstream_rq_total_.value(), | ||
| }; |
There was a problem hiding this comment.
If we take the contention into account, seems this is is not good enough to detect a expired cluster? cc @kyessenov
There was a problem hiding this comment.
Is there a specific scenario you're imagining where this wouldn't work?
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for the contrition. Refreshed my comments.
|
and ping codeowner cc @kyessenov |
Signed-off-by: Alex Bakon <abakon@netflix.com>
3 participants
Commit Message:
Add a configurable timeout to the HTTP ODCDS filter that, when enabled, is attached to discovered clusters. When a cluster discovered via ODCDS hasn't had any streams through it for the timeout interval it becomes eligible for automatic cleanup (though the cleanup sweep might not get to it for up to 2x the timeout). Cleanup removes the cluster and its subscription; if the cluster is needed again for a later request the ODCDS filter will rediscover it, like new. The net effect is lazy garbage collection: ODCDS clusters can stop being a source of unbounded resource growth past the point where they're useful.
Additional Description:
Generated with AI tooling and self-review. Low risk because the feature is disabled by default.
Risk Level: low
Testing: added unit and integration tests, ran existing tests
Docs Changes: new field documented
Release Notes: added to new_features
Platform Specific Features: n/a
[Optional API Considerations:]
The default value disables the feature.