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

Multiple failures in test_vsock #1934

Closed
asomers opened this issue Dec 9, 2022 · 4 comments · Fixed by #1944
Closed

Multiple failures in test_vsock #1934

asomers opened this issue Dec 9, 2022 · 4 comments · Fixed by #1944

Comments

@asomers
Copy link
Member

asomers commented Dec 9, 2022

When we first added the vsock code in #1091 the test contained an assertion that connecting to the socket would fail, because "The current implementation does not support loopback devices". But it did not check the particular failure mode. A recent change (#1915) has caused connect to start succeeding, in at least some cases. It seems that at least some of the original failures were due to a race condition in the test: it closed a socket too early. Now that that is fixed, we see three behaviors:

  • Fails with EINVAL on Linux x86_64, presumably due to the "does not support loopback devices" part.
  • Passes on most qemu platforms. Perhaps it's connecting to the qemu host?
  • Fails with ENODEV on mips and mips64

See #1917 to view the behavior on each platform.

@stefano-garzarella could you please clarify what we should expect from connect, and fix the test? For now I'm going to disable it so it doesn't hold up other PRs.

asomers added a commit to asomers/nix that referenced this issue Dec 9, 2022
That test has inconsistent behavior on different platforms.  See nix-rust#1934 .
asomers added a commit to asomers/nix that referenced this issue Dec 9, 2022
That test has inconsistent behavior on different platforms.  See nix-rust#1934 .
@stefano-garzarella
Copy link
Contributor

@asomers yep, I'll fix the test, making it more reliable.

About connect, it can succeed if it runs in the host that has also the new vsock_loopback module available.
This module will be automatically loaded if there aren't other vsock transports loaded (e.g. in a host or a guest without any vsock devices).

This will make the test to fail, since now it is able to handle loopback.
The different errno depends on the availability of the device.

I think we should avoid trying the connection, since it can change depending on where the test is running.
So, we should always test a failing scenario, maybe using another port. I don't think our goal is to test that vsock is working, but just the interface.

Something like this:

diff --git a/test/sys/test_socket.rs b/test/sys/test_socket.rs
index 2ab6c54..d3fbbaa 100644
--- a/test/sys/test_socket.rs
+++ b/test/sys/test_socket.rs
@@ -2080,10 +2080,9 @@ pub fn test_vsock() {
         )
         .expect("socket failed");
 
-        let sockaddr_host = VsockAddr::new(cid, port);
+        let sockaddr_host = VsockAddr::new(cid, port + 1);
 
-        // The current implementation does not support loopback devices, so,
-        // for now, we expect a failure on the connect.
+        // We expect a failure on the connect since we are using a wrong port.
         assert_ne!(connect(s2, &sockaddr_host), Ok(()));
 
         close(s2).unwrap();

I'll send a PR ASAP.

@stefano-garzarella
Copy link
Contributor

I'll send a PR ASAP.

@asomers or do you prefer to include these changed in #1915 ?

@asomers
Copy link
Member Author

asomers commented Dec 13, 2022

I don't think our goal is to test that vsock is working, but just the interface.

Yes, definitely. In fact, we don't even need to try connect. We just need to be sure that the address type works.

@asomers or do you prefer to include these changed in #1915 ?

No, let's do it standalone.

@stefano-garzarella
Copy link
Contributor

I don't think our goal is to test that vsock is working, but just the interface.

Yes, definitely. In fact, we don't even need to try connect. We just need to be sure that the address type works.

I agree, I'll remove that part.

@asomers or do you prefer to include these changed in #1915 ?

No, let's do it standalone.

Okay, I'll send it!

stefano-garzarella added a commit to stefano-garzarella/nix that referenced this issue Dec 13, 2022
We mainly provide VsockAddr, so let's try to test well that VsockAddr
mapping to libc::sockaddr_vm is correct.

Let's remove all interactions with the socket, since vsock may or may
not be available in the environment.
Testing socket(), bind(), listen(), connect(), etc. caused unexpected
failures, and it's out of scope of this crate.

So let's simplify the vsock test focussing on VsockAddr.
This should work also on graviton, so let's try to re-enable it.

Fixes nix-rust#1934

Signed-off-by: Stefano Garzarella <[email protected]>
bors bot added a commit that referenced this issue Dec 14, 2022
1944: Rework vsock test r=asomers a=stefano-garzarella

We mainly provide VsockAddr, so let's try to test well that VsockAddr mapping to libc::sockaddr_vm is correct.

Let's remove all interactions with the socket, since vsock may or may not be available in the environment.
Testing socket(), bind(), listen(), connect(), etc. caused unexpected failures, and it's out of scope of this crate.

So let's simplify the vsock test focussing on VsockAddr. This should work also on graviton, so let's try to re-enable it.

Fixes #1934

Signed-off-by: Stefano Garzarella <[email protected]>

Co-authored-by: Stefano Garzarella <[email protected]>
@bors bors bot closed this as completed in 4d31ecf Dec 14, 2022
asomers pushed a commit to asomers/nix that referenced this issue Aug 27, 2023
We mainly provide VsockAddr, so let's try to test well that VsockAddr
mapping to libc::sockaddr_vm is correct.

Let's remove all interactions with the socket, since vsock may or may
not be available in the environment.
Testing socket(), bind(), listen(), connect(), etc. caused unexpected
failures, and it's out of scope of this crate.

So let's simplify the vsock test focussing on VsockAddr.
This should work also on graviton, so let's try to re-enable it.

Fixes nix-rust#1934

Signed-off-by: Stefano Garzarella <[email protected]>
asomers pushed a commit to asomers/nix that referenced this issue Aug 27, 2023
We mainly provide VsockAddr, so let's try to test well that VsockAddr
mapping to libc::sockaddr_vm is correct.

Let's remove all interactions with the socket, since vsock may or may
not be available in the environment.
Testing socket(), bind(), listen(), connect(), etc. caused unexpected
failures, and it's out of scope of this crate.

So let's simplify the vsock test focussing on VsockAddr.
This should work also on graviton, so let's try to re-enable it.

Fixes nix-rust#1934

Signed-off-by: Stefano Garzarella <[email protected]>
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 a pull request may close this issue.

2 participants