resource: Fix race with event retries of deleted and subsequently recreated objects #27340

joamaki · 2023-08-08T08:56:37Z

See commits for detailed description.

resource: Fix race condition in handling of Kubernetes object delete event retrying. In the very rare case when an object was created, deleted and re-created with the same name and the handling of the first deletion failed, the handling of delete event may have been retried even though the object was re-created. Only affected features using the Resource-library (LB IPAM, Mutual Auth and ClusterMesh).

bimmlerd

I think there's a race.

pkg/k8s/resource/resource_test.go

pkg/k8s/resource/resource.go

odinuge · 2023-08-08T12:37:51Z

Is there an issue tracking the issues this is causing, and the (potential) consequences of this @joamaki?

Also, do you know what versions of cilium this affects?

joamaki · 2023-08-08T13:37:29Z

Is there an issue tracking the issues this is causing, and the (potential) consequences of this @joamaki?

Also, do you know what versions of cilium this affects?

No issue yet as this was just discovered by reviewing the code. The consequences are luckily
limited since this only affects uses of Resource[T] that retry deletions and only cases where
an object is deleted and then recreated with the same key before the failed delete is successfully
retried.

Currently the potentially affected code paths according to "References" for Event[T].Done with non-nil
argument to a Delete event in v1.14 are:

None of these exist in earlier versions of Cilium. So the impact is only to v1.14 and to features ClusterMesh, LBIPAM, Multi-pool IPAM and Service Mesh / L7 proxy.

joamaki · 2023-08-28T13:20:53Z

/test

bimmlerd

Couple of nits, but LGTM!

pkg/k8s/resource/resource.go

pkg/k8s/resource/resource_test.go

pkg/k8s/resource/resource.go

youngnick

LGTM with @aanm's changes.

joamaki · 2023-09-05T13:26:30Z

/test

bimmlerd · 2023-09-05T13:38:23Z

Is there a way #27549 could be triggered by this?

joamaki · 2023-09-06T08:44:10Z

Is there a way #27549 could be triggered by this?

Don't think so. Probably same as with #23292.

We should perhaps look into doing what pkg/k8s/informer/informer.go does and implement the
Process function ourselves to make sure we mutate the cache.Store and queue work under the same lock to avoid the situation where a new subscriber will see a key via IterKeys(), process it, and then reprocess the same event due to the ResourceEventHandler queueing it.

joamaki · 2023-09-12T12:58:08Z

Is there a way #27549 could be triggered by this?

Don't think so. Probably same as with #23292.

We should perhaps look into doing what pkg/k8s/informer/informer.go does and implement the Process function ourselves to make sure we mutate the cache.Store and queue work under the same lock to avoid the situation where a new subscriber will see a key via IterKeys(), process it, and then reprocess the same event due to the ResourceEventHandler queueing it.

I ended up doing exactly this: Resource[T] now implements its own Process function. This removes the race where a new subscriber can list the keys in the store while a key is being queued for it which led to the double-upsert event.

I also redid the delete object logic to keep a map of last known state of objects per subscriber and not use the "delete object" that informer sees. This keeps things consistent and the code easier to follow.

joamaki · 2023-09-12T13:35:28Z

/test

bimmlerd

Wow, nice simplification.

pkg/k8s/resource/resource.go

aanm

Can we adapt the pkg/k8s/informer.NewInformerWithStore for the pkg/k8s/resource use cases and use it?

Also, we should keep the mutator cache detector code, it was accidentally removed. I'm going to re-add it.

joamaki · 2023-09-15T14:00:48Z

Can we adapt the pkg/k8s/informer.NewInformerWithStore for the pkg/k8s/resource use cases and use it?

Also, we should keep the mutator cache detector code, it was accidentally removed. I'm going to re-add it.

How do I do this with it: https://github.com/cilium/cilium/pull/27340/files#diff-ae3d7e3fabf6dbbdcdd0d826fe38147c28883c23eb2f1ae43f5b80f54f830a7bR699 ?

joamaki · 2023-09-26T06:34:25Z

/test

Resource[T] does not correctly handle the events: Upsert -> Delete with Done(not-nil) -> Upsert (recreate) -> Delete (retry) The retried delete event carries the old initial version of the object causing the recreated object to be incorrectly deleted. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Fix double upserts that were caused by store being manipulated without synchronization with the subscriber queues by processing the deltas under the resource read-lock and doing the initial key listing for new subscriber with a write-lock. This way we cannot accidentally see a key in the store and process it just before the key is queued. As shown by test case in previous commit, the delete events are retried with an old incorrect version of the object causing a recreated object to be deleted. Fix the deletion retrying by always queueing upserts and deletes by key and keeping the last known state of objects emitted to the subscriber. Only emit a delete event if the subscriber has seen its creation and only use a version of the object that the subscriber has observed. Fixes: 4101e2c ("k8s: Add resource package") Signed-off-by: Jussi Maki <jussi@isovalent.com>

The semantics around retrying and the Sync event are subtle. Spell out the properties in a comment for Events(). Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki · 2023-10-04T12:53:41Z

/test

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 8, 2023

bimmlerd requested changes Aug 8, 2023

View reviewed changes

pkg/k8s/resource/resource_test.go Outdated Show resolved Hide resolved

pkg/k8s/resource/resource_test.go Outdated Show resolved Hide resolved

pkg/k8s/resource/resource.go Outdated Show resolved Hide resolved

joamaki changed the title ~~pr/joamaki/fix resource delete race~~ resource: Fix race with event retries of deleted and subsequently recreated objects Aug 9, 2023

joamaki mentioned this pull request Aug 17, 2023

Cilium Identity not getting deleted sometimes in large scale cluster #26514

Closed

2 tasks

joamaki added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Aug 25, 2023

