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

Use shorter virtiofsd and swtpm socket paths #13320

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Apr 12, 2024

Fixes #12539.
These changes generally include getting the file descriptor of the directory of a socket's file to use an alternative shorter path to the same socket. This same fix is applied during VM start (with disk virtiofsd sockets, qemu config virtiofsd sockets and the swtpm sockets), and when hotplugging a disk on a VM (with disk virtiofsd sockets)

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Apr 12, 2024
@hamistao hamistao force-pushed the issue12539/fix_vm_start_failing_due_to_long_socket_path branch from 4ff7490 to b87e251 Compare April 12, 2024 03:04
@github-actions github-actions bot removed Documentation Documentation needs updating API Changes to the REST API labels Apr 12, 2024
@canonical canonical deleted a comment from github-actions bot Apr 12, 2024
lxd/instance/drivers/driver_qemu.go Outdated Show resolved Hide resolved
lxd/instance/drivers/driver_qemu.go Outdated Show resolved Hide resolved
lxd/instance/drivers/driver_qemu_templates.go Outdated Show resolved Hide resolved
@tomponline
Copy link
Member

Has the idea of passing an FD been proved to work? Before we get too bogged down in code style etc?

@hamistao
Copy link
Contributor Author

hamistao commented Apr 12, 2024

Has the idea of passing an FD been proved to work? Before we get too bogged down in code style etc?

No, it hasn't. For now I ask we focus our discussions in either understanding the problem with this approach or suggesting alternatives.

@hamistao hamistao force-pushed the issue12539/fix_vm_start_failing_due_to_long_socket_path branch from b87e251 to c0bd203 Compare April 12, 2024 14:06
@roosterfish
Copy link
Contributor

Another thing you could try is experimenting with the server setting for the chardev as this is set to true by default and I am not sure if qemu isn't the client in this case?

See https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-1240.

@hamistao hamistao force-pushed the issue12539/fix_vm_start_failing_due_to_long_socket_path branch 2 times, most recently from a6c22cd to 241024d Compare May 17, 2024 12:07
@hamistao hamistao marked this pull request as ready for review May 17, 2024 12:56
@hamistao hamistao requested a review from tomponline as a code owner May 17, 2024 12:56
@hamistao hamistao marked this pull request as draft May 17, 2024 22:30
@hamistao hamistao changed the title Use a file descriptor instead of the socket path in qemu.config Use shorter socket paths May 20, 2024
@hamistao hamistao changed the title Use shorter socket paths Use shorter virtiofsd socket paths May 20, 2024
@tomponline
Copy link
Member

@hamistao once this is ready for review please can you mark it as ready for review and let me know. Thanks

@hamistao
Copy link
Contributor Author

@tomponline I found that the same problem was ocurring in other situations(when attaching a device to a running instance and when adding a tpm device) so I changed back to draft to include those additional fixes, will be pushing the next version in a few minutes.

@hamistao hamistao force-pushed the issue12539/fix_vm_start_failing_due_to_long_socket_path branch 5 times, most recently from f9e0f2c to 12900de Compare May 23, 2024 11:47
@hamistao hamistao marked this pull request as ready for review May 23, 2024 15:45
@hamistao hamistao requested a review from roosterfish May 23, 2024 15:45
@hamistao hamistao force-pushed the issue12539/fix_vm_start_failing_due_to_long_socket_path branch from 12900de to 871a419 Compare May 23, 2024 15:53
lxd/util/sys.go Outdated Show resolved Hide resolved
@tomponline
Copy link
Member

How did you get on with this @hamistao ?

@hamistao
Copy link
Contributor Author

hamistao commented Jun 10, 2024

@tomponline The approach of passing the file descriptor to QEMU works but is considerably more complicated than the current apporach so that part will remain the same. All the rest remains as discussed and I will be pushing the changes in a few minutes.

@tomponline
Copy link
Member

The approach of passing the file descriptor to QEMU works but is considerably more complicated than the current apporach so that withh remain the same. All the rest remais as discussed and I will be pushing the changes in a few minutes.

Please can I have some more details on this, also specifically what are you referring to?

@hamistao
Copy link
Contributor Author

hamistao commented Jun 10, 2024

@tomponline Sure! I tried passing a file descriptor to the socket file instead of the path when calling swtpm socket to make the code simpler and remove util.ShortenedSocketPath from TPM creation.
But since the file descriptor that swtpm receives must be to a socket file, it is needed to open the socket before calling swtpm socket and thus util.ShortenedSocketPath is still needed. This is why I stuck to the current approach.
If this wasn't clear enough please let me know.

tomponline
tomponline previously approved these changes Jun 10, 2024
@hamistao hamistao force-pushed the issue12539/fix_vm_start_failing_due_to_long_socket_path branch from 97e9be4 to 5c1750a Compare June 10, 2024 10:56
@tomponline
Copy link
Member

@hamistao i was just about to merge this and but you've pushed a change?

@hamistao
Copy link
Contributor Author

hamistao commented Jun 10, 2024

@tomponline yes, the pushed changes just include the O_PATH approach and the switch from ShortenedSocketPath to ShornetedFilePath, as discussed.

@tomponline
Copy link
Member

OK so ill re review again

@hamistao
Copy link
Contributor Author

@tomponline Sorry if there was some confusion. If those changes aren't needed please let me know so I can remove them.

lxd/util/sys.go Outdated Show resolved Hide resolved
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
…t paths

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao hamistao force-pushed the issue12539/fix_vm_start_failing_due_to_long_socket_path branch from 5c1750a to 0dd71dd Compare June 10, 2024 11:11
@hamistao
Copy link
Contributor Author

@tomponline done!

@tomponline
Copy link
Member

Thanks!

@tomponline tomponline merged commit 8aae96f into canonical:main Jun 10, 2024
27 of 28 checks passed
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.

Failure to start LXD VM due to long UNIX Socket Path
3 participants