Fix data race due to trimming ManagedFields in resource syncer #891
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was observed in a unit test elsewhere that configures a resync period. On resync, the K8s DeltaFIFO retrieves every object from the cache store and re-queues them. It also invokes the transform function which results in a data race when the resource syncer tries to nil the ManagedFields. This is because the object instance is the same as that stored in the cache which was previously accessed by another thread. So mutating it without the protection of a lock is unsafe. This is really an issue with the DeltaFIFO - it is documented that the "TransformFunc sees the object before any other actor, and it is now safe to mutate the object in place instead of making a copy" however this is not the case on a re-sync. The DeltaFIFO should either elide the TransformFunc on re-sync or make a copy of the object retrieved from the cache store. As a workaround, the resource syncer should only set ManagedFields to nil if it is non-nil, which will be the case a a re-sync. Added a unit test to cover this case. Also the object passed to the TransformFunc could be a DeletedFinalStateUnknown so we need to handle that as well. Signed-off-by: Tom Pantelis <[email protected]>
tpantelis
requested review from
Oats87,
skitt,
sridhargaddam and
vthapar
as code owners
|
🤖 Created branch: z_pr891/tpantelis/transform_fn_data_race |
skitt
reviewed
Apr 16, 2024
| Expect(resourceutils.MustToMeta(obj).GetManagedFields()).Should(BeNil()) | ||
|
|
||
| // Sleep a little so a re-sync occurs and doesn't cause a data race. | ||
| time.Sleep(200) |
There was a problem hiding this comment.
Shouldn’t something be checked after the sleep?
There was a problem hiding this comment.
There's nothing we can check here but Ginkgo will fail with a data race if it occurs. So we're just trying to trigger a data race here.
|
Has this been reported upstream? |
|
skitt
approved these changes
Apr 16, 2024
yboaron
approved these changes
Apr 17, 2024
|
🤖 Closed branches: [z_pr891/tpantelis/transform_fn_data_race] |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was observed in a unit test elsewhere that configures a resync period. On re-sync, the K8s
DeltaFIFOretrieves every object from the cache store and re-queues them. It also invokes the transform function which results in a data race when the resource syncer tries to nil theManagedFields. This is because the object instance is the same as that stored in the cache which was previously accessed by another thread. So mutating it without the protection of a lock is unsafe. This is really an issue with theDeltaFIFO- it is documented that the "TransformFunc sees the object before any other actor, and it is now safe to mutate the object in place instead of making a copy" however this is not the case on a re-sync. TheDeltaFIFOshould either elide theTransformFuncon re-sync or make a copy of the object retrieved from the cache store.As a workaround, the resource syncer should only set
ManagedFieldsto nil if it is non-nil, which will be the case a a re-sync. Added a unit test to cover this case.Also the object passed to the
TransformFunccould be aDeletedFinalStateUnknownso we need to handle that as well.