From dd138826bd216463c560a6569453355b0f073013 Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Thu, 21 Sep 2023 15:43:25 +0000 Subject: [PATCH 1/7] test(vsock): extract guest echo server This is to avoid code duplication. Signed-off-by: Nikita Kalyazin --- tests/framework/utils_vsock.py | 11 ++++++++--- tests/integration_tests/functional/test_vsock.py | 9 +++------ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/framework/utils_vsock.py b/tests/framework/utils_vsock.py index eb3756607dd..381d6427169 100644 --- a/tests/framework/utils_vsock.py +++ b/tests/framework/utils_vsock.py @@ -94,6 +94,13 @@ def make_blob(dst_dir, size=BLOB_SIZE): return blob_path, blob_hash.hexdigest() +def start_guest_echo_server(vm): + """Start a vsock echo server in the microVM.""" + cmd = "/tmp/vsock_helper echosrv -d {}".format(ECHO_SERVER_PORT) + ecode, _, stderr = vm.ssh.run(cmd) + assert ecode == 0, stderr + + def check_host_connections(vm, uds_path, blob_path, blob_hash): """Test host-initiated connections. @@ -103,9 +110,7 @@ def check_host_connections(vm, uds_path, blob_path, blob_hash): the hashes they computed for the data echoed back by the server are checked against `blob_hash`. """ - cmd = "/tmp/vsock_helper echosrv -d {}".format(ECHO_SERVER_PORT) - ecode, _, _ = vm.ssh.run(cmd) - assert ecode == 0 + start_guest_echo_server(vm) workers = [] for _ in range(TEST_CONNECTION_COUNT): diff --git a/tests/integration_tests/functional/test_vsock.py b/tests/integration_tests/functional/test_vsock.py index f8940ec7f98..da56ae9fb4d 100644 --- a/tests/integration_tests/functional/test_vsock.py +++ b/tests/integration_tests/functional/test_vsock.py @@ -27,6 +27,7 @@ check_vsock_device, make_blob, make_host_port_path, + start_guest_echo_server, ) NEGATIVE_TEST_CONNECTION_COUNT = 100 @@ -59,9 +60,7 @@ def negative_test_host_connections(vm, uds_path, blob_path, blob_hash): Closes the UDS sockets while data is in flight. """ - cmd = "/tmp/vsock_helper echosrv -d {}".format(ECHO_SERVER_PORT) - ecode, _, _ = vm.ssh.run(cmd) - assert ecode == 0 + start_guest_echo_server(vm) workers = [] for _ in range(NEGATIVE_TEST_CONNECTION_COUNT): @@ -153,9 +152,7 @@ def test_vsock_transport_reset( # Start guest echo server. path = os.path.join(test_vm.jailer.chroot_path(), VSOCK_UDS_PATH) - cmd = f"/tmp/vsock_helper echosrv -d {ECHO_SERVER_PORT}" - ecode, _, _ = test_vm.ssh.run(cmd) - assert ecode == 0 + start_guest_echo_server(test_vm) # Start host workers that connect to the guest server. workers = [] From b24141db7c82fa2de1a260dd071a01bead2680e3 Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Thu, 21 Sep 2023 15:50:12 +0000 Subject: [PATCH 2/7] test(vsock): replace vhost helper echo server with socat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to reduce likelihood of bugs by reducing the amount of hand-written test code, in this case vsock echo server on the guest. See a similar change: commit bfa0dc7b145703ed431d83b352c708b73039ad22 Author: Pablo Barbáchano Date: Wed Jun 7 22:04:38 2023 +0200 test: change the echo server in test_5_snapshots Signed-off-by: Nikita Kalyazin --- tests/framework/utils_vsock.py | 2 +- tests/host_tools/vsock_helper.c | 105 +----------------- .../functional/test_vsock.py | 7 +- 3 files changed, 8 insertions(+), 106 deletions(-) diff --git a/tests/framework/utils_vsock.py b/tests/framework/utils_vsock.py index 381d6427169..bdba5fa9576 100644 --- a/tests/framework/utils_vsock.py +++ b/tests/framework/utils_vsock.py @@ -96,7 +96,7 @@ def make_blob(dst_dir, size=BLOB_SIZE): def start_guest_echo_server(vm): """Start a vsock echo server in the microVM.""" - cmd = "/tmp/vsock_helper echosrv -d {}".format(ECHO_SERVER_PORT) + cmd = f"nohup socat VSOCK-LISTEN:{ECHO_SERVER_PORT},backlog=128,reuseaddr,fork EXEC:'/bin/cat' > /dev/null 2>&1 &" ecode, _, stderr = vm.ssh.run(cmd) assert ecode == 0, stderr diff --git a/tests/host_tools/vsock_helper.c b/tests/host_tools/vsock_helper.c index 0fa1c998bab..00424dd6b6e 100644 --- a/tests/host_tools/vsock_helper.c +++ b/tests/host_tools/vsock_helper.c @@ -2,11 +2,9 @@ // SPDX-License-Identifier: Apache-2.0 // This is a vsock helper tool, used by the Firecracker integration tests, -// to - well - test the virtio vsock device. It can be used to: -// 1. Run a forking echo server, that echoes back any data received from -// a client; and -// 2. Run a vsock echo client, that reads data from STDIN, sends it to an -// echo server, then forwards the server's reply to STDOUT. +// to - well - test the virtio vsock device. It can be used to +// run a vsock echo client, that reads data from STDIN, sends it to an +// echo server, then forwards the server's reply to STDOUT. #include #include @@ -27,12 +25,7 @@ int print_usage() { - fprintf(stderr, "Usage: ./vsock-helper {echosrv [-d] | echo }\n"); - fprintf(stderr, "\n"); - fprintf(stderr, " echosrv start a vsock echo server. The server will accept\n"); - fprintf(stderr, " any incoming connection, and echo back any data\n"); - fprintf(stderr, " received on it.\n"); - fprintf(stderr, " -d can be used to daemonize the server.\n"); + fprintf(stderr, "Usage: ./vsock-helper echo \n"); fprintf(stderr, "\n"); fprintf(stderr, " echo connect to an echo server, listening on CID:port.\n"); fprintf(stderr, " STDIN will be piped through to the echo server, and\n"); @@ -60,66 +53,6 @@ int xfer(int src_fd, int dst_fd) { } -int run_echosrv(uint32_t port) { - - struct sockaddr_vm vsock_addr = { - .svm_family = AF_VSOCK, - .svm_port = port, - .svm_cid = VMADDR_CID_ANY - }; - - int srv_sock = socket(AF_VSOCK, SOCK_STREAM, 0); - if (srv_sock < 0) { - perror("socket()"); - return -1; - } - - int res = bind(srv_sock, (struct sockaddr*)&vsock_addr, sizeof(struct sockaddr_vm)); - if (res) { - perror("bind()"); - return -1; - } - - res = listen(srv_sock, SERVER_ACCEPT_BACKLOG); - if (res) { - perror("listen()"); - return -1; - } - - for (;;) { - struct sockaddr cl_addr; - socklen_t sockaddr_len = sizeof(cl_addr); - int cl_sock = accept(srv_sock, &cl_addr, &sockaddr_len); - if (cl_sock < 0) { - perror("accept()"); - continue; - } - - int pid = fork(); - if (pid < 0) { - perror("fork()"); - close(cl_sock); - continue; - } - - if (!pid) { - int res; - do { - res = xfer(cl_sock, cl_sock); - } while (res > 0); - return res >= 0 ? 0 : -1; - } - - close(cl_sock); - int cstatus; - waitpid(-1, &cstatus, WNOHANG); - printf("New client forked...\n"); - } - - return 0; -} - - int run_echo(uint32_t cid, uint32_t port) { int sock = socket(AF_VSOCK, SOCK_STREAM, 0); @@ -161,36 +94,6 @@ int main(int argc, char **argv) { return print_usage(); } - if (strcmp(argv[1], "echosrv") == 0) { - uint32_t port; - if (strcmp(argv[2], "-d") == 0) { - if (argc < 4) { - return print_usage(); - } - port = atoi(argv[3]); - if (!port) { - return print_usage(); - } - int pid = fork(); - if (pid < 0) return -1; - if (pid) { - printf("Forked vsock echo daemon listening on port %d\n", port); - return 0; - } - setsid(); - close(STDIN_FILENO); - close(STDOUT_FILENO); - close(STDERR_FILENO); - } - else { - port = atoi(argv[2]); - if (!port) { - return print_usage(); - } - } - return run_echosrv(port); - } - if (strcmp(argv[1], "echo") == 0) { if (argc != 4) { return print_usage(); diff --git a/tests/integration_tests/functional/test_vsock.py b/tests/integration_tests/functional/test_vsock.py index da56ae9fb4d..170664fa6e2 100644 --- a/tests/integration_tests/functional/test_vsock.py +++ b/tests/integration_tests/functional/test_vsock.py @@ -4,8 +4,7 @@ In order to test the vsock device connection state machine, these tests will: - Generate a 20MiB random data blob; -- Use `host_tools/vsock_helper.c` to start a listening echo server inside the - guest VM; +- Use `socat` to start a listening echo server inside the guest VM; - Run 50, concurrent, host-initiated connections, each transfering the random blob to and from the guest echo server; - For every connection, check that the data received back from the echo server @@ -173,8 +172,8 @@ def test_vsock_transport_reset( # Whatever we send to the server, it should return the same # value. buf = bytearray("TEST\n".encode("utf-8")) - worker.sock.send(buf) try: + worker.sock.send(buf) # Arbitrary timeout, we set this so the socket won't block as # it shouldn't receive anything. worker.sock.settimeout(0.25) @@ -184,7 +183,7 @@ def test_vsock_transport_reset( assert False, "Connection not closed: response recieved '{}'".format( response.decode("utf-8") ) - except SocketTimeout: + except (SocketTimeout, ConnectionResetError, BrokenPipeError): assert True # Terminate VM. From c2df8fade249dab897788601a022af370edf1ccb Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Thu, 21 Sep 2023 16:02:51 +0000 Subject: [PATCH 3/7] test(vsock): add delay after starting guest echo server This is to avoid a situation where clients do not get reply after they send a connect message, like this: framework/utils_vsock.py:197: in _vsock_connect_to_guest ack_buf = sock.recv(32) E Failed: Timeout >300.0s Signed-off-by: Nikita Kalyazin --- tests/framework/utils_vsock.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/framework/utils_vsock.py b/tests/framework/utils_vsock.py index bdba5fa9576..68b54031573 100644 --- a/tests/framework/utils_vsock.py +++ b/tests/framework/utils_vsock.py @@ -100,6 +100,9 @@ def start_guest_echo_server(vm): ecode, _, stderr = vm.ssh.run(cmd) assert ecode == 0, stderr + # Give the server time to initialise + time.sleep(1) + def check_host_connections(vm, uds_path, blob_path, blob_hash): """Test host-initiated connections. From 126e3da8bcf1f6115c93b03dd55000fcb5325ec5 Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Thu, 21 Sep 2023 16:12:36 +0000 Subject: [PATCH 4/7] test(vsock): avoid running multiple guest echo servers In negative vsock tests, a guest echo server was started both independently and inside check_host_connections() leading to multiple instances running in the guest at the same time. This commit removes starting the server inside check_host_connections(). This makes it inconsistent with check_guest_connections() that still starts a host echo server internally, but removing it is not currenlty warrranted. Signed-off-by: Nikita Kalyazin --- tests/framework/utils_vsock.py | 9 ++++----- .../integration_tests/functional/test_snapshot_basic.py | 4 +++- tests/integration_tests/functional/test_vsock.py | 4 ++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/tests/framework/utils_vsock.py b/tests/framework/utils_vsock.py index 68b54031573..62efb4d9552 100644 --- a/tests/framework/utils_vsock.py +++ b/tests/framework/utils_vsock.py @@ -104,16 +104,14 @@ def start_guest_echo_server(vm): time.sleep(1) -def check_host_connections(vm, uds_path, blob_path, blob_hash): +def check_host_connections(uds_path, blob_path, blob_hash): """Test host-initiated connections. - This will start a daemonized echo server on the guest VM, and then spawn - `TEST_CONNECTION_COUNT` `HostEchoWorker` threads. + This will spawn `TEST_CONNECTION_COUNT` `HostEchoWorker` threads. After the workers are done transferring the data read from `blob_path`, the hashes they computed for the data echoed back by the server are checked against `blob_hash`. """ - start_guest_echo_server(vm) workers = [] for _ in range(TEST_CONNECTION_COUNT): @@ -234,5 +232,6 @@ def check_vsock_device(vm, bin_vsock_path, test_fc_session_root_path, ssh_connec check_guest_connections(vm, path, vm_blob_path, blob_hash) # Test vsock host-initiated connections. + start_guest_echo_server(vm) path = os.path.join(vm.jailer.chroot_path(), VSOCK_UDS_PATH) - check_host_connections(vm, path, blob_path, blob_hash) + check_host_connections(path, blob_path, blob_hash) diff --git a/tests/integration_tests/functional/test_snapshot_basic.py b/tests/integration_tests/functional/test_snapshot_basic.py index de43f0b83a1..f3253aa3167 100644 --- a/tests/integration_tests/functional/test_snapshot_basic.py +++ b/tests/integration_tests/functional/test_snapshot_basic.py @@ -21,6 +21,7 @@ check_host_connections, make_blob, make_host_port_path, + start_guest_echo_server, ) @@ -93,8 +94,9 @@ def test_5_snapshots( ) check_guest_connections(microvm, path, vm_blob_path, blob_hash) # Test vsock host-initiated connections. + start_guest_echo_server(microvm) path = os.path.join(microvm.jailer.chroot_path(), VSOCK_UDS_PATH) - check_host_connections(microvm, path, blob_path, blob_hash) + check_host_connections(path, blob_path, blob_hash) # Check that the root device is not corrupted. check_filesystem(microvm.ssh, "squashfs", "/dev/vda") diff --git a/tests/integration_tests/functional/test_vsock.py b/tests/integration_tests/functional/test_vsock.py index 170664fa6e2..1ae5060115a 100644 --- a/tests/integration_tests/functional/test_vsock.py +++ b/tests/integration_tests/functional/test_vsock.py @@ -78,7 +78,7 @@ def negative_test_host_connections(vm, uds_path, blob_path, blob_hash): # Validate vsock emulation still accepts connections and works # as expected. - check_host_connections(vm, uds_path, blob_path, blob_hash) + check_host_connections(uds_path, blob_path, blob_hash) def test_vsock_epipe(test_microvm_with_api, bin_vsock_path, test_fc_session_root_path): @@ -202,4 +202,4 @@ def test_vsock_transport_reset( # Test host-initiated connections. path = os.path.join(vm2.jailer.chroot_path(), VSOCK_UDS_PATH) - check_host_connections(vm2, path, blob_path, blob_hash) + check_host_connections(path, blob_path, blob_hash) From ee2c4f37a818a71724f42e150e4121c72c464233 Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Thu, 21 Sep 2023 16:17:27 +0000 Subject: [PATCH 5/7] test(vsock): move checking for sigpipe in metrics Move the check that metrics contain an expected sigpipe count closer to the code that triggers the sigpipe signal. This will make it easier to diagnose potential issues related to check_host_connections() logic in the future. Signed-off-by: Nikita Kalyazin --- .../functional/test_vsock.py | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/integration_tests/functional/test_vsock.py b/tests/integration_tests/functional/test_vsock.py index 1ae5060115a..99e273f9e36 100644 --- a/tests/integration_tests/functional/test_vsock.py +++ b/tests/integration_tests/functional/test_vsock.py @@ -76,6 +76,18 @@ def negative_test_host_connections(vm, uds_path, blob_path, blob_hash): # Should fail if Firecracker exited from SIGPIPE handler. assert ecode == 0 + metrics = vm.flush_metrics() + # Validate that at least 1 `SIGPIPE` signal was received. + # Since we are reusing the existing echo server which triggers + # reads/writes on the UDS backend connections, these might be closed + # before a read() or a write() is about to be performed by the emulation. + # The test uses 100 connections it is enough to close at least one + # before write(). + # + # If this ever fails due to 100 closes before read() we must + # add extra tooling that will trigger only writes(). + assert metrics["signals"]["sigpipe"] > 0 + # Validate vsock emulation still accepts connections and works # as expected. check_host_connections(uds_path, blob_path, blob_hash) @@ -105,18 +117,6 @@ def test_vsock_epipe(test_microvm_with_api, bin_vsock_path, test_fc_session_root # are closed with in flight data. negative_test_host_connections(vm, path, blob_path, blob_hash) - metrics = vm.flush_metrics() - # Validate that at least 1 `SIGPIPE` signal was received. - # Since we are reusing the existing echo server which triggers - # reads/writes on the UDS backend connections, these might be closed - # before a read() or a write() is about to be performed by the emulation. - # The test uses 100 connections it is enough to close at least one - # before write(). - # - # If this ever fails due to 100 closes before read() we must - # add extra tooling that will trigger only writes(). - assert metrics["signals"]["sigpipe"] > 0 - def test_vsock_transport_reset( uvm_nano, microvm_factory, bin_vsock_path, test_fc_session_root_path From e17a10b0a3b9180cb1b95a49edba574ac3dd9c4b Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Thu, 21 Sep 2023 16:21:01 +0000 Subject: [PATCH 6/7] test(vsock): use default blob size in epipe test A large blob size (20M) is used in the first part of the test to increase likelihood of triggering the epipe condition. In the second part of the test, a call to check_host_connections() is performed to verify that Firecracker can still operate after receiving a sigpipe. There is no need to use a large blob size in the second part. This commit gets back to the default blob size in the second part of the test to reduce the test duration. Signed-off-by: Nikita Kalyazin --- tests/integration_tests/functional/test_vsock.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration_tests/functional/test_vsock.py b/tests/integration_tests/functional/test_vsock.py index 99e273f9e36..a343e1520bb 100644 --- a/tests/integration_tests/functional/test_vsock.py +++ b/tests/integration_tests/functional/test_vsock.py @@ -89,7 +89,8 @@ def negative_test_host_connections(vm, uds_path, blob_path, blob_hash): assert metrics["signals"]["sigpipe"] > 0 # Validate vsock emulation still accepts connections and works - # as expected. + # as expected. Use the default blob size to speed up the test. + blob_path, blob_hash = make_blob(os.path.dirname(blob_path)) check_host_connections(uds_path, blob_path, blob_hash) From 916495677c854bdd495d09dd5b33482ae47899f7 Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Fri, 22 Sep 2023 09:45:22 +0000 Subject: [PATCH 7/7] test(vsock): make start_guest_echo_server return uds path Calculation of the path is usually done by the calling code anyway. Signed-off-by: Nikita Kalyazin --- tests/framework/utils_vsock.py | 10 +++++++--- .../functional/test_snapshot_basic.py | 3 +-- tests/integration_tests/functional/test_vsock.py | 10 ++++------ 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/framework/utils_vsock.py b/tests/framework/utils_vsock.py index 62efb4d9552..511ef3a8a4e 100644 --- a/tests/framework/utils_vsock.py +++ b/tests/framework/utils_vsock.py @@ -95,7 +95,10 @@ def make_blob(dst_dir, size=BLOB_SIZE): def start_guest_echo_server(vm): - """Start a vsock echo server in the microVM.""" + """Start a vsock echo server in the microVM. + + Returns a UDS path to connect to the server. + """ cmd = f"nohup socat VSOCK-LISTEN:{ECHO_SERVER_PORT},backlog=128,reuseaddr,fork EXEC:'/bin/cat' > /dev/null 2>&1 &" ecode, _, stderr = vm.ssh.run(cmd) assert ecode == 0, stderr @@ -103,6 +106,8 @@ def start_guest_echo_server(vm): # Give the server time to initialise time.sleep(1) + return os.path.join(vm.jailer.chroot_path(), VSOCK_UDS_PATH) + def check_host_connections(uds_path, blob_path, blob_hash): """Test host-initiated connections. @@ -232,6 +237,5 @@ def check_vsock_device(vm, bin_vsock_path, test_fc_session_root_path, ssh_connec check_guest_connections(vm, path, vm_blob_path, blob_hash) # Test vsock host-initiated connections. - start_guest_echo_server(vm) - path = os.path.join(vm.jailer.chroot_path(), VSOCK_UDS_PATH) + path = start_guest_echo_server(vm) check_host_connections(path, blob_path, blob_hash) diff --git a/tests/integration_tests/functional/test_snapshot_basic.py b/tests/integration_tests/functional/test_snapshot_basic.py index f3253aa3167..7975b98d824 100644 --- a/tests/integration_tests/functional/test_snapshot_basic.py +++ b/tests/integration_tests/functional/test_snapshot_basic.py @@ -94,8 +94,7 @@ def test_5_snapshots( ) check_guest_connections(microvm, path, vm_blob_path, blob_hash) # Test vsock host-initiated connections. - start_guest_echo_server(microvm) - path = os.path.join(microvm.jailer.chroot_path(), VSOCK_UDS_PATH) + path = start_guest_echo_server(microvm) check_host_connections(path, blob_path, blob_hash) # Check that the root device is not corrupted. diff --git a/tests/integration_tests/functional/test_vsock.py b/tests/integration_tests/functional/test_vsock.py index a343e1520bb..b3f695ad298 100644 --- a/tests/integration_tests/functional/test_vsock.py +++ b/tests/integration_tests/functional/test_vsock.py @@ -51,7 +51,7 @@ def test_vsock(test_microvm_with_api, bin_vsock_path, test_fc_session_root_path) check_vsock_device(vm, bin_vsock_path, test_fc_session_root_path, vm.ssh) -def negative_test_host_connections(vm, uds_path, blob_path, blob_hash): +def negative_test_host_connections(vm, blob_path, blob_hash): """Negative test for host-initiated connections. This will start a daemonized echo server on the guest VM, and then spawn @@ -59,7 +59,7 @@ def negative_test_host_connections(vm, uds_path, blob_path, blob_hash): Closes the UDS sockets while data is in flight. """ - start_guest_echo_server(vm) + uds_path = start_guest_echo_server(vm) workers = [] for _ in range(NEGATIVE_TEST_CONNECTION_COUNT): @@ -113,10 +113,9 @@ def test_vsock_epipe(test_microvm_with_api, bin_vsock_path, test_fc_session_root # Guest-initiated connections (echo workers) will use this blob. _copy_vsock_data_to_guest(vm.ssh, blob_path, vm_blob_path, bin_vsock_path) - path = os.path.join(vm.jailer.chroot_path(), VSOCK_UDS_PATH) # Negative test for host-initiated connections that # are closed with in flight data. - negative_test_host_connections(vm, path, blob_path, blob_hash) + negative_test_host_connections(vm, blob_path, blob_hash) def test_vsock_transport_reset( @@ -151,8 +150,7 @@ def test_vsock_transport_reset( _copy_vsock_data_to_guest(test_vm.ssh, blob_path, vm_blob_path, bin_vsock_path) # Start guest echo server. - path = os.path.join(test_vm.jailer.chroot_path(), VSOCK_UDS_PATH) - start_guest_echo_server(test_vm) + path = start_guest_echo_server(test_vm) # Start host workers that connect to the guest server. workers = []