Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

performance improvement for list request with labelSelector #2234

Open
likakuli opened this issue Jul 21, 2022 · 13 comments
Open

performance improvement for list request with labelSelector #2234

likakuli opened this issue Jul 21, 2022 · 13 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@likakuli
Copy link
Contributor

What would you like to be added:

Use index to improve the performance of list request with labelSelector

Why is this needed:

LabelSelector is used in many places in karmada-controller to list object. controller-runtime not setup index for label and not support to setup index for label too. So the list request cost O(n). When there are many object, it will cost too much time.

controller-runtime code: https://github.com/kubernetes-sigs/controller-runtime/blob/f46919744bee01060c9084a285e049afffd38c9d/pkg/cache/internal/cache_reader.go#L115-L138

@likakuli likakuli added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 21, 2022
@likakuli
Copy link
Contributor Author

likakuli commented Jul 21, 2022

I don't know whether this function should be implemented in controller-runtime or just in karmada-controller.
There are two points related to this.

  1. controller-runtime not support index when to use labelSelector in list request.
  2. controller-runtime and client-go not support complicated fieldSelector just as the comment.

In our case, we implemented this by two steps.

  1. We add custom manager and cacher used to convert labelSelector to fieldSelector.
  2. We use custom client-go and controller-runtime package to support complicated selector. refer to:
    Thread safe store support complicated index kubernetes/kubernetes#109334
    ✨ Support multiple field selectors kubernetes-sigs/controller-runtime#1838

@RainbowMango
Copy link
Member

cc @yy158775 Please take a look this issue.

@yy158775
Copy link
Contributor

yy158775 commented Jul 26, 2022

From my point of view,I think we can convet labelSelector to fieldSelector with a custom IndexFunc.The IndexFunc can compute the set of indexed values for an object's labels.

Eg: If we want to select resourcebindings with label key "clusterpropagationpolicy.karmada.io/name" = specific value,we can construct a IndexFunc which return "key=value" pair when the "clusterpropagationpolicy.karmada.io/name" exists in object labels.

Such as:
indices["labels"] = {"clusterpropagationpolicy.karmada.io/name=$var":[object_key1,object_key2,object_key3,......],......}
indexers["labels"] = indexfunc

If we need to select objects by other labels,we can edit the indexfunc to support.

@likakuli
Copy link
Contributor Author

If no one is working on it, i can open a pr to support this issue.

@RainbowMango
Copy link
Member

Sure thing! (anyone is welcome to pick a task without an assignment)

Have you done it on your side?

I know @yy158775 started looking at it and working on the demo.

This issue is much more involved. And @yy158775 can cooperate with you.

@likakuli
Copy link
Contributor Author

Our internal version has implemented this function.

@yy158775
Copy link
Contributor

I think this is worth optimizing.So I look foward to seeing your version.

@RainbowMango
Copy link
Member

/assign @likakuli
So, please help to make a PR for this.

@yy158775
Copy link
Contributor

Our internal version has implemented this function.

Are you convenient to open a PR? I am ready to work on this issue with you.

@likakuli
Copy link
Contributor Author

sorry, i will open a pr this weekend. or you can open a pr if you have already completed it. thx

@weilaaa
Copy link
Member

weilaaa commented Oct 28, 2022

In my opinion, we need to fundamentally enhance the capabilities of client-go. See proposal issues: kubernetes/kubernetes#109329

app-->controller-runtime-->client-go this is fine.

@RainbowMango
Copy link
Member

@weilaaa Are you suggesting waiting for the proposed issue in K/K?

@weilaaa
Copy link
Member

weilaaa commented Oct 28, 2022

Yes, it may be the most appropriate way, but I can't guarantee that this proposal will be passed. At least we can wait for the conclusion of the community(K/K) on this proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants