Reduce VirtualHost memory utilization by avoiding CatchAllVirtualCluster when not needed#24182
Conversation
instance when not needed. This is a step towards more memory efficient config data structures. Issue envoyproxy#24154. Currently an instance of CatchAllVirtualCluster is always created for each vhost, but is only used when there are other virtual clusters (and none of them matches). If there are no virtual clusters to begin with, CatchAllVirtualCluster is a pure overhead. This PR switches an instance to a unique_ptr and only creates one when there are virtual clusters present. Signed-off-by: Yury Kats <ykats@google.com>
|
/assign @adisuissa |
|
yurykats is not allowed to assign users. |
|
Assigning Stephan (and myself, on behalf of Yury's request) |
adisuissa
left a comment
There was a problem hiding this comment.
Thanks, left a couple of minor comments.
Signed-off-by: Yury Kats <ykats@google.com>
adisuissa
left a comment
There was a problem hiding this comment.
Can you please check CI, the failure of //test/integration:xfcc_integration_test seems consistent.
Signed-off-by: Yury Kats <ykats@google.com>
|
Removed vcluster stats from xfcc_integration_test, since the test does not setup vclusters and the stats are no longer created by default (they were never updated before). |
I think that is a valid claim, but want to another input. @jmarantz IIUC this PR removes some stats that currently appear, and as such it is a use-facing change. Assuming that these stats are always 0 in this case because vhost isn't explicitly configured (@yurykats correct me if I'm wrong), is it ok to remove them? If so, other than updating the release notes, is there anything else to consider or update? |
|
/assign @jmarantz |
|
I don't have a strong opinion about this particular stat. I'm not sure what monitoring might break for various Envoy deployments. I feel like @zuercher would be able to provide insight or redirect to someone else if needed. |
RateLimitPolicy instances when not needed. Currently an instance of RateLimitPolicyImpl is created in each vhost and route, even if no rate limiting is configured. This PR switches instances to unique_ptr and only creates RateLimitPolicyImpl objects when actually configured. Otherwise a singletong default policy is used. This is a step towards more memory efficient config data structures. Issue envoyproxy#24154. Similar change to Retry, Hedge and InternalRedirect policies was made in PR envoyproxy#24182. Signed-off-by: Yury Kats <ykats@google.com>
Based on https://www.envoyproxy.io/docs/envoy/v1.24.0/api-v3/config/route/v3/route_components.proto#config-route-v3-virtualcluster it seems like virtual clusters exist purely as a way to split upstream stats out for specific endpoints. Given that the stats aren't incremented when no virtual clusters are defined, it seems reasonable to just drop them. We need to add a release note that describes that change, however. |
Signed-off-by: Yury Kats <ykats@google.com>
Thanks for the review, added to release notes. |
Signed-off-by: Yury Kats <ykats@google.com>
Signed-off-by: Yury Kats <ykats@google.com>
|
Removing myself as my review is not needed on this PR and I didn't actually review it :) |
…cy instances when not needed (#24243) Reduce Route/VirtualHost memory utilization by avoiding RateLimitPolicy instances when not needed. This is a step towards more memory efficient config data structures. Issue #24154. Similar change to Retry, Hedge and InternalRedirect policies was made in PR #24182. Risk Level: low Testing: existing tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Yury Kats <ykats@google.com>
* origin/main: Start slow start window on first successful HC, make slow start re-enterable (envoyproxy#23946) contrib-sipproxy: rework sipproxy stats (envoyproxy#24165) build(deps): bump github/codeql-action from 2.1.32 to 2.1.35 (envoyproxy#24292) build(deps): bump node from `c59fb39` to `80844b6` in /examples/ext_authz/auth/http-service (envoyproxy#24274) build(deps): bump actions/upload-artifact from 2 to 3 (envoyproxy#24121) build(deps): bump mysql from `96439dd` to `66efaaa` in /examples/mysql (envoyproxy#24273) build(deps): bump grpcio from 1.50.0 to 1.51.1 in /examples/grpc-bridge/client (envoyproxy#24272) Allow mobile/library/common/jni/ to be built on non-Android platforms. (envoyproxy#24299) generic proxy: added drain support to generic proxy to doing graceful closes on connections when possible (envoyproxy#24220) Reduce Route/VirtualHost memory utilization by avoiding RateLimitPolicy instances when not needed (envoyproxy#24243) Reduce VirtualHost memory utilization by avoiding CatchAllVirtualCluster when not needed (envoyproxy#24182) Fix mobile/tools/check_format.sh to re-apply envoyproxy#2698 (envoyproxy#24300) upstream: don't require `source_address` in `upstream_bind_config` (envoyproxy#24250) Quiche roll 20221201150327 (envoyproxy#24290) Signed-off-by: JP Simard <jp@jpsim.com>
…/docs/sphinx-5.3.0 * origin/main: (50 commits) ci: Bump mobile build images (#24317) ci: update llvm on bazel CI (#24296) ci: Fix flaky coverage limit (#24320) ci: Fix change detection (part 2) (#24264) Start slow start window on first successful HC, make slow start re-enterable (#23946) contrib-sipproxy: rework sipproxy stats (#24165) build(deps): bump github/codeql-action from 2.1.32 to 2.1.35 (#24292) build(deps): bump node from `c59fb39` to `80844b6` in /examples/ext_authz/auth/http-service (#24274) build(deps): bump actions/upload-artifact from 2 to 3 (#24121) build(deps): bump mysql from `96439dd` to `66efaaa` in /examples/mysql (#24273) build(deps): bump grpcio from 1.50.0 to 1.51.1 in /examples/grpc-bridge/client (#24272) Allow mobile/library/common/jni/ to be built on non-Android platforms. (#24299) generic proxy: added drain support to generic proxy to doing graceful closes on connections when possible (#24220) Reduce Route/VirtualHost memory utilization by avoiding RateLimitPolicy instances when not needed (#24243) Reduce VirtualHost memory utilization by avoiding CatchAllVirtualCluster when not needed (#24182) Fix mobile/tools/check_format.sh to re-apply #2698 (#24300) upstream: don't require `source_address` in `upstream_bind_config` (#24250) Quiche roll 20221201150327 (#24290) Cleanup threading/ownership semantics of PlatformBridgeCertValidator (#2713) Add unit tests of the PlatformBridgeCertValidator (#2704) ... Signed-off-by: JP Simard <jp@jpsim.com>
…/docs/pygments-2.13.0 * origin/main: (25 commits) ci: Bump mobile build images (#24317) ci: update llvm on bazel CI (#24296) ci: Fix flaky coverage limit (#24320) ci: Fix change detection (part 2) (#24264) Start slow start window on first successful HC, make slow start re-enterable (#23946) contrib-sipproxy: rework sipproxy stats (#24165) build(deps): bump github/codeql-action from 2.1.32 to 2.1.35 (#24292) build(deps): bump node from `c59fb39` to `80844b6` in /examples/ext_authz/auth/http-service (#24274) build(deps): bump actions/upload-artifact from 2 to 3 (#24121) build(deps): bump mysql from `96439dd` to `66efaaa` in /examples/mysql (#24273) build(deps): bump grpcio from 1.50.0 to 1.51.1 in /examples/grpc-bridge/client (#24272) Allow mobile/library/common/jni/ to be built on non-Android platforms. (#24299) generic proxy: added drain support to generic proxy to doing graceful closes on connections when possible (#24220) Reduce Route/VirtualHost memory utilization by avoiding RateLimitPolicy instances when not needed (#24243) Reduce VirtualHost memory utilization by avoiding CatchAllVirtualCluster when not needed (#24182) Fix mobile/tools/check_format.sh to re-apply #2698 (#24300) upstream: don't require `source_address` in `upstream_bind_config` (#24250) Quiche roll 20221201150327 (#24290) Cleanup threading/ownership semantics of PlatformBridgeCertValidator (#2713) Add unit tests of the PlatformBridgeCertValidator (#2704) ... Signed-off-by: JP Simard <jp@jpsim.com>
Reduce VirtualHost memory utilization by avoiding CatchAllVirtualClusterinstance when not needed.
This is a step towards more memory efficient config data structures. Issue #24154.
Currently an instance of CatchAllVirtualCluster is always created for each vhost, but is only used when there are other virtual clusters (and none of them matches). If there are no virtual clusters to begin with, CatchAllVirtualCluster is a pure overhead.
This PR switches an instance to a unique_ptr and only creates one when there are virtual clusters present.
Removed vcluster stats from xfcc_integration_test, since the test does not setup vclusters and the stats are no longer created by default (they were never updated before). vcluster stats are tested in router_test, tag_extractor_impl_test and http_integration.
Signed-off-by: Yury Kats ykats@google.com
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]