test: clean up network interfaces and SSH helper

Clean up the entwined network interfaces and SSH configuration. Now
those two things should be decoupled and more clear to understand.

Signed-off-by: Pablo Barbáchano <pablob@amazon.com>

Author: pb8o

tests/conftest.py

@@ -385,7 +385,7 @@ def build(self, kernel=None, rootfs=None, **kwargs):
                 # TBD only iff ext4 / rw
                 shutil.copyfile(rootfs_path, rootfs_path2)
                 vm.rootfs_file = rootfs_path2
-                vm.ssh_config["ssh_key_path"] = rootfs.ssh_key().local_path()
+                vm.ssh_key = rootfs.ssh_key().local_path()
             return vm
 
         def kill(self):

tests/framework/artifacts.py

@@ -444,11 +444,6 @@ def net_ifaces(self):
 DEFAULT_NETMASK = 30
 
 
-def create_net_devices_configuration(num):
-    """Define configuration for the requested number of net devices."""
-    return [NetIfaceConfig.with_id(i) for i in range(num)]
-
-
 @dataclass(frozen=True, repr=True)
 class NetIfaceConfig:
     """Defines a network interface configuration."""

tests/framework/builder.py

@@ -124,8 +124,8 @@ def build(
 
         # Download ssh key into the microvm root.
         ssh_key.download(vm.path)
-        vm.ssh_config["ssh_key_path"] = ssh_key.local_path()
-        os.chmod(vm.ssh_config["ssh_key_path"], 0o400)
+        vm.ssh_key = ssh_key.local_path()
+        os.chmod(vm.ssh_key, 0o400)
 
         # Provide a default network configuration.
         if net_ifaces is None or len(net_ifaces) == 0:
@@ -135,18 +135,7 @@ def build(
 
         # Configure network interfaces using artifacts.
         for iface in ifaces:
-            vm.create_tap_and_ssh_config(
-                host_ip=iface.host_ip,
-                guest_ip=iface.guest_ip,
-                netmask_len=iface.netmask,
-                tapname=iface.tap_name,
-            )
-            response = vm.network.put(
-                iface_id=iface.dev_name,
-                host_dev_name=iface.tap_name,
-                guest_mac=iface.guest_mac,
-            )
-            assert vm.api_session.is_status_no_content(response.status_code)
+            vm.add_net_iface(iface)
 
         with open(config.local_path(), encoding="utf-8") as microvm_config_file:
             microvm_config = json.load(microvm_config_file)
@@ -244,16 +233,11 @@ def build_from_snapshot(
                 else vm.create_jailed_resource(disk)
             )
 
-        vm.ssh_config["ssh_key_path"] = snapshot.ssh_key.local_path()
+        vm.ssh_key = snapshot.ssh_key.local_path()
 
         # Create network interfaces.
         for iface in snapshot.net_ifaces:
-            vm.create_tap_and_ssh_config(
-                host_ip=iface.host_ip,
-                guest_ip=iface.guest_ip,
-                netmask_len=iface.netmask,
-                tapname=iface.tap_name,
-            )
+            vm.add_net_iface(iface, api=False)
 
         full_fc_version = vm.version.get_from_api().json()["firecracker_version"]
         if utils.compare_dirty_versions(full_fc_version, "1.0.0") > 0:
@@ -278,12 +262,6 @@ def build_from_snapshot(
             )
         status_ok = vm.api_session.is_status_no_content(response.status_code)
 
-        # Verify response status and cleanup if needed before assert.
-        if not status_ok:
-            # Destroy VM here before we assert.
-            vm.kill()
-            del vm
-
         assert status_ok, response.text
 
         # Return a resumed microvm.

tests/framework/microvm.py

@@ -18,7 +18,7 @@
 import uuid
 import weakref
 from collections import namedtuple
-from functools import cached_property
+from functools import lru_cache
 from pathlib import Path
 from threading import Lock
 from typing import Optional
@@ -30,6 +30,7 @@
 import host_tools.memory as mem_tools
 import host_tools.network as net_tools
 from framework import utils
+from framework.artifacts import NetIfaceConfig
 from framework.defs import FC_PID_FILE_NAME, MAX_API_CALL_DURATION_MS
 from framework.http import Session
 from framework.jailer import JailerContext
@@ -157,15 +158,6 @@ def __init__(
         # Initalize memory monitor
         self.memory_monitor = None
 
-        # The ssh config dictionary is populated with information about how
-        # to connect to a microVM that has ssh capability. The path of the
-        # private key is populated by microvms with ssh capabilities and the
-        # hostname is set from the MAC address used to configure the microVM.
-        self._ssh_config = {
-            "username": "root",
-            "netns_file_path": self.jailer.netns_file_path(),
-        }
-
         # iface dictionary
         self.iface = {}
 
@@ -319,11 +311,6 @@ def log_data(self):
             log_data = self.__log_data
         return log_data
 
-    @property
-    def ssh_config(self):
-        """Get the ssh configuration used to ssh into some microVMs."""
-        return self._ssh_config
-
     @property
     def state(self):
         """Get the InstanceInfo property and return the state field."""
@@ -714,82 +701,31 @@ def patch_drive(self, drive_id, file):
         )
         assert self.api_session.is_status_no_content(response.status_code)
 
-    def ssh_network_config(
-        self,
-        network_config,
-        iface_id,
-        tx_rate_limiter=None,
-        rx_rate_limiter=None,
-        tapname=None,
-    ):
-        """Create a host tap device and a guest network interface.
-
-        'network_config' is used to generate 2 IPs: one for the tap device
-        and one for the microvm. Adds the hostname of the microvm to the
-        ssh_config dictionary.
-        :param network_config: UniqueIPv4Generator instance
-        :param iface_id: the interface id for the API request
-        the guest on this interface towards the MMDS address are
-        intercepted and processed by the device model.
-        :param tx_rate_limiter: limit the tx rate
-        :param rx_rate_limiter: limit the rx rate
-        :return: an instance of the tap which needs to be kept around until
-        cleanup is desired, the configured guest and host ips, respectively.
-        """
-        # Create tap before configuring interface.
-        tapname = tapname or (self.id[:8] + "tap" + iface_id)
-        # The guest is hardcoded to expect an IP in a /30 network,
-        # so we request the IPs to be aligned on a /30 network
-        # See https://github.com/firecracker-microvm/firecracker/blob/main/resources/tests/fcnet-setup.sh#L21
-        # file:../../resources/tests/fcnet-setup.sh#L21
-        netmask_len = 30
-        (host_ip, guest_ip) = network_config.get_next_available_ips(2, netmask_len)
-        tap = self.create_tap_and_ssh_config(host_ip, guest_ip, netmask_len, tapname)
-        guest_mac = net_tools.mac_from_ip(guest_ip)
-
-        response = self.network.put(
-            iface_id=iface_id,
-            host_dev_name=tapname,
-            guest_mac=guest_mac,
-            tx_rate_limiter=tx_rate_limiter,
-            rx_rate_limiter=rx_rate_limiter,
-        )
-        assert self._api_session.is_status_no_content(response.status_code)
-
-        return tap, host_ip, guest_ip
-
-    def create_tap_and_ssh_config(self, host_ip, guest_ip, netmask_len, tapname=None):
-        """Create tap device and configure ssh."""
-        assert tapname is not None
-        tap = net_tools.Tap(
-            tapname, self.jailer.netns, ip="{}/{}".format(host_ip, netmask_len)
-        )
-        self.config_ssh(guest_ip)
-        return tap
-
-    def config_ssh(self, guest_ip):
-        """Configure ssh."""
-        self.ssh_config["hostname"] = guest_ip
-
-    def add_net_iface(self, iface, tx_rate_limiter=None, rx_rate_limiter=None):
+    def add_net_iface(self, iface=None, api=True, **kwargs):
         """Add a network interface"""
+        if iface is None:
+            iface = NetIfaceConfig.with_id(len(self.iface))
         tap = net_tools.Tap(
             iface.tap_name, self.jailer.netns, ip=f"{iface.host_ip}/{iface.netmask}"
         )
-        self.config_ssh(iface.guest_ip)
-        response = self.network.put(
-            iface_id=iface.dev_name,
-            host_dev_name=iface.tap_name,
-            guest_mac=iface.guest_mac,
-            tx_rate_limiter=tx_rate_limiter,
-            rx_rate_limiter=rx_rate_limiter,
-        )
-        assert self.api_session.is_status_no_content(response.status_code)
         self.iface[iface.dev_name] = {
             "iface": iface,
             "tap": tap,
         }
 
+        # If api, call it... there may be cases when we don't want it, for
+        # example during restore
+        if api:
+            response = self.network.put(
+                iface_id=iface.dev_name,
+                host_dev_name=iface.tap_name,
+                guest_mac=iface.guest_mac,
+                **kwargs,
+            )
+            assert self.api_session.is_status_no_content(response.status_code)
+
+        return iface
+
     def start(self, check=True):
         """Start the microvm.
 
@@ -868,10 +804,22 @@ def restore_from_snapshot(
         assert response.ok, response.content
         return True
 
-    @cached_property
+    @lru_cache
+    def ssh_iface(self, iface_idx=0):
+        """Return a cached SSH connection on a given interface id."""
+        guest_ip = list(self.iface.values())[iface_idx]["iface"].guest_ip
+        self.ssh_key = Path(self.ssh_key)
+        return net_tools.SSHConnection(
+            netns_path=self.jailer.netns_file_path(),
+            ssh_key=self.ssh_key,
+            user="root",
+            host=guest_ip,
+        )
+
+    @property
     def ssh(self):
-        """Return a cached SSH connection"""
-        return net_tools.SSHConnection(self.ssh_config)
+        """Return a cached SSH connection on the 1st interface"""
+        return self.ssh_iface(0)
 
     def start_console_logger(self, log_fifo):
         """

tests/framework/s3fetcher.py

@@ -142,7 +142,7 @@ def init_vm_resources(self, microvm_image_name, microvm):
             if resource_key.endswith(self.MICROVM_IMAGE_SSH_KEY_SUFFIX):
                 # Add the key path to the config dictionary and set
                 # permissions.
-                microvm.ssh_config["ssh_key_path"] = microvm_dest_path
+                microvm.ssh_key = microvm_dest_path
                 os.chmod(microvm_dest_path, 0o400)
 
     def list_microvm_images(self, capability_filter: List[str] = None):

tests/framework/stats/producer.py

@@ -30,9 +30,8 @@ def produce(self) -> Any:
         """Return the output of the executed ssh command."""
         rc, stdout, stderr = self._ssh_connection.execute_command(self._cmd)
         assert rc == 0
-        assert stderr.read() == ""
-
-        return stdout.read()
+        assert stderr == ""
+        return stdout
 
 
 class HostCommand(Producer):

tests/framework/utils.py

@@ -396,10 +396,10 @@ def get_free_mem_ssh(ssh_connection):
     _, stdout, stderr = ssh_connection.execute_command(
         "cat /proc/meminfo | grep MemAvailable"
     )
-    assert stderr.read() == ""
+    assert stderr == ""
 
     # Split "MemAvailable:   123456 kB" and validate it
-    meminfo_data = stdout.read().split()
+    meminfo_data = stdout.split()
     if len(meminfo_data) == 3:
         # Return the middle element in the array
         return int(meminfo_data[1])
@@ -540,8 +540,8 @@ def avg(thread_name):
 def run_guest_cmd(ssh_connection, cmd, expected, use_json=False):
     """Runs a shell command at the remote accessible via SSH"""
     _, stdout, stderr = ssh_connection.execute_command(cmd)
-    assert stderr.read() == ""
-    stdout = stdout.read() if not use_json else json.loads(stdout.read())
+    assert stderr == ""
+    stdout = stdout if not use_json else json.loads(stdout)
     assert stdout == expected
 
 
@@ -664,7 +664,7 @@ def generate_mmds_session_token(ssh_connection, ipv4_address, token_ttl):
     cmd += ' -H  "X-metadata-token-ttl-seconds: {}"'.format(token_ttl)
     cmd += " http://{}/latest/api/token".format(ipv4_address)
     _, stdout, _ = ssh_connection.execute_command(cmd)
-    token = stdout.read()
+    token = stdout
 
     return token
 
@@ -776,21 +776,21 @@ def guest_run_fio_iteration(ssh_connection, iteration):
         iteration
     )
     exit_code, _, stderr = ssh_connection.execute_command(fio)
-    assert exit_code == 0, stderr.read()
+    assert exit_code == 0, stderr
 
 
 def check_filesystem(ssh_connection, disk_fmt, disk):
     """Check for filesystem corruption inside a microVM."""
     cmd = "fsck.{} -n {}".format(disk_fmt, disk)
     exit_code, _, stderr = ssh_connection.execute_command(cmd)
-    assert exit_code == 0, stderr.read()
+    assert exit_code == 0, stderr
 
 
 def check_entropy(ssh_connection):
     """Check that we can get random numbers from /dev/hwrng"""
     cmd = "dd if=/dev/hwrng of=/dev/null bs=4096 count=1"
     exit_code, _, stderr = ssh_connection.execute_command(cmd)
-    assert exit_code == 0, stderr.read()
+    assert exit_code == 0, stderr
 
 
 @retry(delay=0.5, tries=5)

tests/framework/utils_cpuid.py

@@ -85,9 +85,8 @@ def check_guest_cpuid_output(
     """Parse cpuid output inside guest and match with expected one."""
     _, stdout, stderr = vm.ssh.execute_command(guest_cmd)
 
-    assert stderr.read() == ""
-    while True:
-        line = stdout.readline()
+    assert stderr == ""
+    for line in stdout.split("\n"):
         if line != "":
             # All the keys have been matched. Stop.
             if not expected_key_value_store:
@@ -134,7 +133,7 @@ def build_cpuid_dict(raw_cpuid_output):
     """Build CPUID dict based on raw cpuid output"""
     cpuid_dict = {}
     ptrn = re.compile("^ *(.*) (.*): eax=(.*) ebx=(.*) ecx=(.*) edx=(.*)$")
-    for line in raw_cpuid_output:
+    for line in raw_cpuid_output.strip().split("\n"):
         match = re.match(ptrn, line)
         assert match, f"`{line}` does not match the regex pattern."
         leaf, subleaf, eax, ebx, ecx, edx = [int(x, 16) for x in match.groups()]
@@ -159,7 +158,7 @@ def get_guest_cpuid(vm, leaf=None, subleaf=None):
     else:
         read_cpuid_cmd = "cpuid -r | sed '/CPU 1/q' | grep -v CPU"
     _, stdout, stderr = vm.ssh.execute_command(read_cpuid_cmd)
-    assert stderr.read() == ""
+    assert stderr == ""
 
     return build_cpuid_dict(stdout)
 

tests/framework/utils_iperf.py

@@ -111,9 +111,9 @@ def spawn_iperf3_client(self, client_idx):
         )
         rc, stdout, stderr = self._microvm.ssh.execute_command(pinned_cmd)
 
-        assert rc == 0, stderr.read()
+        assert rc == 0, stderr
 
-        return stdout.read()
+        return stdout
 
     def guest_command(self, port_offset):
         """Builds the command used for spawning an iperf3 client in the guest"""