-
Notifications
You must be signed in to change notification settings - Fork 962
Blockhound: Allow blocking call in kubernetes-client WatchEventsListener
#6287
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6287 +/- ##
============================================
- Coverage 74.46% 0 -74.47%
============================================
Files 1963 0 -1963
Lines 82437 0 -82437
Branches 10764 0 -10764
============================================
- Hits 61385 0 -61385
+ Misses 15918 0 -15918
+ Partials 5134 0 -5134 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
It looks like the CI is still failing due to a blocking call. Can you check this? |
@jrhee17 Ok let me check |
Motivation: Blockhound detects blocked thread after upgrading kubernetes-client 7.3.1 (line#6271) Modification: - Remove `io.fabric8.kubernetes.client.http.StandardHttpRequest$Builder` from allow list, as it uses UUID created via AtomicLong now. - Add `io.fabric8.kubernetes.client.server.mock.WatchEventsListener` to allow list, because it is only used in the mock server. Result: Blockhound passes Signed-off-by: Seonghyeon Cho <[email protected]>
e6f6e37
to
a000801
Compare
builder.allowBlockingCallsInside( | ||
"io.fabric8.kubernetes.client.http.StandardHttpRequest$Builder", | ||
"build"); | ||
"io.fabric8.kubernetes.client.server.mock.WatchEventsListener", "onClosed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work, but I’d prefer not to add test-related code into the production codebase. What do you think about introducing a separate KubernetesTestBlockHoundIntegration
class under the test directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good :)
I will try it
Signed-off-by: Seonghyeon Cho <[email protected]>
Signed-off-by: Seonghyeon Cho <[email protected]>
Signed-off-by: Seonghyeon Cho <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 👍 👍 👍
Sorry, still looks like the blockhound CI is failing |
Oops, let me also take a look. |
After we update the version from 6.x to 7.x, the test suite uses the embedded Vert.x server: Vert.x utilizes Let's wait for their answer. 😉 |
Motivation:
Blockhound detects blocked thread after upgrading kubernetes-client 7.3.1 (#6271, https://github.com/line/armeria/actions/runs/15771639756/job/44457567699)
Modification:
io.fabric8.kubernetes.client.http.StandardHttpRequest$Builder
from allow list, as it uses UUID created via AtomicLong now.io.fabric8.kubernetes.client.server.mock.WatchEventsListener
to allow list, because it is only used in the mock server.Result:
Blockhound passes.