From 64eb8bfeb066882d7d107aae27516a35abeca7bc Mon Sep 17 00:00:00 2001 From: Si Beaumont Date: Thu, 21 Nov 2024 10:33:24 +0000 Subject: [PATCH] tests: Remove asserts on the value of local Vsock CID (#2975) ### Motivation: We have some tests for our Vsock support that only run when we detect the system supports Vsock. These weren't running in CI but, now we've migrated CI to GitHub Actions, they can. In prototyping this though, I discovered that one of our tests was too strict, and will fail for VMs running on Hyper-V, which is the hypervisor used by GitHub actions. Specifically, we have support for getting the local context ID by a channel option. This is implemented by an ioctl, but the semantics of the return value differ between the various Vsock transport kernel modules: - `virtio_transport`: returns the guest CID, which is a number greater than `VMADDR_CID_HOST`, but less than `VMADDR_CID_ANY`, and returns `VMADDR_CID_ANY` as an error. - `vsock_loopback`: returns `VMADDR_CID_LOCAL` always. - `vmci_transport`: returns the guest CID if the guest and host transport are active; `VMADDR_CID_HOST` if only the host transport is active; and `VMCI_INVALID_ID` otherwise, which happens to be the same value as `VMADDR_CID_ANY`. - `hyperv_transport`: returns `VMADDR_CID_ANY` always. For this reason, we should probably remove any attempts to interpret the validity of the value that comes back from the driver and users will need to know what to do with it. ### Modifications: - tests: Only run Vsock echo tests on Linux, when `vsock_loopback` is loaded. - tests: Remove asserts on the value of local Vsock CID. - ci: Add pipeline that runs Vsock tests. ### Result: - Vsock tests will no longer run unless `vsock_loopback` kernel module is loaded. - Vsock tests will no longer fail in Hyper-V VMs. - Vsock tests will now run in CI. --- .github/workflows/pull_request.yml | 18 ++++++++++++++++++ Sources/NIOPosix/VsockAddress.swift | 2 +- .../AsyncChannelBootstrapTests.swift | 2 +- Tests/NIOPosixTests/EchoServerClientTest.swift | 2 +- Tests/NIOPosixTests/TestUtils.swift | 17 ++++------------- Tests/NIOPosixTests/VsockAddressTest.swift | 8 ++++---- 6 files changed, 29 insertions(+), 20 deletions(-) diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 81ac778df8..1e5a3b7340 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -41,3 +41,21 @@ jobs: with: name: "Integration tests" matrix_linux_command: "apt-get update -y -q && apt-get install -y -q lsof dnsutils netcat-openbsd net-tools curl jq && ./scripts/integration_tests.sh" + + vsock-tests: + name: Vsock tests + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + persist-credentials: false + - name: Load vsock_loopback kernel module + run: sudo modprobe vsock_loopback + - name: Build package tests + run: swift build --build-tests + - name: Run Vsock tests + shell: bash # explicitly choose bash, which ensures -o pipefail + run: swift test --filter "(?i)vsock" | tee test.out + - name: Check for skipped tests + run: test -r test.out && ! grep -i skipped test.out diff --git a/Sources/NIOPosix/VsockAddress.swift b/Sources/NIOPosix/VsockAddress.swift index 40b33673ae..437bbba0aa 100644 --- a/Sources/NIOPosix/VsockAddress.swift +++ b/Sources/NIOPosix/VsockAddress.swift @@ -217,7 +217,7 @@ extension VsockAddress.ContextID { /// - On Darwin, the `ioctl()` request operates on a socket. /// - On Linux, the `ioctl()` request operates on the `/dev/vsock` device. /// - /// - Note: On Linux, ``local`` may be a better choice. + /// - Note: The semantics of this `ioctl` vary between vsock transports on Linux; ``local`` may be more suitable. static func getLocalContextID(_ socketFD: NIOBSDSocket.Handle) throws -> Self { #if canImport(Darwin) let request = CNIODarwin_IOCTL_VM_SOCKETS_GET_LOCAL_CID diff --git a/Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift b/Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift index 62dfbbc291..e1dc3d215b 100644 --- a/Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift +++ b/Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift @@ -1141,7 +1141,7 @@ final class AsyncChannelBootstrapTests: XCTestCase { // MARK: VSock func testVSock() async throws { - try XCTSkipUnless(System.supportsVsock, "No vsock transport available") + try XCTSkipUnless(System.supportsVsockLoopback, "No vsock loopback transport available") let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 3) defer { try! eventLoopGroup.syncShutdownGracefully() diff --git a/Tests/NIOPosixTests/EchoServerClientTest.swift b/Tests/NIOPosixTests/EchoServerClientTest.swift index f32c15b284..2be0133d9d 100644 --- a/Tests/NIOPosixTests/EchoServerClientTest.swift +++ b/Tests/NIOPosixTests/EchoServerClientTest.swift @@ -234,7 +234,7 @@ class EchoServerClientTest: XCTestCase { } func testEchoVsock() throws { - try XCTSkipUnless(System.supportsVsock, "No vsock transport available") + try XCTSkipUnless(System.supportsVsockLoopback, "No vsock loopback transport available") let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) diff --git a/Tests/NIOPosixTests/TestUtils.swift b/Tests/NIOPosixTests/TestUtils.swift index c646379156..c2bb121903 100644 --- a/Tests/NIOPosixTests/TestUtils.swift +++ b/Tests/NIOPosixTests/TestUtils.swift @@ -30,19 +30,10 @@ extension System { } } - static var supportsVsock: Bool { - #if canImport(Darwin) || os(Linux) || os(Android) - guard let socket = try? Socket(protocolFamily: .vsock, type: .stream) else { return false } - XCTAssertNoThrow(try socket.close()) - #if !canImport(Darwin) - do { - let fd = try Posix.open(file: "/dev/vsock", oFlag: O_RDONLY | O_CLOEXEC) - try Posix.close(descriptor: fd) - } catch { - return false - } - #endif - return true + static var supportsVsockLoopback: Bool { + #if os(Linux) || os(Android) + guard let modules = try? String(contentsOf: URL(fileURLWithPath: "/proc/modules")) else { return false } + return modules.split(separator: "\n").compactMap({ $0.split(separator: " ").first }).contains("vsock_loopback") #else return false #endif diff --git a/Tests/NIOPosixTests/VsockAddressTest.swift b/Tests/NIOPosixTests/VsockAddressTest.swift index c32924a8bb..6c9f44c101 100644 --- a/Tests/NIOPosixTests/VsockAddressTest.swift +++ b/Tests/NIOPosixTests/VsockAddressTest.swift @@ -58,15 +58,15 @@ class VsockAddressTest: XCTestCase { } func testGetLocalCID() throws { - try XCTSkipUnless(System.supportsVsock) + try XCTSkipUnless(System.supportsVsockLoopback, "No vsock loopback transport available") let socket = try ServerSocket(protocolFamily: .vsock, setNonBlocking: true) defer { try? socket.close() } - // Check the local CID is valid: higher than reserved values, but not VMADDR_CID_ANY. + // Check we can get the local CID using the static property on ContextID. let localCID = try socket.withUnsafeHandle(VsockAddress.ContextID.getLocalContextID) - XCTAssertNotEqual(localCID, .any) - XCTAssertGreaterThan(localCID.rawValue, VsockAddress.ContextID.host.rawValue) + + // Check the local CID from the socket matches. XCTAssertEqual(try socket.getLocalVsockContextID(), localCID) // Check the local CID from the channel option matches.