-
Notifications
You must be signed in to change notification settings - Fork 255
Handle vfkit connection in goroutine to prevent blocking #4753
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
Reviewer's GuideRefactors the macOS daemon listener to accept vfkit connections in a background goroutine, eliminating blocking behavior during connection setup and updating error handling to log failures asynchronously. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Hey @vyasgun - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
6bbd112
to
93920c1
Compare
Edit: I had some local changes also during testing and after removing those it is working as expected. will wait for @cfergeau for approval and then merge it. |
if cancel != nil { | ||
logging.Warnf("new connection to %s. Closing old connection", constants.UnixgramSocketPath) | ||
cancel() | ||
} |
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.
To avoid the cancel
/ctx
global variables, I’d change unixgramListener
to have an additional return value of type CancelFunc
. The cancellation can then be handled in the code calling vsockListener
.
Even if we keep the code as it is currently, I don’t think ctx
needs to be global, only cancel
.
And not related to this change, but I wonder if unixgramListen
would be a better name for this method instead of unixgramListener
(but just a comment in passing, nothing important)
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.
I split the function into two: setupUnixgramListener
and handleUnixgramConnection
since cancel()
needs to be called once the initial connection is established (transport.AcceptVfkit
returns). The cancellation is now being handled by the caller function.
For the names, what do you think of the two new functions?
6f9d62e
to
e0cd9b6
Compare
CI is failing with following error
|
e0cd9b6
to
409e071
Compare
@praveenkumar these are due to missing duplicate definitions for other OSes. I will add them |
@vyasgun before push make sure you run |
66c0f4b
to
4cb88d0
Compare
The failing test |
@vyasgun: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Moved AcceptVfkit handling into a goroutine to prevent blocking the listener and ensure responsiveness. Added logic to cancel any existing context before accepting a new connection.
Description
A bug is being consistently seen where crc is unable to connect to the daemon on restart/recreation. This fix will help prevent seeing this issue.
Fixes: #4736
Relates to: #4738
Type of change
test, version modification, documentation, etc.)
Proposed changes
Testing
Contribution Checklist
Summary by Sourcery
Bug Fixes: