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

Introduce a client-server model #23

Merged
merged 1 commit into from
Jun 5, 2024
Merged

Introduce a client-server model #23

merged 1 commit into from
Jun 5, 2024

Conversation

slp
Copy link
Collaborator

@slp slp commented Jun 1, 2024

With the introduction of 47fdd45 (Default guest memory to 80% of total RAM on host), we're targeting a use case in which we expect to be running a single, large VM.

To accommodate this use case, we're switching here to a client/server model. The first time krun is launched, in addition to running the command, it starts a small server in the VM listening on a TCP port. On the host side, it also creates a lock file, writes the listening port number to it, and calls flock() on it to prevent other instances from running in parallel.

When it's executed again, krun detects there's another instance running, and contacts the server in the VM to request the launch of the command. The stdout and stderr of the command are redirected to $XDG_RUNTIME_DIR/krun-$COMMAND-$TIMESTAMP.[stdout|stderr]

@slp slp requested a review from teohhanhui as a code owner June 1, 2024 20:50
@slp slp mentioned this pull request Jun 1, 2024
@teohhanhui
Copy link
Collaborator

Any reason for using a TCP port vs a Unix socket? Would vsock not work well for that?

@slp
Copy link
Collaborator Author

slp commented Jun 2, 2024

Any reason for using a TCP port vs a Unix socket? Would vsock not work well for that?

Right now libkrun only supports outgoing (VM -> host) connections with vsock. Implementing it the other way around is doable, but would be quite expensive to make it reliable, specially compared with simply using a TCP socket.

@slp slp force-pushed the server-model branch 2 times, most recently from 911bc06 to f6eebcb Compare June 2, 2024 20:54
@slp slp force-pushed the server-model branch 4 times, most recently from d3edb7e to 26d9205 Compare June 3, 2024 10:47
@teohhanhui
Copy link
Collaborator

I'll need more time to review the rest of the code, if that's alright.

Copy link
Collaborator

@teohhanhui teohhanhui left a comment

Choose a reason for hiding this comment

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

More uniform CLI options...

(If the user decides to introduce a mismatch between krun --server-port and krun-server --port then tough luck...)

crates/krun-server/src/cli_options.rs Show resolved Hide resolved
crates/krun-server/src/cli_options.rs Outdated Show resolved Hide resolved
crates/krun-server/src/cli_options.rs Show resolved Hide resolved
crates/krun-server/src/bin/krun-server.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@teohhanhui teohhanhui left a comment

Choose a reason for hiding this comment

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

Didn't manage to go through all of the code (it's quite a lot), but let's ship it!

With the introduction of 47fdd45 (Default guest memory to 80% of total
RAM on host), we're targeting a use case in which we expect to be
running a single, large VM.

To accommodate this use case, we're switching here to a client/server
model. The first time krun is launched, in addition to running the
command, it starts a small server in the VM listening on a TCP port. On
the host side, it also creates a lock file, writes the listening port
number to it, and calls flock() on it to prevent other instances from
running in parallel.

When it's executed again, krun detects there's another instance running,
and contacts the server in the VM to request the launch of the command.
The stdout and stderr of the command are redirected to
$XDG_RUNTIME_DIR/krun-$COMMAND-$TIMESTAMP.[stdout|stderr]

Co-authored-by: Teoh Han Hui <teohhanhui@gmail.com>
Signed-off-by: Sergio Lopez <slp@sinrega.org>
@slp
Copy link
Collaborator Author

slp commented Jun 5, 2024

Didn't manage to go through all of the code (it's quite a lot), but let's ship it!

Yeah, sorry about that. This turned to be a huge PR basically because we're changing a significant part of the original behavior and UX. I hope we don't need to deal with such a large change anymore. Everything else I have line up (including a juicy one that drastically decreases start-up time ;-) is much smaller.

I've just applied your last suggestions and will be merging this one. Thanks a lot for the review!

@slp slp merged commit 5f613e3 into main Jun 5, 2024
@slp slp deleted the server-model branch June 5, 2024 09:59
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.

2 participants