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

QEMU: Occupy vsock Context ID through syscall #11993

Merged
merged 2 commits into from
Jul 13, 2023

Conversation

roosterfish
Copy link
Contributor

@roosterfish roosterfish commented Jul 11, 2023

When trying to acquire a new vsock Context ID, we want to occupy it right away to prevent collisions.
This helps for scenarios where two VMs have the same identical volatile.vsock_id (VM export/import) and
in case LXD is used in a nested environment.

If either the acquisition or the syscall fails, the next one gets selected.

Follow up on #11896 and observed during investigation of #11907

@roosterfish roosterfish force-pushed the fix_vsock_startup_race branch from c5072cf to d5401c3 Compare July 11, 2023 10:52
@tomponline tomponline marked this pull request as draft July 11, 2023 12:11
@roosterfish roosterfish force-pushed the fix_vsock_startup_race branch from d5401c3 to 769c415 Compare July 12, 2023 14:31
@roosterfish roosterfish changed the title QEMU: Lock the vsock ID selection QEMU: Occupy vsock Context ID through syscall Jul 12, 2023
@roosterfish roosterfish force-pushed the fix_vsock_startup_race branch from 769c415 to e01abe9 Compare July 12, 2023 14:36
@roosterfish roosterfish marked this pull request as ready for review July 12, 2023 15:19
@roosterfish roosterfish requested a review from tomponline July 12, 2023 15:19
@roosterfish roosterfish force-pushed the fix_vsock_startup_race branch 4 times, most recently from 780b4d4 to e30f707 Compare July 13, 2023 08:01
@roosterfish roosterfish force-pushed the fix_vsock_startup_race branch from e30f707 to 5b7a53b Compare July 13, 2023 08:22
@roosterfish roosterfish force-pushed the fix_vsock_startup_race branch from 5b7a53b to b666277 Compare July 13, 2023 08:56
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Please can you do a full suite of VM tests, so VMs running inside different nested containers, VMs running inside nested VMs etc to check for any regressions.

@roosterfish
Copy link
Contributor Author

roosterfish commented Jul 13, 2023

I have created a small script for testing those scenarios, the outcome is what we expect.

Gist: https://gist.github.com/roosterfish/ea62fe1f7fff2838eb786d4444901c1c
Should I put it somewhere else?

@tomponline
Copy link
Member

@roosterfish you could open a PR on https://github.com/canonical/lxd-ci put it in a new "tests" directory please

@roosterfish roosterfish force-pushed the fix_vsock_startup_race branch from b666277 to 5d81e22 Compare July 13, 2023 13:11
@tomponline
Copy link
Member

@roosterfish did your manual tests pass OK ?

When trying to acquire a new vsock Context ID, we want to occupy it right away to prevent collisions.
This helps for scenarios where two VMs have the same identical volatile.vsock_id (VM export/import) and
in case LXD is used in a nested environment.

If either the acquisition or the syscall fails, the next one gets selected.

Signed-off-by: Julian Pelizäus <[email protected]>
@roosterfish roosterfish force-pushed the fix_vsock_startup_race branch from 5d81e22 to 293f36f Compare July 13, 2023 14:31
@roosterfish
Copy link
Contributor Author

@roosterfish did your manual tests pass OK ?

Yes, all clear.

@tomponline
Copy link
Member

@roosterfish thanks for your thoroughness on picking up these issues and fixing them.

@tomponline tomponline merged commit 1d792a6 into canonical:main Jul 13, 2023
@roosterfish roosterfish deleted the fix_vsock_startup_race branch July 20, 2023 09:06
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.

3 participants