optimization: allow for lazy sidecar initialization#47221
optimization: allow for lazy sidecar initialization#47221istio-testing merged 12 commits intoistio:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
@howardjohn could you please have a look at this? |
|
/test all |
pilot/pkg/model/sidecar.go
Outdated
| if !features.EnableLazySidecarEvaluation { | ||
| initSidecarScopeInternalIndexes(ps, out, configNamespace) | ||
| } else { | ||
| out.initFunc = sync.OnceFunc(func() { |
There was a problem hiding this comment.
Can we just call so that we do not have to call initFunc seperately in tests and what we execute is same when EnableLazySidecarEvaluation is enabled/disabled?
if !features.EnableLazySidecarEvaluation {
out.initFunc()
}
There was a problem hiding this comment.
Sure, I can do that I was just trying to avoid adding any overhead when flag is disabled. Regarding tests, I still need to call initFunc in some tests because I need to make sure SidecarScopes are initialized to be able to compare them.
There was a problem hiding this comment.
@ramaraochavali I pushed some changes, let me know if they seem ok
|
How many sidecars do you have and what is the kind of benefit you have seen with this change? |
We currently have 2.5k sidecars but will eventually have much more. We currently have debounce times of about 30s which is waay too much and almost all of this 30s is spent rebuilding sidecar indexes. We still haven't been able to test this change in production to see the benefit, will try to deploy this week and see if I can get some measurements. |
|
At the very least if we are going to make some large changes like this, we ought to have performance numbers indicating the changes. |
totally agree, i'll get some numbers in the coming days, I wanted to open the PR in the meantime to get your feelings about this. |
|
Following up on this, today I managed to deploy this, here's the impact: Debounce times were fixed at CPU usage certainly increased, because now we're using mutliple goroutines to process sidecars, this is intended and actually what is expected on our use case, concurrency can be tweaked with clarification: yellow lines are Max, purple are P95 and blue are Avg. |
|
not related: We may need another metric fot the overall time between a config update to xds push done |
indeed |
I've tried similar, its pretty hard actually with the way our current design is. |
|
not stale |
|
@sschepens Has this modification been applied to the production environment in your company? |
|
rebased this, made it enabled by default and made concurrent conversion only work when lazy initialization is disabled, let me know if this is ok or if I should change the default behaviour |
keithmattix
left a comment
There was a problem hiding this comment.
This LGTM but holding off on approving since I wasn't involved in the original context
Let us flip the behaviour as follows
Add a release note stating that concurrency will be deprecated and will be removed in future so that people can validate this new behaviour by removing concurrency flag? @wulianglongrd Any concerns with the above since you implemented the concurrency and might be using it in prod already? |
Done, also fixed tests and removed function |
|
Thank you. Will take a look tomorrow |
ramaraochavali
left a comment
There was a problem hiding this comment.
Thank you. This is awesome improvement.
|
Thanks, I'm happy with the good optimization.
Yes, we've been using concurrency since January 2024 until now (built it ourselves), and if I recall correctly, the
Out of curiosity, how much of a performance difference is there between concurrency and this implementation? It would be nice to have comparative data. |
It really depends on the use case, this feature allows for Istio to never perform sidecar scope conversion of unused sidecar scopes which can really save a lot of time depending on the use case. (If you have a large mesh then it's highly likely that and istiod instance doesn't have a sidecar connected for every sidecar scope and also there are times when sidecar scopes are not actually updated to the proxies). In the case where all sidecar scopes are used, then this feature should provide roughly the same benefit as the previous concurrency, but using the "standard" But maybe @ramaraochavali can provide more info given that they've recently tested both features. |
|
@ramaraochavali can you remove the hold? |
|
@sschepens: The following test failed, say
DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
We have seen significant reduction in proxy convergence time with this change when there are many sidecars and there is a churn in the cluster. Concurrency + Increased resource has improved but not as significant as this change. I will share more details tomorrow |
* upstream/master: (28 commits) Automator: update common-files@master in istio/istio@master (istio#56545) Automator: update proxy@master in istio/istio@master (istio#56544) Automator: update go-control-plane in istio/istio@master (istio#56543) Automator: update proxy@master in istio/istio@master (istio#56540) Automator: update ztunnel@master in istio/istio@master (istio#56532) Ambient: In ambient index, filter configs by revision (istio#56477) Automator: update istio/client-go@master dependency in istio/istio@master (istio#56539) Automator: update proxy@master in istio/istio@master (istio#56538) Automator: update common-files@master in istio/istio@master (istio#56537) optimization: allow for lazy sidecar initialization (istio#47221) static collection eager indexes (istio#56530) fix typo in flag (istio#56534) feat: enable support for proxy protocol on status port (istio#55986) remove finding of pods by IP (istio#56502) Automator: update proxy@master in istio/istio@master (istio#56528) migrate file monitor to krt (istio#55970) Automator: update istio/client-go@master dependency in istio/istio@master (istio#56525) Automator: update ztunnel@master in istio/istio@master (istio#56518) Fix crash in merging http routes (istio#56499) krt: add assertions (istio#56510) ...
Please provide a description of this PR:
I know this PR will probably generate a lot of controversy, please read on.
In large deployments, most of the Istio's work is done in
initSidecarScopesto scope configurations that are only visible to each Sidecar and it has a couple of problems:My proposal is to allow SidecarScopes to be lazy initialized, that means that Sidecar Scopes still are created but empty of indexes, whenever a Proxy selects a SidecarScope it is initialized (using
sync.OnceFuncwhich guarantees that it's only done once). What this allows is for SidecarScopes initialization to be almost zero unless the SidecarScope is actually used and also allow SidecarScopes computation to be parallelized in the goroutines that end up generating the XDS config, this may sound strange, but it's actually already ocurring when Sidecar Scopes have to be derived.