tests: microvm: consolidate microvm paths and chroot

Currently the dir structure looks like:

    /srv/.../microvm-dir/chroot

and we tend to create files in microvm-dir and then hardlink them into
the chroot dir.

While we are simplifying it, also do away with chroot/path/fsfiles and
convert more strings to `Path`.

Signed-off-by: Pablo Barbáchano <[email protected]>

Author: pb8o

tests/README.md

@@ -349,25 +349,6 @@ pass for the PR to be merged.
 Add a new function annotated with `#[test]` in
 [`integration_tests.rs`](../src/vmm/tests/integration_tests.rs).
 
-## Working With Guest Files
-
-There are helper methods for writing to and reading from a guest filesystem. For
-example, to overwrite the guest init process and later extract a log:
-
-```python
-def test_with_any_microvm_and_my_init(test_microvm_any):
-    # [...]
-    test_microvm_any.slot.fsfiles['mounted_root_fs'].copy_to(my_init, 'sbin/')
-    # [...]
-    test_microvm_any.slot.fsfiles['mounted_root_fs'].copy_from('logs/', 'log')
-```
-
-`copy_to()` source paths are relative to the host root and destination paths are
-relative to the `mounted_root_fs` root. Vice versa for `copy_from()`.
-
-Copying files to/from a guest file system while the guest is running results in
-undefined behavior.
-
 ## Example Manual Testrun
 
 Running on an EC2 `.metal` instance with an `Amazon Linux 2` AMI:

tests/framework/http_api.py

@@ -97,7 +97,7 @@ class Api:
 
     def __init__(self, api_usocket_full_name):
         self.socket = api_usocket_full_name
-        url_encoded_path = urllib.parse.quote_plus(api_usocket_full_name)
+        url_encoded_path = urllib.parse.quote_plus(str(self.socket))
         self.endpoint = DEFAULT_SCHEME + url_encoded_path
         self.session = Session()
 

tests/framework/jailer.py

@@ -25,21 +25,6 @@ class JailerContext:
     Each microvm will have a jailer configuration associated with it.
     """
 
-    # Keep in sync with parameters from code base.
-    jailer_id = None
-    exec_file = None
-    uid = None
-    gid = None
-    chroot_base = None
-    daemonize = None
-    new_pid_ns = None
-    extra_args = None
-    api_socket_name = None
-    cgroups = None
-    resource_limits = None
-    cgroup_ver = None
-    parent_cgroup = None
-
     def __init__(
         self,
         jailer_id,
@@ -64,11 +49,12 @@ def __init__(
         further adjusted by each test even with None values.
         """
         self.jailer_id = jailer_id
-        assert jailer_id is not None
+        assert jailer_id
         self.jailer_bin_path = jailer_binary_path
         self.exec_file = exec_file
         self.uid = uid
         self.gid = gid
+        assert chroot_base is not None
         self.chroot_base = Path(chroot_base)
         self.netns = netns
         self.daemonize = daemonize
