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

Fix data race during Hubble setup #28322

Merged
merged 1 commit into from Oct 4, 2023

Conversation

glrf
Copy link
Contributor

@glrf glrf commented Sep 28, 2023

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • 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.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this 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.

This commit fixes a data race where the hubbleObserver is being initialized in a new goroutine while the StatusCollector already tries to check Hubble's status.

We fix the data race by switching to an atomic pointer to access the hubbleObserver in the Daemon struct.

Fixing the data race by simply waiting to start the status collector until hubble is initialized does not work, as launchHubble could take up to 30s to return while waiting for a TLS certificate and we don't want to block agent startup for this.

Fixes: #28291

This commit fixes a data race where the setup of the hubbleObserver is
being initialized in a new goroutine while the StatusCollector already
tries to check Hubble's status.

We fix the data race by switching to an atomic pointer to access the
`hubbleObserver` in the Daemon struct.

Fixing the data race by simply waiting to start the status collector
until hubble is initialized does not work, as `launchHubble` could take
up to 30s to return while waiting for a TLS certificate and we don't
want to block agent startup for this.

Fixes: cilium#28291

Signed-off-by: Fabian Fischer <fabian.fischer@isovalent.com>
@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 Sep 28, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Sep 28, 2023
@glrf glrf changed the title Fix data race during Hubble setup in the daemon Fix data race during Hubble setup Sep 28, 2023
@glrf glrf marked this pull request as ready for review September 28, 2023 11:38
@glrf glrf requested review from a team as code owners September 28, 2023 11:38
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Just one comment inline about a behavioral difference introduced in the refactoring.

daemon/cmd/hubble.go Show resolved Hide resolved
@giorio94 giorio94 added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay labels Sep 28, 2023
@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 Sep 28, 2023
@kaworu kaworu requested a review from gandro October 2, 2023 07:50
@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 3, 2023
@ti-mo ti-mo merged commit eee2414 into cilium:main Oct 4, 2023
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup This includes no functional changes. kind/community-contribution This was a contribution made by a community member. 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. sig/hubble Impacts hubble server or relay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Race condition in (*Daemon).getHubbleStatus()
6 participants