Reworking kube-proxy to only compute endpointChanges on apply#83206
Conversation
7442a93 to
24dc65f
Compare
|
/retest |
pkg/proxy/endpoints.go
Outdated
There was a problem hiding this comment.
may need a more significant name than getChangesAndMarkApplied. This function essentailly emits the changes in cache and refresh the cache. Plus, maybe other stuff
Naming Idea:
CheckoutChanges
There was a problem hiding this comment.
Thanks, CheckoutChanges is a much better name. Added some unit tests for this as well.
pkg/proxy/endpointslicecache.go
Outdated
There was a problem hiding this comment.
It may helped the performance more by not comparing the difference and just trigger sync as always.
But it may not make any difference given the test result.
There was a problem hiding this comment.
Unfortunately both the EndpointChangesPending metric and the last change trigger times rely on this being available on EndpointSlice addition (computing at proxier sync would not be helpful). The profiling I've done suggests that the full updatePending function, including this diffing, takes ~3% of total kube-proxy compute time (0.12s of 3.61s scaling to 10k endpoints). The actual esInfoChanged call within that is small enough to not be reported. Given the relative insignificance of that time I think it's worth maintaining the metrics as they exist.
pkg/proxy/endpoints.go
Outdated
There was a problem hiding this comment.
consider moving all these into getChangesAndMarkApplied
There was a problem hiding this comment.
I couldn't find a great way to integrate it there, but I did move this into a similarly named function.
|
Consider putting endpointslicecache.go into a different package (e.g. endpointslice/util or apimachinery), and define the interface better for reuse. Other consumer will need similar logic for this. |
98040fc to
e59fdf7
Compare
|
/retest |
e59fdf7 to
8178d13
Compare
|
/retest |
pkg/proxy/endpoints.go
Outdated
There was a problem hiding this comment.
nit: consider enclose the critical section into a built in func
func(){
ect.lock.Lock()
defer ect.lock.Unlock()
...
}()
There was a problem hiding this comment.
you have lock in endpointSliceCache already. Do you still need lock here?
There was a problem hiding this comment.
After discussing this a bit more I think it makes sense to hold off on any additional changes here until endpointslicecache can be moved into its own package.
Computing EndpointChanges is a relatively expensive operation for kube-proxy when Endpoint Slices are used. This had been computed on every EndpointSlice update which became quite inefficient at high levels of scale when multiple EndpointSlice update events would be triggered before a syncProxyRules call. Profiling results showed that computing this on each update could consume ~80% of total kube-proxy CPU utilization at high levels of scale. This change reduced that to as little as 3% of total kube-proxy utilization at high levels of scale. It's worth noting that the difference is minimal when there is a 1:1 relationship between EndpointSlice updates and proxier syncs. This is primarily beneficial when there are many EndpointSlice updates between proxier sync loops.
8178d13 to
8e7de45
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, robscott The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Computing EndpointChanges is a relatively expensive operation for kube-proxy when Endpoint Slices are used. This had been computed on every EndpointSlice update which became quite inefficient at high levels of scale when multiple EndpointSlice update events would be triggered before a syncProxyRules call.
Profiling results showed that computing this on each update could consume ~80% of total kube-proxy CPU utilization at high levels of scale. This change reduced that to as little as 3% of total kube-proxy utilization at high levels of scale.
It's worth noting that the difference is minimal when there is a 1:1 relationship between EndpointSlice updates and proxier syncs. This is primarily beneficial when there are many EndpointSlice updates between proxier sync loops.
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig network
/cc @freehan
/priority important-longterm