-
Notifications
You must be signed in to change notification settings - Fork 3.4k
pkg/ciliumidentity: Fix DeleteUsedCIDIsRecreated test #35466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkg/ciliumidentity: Fix DeleteUsedCIDIsRecreated test #35466
Conversation
/cc @pippolo84 |
7877103
to
2a5a47d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ovidiu.
Coverage for this test is important because it's one of the paths CID controller could interact with the legacy CID GC.
Also, refactoring the code to make it shorter is great.
6e62e02
to
f97e41a
Compare
1149572
to
d2cac6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! 💯
Looks like `k8sClient.FakeClientset` is not properly synchronized and looks like is not properly propagating updating the store in a highly concurrent environment. Instead of relying on both .Events() and .List() moved to only .List() with Eventually() to nicely wait for the re-generation. The if statement could be replaced with a for-loop to break when encountering the identity not found, performed multiple stresstest runs and seems to behave similar. Keeping the test without the for-loop to speed up the testing. Signed-off-by: Ovidiu Tirla <otirla@google.com>
The initial sync is not required as the events are processed using the watcher .Events(). Signed-off-by: Ovidiu Tirla <otirla@google.com>
d2cac6d
to
595aeaf
Compare
rebased on main |
/test |
Fixes the
TestDeleteUsedCIDIsRecreated
by re-working the fakeclientset. Encountered synchronization issues on the client so moved to only check the .List() instead of both. Removed the initial sync and ordered the initial event processing.Fixes: #35135