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

Implement feature toggle for GA forwarding mode in QEMU #3255

Closed
wants to merge 2 commits into from

Conversation

arixmkii
Copy link
Contributor

Fixes #3177

Current code supports 2 modes:

  • forwarding via SSH forwarding
  • forwarding by QEMU via serialport

In GA they are exclusive modes - it either talks to device or exposes Unix socket inside VM.

At some point the GA part was switched to Unix socket only, but QEMU start command line was not updated. SSH forwarder takes care of it by removing Unix socket before starting its own, but there is no point in having it, when it can't be used.

This code introduces feature toggle allowing to use either mode. Keeping the "unstable" option could be beneficial for environments, where Unix socket forwarding is tricky (Windows hosts).

@arixmkii
Copy link
Contributor Author

arixmkii commented Feb 17, 2025

Replaces #3148

Edit: fixed link

@arixmkii arixmkii marked this pull request as draft February 17, 2025 21:40
@arixmkii
Copy link
Contributor Author

arixmkii commented Feb 17, 2025

Converted to Draft. The code looks legit, but I haven't done manual testing, nor I had time to integrate it with my CI builds for Windows.

Publishing it for some early reviews to collect feedback that I'm not doing something absolutely stupid.

Edit. I'm sorry for pushing this with broken unit tests. I'm still getting used to codebase.

@@ -98,6 +98,7 @@ type VMOpts struct {

type QEMUOpts struct {
MinimumVersion *string `yaml:"minimumVersion,omitempty" json:"minimumVersion,omitempty" jsonschema:"nullable"`
VirtioGA *bool `yaml:"virtioGA,omitempty" json:"virtioGA,omitempty" jsonschema:"nullable"`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be guestAgentTransportType *string ?
Also, this may potentially apply to non-QEMU VM drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! I will check if this could be applicable for other platforms. Having it as typed option instead of boolean flag feels like a better design.

Current code supports 2 modes:
- forwarding via SSH forwarding
- forwarding by QEMU via serialport

In GA they are exclusive modes - it either talks to device or exposes
Unix socket inside VM.

At some point the GA part was switched to Unix socket only, but QEMU
start command line was not updated. SSH forwarder takes care
of it by removing Unix socket before starting its own, but there is no
point in having it, when it can't be used.

This code introduces feature toggle allowing to use either mode. Keeping
the "unstable" option could be beneficial for environments, where Unix
socket forwarding is tricky (Windows hosts).

Signed-off-by: Arthur Sengileyev <[email protected]>
@arixmkii
Copy link
Contributor Author

I rebased it, because I believe it still has value before a more complete feature is implemented as requested in review.

I also enabled QEMU integration tests to prove that this actually work (after all previous patches merged already). I had to disable mount tests for QEMU on Windows, because it would need at least these changes:

@arixmkii arixmkii mentioned this pull request Mar 22, 2025
@arixmkii
Copy link
Contributor Author

default-windows.yaml is just a copy of default.yaml with mounts section removed.

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.

QEMU: Support feature toggle for GA via serial port
2 participants