joamaki force-pushed the pr/joamaki/fix-resource-delete-race branch from 26801fc to 49258fc Compare August 25, 2023 07:36

joamaki requested a review from bimmlerd August 28, 2023 11:25

joamaki marked this pull request as ready for review August 28, 2023 11:26

joamaki requested a review from a team as a code owner August 28, 2023 11:26

joamaki requested a review from youngnick August 28, 2023 11:26

joamaki added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Aug 28, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 28, 2023

maintainer-s-little-helper bot added this to Needs backport from main in 1.14.2 Aug 28, 2023

bimmlerd approved these changes Aug 29, 2023

View reviewed changes

pkg/k8s/resource/resource.go Outdated Show resolved Hide resolved

pkg/k8s/resource/resource.go Outdated Show resolved Hide resolved

pkg/k8s/resource/resource_test.go Show resolved Hide resolved

pkg/k8s/resource/resource.go Outdated Show resolved Hide resolved

joamaki force-pushed the pr/joamaki/fix-resource-delete-race branch 2 times, most recently from 97e1d2e to 3c84e87 Compare August 31, 2023 12:05

aanm self-requested a review September 1, 2023 08:46

aanm requested changes Sep 1, 2023

View reviewed changes

pkg/k8s/resource/resource.go Outdated Show resolved Hide resolved

pkg/k8s/resource/resource.go Outdated Show resolved Hide resolved

christarazi added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. kind/bug This is a bug in the Cilium logic. labels Sep 1, 2023

youngnick approved these changes Sep 4, 2023

View reviewed changes

joamaki requested a review from aanm September 4, 2023 11:29

joamaki force-pushed the pr/joamaki/fix-resource-delete-race branch from 3c84e87 to 8e06adc Compare September 5, 2023 13:05

joamaki force-pushed the pr/joamaki/fix-resource-delete-race branch from 8e06adc to 5f73cc4 Compare September 6, 2023 08:26

bimmlerd approved these changes Sep 13, 2023

View reviewed changes

pkg/k8s/resource/resource.go Outdated Show resolved Hide resolved

joamaki force-pushed the pr/joamaki/fix-resource-delete-race branch from eb36475 to 316751a Compare September 15, 2023 11:19

joamaki mentioned this pull request Sep 15, 2023

CI: Cilium Runtime: privileged tests: TestResource_WithoutIndexers #27549

Closed

joamaki force-pushed the pr/joamaki/fix-resource-delete-race branch from 316751a to 2737615 Compare September 15, 2023 13:23

aanm approved these changes Sep 15, 2023

View reviewed changes

joamaki force-pushed the pr/joamaki/fix-resource-delete-race branch 3 times, most recently from 83cd574 to 11ff2cf Compare September 29, 2023 11:02

joamaki added 3 commits October 4, 2023 13:30

resource: Improve documentation on Events()

b47a92c

The semantics around retrying and the Sync event are subtle. Spell out the properties in a comment for Events(). Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki force-pushed the pr/joamaki/fix-resource-delete-race branch from 11ff2cf to b47a92c Compare October 4, 2023 10:33

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 4, 2023

joamaki merged commit 2d00e56 into cilium:main Oct 6, 2023
59 of 61 checks passed

joamaki mentioned this pull request Oct 10, 2023

v1.14 Backports 2023-10-10 #28494

Merged

4 tasks

joamaki added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 10, 2023

maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.3 Oct 10, 2023

joestringer mentioned this pull request Oct 11, 2023

goleak: Errors on successful test run: found unexpected goroutines: k8s.io/client-go/util/workqueue.(*delayingType).waitingLoop #28477

Closed

github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Oct 12, 2023

maintainer-s-little-helper bot removed this from Backport pending to v1.14 in 1.14.3 Oct 12, 2023

maintainer-s-little-helper bot added this to Backport done to v1.14 in 1.14.3 Oct 12, 2023

jrajahalme mentioned this pull request Oct 18, 2023

Prepare for release v1.14.3 #28680

Merged

pavels mentioned this pull request Oct 28, 2023

Cilium thinks some endpoints for ClusterIP service is (terminating) when they are not #28490

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource: Fix race with event retries of deleted and subsequently recreated objects #27340

resource: Fix race with event retries of deleted and subsequently recreated objects #27340

joamaki commented Aug 8, 2023 •

edited

bimmlerd left a comment

odinuge commented Aug 8, 2023

joamaki commented Aug 8, 2023

joamaki commented Aug 28, 2023

bimmlerd left a comment

youngnick left a comment

joamaki commented Sep 5, 2023

bimmlerd commented Sep 5, 2023

joamaki commented Sep 6, 2023

joamaki commented Sep 12, 2023

joamaki commented Sep 12, 2023

bimmlerd left a comment

aanm left a comment

joamaki commented Sep 15, 2023

joamaki commented Sep 26, 2023

joamaki commented Oct 4, 2023

resource: Fix race with event retries of deleted and subsequently recreated objects #27340

resource: Fix race with event retries of deleted and subsequently recreated objects #27340

Conversation

joamaki commented Aug 8, 2023 • edited

bimmlerd left a comment

Choose a reason for hiding this comment

odinuge commented Aug 8, 2023

joamaki commented Aug 8, 2023

joamaki commented Aug 28, 2023

bimmlerd left a comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

joamaki commented Sep 5, 2023

bimmlerd commented Sep 5, 2023

joamaki commented Sep 6, 2023

joamaki commented Sep 12, 2023

joamaki commented Sep 12, 2023

bimmlerd left a comment

Choose a reason for hiding this comment

aanm left a comment

Choose a reason for hiding this comment

joamaki commented Sep 15, 2023

joamaki commented Sep 26, 2023

joamaki commented Oct 4, 2023

joamaki commented Aug 8, 2023 •

edited