- Notifications
You must be signed in to change notification settings - Fork237
Annotation removal using locking#3015
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
d650f86 to8558c1aCompare
csviri left a 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.
I generally like this approarch. What we should do is also document it on high level, like javadoc onTemporaryResourceCache. Like the general idea; using lock striping for IO operations, etc.
Not sure if all the scenarios are covered with unit tests, probably those could be also added.
Will have to spend more time to think through all the scenarion, overall LGTM, added some comments, and questions to clarify.
| } | ||
| try { | ||
| temporaryResourceCache.startModifying(id); | ||
| varupdated =updateMethod.apply(resourceToUpdate); |
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.
We might want to iterate over this, the start modifyn and recent resource update should be called from an utils, but fine by me now as it is
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.
I had initially started down the path of a common method with the primary cache utils - but for clarity this seemed like a better place to start.
Are you expecting that we'll want to push the event skipping logic down into ManagedInformerEventSource, and/or are you thinking that we'll effectively be able to use a Context rather than a direct reference to the event source?
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.
I think using the Context will be fine, from a utility method where it passed later.
So not complaining, it is fine like this. And we will se if we want to iterate.
| .equals(previousResourceVersion)) | ||
| ||isLaterResourceVersion(resourceId,newResource,cachedResource))) { | ||
| // first check against the source in general - this also prevents resurrecting resources when | ||
| // we've already seen the deletion event |
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.
As mentioned on other PR , would be nice to describe the resurrection scenario more in detial
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.
Expanded on that, and updated the class description.
| boolean[]known =newboolean[1]; | ||
| synchronized (this) { | ||
| if (!unknownState) { | ||
| latestResourceVersion =resource.getMetadata().getResourceVersion(); |
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.
While is this better this way as using the latest sync version?
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.
We are effectively using the TemporaryResourceCache to reason about things the reconciler was responsible for creating / updating.
If we skip putting something in the cache, then we may not skip that event as intended.
Let's say the informer has seen:
- create v1
- modify v2 - the reconciler did this
- modify v3 - something else did this
But has only delivered to the TemporaryResourceCache
- create v1
And we're now attempting a put with v2. If we use the informer latest version, we won't actually put v2 into the TemporaryResourceCache as it looks out-of-date. But then on onAddOrUpdateEvent we'll answer that the event wasn't known and trigger a reconciliation.
So by using a local notion of latestResourceVersion we're reasoning over what the TemporaryResourceCache actually knows.
Also because the get logic has changed to check both the TemporaryResourceCache and the informer cache, we won't actually return an out-of-date resource due to the put.
| privatefinalGroupVersionKindgroupVersionKind; | ||
| privatefinalInformerConfiguration<R>informerConfig; | ||
| privatefinalKubernetesClientkubernetesClient; | ||
| privatefinalbooleancomparableResourceVersions; |
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.
We should add this also to@Informer annotation
| @Override | ||
| publicvoidonDelete(Rresource,booleanb) { | ||
| publicsynchronizedvoidonDelete(Rresource,booleanb) { |
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.
I don't understand why we have this synchronized, could you elaborate pls?
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.
The current logic has start and the onAddOrUpdate methods synchronized. I'm assuming that the intention there is to effectively block event handling until the primaryToSecondaryIndex has been built from the initial listing. I don't think delete should be handled differently.
That could also be done with a more granular lock around the usages of primaryToSecondaryIndex.
In general there's effectively a single threaded model of event delivery, so unless users are calling the onXXX methods manually, those calls will be serialized.
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.
That could also be done with a more granular lock around the usages of primaryToSecondaryIndex.
Yes, that might be cleaner.
In general there's effectively a single threaded model of event delivery, so unless users are calling the onXXX methods manually, those calls will be serialized.
Yeah, synchronizing the start with those makes it a bit safer and easier to reason about (without a real performance impact )
8558c1a tod0d67fdCompare| */ | ||
| defaultbooleanparseResourceVersionsForEventFilteringAndCaching() { | ||
| returnfalse; | ||
| returnDEFAULT_COMPARABLE_RESOURCE_VERSIONS; |
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.
This needs to go to@Informer configuration and into ControllerConfiguration with dynamic configuration, but we can do it as a separate PR
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
21b4035 toc950617Compareshawkins commentedOct 24, 2025
Tried to clean up a bit more of the configuration and went ahead and switched to the comparableResourceVersions term - not sure how much backwards compatibility you'll want. I need to look more into the Informer and ControllerConfiguration - I had started to add stuff, but it looks to be a little involved. Perhaps a follow-up pr would be best. |
and more of the previous annotation handlingSigned-off-by: Steve Hawkins <shawkins@redhat.com>
c950617 to9d27d43Comparecsviri left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Hi@shawkins , this looks good to me, we can cleanup configs in a separate PR. In case happy to help with that.
shawkins commentedOct 30, 2025
de30b10 intooperator-framework:v5.3Uh oh!
There was an error while loading.Please reload this page.
csviri commentedOct 30, 2025
Thank you@shawkins !! |
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Uh oh!
There was an error while loading.Please reload this page.
Replacement / alternative to#2999.
Layered on the tombstone removal changes / default enablement of parsing resource versions. It turns out that some of the tombstone changes needed refinement anyway. For example, we need to compare to a localized latest version, not to the cache.
This seems like the most straight-forward way to do this with locking - but of course that means we have to be very careful with what locks are already held. If this seems too risky, then queuing managed in the TemporaryResourceCache could be used instead, but that will need more code restructuring.
These changes also assume we only want to deal with this skipping problem when using the InformerEventSource. That could be generalized if needed for#3011
Much like the PrimaryUpdateAndCacheUtils, users will need to call InformerEventSource.PrimaryUpdateAndCacheUtils, to properly use this via custom logic.
This does not do any cleanup the extraneous puts into the cache because of onCreated / onUpdated being called after the updateAndCacheResource method.
This also does not try to reason about deletes.
cc@csviri@metacosm