@lavalamp, PTAL

GCE e2e build/test failed for commit a1feee8.

• Build Log
• Test Artifacts
• Internal Jenkins Results

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@jsafrane I am just a little bit confused about this race. IIUC you mean that there is race condition between Delete and Resync, I think after Delete, the item should be deleted, then Resync will not see that item.

@adohe, resync happens when Delete event is in queue and not processed yet. Then you get both Delete and Sync events in the queue and you hit the bug.

Notice that Delete does not remove the object being deleted from f.knownObjects, so subsequent Resync can still find it there. (umm, that could mean we could fix it in DeltaFIFO and not on the controller level...)

oops, unit test did not pass, I'll look at it tomorrow. Anyway, you can fix it by yourself, the PR here is just to show what's probably wrong.

@jsafrane I think the Resync method could be improved to fix this.

Thanks for the PR-- I need to think about this carefully to understand. I'm swamped, it may take a bit before I have a chance.

@jsafrane I think this PR can fix the race condition you describe. I like this fix. 👍

The tests are failing because the initial sync is done as Sync events which are ignored because of this PR. I think the real fix will be few lines changed here and there, however I am pretty swamped too :to investigate how these reflectors and informers and queues work together.

Also I will take a look at the failed test.

When controller receives 'object deleted' event from etcd and at the same time it starts periodic sync, "Sync" event can be enqueued after "Delete" event.

How is it possible?
If the queue gets Delete 'A' event first, 'A' should be deleted from remote store. If we get Sync, it means reflector will List from remote store and return the dataset that doesn't contain 'A'. In this case, syncWith() wouldn't return Sync event anyway. Right?

Resync and Relist are separated now. Resync is from the local store, so the race described is possible.

This problem and the #27004 are exposed by the separation of resync and relist.

I see.
Thanks @caesarxuchao for the explanation.

@hongchaodeng perhaps the cure for this issue and #27004 is to wait for the queue to be drained before reading the store (i.e., before the resyncing in this issue, and before checking if an object exists in store in #27004).

@lavalamp is this something we want for 1.3?

Yes, if the list/sync split exacerbated this then we should fix that.

On Wed, Jun 8, 2016 at 11:35 AM, Saad Ali notifications@github.com wrote:

@lavalamp https://github.com/lavalamp is this something we want for 1.3?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26167 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAnglhv7jZ_IIJ6dgJVCma7BEellcy7kks5qJwtjgaJpZM4Ilh2H
.

I'm marking it as 'do not merge', my fix is wrong. I'm leaving it open just for documentation and search purposes, feel free to close this PR when you have a better one.

I am closing the pr per author's comment above. Please document this as known issue in #27076.