Skip to content

Conversation

ovidiutirla
Copy link
Contributor

@ovidiutirla ovidiutirla commented Oct 22, 2024

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.

go test -c ./operator/pkg/ciliumidentity && stress -p 128 ./ciliumidentity.test -test.run 'TestDeleteUsedCIDIsRecreated' -test.timeout=10s
45h54m50s: 32618794 runs so far, 0 failures (0.00%), 128 active

Fixes: #35135

@ovidiutirla ovidiutirla requested a review from a team as a code owner October 22, 2024 08:46
@ovidiutirla ovidiutirla requested a review from dlapcevic October 22, 2024 08:46
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 22, 2024
@ovidiutirla ovidiutirla changed the title Feature/fix operator recreate cid pkg/ciliumidentity: Fix DeleteUsedCIDIsRecreated test Oct 22, 2024
@ovidiutirla ovidiutirla added area/operator Impacts the cilium-operator component release-note/misc This PR makes changes that have no direct user impact. labels Oct 22, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 22, 2024
@ovidiutirla
Copy link
Contributor Author

/cc @pippolo84

@ovidiutirla ovidiutirla force-pushed the feature/fix-operator-recreate-cid branch from 7877103 to 2a5a47d Compare October 22, 2024 09:16
Copy link
Contributor

@dlapcevic dlapcevic left a 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.

@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 23, 2024
@ovidiutirla ovidiutirla force-pushed the feature/fix-operator-recreate-cid branch 3 times, most recently from 6e62e02 to f97e41a Compare October 24, 2024 06:44
@pippolo84 pippolo84 self-requested a review October 24, 2024 19:44
@pippolo84 pippolo84 removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 24, 2024
@ovidiutirla ovidiutirla force-pushed the feature/fix-operator-recreate-cid branch 2 times, most recently from 1149572 to d2cac6d Compare October 25, 2024 07:38
Copy link
Member

@pippolo84 pippolo84 left a 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>
@ovidiutirla ovidiutirla force-pushed the feature/fix-operator-recreate-cid branch from d2cac6d to 595aeaf Compare October 30, 2024 09:00
@ovidiutirla
Copy link
Contributor Author

rebased on main

@ovidiutirla
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 30, 2024
@pchaigno pchaigno added this pull request to the merge queue Oct 30, 2024
Merged via the queue into cilium:main with commit e9f4803 Oct 30, 2024
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operator Impacts the cilium-operator component ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: operator/pkg/ciliumidentity: TestDeleteUsedCIDIsRecreated: Flakes
4 participants