changes to simplify the recording mechanism

[shawkins]

Retargeting of #2005

[shawkins]

To move this out of draft there are a couple of things to cover:

• should the feature of adding the annotation be enabled/disabled via flag? If it's turned off, that just means we won't skip the event if it comes earlier than the update response. As mentioned above that may not be a big deal - operators that are working off of caches and that have effective matchers will just use a few extra cpu cycles.
• Is there a better way to do filtering of the annotation rather than adding special case logic in the matcher - and does the annotation need to be accountted for in the non-SSA case as well?
• what tests to include - integration? or will some unit tests suffice

[metacosm]

Can we rebase this so that only the relevant changes show up in the diffs, please?

[csviri]

should the feature of adding the annotation be enabled/disabled via flag? If it's turned off, that just means we won't skip the event if it comes earlier than the update response. As mentioned above that may not be a big deal - operators that are working off of caches and that have effective matchers will just use a few extra cpu cycles.

Good question, I think that could be there but we can add it a separate pull request, would we add separate one for caching and one for event propagation?

[csviri]

what tests to include - integration? or will some unit tests suffice

I think there should be related IT which we might to update to check if there are annotations present, and unit tests yes.

[shawkins]

Good question, I think that could be there but we can add it a separate pull request, would we add separate one for caching and one for event propagation?

We have a couple of inter-related features here.

• skipping events if they come in too quickly via the annotation
• Not currently implemented, but mentioned elsewhere, maintaining a bounded cache of resource versions the dependent resource was responsible and skipping those events if they arrive late (after you have already operated on the temporary cache item).
• working off of a more updated cache (which without resource version parsing works in a limited fashion)

The relevant options could be:

• a flag for using the annotation
• a flag to parse resource versions
• a flag to disable all cache / event optimizations

[shawkins]

Ok, this should be considered out of draft. The integration test should work again based upon using a dependent resource instead - although it may look too hackish.

[csviri]

LGTM

[metacosm]

I have more changes that I think could clean some things up but I'd need access to push to your branch. Also, I think we need some kind of documentation on this change, in particular regarding the logic used to detect whether a change has already been processed.
More generally speaking, it seems like this behavior is only limited to Kubernetes dependents. Is that really true? What happens for informer-based events which are not coming from dependents?

[shawkins]

I have more changes that I think could clean some things up but I'd need access to push to your branch.

Maintainer access was checked.

Also, I think we need some kind of documentation on this change, in particular regarding the logic used to detect whether a change has already been processed.

In rough terms the annotation is being used to detect when you recieve an event ahead of when your blocking create/update call returns. If you have already moved ahead to a temporarily cached resource, or anything about your local state does not still match what is in the annotation value, we'll assume the event is needed. @csviri has suggested using another issue / pr to use a flag for this optimization. If disabled and the matching logic isn't being tripped up, then it's not a big deal to initiate another reconciliation cycle. I'm not sure what a good place is to document this, or if it should just wait for the issue that makes the feature conditional.

More generally speaking, it seems like this behavior is only limited to Kubernetes dependents. Is that really true? What happens for informer-based events which are not coming from dependents?

There are two things working together here, the KubernetesDependentResource and the InformerEventSource. That is really no different than before - only the KubernetesDependentResource had the calls to event filtering logic. Are you asking about the case of someone creating their own dependent like resource using the methods like InformerEventSource.prepareForCreateOrUpdateEventFiltering? You could still expose something like that here, you would just need different signatures as they would need to be responsible for adding the annotation and so need access to the previous revision.

Is there a better way to do filtering of the annotation rather than adding special case logic in the matcher - and does the annotation need to be accountted for in the non-SSA case as well?

Answering my own previous question - if someone were using a custom matcher and expecting labelAndAnnotation equality, then yes this new annotation would be a problem.

[metacosm]

I don't think that we need that part as the client always make sure the annotations exist, if I'm not mistaken. Unless, of course, we want to safeguard here, regardless of what the client does.

[shawkins]

The client will initialize the annotation, but the desired is coming from the user logic (in the ssa case or if overriding the update matcher) and could have been set to null. I was just trying to match a similar guard that is used in addDefaultSecondaryToPrimaryMapperAnnotations - but I'd be fine if the assumption was that the user must leave the annotations as non-null and remove the initialization from both of those places.

Also it may not need to be part of these changes, but it's making things more complicated than needed to diffentiate the ssa and non-ssa cases wrt methods like addReferenceHandlingMetadata - I think this should be refactored so that the update / match logic can read more like create so that we'll take the same pre-steps in both cases.

[shawkins]

@metacosm see what you think of latest commit