🐛 Populate GVK for listed objects #389
Conversation
k8s-ci-robot
added
cncf-cla: yes
| @@ -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() | |||
There was a problem hiding this comment.
I don't believe that we need to deep copy here because these objects are all created from within this function?
Thoughts?
There was a problem hiding this comment.
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 🤷♂️
There was a problem hiding this comment.
👍 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!
There was a problem hiding this comment.
I think we should've been deepcopying anyway, since that's what we do elsewhere anyway.
|
|
||
| By("verifying that the returned pods have GVK populated") | ||
| Expect(out.Items).NotTo(BeEmpty()) | ||
| Expect(out.Items).Should(SatisfyAny(HaveLen(3), HaveLen(4))) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤷♂️
|
/assign @pwittrock |
Otherwise you can get races when getting and listing simultaneously
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12, JoelSpeed 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
the
approved
Fixes #284. Populates GroupVersionKind for objects returned by the List method.
This supersedes #296. I've rebased the branch, fixed up the conflicts and found an issue where we seemed to not be deepcopying items out of the cache before returning.
I've maintained @liyinan926's ownership of the commits so they should still be credited for their work.
/CC @DirectXMan12