Skip to content

Conversation

marqc
Copy link
Contributor

@marqc marqc commented Jun 10, 2024

Please ensure your pull request adheres to the following guidelines:

  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.

In rare cases when dynamic exporter lifecycle Stop() function is called during config reload it may cause deadlock on mutex. This change stops config watcher ticker before locking the mutex, as mutex lock is effectively needed only to terminate configured exporters, not for terminating config watcher itself.

Fixes: #32587

Fix #32587 concurrent hubble dynamic exporter stop and reload

In rare cases when dynamic exporter lifecycle Stop() function is called
during config reload it may cause deadlock on mutex. This change stops
config watcher ticker before locking the mutex, as mutex lock is
effectively needed only to terminate configured exporters, not for
terminating config watcher itself.

Fixes: cilium#32587
Signed-off-by: Marek Chodor <mchodor@google.com>
@marqc marqc requested a review from a team as a code owner June 10, 2024 08:27
@marqc marqc requested a review from chancez June 10, 2024 08:27
@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 Jun 10, 2024
@marqc marqc force-pushed the issue-32587-fix-concurrent-dynamic-exporter-reload-and-stop branch from 0d64012 to 7848b56 Compare June 10, 2024 08:34
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@rolinh rolinh added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 10, 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 Jun 10, 2024
@rolinh rolinh added sig/hubble dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 10, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 10, 2024
@rolinh rolinh removed the request for review from chancez June 10, 2024 09:01
@rolinh
Copy link
Member

rolinh commented Jun 10, 2024

/test

@rolinh rolinh enabled auto-merge June 10, 2024 09:01
@marqc
Copy link
Contributor Author

marqc commented Jun 10, 2024

/test

@rolinh rolinh added this pull request to the merge queue Jun 10, 2024
@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 Jun 10, 2024
Merged via the queue into cilium:main with commit e3de640 Jun 10, 2024
@marqc marqc deleted the issue-32587-fix-concurrent-dynamic-exporter-reload-and-stop branch June 12, 2024 09:43
@giorio94 giorio94 mentioned this pull request Jun 12, 2024
4 tasks
@giorio94 giorio94 added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 labels Jun 12, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: TestDynamicExporterLifecycle
3 participants