Proposal for indexing labels to support efficient queries by label selectors #2203
Conversation
k8s-ci-robot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
sig/architecture
k8s-ci-robot
added
the
cncf-cla: yes
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
@kubernetes/sig-api-machinery-api-reviews |
k8s-ci-robot
added
sig/api-machinery
| ReplicaSet, when selecting pods, need to select all the pods by namespace and | ||
| then select the ones wanted filtering by index. While running a large number | ||
| of pods, we observed that listing all pods and then filtering takes several | ||
| seconds. This proposal presents solutions to improve this. |
There was a problem hiding this comment.
Can you describe the scenario you are hitting? With the watch cache enabled I have never seen a filtered label query take longer than 1s even on large sets, so I’d want to understand the data you have and how it could be applied.
There was a problem hiding this comment.
Eventually we will have to do this. However, I don't want to do it too early because it's a lot of complexity, and I'm not sure apiserver is the right place to do it. I think it makes more sense to have the storage layer do this. This requires either adding a layer between apiserver and etcd, or implementing a bunch of features in etcd. Neither is going to happen soon. In the meantime, I don't want to do this in apiserver unless it's absolutely necessary.
There was a problem hiding this comment.
@lavalamp Adding a layer between apiserver and etcd sounds like a reasonable approach, build DB like index query into etcd seems like not that possible. So this KEP just focus on client side.
|
|
||
| This proposal presents solutions to make the queries by label selectors in the | ||
| list API efficient. | ||
|
|
There was a problem hiding this comment.
To make it clear, this proposal focus on client cache or apiserver watch cache?
There was a problem hiding this comment.
See Proposal section 1 and 4. It focuses on client cash, while apiserver watch cache is optional.
There was a problem hiding this comment.
These two things shouldn't be in the same KEP, they don't have much in common.
There was a problem hiding this comment.
Removed the apiserver side cache. We will do it in the client side first.
keps/0012-label-index.md
Outdated
| all the index for all labels as the first step. | ||
|
|
||
| 4. `Optional` Adds index in API-server cache to improve queries directly going | ||
| to API-server. |
There was a problem hiding this comment.
actually there are two things: first we need to evaluate controller side cache label index, then we could go to API-server cache.
There was a problem hiding this comment.
You can already build a client-side index for isolated queries pretty easily.
There was a problem hiding this comment.
Removed the apiserver side cache. We will do it in the client side first.
|
|
||
| ### Proposal | ||
|
|
||
| 1. Adds label based index in controllers to facilitate lookup by label. |
There was a problem hiding this comment.
This is not really a design :)
How will this be accomplished? What components will change? How will you support all of our selector options, in particular IN?
(I'm not saying these questions are not answerable and we can't do it, I'm saying they are and this document should provide the answers)
There was a problem hiding this comment.
Oh I missed the word "controllers", you can already add indexes. Which controllers do you want to change?
There was a problem hiding this comment.
I am not sure if you need to have a super detailed design in the kep itself but it needs to be somewhere.
| Since objects are created/deleted dynamically, we also need to update the index | ||
| efficiently. | ||
|
|
||
| 2. Calculates the count for each index and evaluate the most selective first. |
There was a problem hiding this comment.
So, ah, I don't want to own a query planning engine without a lot of justification. We don't add value by implementing DB functions, at least not so far. Even though this sounds like a lot of fun to write.
|
|
||
| 2. Calculates the count for each index and evaluate the most selective first. | ||
|
|
||
| 3. Provides the ability in API for users to mark which labels to index or build |
There was a problem hiding this comment.
I'm against passing this complexity on to users. Or are you talking about an API for configuring controller manager (as a substitute for a flag)? Even there I'd expect this to be just on or off, not require an administrator to figure out what their expensive label selectors are.
| reviewers: | ||
| - TBD | ||
| approvers: | ||
| - TBD |
There was a problem hiding this comment.
are these people approving the KEP or approving the PRs that result? If the former, add me & deads2k.
| - TBD | ||
| creation-date: "2018-05-31" | ||
| last-updated: "2018-05-31" | ||
| status: provisional |
There was a problem hiding this comment.
This should not be merged it its current state. It needs: 1) better justification 2) focus on apiserver or clients and 3) a more detailed mechanism design.
|
|
||
| * With increasing number of pods, the response time of the list operations | ||
| increases linearly. | ||
| * With more than 10K pods, it takes 14 seconds to list all the pods, and 5 |
There was a problem hiding this comment.
And with client side filtering, it will take 14 seconds in both cases? I don't understand how this scenario can measure client performance?
|
/hold This KEP is still ambiguous in multiple places about what it wants to do clientside vs in the server. I think that should be very clear throughout before the discussion continues too much further. |
k8s-ci-robot
added
the
do-not-merge/hold
|
@ccding: PR needs rebase. Instructions 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/test-infra repository. |
k8s-ci-robot
added
the
needs-rebase
kubernetes/kubernetes#4817