StateDB-based K8s fake client object tracker #39446
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a4ee357 to
39d8681
Compare
39d8681 to
297ee87
Compare
joamaki
commented
May 8, 2025
joamaki
commented
May 8, 2025
joamaki
commented
May 8, 2025
bimmlerd
reviewed
May 9, 2025
There was a problem hiding this comment.
nice! I've added a comment re a deadlock that I think should happen, but maybe I misunderstood something.
One more place to clean up might be the controlplane tests, where there's the whole infra for waiting for the watch to be established. Otherwise I can do that in a separate PR.
I'm planning to remove test/controlplane completely soon so probably not worth it. |
70f37e5 to
ddc224a
Compare
bimmlerd
approved these changes
May 21, 2025
harsimran-pabla
approved these changes
May 21, 2025
ysksuzuki
approved these changes
May 22, 2025
tommyp1ckles
approved these changes
May 23, 2025
ddc224a to
8a10a10
Compare
This comment was marked as outdated.
This comment was marked as outdated.
af7c346 to
9967569
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
/test |
9967569 to
9b38ae1
Compare
dylandreimerink
approved these changes
May 28, 2025
The default testing.ObjectTracker that comes with client-go does not properly implement the Watch() method: 1) Does not support watching from a specific ResourceVersion 2) Does not support multiple concurrent Watch() calls 3) Does not support FieldSelector This has been an ongoing pain as all the testing code needs to make sure the implementation does not call Watch() too early. This has required either populating the ObjectTracker before starting the implementation or synchronizing with the implementation to add objects only after Watch() has been called (e.g. db/initialized or k8s/wait-watchers). This commit implements a StateDB based ObjectTracker that properly implements Watch() and FieldSelector. Signed-off-by: Jussi Maki <jussi@isovalent.com>
With the new fake client object tracker we no longer need to synchronize with the reflectors before adding objects, so we can remove db/initialized. Interestingly enough this change triggered the goleak to hit the workqueue goroutine leak from Resource[T]. We don't have a way for waiting for that background goroutine to exit so we'll just need to ignore it the same way pkg/k8s/resource/resource_test.go does. Fix the node name in hostport.txtar now that FieldSelector is implemented. Signed-off-by: Jussi Maki <jussi@isovalent.com>
No need to wait for watchers anymore so remove db/initialized calls from tests. Signed-off-by: Jussi Maki <jussi@isovalent.com>
Now that we're using our own object tracker the Tracker() method cannot be used as that returns the wrong object tracker. It anyway makes more sense to go via the typed API. Fix the Resource[T] tests to not use Tracker() and also not to use ResourceVersion as that's now correctly populated by the object tracker. Signed-off-by: Jussi Maki <jussi@isovalent.com>
With the proper object tracker we no longer need this. Signed-off-by: Jussi Maki <jussi@isovalent.com>
Since we're using Tracker() method in bunch of tests make it point to our ObjectTracker. Unfortunately there's no clean way to do this so I need to just play tricks with unsafe.Pointer. At least the result can be validated. Signed-off-by: Jussi Maki <jussi@isovalent.com>
As db/initialized is not necessary anymore just for waiting for reflector/informer to sync, remove remaining uses of it. Fix the remaining field selector issues. Signed-off-by: Jussi Maki <jussi@isovalent.com>
9b38ae1 to
d523bb3
Compare
|
/test |
sayboras
approved these changes
May 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements a new
testing.ObjectTrackeron top of StateDB that fixes the long-standing pain point with the k8s fake client, namely that itsWatchimplementation does not support multiple watchers and it will miss events that happen betweenListandWatch. This new implementation understandsResourceVersionand allows starting to watch from older versions without missing events.This will also help doing more complicated integration tests where we would e.g. run the agent and the operator hives in the same process with a single fake client & tracker and we would be able to have both watching the same resources.
An extra bonus is that we now have debug logging for every k8s action that is done: