rough in of tracking things that are defaults and normalizations #2766
Conversation
Signed-off-by: Steve Hawkins <[email protected]>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
|
||
| SSAState overrides = defaultsAndNormalizations.get(new CacheKey(ResourceID.fromResource(actual), desired.hashCode())); | ||
| if (overrides != null) { | ||
| // if containsAll(overrides.desired, state.desired) && containsAll(overrides.actual, state.actual) |
There was a problem hiding this comment.
I assume if this is not true just return false?
There was a problem hiding this comment.
If it is true might not always mean match, or?
There was a problem hiding this comment.
I assume here goes return true?
It wouldn't yet because the maps have been minimized they will only contain entries that represent normalizations, or defaults - it's basically a diff, but in map form. So an entry that is only in the actual map, that would be a default, and an entry that appears in both but with different values is a normalization.
We know that the desired state matches up to the java hash, but to make sure we're not seeing a false positive we'll still double check that the desired and actual states here contain everything from the diffs - if they do, we can prune all that and proceed with the rest of the comparison - reasonably, but possibly not completely, assured that we've made the correct assumption.
I'm minimizing the maps because I'm not sure we know how long we'll have to hold on to these entries to prevent the looping behavior. The temporary resource cache and previous annotation will prevent us from consuming our own modification, but if another entity modifies (or increments the resourceVersion in any way) the resource, that will trigger our reconciliation and increment of the previous annotation and so forth. If we think that the number of these we would have to track is sufficiently limited, then we could use the full desired state as the key instead of a weak hash.
|
Not sure if I follow all the details, especially the missing ones. Wonder if this would not require optimistic locking to be absolutely correct, is when we apply a desired state that is based on "actual" from the cache, but let say this cache is stale, than we calculate the state based on outdated values (since it is based on response from the server) , or? (so theoretically another manager/controller could have added a field that is now considered as default, I think defaults do not show up in managed fields, maybe also we should check if the defaults are not in managed fields or other managers? - not sure just thinking out loud) |
|
@shawkins maybe we could also take a look together on our community meeting. |
You're talking specifically here about the changes in SSA matching were we try to determine if we can skip the event correct? The other two potential usages, wouldn't have any locking concerns:
For the SSA matching logic I can't quite see a locking concern either - maybe you assumed that we were skipping the rest of the matching logic because I didn't flesh things out fully.
I can confirm inconsistent behavior wrt defaults - at least for my latest keycloak issue along these lines, the default for a feildRef apiVersion ends up looking like it is managed by the operator even when it is not specified. If the field is owned by another manager, then it will get pruned as expected. As you are getting if there were some case where a non-managed default could be added later (not by the operator create / update), then yes that would not be understood by this additional logic - it would have to be covered instead by the SSA matche's built-in logic. |
Sounds good, I will join the next one. |
closes: #2249
This is an alternative to fleshing out the approach in #2749
Here the desired state and the state returned from kubernetes is used to determine what values have been normalized and defaulted. It's stored in a cache on the SSA matcher - that cache will need to be size / time bound, but that has not been added yet.
We can use that information in a couple of ways.
The first roughed in here is to try and augment our matches decision making - if it looks like the same desired state was requested, and the actual contains all the overriden values, then we can assume that things still match. To cut down on the memory footprint this is based upon a hash of the desired state, rather than the full desired state.
Another, simpler and potentially safer approach, would be to just use this cache to determine if the previous annotation should be used.
Finally rather than caching or trying to use this in some novel way, we could at least log that what is happening and make it clear what will happen if use of the previous annotation is enabled.
WDYT @xstefank @csviri @metacosm