@@ -79,7 +65,6 @@ def __init__(
         self.resource_limits = resource_limits
         self.cgroup_ver = cgroup_ver
         self.parent_cgroup = parent_cgroup
-        assert chroot_base is not None
 
     # Disabling 'too-many-branches' warning for this function as it needs to
     # check every argument, so the number of branches will increase
@@ -95,8 +80,7 @@ def construct_param_list(self):
         jailer_param_list = [str(self.jailer_bin_path)]
 
         # Pretty please, try to keep the same order as in the code base.
-        if self.jailer_id is not None:
-            jailer_param_list.extend(["--id", str(self.jailer_id)])
+        jailer_param_list.extend(["--id", str(self.jailer_id)])
         if self.exec_file is not None:
             jailer_param_list.extend(["--exec-file", str(self.exec_file)])
         if self.uid is not None:
@@ -134,17 +118,14 @@ def construct_param_list(self):
 
     # pylint: enable=too-many-branches
 
-    def chroot_base_with_id(self):
-        """Return the MicroVM chroot base + MicroVM ID."""
-        return self.chroot_base / Path(self.exec_file).name / self.jailer_id
+    @property
+    def chroot(self):
+        """Return where the jailer will place the chroot"""
+        return self.chroot_base / self.exec_file.name / self.jailer_id / "root"
 
     def api_socket_path(self):
         """Return the MicroVM API socket path."""
-        return os.path.join(self.chroot_path(), self.api_socket_name)
-
-    def chroot_path(self):
-        """Return the MicroVM chroot path."""
-        return self.chroot_base_with_id() / "root"
+        return self.chroot / self.api_socket_name
 
     def jailed_path(self, file_path, create=False, subdir="."):
         """Create a hard link or block special device owned by uid:gid.
@@ -154,17 +135,16 @@ def jailed_path(self, file_path, create=False, subdir="."):
         valid within the jail.
         """
         file_path = Path(file_path)
-        chroot_path = Path(self.chroot_path())
-        global_p = chroot_path / subdir / file_path.name
+        global_p = self.chroot / subdir / file_path.name
         global_p.parent.mkdir(parents=True, exist_ok=True)
         jailed_p = Path("/") / subdir / file_path.name
-        if create:
+        if create and not global_p.exists():
             stat_src = file_path.stat()
             if file_path.is_block_device():
                 perms = stat.S_IRUSR | stat.S_IWUSR
                 os.mknod(global_p, mode=stat.S_IFBLK | perms, device=stat_src.st_rdev)
             else:
-                stat_dst = chroot_path.stat()
+                stat_dst = self.chroot.stat()
                 if stat_src.st_dev == stat_dst.st_dev:
                     # if they are in the same device, hardlink
                     global_p.unlink(missing_ok=True)
@@ -173,14 +153,14 @@ def jailed_path(self, file_path, create=False, subdir="."):
                     # otherwise, copy
                     shutil.copyfile(file_path, global_p)
 
-            os.chown(global_p, self.uid, self.gid)
+        os.chown(global_p, self.uid, self.gid)
         return str(jailed_p)
 
     def setup(self):
         """Set up this jailer context."""
-        os.makedirs(self.chroot_base, exist_ok=True)
+        os.makedirs(self.chroot, exist_ok=True)
         # Copy the /etc/localtime file in the jailer root
-        self.jailed_path("/etc/localtime", subdir="etc")
+        self.jailed_path("/etc/localtime", create=True, subdir="etc")
 
     def cleanup(self):
         """Clean up this jailer context."""
@@ -228,30 +208,25 @@ def _kill_cgroup_tasks(self, controller):
         disappears. The retry function that calls this code makes
         sure we do not timeout.
         """
-        # pylint: disable=subprocess-run-check
-        tasks_file = "/sys/fs/cgroup/{}/{}/{}/tasks".format(
-            controller, FC_BINARY_NAME, self.jailer_id
+        tasks_file = Path(
+            f"/sys/fs/cgroup/{controller}/{FC_BINARY_NAME}/{self.jailer_id}/tasks"
         )
 
         # If tests do not call start on machines, the cgroups will not be
         # created.
-        if not os.path.exists(tasks_file):
+        if not tasks_file.exists():
             return True
 
-        cmd = "cat {}".format(tasks_file)
-        result = utils.check_output(cmd)
-
-        tasks_split = result.stdout.splitlines()
-        for task in tasks_split:
-            if os.path.exists("/proc/{}".format(task)):
+        for task in tasks_file.read_text(encoding="ascii").splitlines():
+            if Path(f"/proc/{task}").exists():
                 raise TimeoutError
         return True
 
     @property
     def pid(self):
         """Return the PID of the jailed process"""
         # Read the PID stored inside the file.
-        pid_file = Path(self.chroot_path()) / (self.exec_file.name + ".pid")
+        pid_file = self.chroot / (self.exec_file.name + ".pid")
         if not pid_file.exists():
             return None
         return int(pid_file.read_text(encoding="ascii"))

tests/framework/microvm.py

@@ -380,14 +380,6 @@ def firecracker_version(self):
         _, stdout, _ = utils.check_output(f"{self.fc_binary_path} --version")
         return re.match(r"^Firecracker v(.+)", stdout.partition("\n")[0]).group(1)
 
-    @property
-    def path(self):
-        """Return the path on disk used that represents this microVM."""
-        return self.jailer.chroot_base_with_id()
-
-    # some functions use this
-    fsfiles = path
-
     @property
     def id(self):
         """Return the unique identifier of this microVM."""
@@ -462,14 +454,16 @@ def create_jailed_resource(self, path):
         """Create a hard link to some resource inside this microvm."""
         return self.jailer.jailed_path(path, create=True)
 
-    def get_jailed_resource(self, path):
-        """Get the relative jailed path to a resource."""
+    def jail_path(self, path):
+        """Get the relative jailed path to a resource.
+
+        Also fix permissions if needed"""
         return self.jailer.jailed_path(path, create=False)
 
     @property
     def chroot(self):
         """Get the chroot of this microVM."""
-        return self.jailer.chroot_path()
+        return self.jailer.chroot
 
     def pin_vmm(self, cpu_id: int) -> bool:
         """Pin the firecracker process VMM thread to a cpu list."""
@@ -544,23 +538,24 @@ def spawn(
         self.api = Api(self.jailer.api_socket_path())
 
         if log_file is not None:
-            self.log_file = Path(self.path) / log_file
+            self.log_file = self.chroot / log_file
             self.log_file.touch()
-            self.create_jailed_resource(self.log_file)
             # The default value for `level`, when configuring the logger via cmd
             # line, is `Info`. We set the level to `Debug` to also have the boot
             # time printed in the log.
-            self.jailer.extra_args.update({"log-path": log_file, "level": log_level})
+            self.jailer.extra_args["log-path"] = self.jail_path(log_file)
+            self.jailer.extra_args["level"] = log_level
             if log_show_level:
                 self.jailer.extra_args["show-level"] = None
             if log_show_origin:
                 self.jailer.extra_args["show-log-origin"] = None
 
         if metrics_path is not None:
-            self.metrics_file = Path(self.path) / metrics_path
+            self.metrics_file = self.chroot / metrics_path
             self.metrics_file.touch()
-            self.create_jailed_resource(self.metrics_file)
-            self.jailer.extra_args.update({"metrics-path": self.metrics_file.name})
+            self.jailer.extra_args["metrics-path"] = self.jail_path(
+                self.metrics_file.name
+            )
         else:
             assert not emit_metrics
 
@@ -784,9 +779,9 @@ def patch_drive(self, drive_id, file=None):
         if file:
             self.api.drive.patch(
                 drive_id=drive_id,
-                path_on_host=self.create_jailed_resource(file.path),
+                path_on_host=self.create_jailed_resource(file),
             )
-            self.disks[drive_id] = Path(file.path)
+            self.disks[drive_id] = Path(file)
         else:
             self.api.drive.patch(drive_id=drive_id)
 
@@ -942,7 +937,7 @@ def ssh_iface(self, iface_idx=0):
             ssh_key=self.ssh_key,
             user="root",
             host=guest_ip,
-            control_path=Path(self.chroot()) / f"ssh-{iface_idx}.sock",
+            control_path=self.chroot / f"ssh-{iface_idx}.sock",
             on_error=self._dump_debug_information,
         )
         self._connections.append(connection)
@@ -1031,7 +1026,7 @@ def build(self, kernel=None, rootfs=None, **kwargs):
             # copy only iff not a read-only rootfs
             rootfs_path = rootfs
             if rootfs_path.suffix != ".squashfs":
-                rootfs_path = Path(vm.path) / rootfs.name
+                rootfs_path = vm.chroot / rootfs.name
                 shutil.copyfile(rootfs, rootfs_path)
             vm.rootfs_file = rootfs_path
             vm.ssh_key = ssh_key
@@ -1049,9 +1044,9 @@ def kill(self):
         for vm in self.vms:
             vm.kill()
             vm.jailer.cleanup()
-            chroot_base_with_id = vm.jailer.chroot_base_with_id()
-            if len(vm.jailer.jailer_id) > 0 and chroot_base_with_id.exists():
-                shutil.rmtree(chroot_base_with_id)
+            chroot = vm.jailer.chroot
+            if len(vm.jailer.jailer_id) > 0 and chroot.exists():
+                shutil.rmtree(chroot)
             vm.netns.cleanup()
 
         self.vms.clear()

tests/framework/utils_vsock.py

@@ -107,7 +107,7 @@ 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)
+    return vm.chroot / VSOCK_UDS_PATH
 
 
 def check_host_connections(uds_path, blob_path, blob_hash):
@@ -156,7 +156,7 @@ def check_guest_connections(vm, server_port_path, blob_path, blob_hash):
 
         # Link the listening Unix socket into the VM's jail, so that
         # Firecracker can connect to it.
-        vm.create_jailed_resource(server_port_path)
+        vm.jail_path(server_port_path)
 
         # Increase maximum process count for the ssh service.
         # Avoids: "bash: fork: retry: Resource temporarily unavailable"
@@ -205,7 +205,7 @@ def make_host_port_path(uds_path, port):
 def _vsock_connect_to_guest(uds_path, port):
     """Return a Unix socket, connected to the guest vsock port `port`."""
     sock = socket(AF_UNIX, SOCK_STREAM)
-    sock.connect(uds_path)
+    sock.connect(str(uds_path))
 
     buf = bytearray("CONNECT {}\n".format(port).encode("utf-8"))
     sock.send(buf)
@@ -238,7 +238,7 @@ def check_vsock_device(vm, bin_vsock_path, test_fc_session_root_path, ssh_connec
     _copy_vsock_data_to_guest(ssh_connection, blob_path, vm_blob_path, bin_vsock_path)
 
     # Test vsock guest-initiated connections.
-    path = os.path.join(vm.path, make_host_port_path(VSOCK_UDS_PATH, ECHO_SERVER_PORT))
+    path = vm.chroot / make_host_port_path(VSOCK_UDS_PATH, ECHO_SERVER_PORT)
     check_guest_connections(vm, path, vm_blob_path, blob_hash)
 
     # Test vsock host-initiated connections.

tests/host_tools/drive.py

@@ -31,7 +31,7 @@ def __init__(self, path: str = None, size: int = 256, fs_format: str = "ext4"):
         if fs_format not in self.KNOWN_FILEFS_FORMATS:
             raise ValueError("Format not in: + " + str(self.KNOWN_FILEFS_FORMATS))
         # Here we append the format as a
-        path = os.path.join(path + "." + fs_format)
+        path = os.path.join(str(path) + "." + fs_format)
 
         if os.path.isfile(path):
             raise FileExistsError("File already exists: " + path)