-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
resource: Work around a rare race in initial sync #23292
Conversation
TestResource_WithFakeClient is sometimes failing due to seeing two Upserts instead of Upsert+Sync. This is due to the fact that we add the initial set of keys from the store to the queue when subscribing in Events() and the fact that adding to the store is happening before our event handler runs. The sequence of events looks like this: [informer 1/2] add object A to store [resource] subscribe and add object with key A to queue [resource] dequeue A and emit Event{Upsert, A} [informer 2/2] call AddFunc handler for A and queue A [resource] dequeue A and emit Event{Upsert, A} Work around the issue by allowing the extra Upsert event in the test case. Proper fix for this would be to either detect a duplicate using ResourceVersion, or implementing a custom Process function that synchronizes the updating of the store with subscribing (e.g. pkg/k8s/informer/informer.go). Fixes: cilium#23079 Fixes: 4101e2c ("k8s: Add resource package") Signed-off-by: Jussi Maki <jussi@isovalent.com>
I marked this for backport to 1.13 since it looks like it will mitigate a unit test failure. If this relies on changes only in the master branch, feel free to remove the label. |
/test Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed: Click to show.Test Name
Failure Output
If it is a flake and a GitHub issue doesn't already exist to track it, comment Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed: Click to show.Test Name
Failure Output
If it is a flake and a GitHub issue doesn't already exist to track it, comment |
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
@joamaki Why were the end-to-end tests executed here? AFAICS, it's only affecting the non-privileged unit tests which run in Travis CI. Is that was a mistake, I can probably merge. |
Yep only Travis CI needed to check that the test passes. Go ahead and merge. |
@joamaki is the backport for 1.13 still on your radar, or is it fine to drop that? |
Taking that as a no :). |
@julianwiedmann oops, sorry about the delay (had half-answered this and then it got buried). Yes no need to backport to 1.13. |
TestResource_WithFakeClient is sometimes failing due to seeing two Upserts instead of Upsert+Sync. This is due to the fact that we add the initial set of keys from the store to the queue when subscribing in Events() and the fact that adding to the store is happening before our event handler runs. The sequence of events looks like this:
[informer 1/2] add object A to store
[resource] subscribe and add object with key A to queue
[resource] dequeue A and emit Event{Upsert, A}
[informer 2/2] call AddFunc handler for A and queue A
[resource] dequeue A and emit Event{Upsert, A}
Work around the issue by allowing the extra Upsert event in the test case.
Proper fix for this would be to either detect a duplicate using ResourceVersion, or implementing a custom (cache.Config) Process function that synchronizes the updating of the store with subscribing (e.g. pkg/k8s/informer/informer.go).
Fixes: #23079
Fixes: 4101e2c ("k8s: Add resource package")