performance: use cached kube client for the infra runner#8764
performance: use cached kube client for the infra runner#8764zhaohuabing wants to merge 16 commits intoenvoyproxy:mainfrom
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a105d0adde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88c09aedc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (53.37%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8764 +/- ##
==========================================
- Coverage 73.64% 73.52% -0.13%
==========================================
Files 245 245
Lines 48864 48962 +98
==========================================
+ Hits 35985 35998 +13
- Misses 10874 10954 +80
- Partials 2005 2010 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zhaohuabing
force-pushed
the
kub-infra-provider-cache-client
branch
from
b61799a to
97c2e50
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
zhaohuabing
force-pushed
the
kub-infra-provider-cache-client
branch
5 times, most recently
from
cdb0581 to
4025657
Compare
zhaohuabing
force-pushed
the
kub-infra-provider-cache-client
branch
from
4025657 to
b8db1ff
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8db1ff9a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
nice :) |
| go r.updateProxyInfraFromSubscription(ctx, sub) | ||
|
|
||
| // Enable global ratelimit if it has been configured. | ||
| if r.EnvoyGateway.RateLimit != nil { |
There was a problem hiding this comment.
Ratelimit server deployment initialization was moved to the IR subscriptor because now the infra runner is using the shared cache client from the kube provider, and the cached client is not initialized yet when the infra runner starts.
zhaohuabing
force-pushed
the
kub-infra-provider-cache-client
branch
8 times, most recently
from
5f162ef to
8821341
Compare
zhaohuabing
changed the title
zhaohuabing
force-pushed
the
kub-infra-provider-cache-client
branch
from
d44aada to
b146478
Compare
|
/retest |
zhaohuabing
force-pushed
the
kub-infra-provider-cache-client
branch
from
9ffed45 to
e69d425
Compare
| Stderr io.Writer | ||
| // KubernetesClient holds the controller-runtime client created by the Kubernetes provider. | ||
| // This is used by the infrastructure runner to create the envoy proxy and rate limit infra resources. | ||
| KubernetesClient *KubernetesClientHolder |
There was a problem hiding this comment.
A KubernetesClientHolder is used to hold the shared kube client from the controller runtime.
config.Server is passed by value to runners, and changing it to reference is not safe since runners have modified the members of config.Server, like the logger.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e69d425119
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func(update message.Update[string, *ir.Infra], errChan chan error) { | ||
| r.Logger.Info("received an update", "key", update.Key, "delete", update.Delete) | ||
|
|
||
| r.ensureRateLimitInfraInitialized(ctx) |
There was a problem hiding this comment.
Cleanup is now gated on receiving an InfraIR update: DeleteRateLimitInfra runs only via ensureRateLimitInfraInitialized, which is called from the subscription callback. If Envoy Gateway starts with rate limiting disabled and no Gateway/Infra updates occur, stale envoy-ratelimit resources from prior runs are never removed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This should be acceptable. A line has been added to the release note to explain the behavior change.
zhaohuabing
force-pushed
the
kub-infra-provider-cache-client
branch
from
59592ee to
62a23b9
Compare
| if svrCfg.EnvoyGateway.GatewayNamespaceMode() { | ||
| // Keep ServiceAccount/Deployment unfiltered because the Envoy Gateway controller service account and deployment | ||
| // are needed to watch for changes, and EG controller's labels can vary across install methods (for example Helm nameOverride/custom chart naming). | ||
| // Filtering these kinds by labels can hide the controller objects from the cache. |
There was a problem hiding this comment.
Keep ServiceAccount/Deployment unfiltered because the Envoy Gateway controller service account and deployment are needed to watch for changes, and EG controller's labels can be customzied while installing the Helm using nameOverride.
{{/*
Expand the name of the chart.
*/}}
{{- define "eg.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}
I checked that nameOverride is not defined in the values.yaml so it's not exposed. Is it safe to assume the eg.name is .Chart.Name, and EG deploy always has a fixed label app.kubernetes.io/name: gateway-helm ? cc @envoyproxy/gateway-maintainers
| # Enhancements that improve performance. | ||
| performance improvements: | | ||
| Reduce chances of listener drain due to Lua policy updates by migrating to LuaPerRoute. | ||
| Reduced Kubernetes API server calls by reusing the cached controller-runtime client from the controller manager for infrastructure reconciliation. Notably, ratelimit server creation/cleanup can now be delayed until the first Gateway is created. In GatewayNamespaceMode, this may also increase memory usage because additional ServiceAccount and Deployment objects are kept in the cache. |
There was a problem hiding this comment.
In GatewayNamespaceMode, this may also increase memory usage because additional ServiceAccount and Deployment objects are kept in the cache.
Actually the impact to memory may not be that bad, since EG already kept Deployment in cache. This PR only adds ServiceAccounts.
| }, | ||
| &appsv1.Deployment{}: { | ||
| UnsafeDisableDeepCopy: new(true), | ||
| }, |
There was a problem hiding this comment.
This has been moved down so we can filter deployment by namspace.
So actually the impact to memory may not be that bad, since EG already kept Deployment in cache. This PR only adds ServiceAccounts.
This PR also improves memory usage in the default mode by only caching Deployments in the controller namespace.
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
zhaohuabing
force-pushed
the
kub-infra-provider-cache-client
branch
from
b93adac to
cc3962c
Compare
5 participants
This PR reuses the cached controller-runtime client from the controller manager for infrastructure reconciliation to reduce Kubernetes API server calls.
Please note that the rate limit server is now created only after the first
Gatewayis created. This is because the cached Kubernetes client in the kube provider is initialized asynchronously and may not be ready when the server starts.The changed behavior of the Infra runner are covered by existing e2e tests test/e2e/tests/envoyproxy.go.
Release note: yes.