Skip to content

Commit

Permalink
tests: Remove asserts on the value of local Vsock CID (#2975)
Browse files Browse the repository at this point in the history
### 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.
  • Loading branch information
simonjbeaumont authored Nov 21, 2024
1 parent 2a3a333 commit 64eb8bf
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 20 deletions.
18 changes: 18 additions & 0 deletions .github/workflows/pull_request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion Sources/NIOPosix/VsockAddress.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Tests/NIOPosixTests/AsyncChannelBootstrapTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion Tests/NIOPosixTests/EchoServerClientTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
17 changes: 4 additions & 13 deletions Tests/NIOPosixTests/TestUtils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions Tests/NIOPosixTests/VsockAddressTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 64eb8bf

Please sign in to comment.