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.

Sign up for GitHub

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

🐛 Populate GVK for listed objects #389

Merged
merged 4 commits into from
May 15, 2019
Merged

🐛 Populate GVK for listed objects #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
839d5aa
Populate GVK for listed objects
liyinan926 Jan 16, 2019
88f5187
Check that the right GVK gets populated
liyinan926 Jan 16, 2019
8f8b3f5
Account for regular cache and multinamespaced cache in List GVK test
JoelSpeed Apr 3, 2019
fc22506
Copy object before modifying it
JoelSpeed Apr 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions pkg/cache/cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,19 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
}
})

It("should be able to list objects with GVK populated", func() {
By("listing pods")
out := &kcorev1.PodList{}
Expect(informerCache.List(context.Background(), out)).To(Succeed())

By("verifying that the returned pods have GVK populated")
Expect(out.Items).NotTo(BeEmpty())
Expect(out.Items).Should(SatisfyAny(HaveLen(3), HaveLen(4)))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are getting 4 pods, I think that is a bug in the namespaced cache implementation. I will attempt to re-create locally.

I think we may need to file an issue and merge this with a TODO remove HaveLen(4) IMO.

/cc @DirectXMan12

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know if you managed to repro locally or not, but to me it looks like the regular cache will be returning 4 pods as it lists across all namespaces(?) whereas the multinamespaced cache will only list 3 as there is a pod in a namespace that it isn't caching?

I haven't looked into this thoroughly though so am kinda guessing how the two different caches behave 🤷‍♂️

for _, p := range out.Items {
Expect(p.GroupVersionKind()).To(Equal(kcorev1.SchemeGroupVersion.WithKind("Pod")))
}
})

It("should be able to list objects by namespace", func() {
By("listing pods in test-namespace-1")
listObj := &kcorev1.PodList{}
Expand Down
4 changes: 3 additions & 1 deletion pkg/cache/internal/cache_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ func (c *CacheReader) List(_ context.Context, out runtime.Object, opts ...client
if !isObj {
return fmt.Errorf("cache contained %T, which is not an Object", obj)
}
runtimeObjs = append(runtimeObjs, obj)
outObj := obj.DeepCopyObject()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that we need to deep copy here because these objects are all created from within this function?

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the DeepCopy because it stopped the race detector going off when I was running tests so there must be something funky going on here but I'm not sure what 🤷‍♂️

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 wonder if it is what the indexer gives us for each list is just the cached object? I will look it up to see eventually :) thanks for the explanation!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should've been deepcopying anyway, since that's what we do elsewhere anyway.

outObj.GetObjectKind().SetGroupVersionKind(c.groupVersionKind)
runtimeObjs = append(runtimeObjs, outObj)
}
filteredItems, err := objectutil.FilterWithLabels(runtimeObjs, labelSel)
if err != nil {
Expand Down