IIUC, It sounds like a simple short-term fix can be adding an in-memory cache of selector per service. That would avoid recreating the selector every time.

This would be a good PR for new contributor. If anyone wants to send an PR, please assign it to me. I am willing to shepherd it.

IIUC, It sounds like a simple short-term fix can be adding an in-memory cache of selector per service. That would avoid recreating the selector every time.

Yeah probably. Though it obviously complicates the code, so my main goal was to ensure that the new API will not have this problem.

@freehan @wojtek-t I would like to work on this! As a first time contributor, I would appreciate some help.

@yashbhutwala - nice to meet you.
From my comment above it may not be clear, but I'm not 100% convinced that it's a good first task.

But to describe in more details:

• on every add/update/delete Pod action, endpointsController is calling getPodMemberships which call GetPodServices:
  https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/listers/core/v1/service_expansion.go#L36
• this internally creates a selector: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/labels/selector.go#L871
• that building a selector (allocations + sorting mostly) is becoming expensive if called that frequently

What @freehan is suggesting that, we may want to cache "service->selector" mapping inside endpoints controller and use that instead of recomputing that every single time.

I'm a bit worried that it may complicate the code, but I think I may be too pesimistic here. So it's probably worth trying.

So what you would need to do is:

• add this mapping service->selector in endpoints controller
• add logic that is keeping this mapping up-to-date
• instead of using GetPodServices(), we will be "listing all services from that namespace" and then filtering that ourservles
  Shouldn't be that much work...

Thanks @wojtek-t for laying out an outline for me! I'll shoot my best shot, and put up a PR, perhaps you can guide it along? P.S. what's your username in the slack group?

[ignore, for my own reference; important links]:

https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/endpoint/endpoints_controller.go#L185

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/listers/core/v1/service_expansion.go#L49

https://github.com/kubernetes/api/blob/master/apps/v1/types.go#L723

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/labels/selector.go#L871

Hey @yashbhutwala , what's the status of this issue? Still work on that?

@shinytang6 @wojtek-t @freehan I made a PR here: #75965. Review/feedback/suggestions appreciated! :)

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

/remove-lifecycle stale
/lifecycle frozen

@wojtek-t
SelectorFromValidatedSet calls sort. However, the goal for creating the selector is for calling selector.Matches where the sorting can be skipped.

		selector := labels.Set(service.Spec.Selector).AsSelectorPreValidated()

How about passing a flag to AsSelectorPreValidated() which skips the sorting step ?

How about passing a flag to AsSelectorPreValidated() which skips the sorting step ?

AsSelectorPreValidated() is already a bit of a hack by iself (I know I'm the one who added it). And I'm bit reluctant of making it even more strange looking...

Can SelectorFromValidatedSet skip calling sort ?
I would think that the selectors have equal weight.

Can SelectorFromValidatedSet skip calling sort ?

It can't - it would e.g. break the assumption that you can compare them.
Or will be printed randomly when you print them.
The potential drawbacks are larger than the gain from it (i.e. majority of selectors are fairly small and memory allocations/gc connected to building selector are usually more expensive the this sort).

/assign @robscott

@bowei:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

• instead of using GetPodServices(), we will be "listing all services from that namespace" and then filtering that ourservles

@wojtek-t it seems selector-cache solution has to rewrite GetPodServices function, is it feasible to change so many code?

Not necessary - see #73527

I meant #75965

I meant #75965

yes, most logic of GetPodServices has been rewrite in #75965

@wojtek-t I'd like to work on this as a first issue if you don't mind. Should we iterate on #75965 or try afresh?

@wojtek-t does the same problem exist in endpointslice controller?

seems #75965 has something wrong and no one maintained for a long time, I raised a new pr

@wojtek-t Excuse me, I have one concern about the service selector: should cache updated while service selector changed?

@wojtek-t Excuse me, I have one concern about the service selector: should cache updated while service selector changed?

yes