feat: Resource Event De-duplication

[erin-allison]

When an event for a given resource is added to the queue, all enqueued events (those currently waiting for their "Delay" to elapse) for that resource are unsubscribed from and are thus ignored.

For example, if I were to queue a "NotModified" event for a resource with a delay of 10 seconds, then 5 seconds later queue a "Deleted" event, the "Deleted" event would process immediately, and the "NotModified" event would be ignored and not processed.

This has no effect on events which are currently in the "UpdateResourceData" or "HandleEvent" steps, as those are already past the de-duplication step.

This would resolve #280, probably #365

Resolves #280

This should also resolve #365 since the cause is a significant difference in the processing pipelines for watcher events vs errored vs requeued events, whereas part of what I do here is to unify that logic (part of what was needed to de-duplicate things).

[erin-allison]

Another item of note is this should allow us to fairly easily sort out #340 by using something like https://github.com/amoerie/keyed-semaphores to disallow processing the same resource multiple times at once, without blocking all concurrency.

[erin-allison]

Items left for me to finish on here:

• thoroughly test to make sure this actually works as intended
• update testing code to allow testing the queue logic (MockManagedResourceController currently bypasses the queue)
• code cleanup

[buehler]

Thank you @erin-allison!
This looks amazing. I haven't had much time to work on this project lately. It's encouraging to see that you dive into the code as well :-) Thank you!

I'm thrilled to review the finished work

[erin-allison]

Thank you @erin-allison!
This looks amazing. I haven't had much time to work on this project lately. It's encouraging to see that you dive into the code as well :-) Thank you!

Happy to help; I don't actively use the SDK right now but it's a project I'm familiar with that I know I can help contribute to as I have time.

I'm thrilled to review the finished work

Careful what you wish for 😉 my eyes are set next on #362 and some of the ideas I had mentioned in #355

[erin-allison]

Upon further digging into how MockManagedResourceController<T> is built right now, I can't easily make the unit tests verify my code works, because it completely bypasses the queuing components. I'll spin up a local cluster and a dummy operator to confirm with, but properly automating tests for that will have to come with a decent bit of refactoring later.

[buehler]

Oh sh**. Yeah, you're right, the mock controller just assumes that the queue works but this obviously does not work when exactly this queue should be tested. To be honest, I did this kind of confirmation by myself when I did not know how to test certain components.

Maybe you have a good idea on how to refactor the mock queue controller and testing in general (ref to #282) to enable such behaviour and use the real queue for testing?

[erin-allison]

I have ideas, it'll just take a bit of work as I'll need to basically mock all the services that get injected into it's constructor and wire them up with dummy functionality.

[erin-allison]

@buehler I wouldn't normally cram this much into one PR but in order to test the changes to the event queue, I had to do some other changes, so the thousand-foot overview of what all I did here...

1. ManagedResourceController had the event queuing functions split off into EventQueue<T>
   This was necessary to test the two independently. The tests you already had, to test the controller side, mostly remained the same, whereas I am now able to isolate the queue and validate it on its own.

2. ResourceCache and ResourceWatcher had interfaces extracted
   This isn't huge, but was necessary for me to be able to build fairly no-op instances with Moq. I should've followed that pattern with MockEventQueue but had already built the class; using Moq for that too came as an after-thought.

3. MockManagedResourceController has been deleted
   We now act on the actual class and pass no-op dependencies into it instead of sub-classing to cut functionality out.

[erin-allison]

Oh hang on, I need to rebase... there's a lot that's been updated. And then I may as well tweak another couple things where I changed my mind.

I'd put it back to draft but I can't find if I can.

[erin-allison]

Ok, I think this is ready to go; I undid one item that I removed without thinking about it being breaking, so this should have no impact on anything public aside from a couple extra parameters that I exposed on the testing stuff (but there are default values so call signatures should still be valid).

[buehler]

Thank you @erin-allison!

[buehler]

FYI: you are not forgotten. I just hadn't had the time to review it 🙈

[github-actions]

🎉 This PR is included in version 6.4.0 🎉

The release is available on:

• GitHub release
• v6.4.0
• v6.4.0

Your semantic-release bot 📦🚀