Skip to content
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

identity/cache: fix panic when re-init of cache after close. #25269

Merged

Conversation

tommyp1ckles
Copy link
Contributor

@tommyp1ckles tommyp1ckles commented May 4, 2023

The cache appears to be intended to tolerate being initialized, closed then reinitialized. However the events channel is not reset correctly leading to a double close panic in tests.

This ensures that the events channel is set to nil after being closed, and checked upon further closes.
As well, if init is called on Close()'d cache, the events channel/watcher will be recreated.

Also, added tests to check functionality/correctness in this case.

Fixes: #25235

@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 May 4, 2023
@tommyp1ckles
Copy link
Contributor Author

/test

1 similar comment
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles marked this pull request as ready for review May 5, 2023 17:37
@tommyp1ckles tommyp1ckles requested a review from a team as a code owner May 5, 2023 17:37
@tommyp1ckles tommyp1ckles requested a review from nebril May 5, 2023 17:37
@tommyp1ckles tommyp1ckles added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label May 5, 2023
@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 May 5, 2023
@tommyp1ckles tommyp1ckles added area/CI Continuous Integration testing issue or flake dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 5, 2023
@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 May 5, 2023
@tommyp1ckles tommyp1ckles added the release-note/ci This PR makes changes to the CI. label May 5, 2023
The cache appears to be intended to tolerate being initialized, closed
then reinitialized. However the events channel is not reset correctly leading
to a double close panic in tests.

This ensures that the events channel is set to nil after being closed, and checked
upon further closes.
As well, if init is called on Close()'d cache, the events channel/watcher will be recreated.

Also, added tests to check functionality/correctness in this case.

Fixes: cilium#25235

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/fix-identity-cache-panic branch from 2d259a0 to 1e731b8 Compare May 8, 2023 16:34
@tommyp1ckles
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 May 8, 2023
@tommyp1ckles tommyp1ckles merged commit 3907a9a into cilium:main May 8, 2023
57 checks passed
@tommyp1ckles tommyp1ckles deleted the pr/tp/fix-identity-cache-panic branch May 8, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: daemon/cmd test panic'ed in TravisCI
2 participants