Use gvproxy notify socket instead of polling for readiness#29048
Conversation
Honny1
force-pushed
the
gvproxy-notify
branch
5 times, most recently
from
4e07dbd to
dba750e
Compare
| gvProxyReadyCtx, gvProxyReadyCancel := context.WithTimeout(context.Background(), machine.GvProxyReadyTimeout) | ||
| defer gvProxyReadyCancel() | ||
| if err := gvProxyNotifier.WaitReady(gvProxyReadyCtx); err != nil { | ||
| logrus.Warnf("gvproxy readiness notification not received: %v; continuing without it", err) |
There was a problem hiding this comment.
should this be a hard error instead?
| } | ||
| c.Args = append(c.Args, "-notification", "unix://"+socketPath) | ||
|
|
||
| logrus.Debugf("gvproxy command-line: %s %s", binary, strings.Join(cmd.ToCmdline(), " ")) |
There was a problem hiding this comment.
this will not catch c.Args in the debug log so it will miss -notification.... is it intended?
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
seems like a good improvement, just some comments one the implementation
| listener, err := net.Listen("unix", socketPath) | ||
| 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.(*net.UnixListener).SetUnlinkOnClose(false) |
There was a problem hiding this comment.
you can avoid the type cast by calling net.ListenUnix() instead which returns net.UnixListener
| select { | ||
| case n.connectedCh <- msg.MacAddress: | ||
| default: | ||
| } |
There was a problem hiding this comment.
unless I am missing something nothing ever consumes connectedCh so why bother with a extra channel. I would just not do anything here
| case gvProxyTypes.ConnectionClosed: | ||
| logrus.Debugf("gvproxy: VM disconnected (mac=%s)", msg.MacAddress) |
There was a problem hiding this comment.
would this make sense to report as error in the error channel, if we see a disconnect even nothing good happened and it likely makes not sense to keep waiting until timeout
| return nil, nil, err | ||
| } | ||
| // Wait on gvproxy to be running and aware | ||
| if err := sockets.WaitForSocketWithBackoffs(gvProxyMaxBackoffAttempts, gvProxyWaitBackoff, gvproxySocket.GetPath(), "gvproxy"); err != nil { |
There was a problem hiding this comment.
there are just two callers of this function which you removed so WaitForSocketWithBackoffs should be removed as well IMO
| logrus.Debugf("notification listener accept error: %v", err) | ||
| return | ||
| } | ||
| go n.handleConnection(ctx, conn) |
There was a problem hiding this comment.
do we ever want to deal with more than one connection at a time? the parsing of the json message seems fast enough that we would not need to spawn another goroutine for it
| if errors.Is(err, net.ErrClosed) { | ||
| return | ||
| } | ||
| logrus.Debugf("notification listener accept error: %v", err) |
There was a problem hiding this comment.
I think this should be a visible error log, if we fail to accept a gvproxy conn we should see it as otherwsie we just timeout on for the event later and then have no way to know why we timed out.
|
/packit retest-failed |
|
@Honny1 Don't blindly rerun machine tests on PRs like that. hyperV seems hard broken
|
Fixes: https://redhat.atlassian.net/browse/RUN-4471 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
|
Blocked by containers/gvisor-tap-vsock#693 |
4 participants
Replace backoff-based polling for gvproxy socket existence with the new
--notificationflag from gvisor-tap-vsock#566. Podman creates a Unix listener and gvproxy sends a JSON ready message when its listeners are up. Faster, no race conditions.Fixes: https://redhat.atlassian.net/browse/RUN-4471
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?