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

devices: Fix panic in tests when logger used after stopping #32551

Merged

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented May 15, 2024

The netlink unsubscribing is async and the error callback is sometimes called with an error after unsubscribing:

  panic: Log in goroutine after TestDevicesController has completed:
  time=2024-05-14T15:40:53.086Z level=WARN
  msg="Netlink error received, restarting" module=devices-controller
  error="Receive failed: Receive called on a closed socket"

Ignore the error if we've cancelled the context.

Before this fix:

$ PRIVILEGED_TESTS=1 sudo -E stress go test . -test.v -test.run Devices -test.count 2
20s: 32 runs so far, 25 failures (78.12%)

After this fix:

$ PRIVILEGED_TESTS=1 sudo -E stress go test . -test.v -test.run Devices -test.count 2
30s: 51 runs so far, 0 failures

The netlink unsubscribing is async and the error callback is sometimes
called with an error after unsubscribing:

  panic: Log in goroutine after TestDevicesController has completed:
  time=2024-05-14T15:40:53.086Z level=WARN
  msg="Netlink error received, restarting" module=devices-controller
  error="Receive failed: Receive called on a closed socket"

Ignore the error if we've cancelled the context.

Before this fix:
$ PRIVILEGED_TESTS=1 sudo -E stress go test . -test.v -test.run Devices -test.count 2
20s: 32 runs so far, 25 failures (78.12%)

After this fix:
$ PRIVILEGED_TESTS=1 sudo -E stress go test . -test.v -test.run Devices -test.count 2
30s: 51 runs so far, 0 failures

Fixes: 18ed625 ("devices: Convert from logrus to slog")
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label May 15, 2024
@joamaki joamaki requested a review from a team as a code owner May 15, 2024 13:55
@joamaki joamaki requested a review from gentoo-root May 15, 2024 13:55
@joamaki
Copy link
Contributor Author

joamaki commented May 15, 2024

/test

@joamaki joamaki enabled auto-merge May 15, 2024 14:24
@joamaki joamaki added this pull request to the merge queue May 16, 2024
Merged via the queue into cilium:main with commit 66b31eb May 16, 2024
66 checks passed
@joamaki joamaki deleted the pr/joamaki/devices-controller-test-log-panic branch May 16, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

2 participants