-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ Cache-Backed Client: Support listOpts.Limit
#1479
Conversation
Hi @MadhavJivrajani. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
@MadhavJivrajani can you please add a test for the |
Oh woops, sorry about that. I'll add it :) Thanks! |
@alvaroaleman I've added the test, could you PTAL? |
@alvaroaleman could you PTAL? |
/ok-to-test |
- add test for informer cache limit option Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
Hey @alvaroaleman, the |
It would be good to support it everywhere at once, if that isn't too much work. |
Sure, no worries. 😄 |
Hey @alvaroaleman, could you PTAL? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MadhavJivrajani thanks, sorry, this fell of my radar. The MultiNamespaceCache implementation is incorrect and just passes the tests by coincidence, because we only have one pod per namespace in the test scenario.
If you do something like this it instantly starts failing:
diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go
index 9a41662a..6f0cdd39 100644
--- a/pkg/cache/cache_test.go
+++ b/pkg/cache/cache_test.go
@@ -100,6 +100,7 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
knownPod2 client.Object
knownPod3 client.Object
knownPod4 client.Object
+ knownPod99 client.Object
)
BeforeEach(func() {
@@ -119,6 +120,7 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
Expect(err).NotTo(HaveOccurred())
// Includes restart policy since these objects are indexed on this field.
knownPod1 = createPod("test-pod-1", testNamespaceOne, kcorev1.RestartPolicyNever)
+ knownPod99 = createPod("test-pod-99", testNamespaceOne, kcorev1.RestartPolicyNever)
knownPod2 = createPod("test-pod-2", testNamespaceTwo, kcorev1.RestartPolicyAlways)
knownPod3 = createPodWithLabels("test-pod-3", testNamespaceTwo, kcorev1.RestartPolicyOnFailure, map[string]string{"common-label": "common"})
knownPod4 = createPodWithLabels("test-pod-4", testNamespaceThree, kcorev1.RestartPolicyNever, map[string]string{"common-label": "common"})
@@ -149,6 +151,7 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
deletePod(knownPod2)
deletePod(knownPod3)
deletePod(knownPod4)
+ deletePod(knownPod99)
informerCacheCancel()
})
@@ -295,7 +298,7 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
Expect(errors.IsTimeout(err)).To(BeTrue())
})
- It("should set the Limit option and limit number of objects to Limit when List is called", func() {
+ FIt("should set the Limit option and limit number of objects to Limit when List is called", func() {
By("setting the Limit option to 1")
opts := &client.ListOptions{Limit: int64(1)}
limitListOpts := &client.ListOptions{}
I am fine with both leaving it for a follow-up (but then, please disable the test for the multi-namespace cache) or fixing it here (I think it actually shouldn't be too hard).
If you fix it, please also change the tests to actually have a namespace with more than one object :)
Will do 👍🏻 |
/hold |
@alvaroaleman any idea as to why it says:
For some reason, both |
/remove-hold |
Lol, thanks for noticing. It seems we don't run any unittests in CI currently. I've opened #1503 for that (And we will not merge anything until that one is in). |
/test pull-controller-runtime-test-master |
Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
listOpts.Limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, MadhavJivrajani 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 |
* Enable list opts in informer cache List - add test for informer cache limit option Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com> * enable Limit option in multi namespace cache Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
Enabled list options in
List
implementation ofReader
by the infromer cache in order to respectclient.Limit
if set.Fix #1422