Fix race condition in Deleted condition #3550
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3550 +/- ##
=======================================
Coverage 41.13% 41.13%
=======================================
Files 87 87
Lines 12910 12906 -4
=======================================
- Hits 5310 5309 -1
+ Misses 7205 7203 -2
+ Partials 395 394 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| factory.WaitForCacheSync(des.stopper) | ||
| // Start the new informer by calling factory.Start (which is idempotent). | ||
| // This ensures that the informer is started exactly once and is cleaned up later. | ||
| factory.Start(des.stopper) |
There was a problem hiding this comment.
See implementation of factory.Start and factory.WaitForCacheSync. Note that the informer was created earlier by the factory (via ForResource).
| close(stopper) | ||
| factory.Shutdown() | ||
| }() | ||
| factory.Start(stopper) |
There was a problem hiding this comment.
Rationale: There are no informers to start at this point, so this is a no-op. See factory.Start.
| _, err := dc.getter.Get(ctx, dc.Object().GetName(), metav1.GetOptions{}) | ||
| if err == nil { | ||
| // Still exists. | ||
| dc.deleted.Store(false) |
There was a problem hiding this comment.
Rationale: the deleted field should transition from false to true, never true to false. This line of code was assumedly thought to be a no-op. In practice it overwrites the result from the watcher goroutine.
|
This PR has been shipped in release v4.22.1. |
|
This PR has been shipped in release v4.22.0. |
3 participants
Proposed changes
This PR fixes a race condition in the await logic that leads to Pulumi stalling until the await timeout is reached (e.g. after 600s). The condition logic races between informer events and GET call(s) to observe the deletion of the object. In the edge case that the 'deleted' watch event is observed before the GET call completes, the system may become stuck because the getter overwrites the watcher's observation (as stored in the
cond.deletedfield).In other words the
deletedflag should be idempotent, and never transition from true to false.Also, I notice that the informer could be more reliably started by the factory, since factory.Start is idempotent. This will ensure that the informer is considered during shutdown, and that WaitForCacheSync would work as expected.
Related issues (optional)
closes #3317