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

Support listen on vsock #202

Closed
wants to merge 1 commit into from
Closed

Conversation

jeffrey4l
Copy link
Contributor

metrics can be retrive without nic between host and qemu guest through vsock.

more info check

@jeffrey4l jeffrey4l force-pushed the master branch 3 times, most recently from 322c7e8 to cc3c96a Compare February 26, 2024 08:26
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea. This also needs to be added to the Go documentation and probably the flag help string in web/kingpinflag/flag.go.

web/tls_config.go Outdated Show resolved Hide resolved
@SuperQ
Copy link
Member

SuperQ commented Feb 26, 2024

Please add documentation to the ListenAndServe func docs (before like 285). A link to https://wiki.qemu.org/Features/VirtioVsock would be useful there.

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good.

With this patch, node_exporter can run with `--web.listen-address=vsock:
//:9100` to listen on vsock in qemu guest. Then host can get the
metrics.

Signed-off-by: Jeffrey Zhang <[email protected]>
@bwplotka
Copy link
Member

👍🏽

@SuperQ
Copy link
Member

SuperQ commented Apr 22, 2024

Would you please rebase to fix the conflict?

Copy link
Member

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I'd like some testing for ListenAndServe here

go func() {
defer func() {
if recover() != nil {
recordConnectionError(errors.New("Panic starting server"))
}
}()
flags := FlagConfig{
WebListenAddresses: &([]string{port}),
WebSystemdSocket: OfBool(false),
WebConfigFile: &test.YAMLConfigPath,
}
err := ListenAndServe(server, &flags, testlogger)
recordConnectionError(err)
}()

To make sure it listens both on tcp and vsock if specified.

But I won't block on this if other people don't think this is necessary 👍

} else {
listener, err = net.Listen("tcp", address)
if err != nil {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be return err? It silently swallows an error condition.

web/kingpinflag/flag.go Show resolved Hide resolved
@Leonid99 Leonid99 mentioned this pull request May 5, 2024
@Leonid99
Copy link
Contributor

Leonid99 commented May 5, 2024

Would you please rebase to fix the conflict?

For your consideration, I rebased this branch as #220 to allow it to be merged (with original authorship of @jeffrey4l). The only conflict is go.mod.

Copy link

@septatrix septatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior can already be archived using the existing systemd socket use case as those are not only able to listen on TCP/IP (AF_INET/AF_INET6) and Unix Domain Sockets (AF_UNIX), but also on Vsock (AF_VSOCK) - among some others.

@SuperQ
Copy link
Member

SuperQ commented Aug 1, 2024

@septatrix

Interesting, so you're saying that this isn't needed? Do we just need to update the documentation?

@septatrix
Copy link

@septatrix

Interesting, so you're saying that this isn't needed? Do we just need to update the documentation?

Yes, at least for systems using systemd (or a compatible implementation of the listener interface). See man:systemd.socket:

If the address string is a string in the format "vsock:x:y", it is read as CID x on a port y address in the AF_VSOCK family. The CID is a unique 32-bit integer identifier in AF_VSOCK analogous to an IP address. Specifying the CID is optional, and may be set to the empty string. "vsock" may be replaced with "vsock-stream", "vsock-dgram" or "vsock-seqpacket" to force usage of the corresponding socket type.

@Leonid99
Copy link
Contributor

Leonid99 commented Aug 2, 2024

that this isn't needed

@SuperQ
I would advocate to merge this change anyway. Using systemd for sockets translation isn't different from other methods of socket translation, such as socat, i.e. it's always possible, for example, expose a socket that listens on a TCP port to local domain sockets, etc., but most network services support listening on both natively, without socat or systemd.

Also, another reminder that a rebased version of this PR exists as #220. (I am not trying to claim credit, I'm just interested in having this merged).

@SuperQ
Copy link
Member

SuperQ commented Aug 2, 2024

Closing in favor of #220

@SuperQ SuperQ closed this Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants