-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Use gvproxy notify socket instead of polling for readiness #29048
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
Open
Honny1
wants to merge
1
commit into
podman-container-tools:main
Choose a base branch
from
Honny1:gvproxy-notify
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| package machine | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "net" | ||
| "os" | ||
| "path/filepath" | ||
|
|
||
| gvProxyTypes "github.com/containers/gvisor-tap-vsock/pkg/types" | ||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| const notifySocketName = "gvproxy-notify.sock" | ||
|
|
||
| // GvProxyNotifier listens on a unix socket for JSON notification messages | ||
| // from gvproxy. | ||
| type GvProxyNotifier struct { | ||
| listener *net.UnixListener | ||
| socketPath string | ||
| readyCh chan struct{} | ||
| errorCh chan error | ||
| } | ||
|
|
||
| // SocketPath returns the path to the notification unix socket. | ||
| func (n *GvProxyNotifier) SocketPath() string { | ||
| return n.socketPath | ||
| } | ||
|
|
||
| // NewGvProxyNotifier creates a notification listener socket in the given runtime directory. | ||
| // | ||
| // The socket begins accepting connections at the kernel level immediately upon creation | ||
| // (via net.Listen), so gvproxy can be started and connect before Start() is called without | ||
| // losing messages. The kernel backlog buffers both the connection and any data sent on it | ||
| // until Accept()/Read() consume them. | ||
| // | ||
| // The notifier is only used during machine start. Close() stops the listener but | ||
| // intentionally leaves the socket file on disk because gvproxy remains running after | ||
| // start completes and may still dial the socket for later notifications. The socket | ||
| // file is removed by CleanupGvProxyNotifySocket, which is called from CleanupGVProxy | ||
| // during machine stop after gvproxy has exited. Any stale socket from a previous run | ||
| // is removed at the top of this constructor before creating a new listener. | ||
| func NewGvProxyNotifier(runtimeDir string) (*GvProxyNotifier, error) { | ||
| socketPath := filepath.Join(runtimeDir, notifySocketName) | ||
|
|
||
| if err := os.Remove(socketPath); err != nil && !errors.Is(err, os.ErrNotExist) { | ||
| return nil, fmt.Errorf("removing old notification socket: %w", err) | ||
| } | ||
|
|
||
| listener, err := net.ListenUnix("unix", &net.UnixAddr{Name: socketPath, Net: "unix"}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating notification listener: %w", err) | ||
| } | ||
|
|
||
| // Prevent Close() from removing the socket file. gvproxy may still | ||
| // dial it for notifications after the listener is closed. The file is | ||
| // removed by CleanupGvProxyNotifySocket during machine stop. | ||
| listener.SetUnlinkOnClose(false) | ||
|
|
||
| return &GvProxyNotifier{ | ||
| listener: listener, | ||
| socketPath: socketPath, | ||
| readyCh: make(chan struct{}, 1), | ||
| errorCh: make(chan error, 1), | ||
| }, nil | ||
| } | ||
|
|
||
| // CleanupGvProxyNotifySocket removes the notification socket file from the given runtime directory. | ||
| // | ||
| // This is called from CleanupGVProxy during machine stop, at which point the notifier | ||
| // (which only runs during machine start) has long since closed its listener. | ||
| func CleanupGvProxyNotifySocket(runtimeDir string) { | ||
| socketPath := filepath.Join(runtimeDir, notifySocketName) | ||
| if err := os.Remove(socketPath); err != nil && !errors.Is(err, os.ErrNotExist) { | ||
| logrus.Debugf("failed to remove gvproxy notification socket: %v", err) | ||
| } | ||
| } | ||
|
|
||
| // Close stops the notifier listener. | ||
| // | ||
| // The socket file is intentionally left on disk because gvproxy may still dial it for later notifications | ||
| // (connection_established, connection_closed). | ||
| // The file is cleaned up by CleanupGvProxyNotifySocket after gvproxy exits. | ||
| func (n *GvProxyNotifier) Close() { | ||
| n.listener.Close() | ||
| } | ||
|
|
||
| // Start begins accepting connections and processing notifications. | ||
| // | ||
| // It blocks until the context is canceled or the listener is closed, | ||
| // intended to be run as a goroutine. | ||
| func (n *GvProxyNotifier) Start(ctx context.Context) { | ||
| for { | ||
| conn, err := n.listener.Accept() | ||
| if err != nil { | ||
| if ctx.Err() != nil || errors.Is(err, net.ErrClosed) { | ||
| return | ||
| } | ||
| logrus.Errorf("notification listener accept error: %v", err) | ||
| return | ||
| } | ||
| n.handleConnection(ctx, conn) | ||
| } | ||
| } | ||
|
|
||
| func (n *GvProxyNotifier) handleConnection(ctx context.Context, conn net.Conn) { | ||
| defer conn.Close() | ||
| decoder := json.NewDecoder(conn) | ||
|
|
||
| for { | ||
| if ctx.Err() != nil { | ||
| return | ||
| } | ||
|
|
||
| var msg gvProxyTypes.NotificationMessage | ||
| if err := decoder.Decode(&msg); err != nil { | ||
| if ctx.Err() != nil { | ||
| return | ||
| } | ||
| logrus.Debugf("notification decode error: %v", err) | ||
| return | ||
| } | ||
|
|
||
| logrus.Debugf("gvproxy notification received: type=%s mac=%s", msg.NotificationType, msg.MacAddress) | ||
|
|
||
| switch msg.NotificationType { | ||
| case gvProxyTypes.Ready: | ||
| select { | ||
| case n.readyCh <- struct{}{}: | ||
| default: | ||
| } | ||
| case gvProxyTypes.ConnectionEstablished: | ||
| logrus.Debugf("gvproxy: VM connected (mac=%s)", msg.MacAddress) | ||
| case gvProxyTypes.HypervisorError: | ||
| select { | ||
| case n.errorCh <- fmt.Errorf("gvproxy reported hypervisor error"): | ||
| default: | ||
| } | ||
| case gvProxyTypes.ConnectionClosed: | ||
| select { | ||
| case n.errorCh <- fmt.Errorf("gvproxy: VM disconnected unexpectedly (mac=%s)", msg.MacAddress): | ||
| default: | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // WaitReady blocks until the "ready" notification is received or the context expires. | ||
| func (n *GvProxyNotifier) WaitReady(ctx context.Context) error { | ||
| select { | ||
| case <-n.readyCh: | ||
| return nil | ||
| case err := <-n.errorCh: | ||
| return err | ||
| case <-ctx.Done(): | ||
| return fmt.Errorf("timeout waiting for gvproxy ready notification: %w", ctx.Err()) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| //go:build amd64 || arm64 | ||
|
|
||
| package machine | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "errors" | ||
| "net" | ||
| "os" | ||
| "testing" | ||
| "time" | ||
|
|
||
| gvproxyTypes "github.com/containers/gvisor-tap-vsock/pkg/types" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestGvProxyNotifier_Ready(t *testing.T) { | ||
| dir := t.TempDir() | ||
| notifier, err := NewGvProxyNotifier(dir) | ||
| require.NoError(t, err) | ||
| defer notifier.Close() | ||
|
|
||
| go notifier.Start(t.Context()) | ||
|
|
||
| conn, err := net.Dial("unix", notifier.SocketPath()) | ||
| require.NoError(t, err) | ||
| defer conn.Close() | ||
|
|
||
| err = json.NewEncoder(conn).Encode(gvproxyTypes.NotificationMessage{ | ||
| NotificationType: gvproxyTypes.Ready, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| waitCtx, waitCancel := context.WithTimeout(t.Context(), 2*time.Second) | ||
| defer waitCancel() | ||
| err = notifier.WaitReady(waitCtx) | ||
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestGvProxyNotifier_HypervisorError(t *testing.T) { | ||
| dir := t.TempDir() | ||
| notifier, err := NewGvProxyNotifier(dir) | ||
| require.NoError(t, err) | ||
| defer notifier.Close() | ||
|
|
||
| go notifier.Start(t.Context()) | ||
|
|
||
| conn, err := net.Dial("unix", notifier.SocketPath()) | ||
| require.NoError(t, err) | ||
| defer conn.Close() | ||
|
|
||
| err = json.NewEncoder(conn).Encode(gvproxyTypes.NotificationMessage{ | ||
| NotificationType: gvproxyTypes.HypervisorError, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| waitCtx, waitCancel := context.WithTimeout(t.Context(), 2*time.Second) | ||
| defer waitCancel() | ||
| err = notifier.WaitReady(waitCtx) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "hypervisor error") | ||
| } | ||
|
|
||
| func TestGvProxyNotifier_Timeout(t *testing.T) { | ||
| dir := t.TempDir() | ||
| notifier, err := NewGvProxyNotifier(dir) | ||
| require.NoError(t, err) | ||
| defer notifier.Close() | ||
|
|
||
| go notifier.Start(t.Context()) | ||
|
|
||
| waitCtx, waitCancel := context.WithTimeout(t.Context(), 100*time.Millisecond) | ||
| defer waitCancel() | ||
| err = notifier.WaitReady(waitCtx) | ||
| assert.Error(t, err) | ||
| assert.ErrorIs(t, err, context.DeadlineExceeded) | ||
| } | ||
|
|
||
| func TestCleanupGvProxyNotifySocket(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| notifier, err := NewGvProxyNotifier(dir) | ||
| require.NoError(t, err) | ||
| socketPath := notifier.SocketPath() | ||
| notifier.Close() | ||
|
|
||
| _, err = os.Stat(socketPath) | ||
| assert.NoError(t, err, "socket file should still exist after Close()") | ||
|
|
||
| CleanupGvProxyNotifySocket(dir) | ||
| _, err = os.Stat(socketPath) | ||
| assert.True(t, errors.Is(err, os.ErrNotExist), "socket file should be gone after cleanup") | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
there are just two callers of this function which you removed so WaitForSocketWithBackoffs should be removed as well IMO