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

Revert virtserialport back to unix socket for QEMU guest agent communication #2112

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

jandubois
Copy link
Member

The serial port sometimes doesn't seem to work: #2064.

The vsock implementation used for VZ doesn't seem to have this problem.

It makes sense to revert to the old mechanism for QEMU until the serialport has been fixed (if possible).

Just commenting out the virtioPort will fall back to the old socket method thanks to #2006.

@jandubois jandubois requested a review from a team January 3, 2024 20:16
@@ -128,7 +128,8 @@ func New(instName string, stdout io.Writer, sigintCh chan os.Signal, opts ...Opt
}
vSockPort = port
} else if *y.VMType == limayaml.QEMU {
virtioPort = filenames.VirtioPort
// virtserialport doesn't seem to work reliably: https://github.com/lima-vm/lima/issues/2064
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what exactly I would report; I have no more information beyond that the code in Lima sometimes fails. It doesn't even fail reliably, which is why the bisect script restarts the VM up to 100 times before accepting that the code works properly.

I haven't checked if the problem happens on the sending or the receiving side, as we are not getting any errors.

So I feel this would need some more investigation before reporting to upstream, but unfortunately I'm out of time right now. 😞

@AkihiroSuda AkihiroSuda added this to the v0.20.0 milestone Jan 3, 2024
AkihiroSuda
AkihiroSuda previously approved these changes Jan 3, 2024
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda
Copy link
Member

Shall we release a new version after merging this?
(v0.19.2 ? v0.20 ?)

@jandubois jandubois marked this pull request as draft January 3, 2024 22:05
@jandubois
Copy link
Member Author

Converted to draft because it only works properly with Ubuntu, but not with Alpine. Will investigate...

…ication

The serial port sometimes doesn't seem to work: lima-vm#2064

Signed-off-by: Jan Dubois <jan.dubois@suse.com>
@jandubois
Copy link
Member Author

it only works properly with Ubuntu, but not with Alpine

I think I fixed it now (just some quoting issues in /etc/init.d/lima-guestagent), but will leave it as draft until I have done more testing.

@jandubois jandubois marked this pull request as ready for review January 3, 2024 23:51
@jandubois
Copy link
Member Author

will leave it as draft until I have done more testing

Seems to work fine with Alpine as well now.

Shall we release a new version after merging this?

I think so, as running into the flakiness is annoying, and not immediately obvious if it is your fault or not. Don't really care about the version 0.19.2 vs. 0.20.0; either one works for me.

@@ -128,7 +128,8 @@ func New(instName string, stdout io.Writer, sigintCh chan os.Signal, opts ...Opt
}
vSockPort = port
} else if *y.VMType == limayaml.QEMU {
virtioPort = filenames.VirtioPort
// virtserialport doesn't seem to work reliably: https://github.com/lima-vm/lima/issues/2064
virtioPort = "" // filenames.VirtioPort
Copy link
Member Author

Choose a reason for hiding this comment

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

Re-assigning the empty string here to satisfy the linter; couldn't get it to shut up blathering about empty branches and empty blocks if I left everything commented out...

@AkihiroSuda
Copy link
Member

@balajiv113 LGTY?

Copy link
Member

@balajiv113 balajiv113 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants