fix: dependent event handling with temp cache not cleared in some corner cases #1994
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| // if the resource is not added to the temp cache, it is cleared, since | ||
| // the cache is cleared by subsequent events after updates, but if those did not received | ||
| // the temp cache is still filled at this point with an old resource | ||
| log.debug("Cleaning temporal cache for resource id: {}", resourceID); |
There was a problem hiding this comment.
@shawkins this will always clear the temp cache if not propagated.
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
csviri
changed the title
csviri
changed the title
...io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
Outdated
Show resolved
Hide resolved
| @@ -138,15 +142,23 @@ public R create(R target, P primary, Context<P> context) { | |||
| } | |||
|
|
|||
| public R update(R actual, R target, P primary, Context<P> context) { | |||
| if (log.isDebugEnabled()) { | |||
There was a problem hiding this comment.
Why check if debug login is enabled here and not elsewhere?
There was a problem hiding this comment.
Since creating a new object in that case: ResourceID.fromResource(actual). Want to do that only if really needed
| @@ -277,22 +280,32 @@ private void handleRecentResourceOperationAndStopEventRecording(Operation operat | |||
| R newResource, R oldResource) { | |||
| ResourceID resourceID = ResourceID.fromResource(newResource); | |||
| try { | |||
| // what if the prev event delayed, thus that even did not clear the temp cache. | |||
There was a problem hiding this comment.
I don't understand this comment
There was a problem hiding this comment.
removed that, just probably some note for myself. good catch :)
...n/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java
Outdated
Show resolved
Hide resolved
…tor/processing/dependent/kubernetes/KubernetesDependentResource.java Co-authored-by: Chris Laprun <[email protected]>
…tor/processing/event/source/informer/InformerEventSource.java Co-authored-by: Chris Laprun <[email protected]>
|
jFYI – this change seem to cause issues with K8s <1.23. But that should be ok as even 1.23 is quite old by now and unsupported. |
|
I guess our tests do not cover this change properly because this was tested without errors on 1.23… |
|
1.23 fixes it. You need to be on <1.23, we discovered it with 1.22. |
|
@vmuzikar could you pls create a new issue and provide some additional information, ideally also logs, but instructions to reproduce would help too. thx! |
|
I can create an issue but does JOSDK really support this old K8s versions? :) |
|
We are not testing it now, there is now also a discussion, that if we should support older versions, or just the versions what also Kubernetes supports. What is the strategy of KeyCloak on this @vmuzikar ? :) |
Formally ,we're aligned with the OCP lifecycle. The oldest supported version now will be 4.11 which translates to K8s 1.24. |
|
Thank you @vmuzikar |
|
The issue is actually already reported here (which I missed before): #2033 |
No description provided.