feat: cache update on update control #2789
Conversation
I think this never worked partially because the crd generator maven plugin is not released into snapshot repo. For now I would just remove this. Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
csviri
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
| * | ||
| * @return if the framework should cache the updated resource for next reconciliation | ||
| */ | ||
| default boolean cacheUpdatedResourcesViaUpdateControl() { |
There was a problem hiding this comment.
Does it make sense to have a different option for this? More precisely, wouldn't it make sense to activate automatic caching whenever parsing of the resource version is activated? I mean the name of the parse method already implies that it is used for caching.
There was a problem hiding this comment.
Not exactly, parsing resource version was done for now only in dependent resources.
I think this should be an opt-in feature since it does not strictly follows K8S API contract, so user should be aware. I would not turn it on by default for the low level API.
There was a problem hiding this comment.
Also if there are some issues user can still turn it off.
There was a problem hiding this comment.
I think that adding options for corner cases makes things a lot more complex to understand, especially when the setting only applies when some other setting is enabled. To me, all these options should really be hidden under a single one where everything that can be done by assuming the resource versions grow monotonically should be done automatically when parsing is activated.
Also, if there are potential issues, then we probably shouldn't have the features to start with. Either we can implement it correctly or we shouldn't provide the feature. That's something I actually dislike with the SSA support: it's not working in all cases and makes things more complex to the point that people might not want to use it or actually revert to using optimistic-looking based updates. This isn't great.
There was a problem hiding this comment.
Also, the method name isn't correct as the setting impacts more than updates via UpdateControl, at least from what I can tell.
There was a problem hiding this comment.
That's something I actually dislike with the SSA support: it's not working in all cases and makes things more complex to the point that people might not want to use it or actually revert to using optimistic-looking based updates. This isn't great.
I don't see what SSA has to do with optimistic locking. OL can be turned on all update / patch types.
There was a problem hiding this comment.
Also, if there are potential issues, then we probably shouldn't have the features to start with. Either we can implement it correctly or we shouldn't provide the feature.
It's not about potential issues, I don't see any, but it is good to have a failsafe. Note that this what we have is again bending a bit the Kuberentes API contract. It is safe with ETCD at the backed, might be different for a non etc etc. For what we provide the alternative explicit cache.
| * next reconciliation will receive the most up-to-date resource. This is a consequence of | ||
| * Informers design in the Operator pattern. | ||
| * | ||
| * <p>The framework can be smart about it and make such guarantees, but this can be done |
There was a problem hiding this comment.
This whole paragraph is less explicit and clear that what it could be, imo.
There was a problem hiding this comment.
Will take a look thx.
| */ | ||
| default boolean parseResourceVersionsForEventFilteringAndCaching() { | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * If you update the primary resource from the reconciler with or without {@link | ||
| * io.javaoperatorsdk.operator.api.reconciler.UpdateControl} it is not guaranteed that for the |
There was a problem hiding this comment.
This doesn't really make sense. What's the point of referring to UpdateControl if that can happen whether we use it or not?
| * | ||
| * @return if the framework should cache the updated resource for next reconciliation | ||
| */ | ||
| default boolean cacheUpdatedResourcesViaUpdateControl() { |
There was a problem hiding this comment.
I think that adding options for corner cases makes things a lot more complex to understand, especially when the setting only applies when some other setting is enabled. To me, all these options should really be hidden under a single one where everything that can be done by assuming the resource versions grow monotonically should be done automatically when parsing is activated.
Also, if there are potential issues, then we probably shouldn't have the features to start with. Either we can implement it correctly or we shouldn't provide the feature. That's something I actually dislike with the SSA support: it's not working in all cases and makes things more complex to the point that people might not want to use it or actually revert to using optimistic-looking based updates. This isn't great.
| * | ||
| * @return if the framework should cache the updated resource for next reconciliation | ||
| */ | ||
| default boolean cacheUpdatedResourcesViaUpdateControl() { |
There was a problem hiding this comment.
Also, the method name isn't correct as the setting impacts more than updates via UpdateControl, at least from what I can tell.
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@metacosm I added docs and refined the javadoc and naming, also based on our discussion yesterday. However did not do any fundamental changes on logic, other than throw an exception if the If you find this too confusing, we could simply not support this in this way (let's discuss that) and we can close this PR. I created an issue v6 (which I guess won't come this year), to remove the update/patch methods from UpdateControl in the future: |
|
See this PR: It will provide also the lock version of of the updated resources described here: That would be an other feature flag etc to support internally and since in the future we probably will leave this see also: #2797 |
No description provided.