[v4,4/5] dts: add OS abstractions for creating virtual functions

From: Jeremy Spewock <jspewock@iol.unh.edu>

Virtual functions in the framework are created using SR-IOV. The process
for doing this can vary depending on the operating system, so the
commands to create VFs have to be abstracted into different classes
based on the operating system. This patch adds the stubs for methods
that create VFs and get the PCI addresses of all VFs on a port to the
abstract class as well as a linux implementation for the methods.

Bugzilla ID: 1500

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/testbed_model/linux_session.py | 40 +++++++++++++++++++-
 dts/framework/testbed_model/os_session.py    | 40 ++++++++++++++++++++
 2 files changed, 79 insertions(+), 1 deletion(-)
  

> diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py

> @@ -210,3 +214,37 @@ def configure_ipv4_forwarding(self, enable: bool) -> None:
>           """Overrides :meth:`~.os_session.OSSession.configure_ipv4_forwarding`."""
>           state = 1 if enable else 0
>           self.send_command(f"sysctl -w net.ipv4.ip_forward={state}", privileged=True)
> +
> +    def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool:
> +        """Overrides :meth:`~.os_session.OSSession.set_num_virtual_functions`."""
> +        sys_bus_path = f"/sys/bus/pci/devices/{pf_port.pci}/sriov_numvfs".replace(":", "\\:")
> +        curr_num_vfs = int(self.send_command(f"cat {sys_bus_path}").stdout)
> +        if num > 0 and curr_num_vfs >= num:
> +            self._logger.info(
> +                f"{curr_num_vfs} VFs already configured on port {pf_port.identifier.pci} on node "
> +                f"{pf_port.identifier.node}."
> +            )
> +            return False
> +        elif num > 0 and curr_num_vfs > 0:

These two conditions could be written as:
if num > 0:
     if curr_num_vfs >= num:
         return False
     elif curr_num_vfs > 0:
         raise InternalError()

Maybe it's not worth the extra indent, but it's easier to understand, so 
I lean towards doing it this way.


> +    def get_pci_addr_of_vfs(self, pf_port: Port) -> list[str]:
> +        """Overrides :meth:`~.os_session.OSSession.get_pci_addr_of_vfs`."""
> +        sys_bus_path = f"/sys/bus/pci/devices/{pf_port.pci}".replace(":", "\\:")
> +        curr_num_vfs = int(self.send_command(f"cat {sys_bus_path}/sriov_numvfs").stdout)
> +        if curr_num_vfs > 0:
> +            pci_addrs = self.send_command(
> +                'awk -F "PCI_SLOT_NAME=" "/PCI_SLOT_NAME=/ {print \\$2}" '
> +                + f"{sys_bus_path}/virtfn*/uevent",

We could use a TextParser here. Not sure if it's a good fit though.


> diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py

> @@ -395,3 +395,43 @@ def configure_ipv4_forwarding(self, enable: bool) -> None:

> +    @abstractmethod
> +    def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool:
> +        """Update the number of virtual functions (VFs) on a port.
> +
> +        It should be noted that, due to the nature of VFs, if there are already VFs that exist on
> +        the physical function (PF) prior to calling this function, additional ones cannot be added.
> +        The only way to add more VFs is to remove the existing and then set the desired amount. For
> +        this reason, this method will handle creation in the following order:
> +
> +        1. Use existing VFs on the PF if the number of existing VFs is greater than or equal to
> +        `num`
> +        2. Throw an exception noting that VFs cannot be created if the PF has some VFs already set
> +        on it, but the total VFs that it has are less then `num`.
> +        3. Create `num` VFs on the PF if there are none on it already
> +
> +        Args:
> +            num: The number of VFs to set on the port.
> +            pf_port: The port to add the VFs to.
> +
> +        Raises:
> +            InternalError: If `pf_port` has less than `num` VFs configured on it
> +                already.
> +
> +        Returns:
> +            :data:`True` if this method successfully created VFs, :data:`False` if existing VFs
> +            were used instead.
> +        """

The whole docstring talks only about the creation of VFs, but we can 
also remove VFs with this.
  

Usual problem with the commit subject.

On 23/09/2024 19:42, jspewock@iol.unh.edu wrote:
> +    def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool:

I'd call this set_num_of_virtual_functions, set_virtual_functions_num, 
or set_number_of_vfs. I think I prefer the last one.

> +        """Overrides :meth:`~.os_session.OSSession.set_num_virtual_functions`."""
> +        sys_bus_path = f"/sys/bus/pci/devices/{pf_port.pci}/sriov_numvfs".replace(":", "\\:")
> +        curr_num_vfs = int(self.send_command(f"cat {sys_bus_path}").stdout)
> +        if num > 0 and curr_num_vfs >= num:
> +            self._logger.info(
> +                f"{curr_num_vfs} VFs already configured on port {pf_port.identifier.pci} on node "
> +                f"{pf_port.identifier.node}."
> +            )
> +            return False
> +        elif num > 0 and curr_num_vfs > 0:
> +            self._logger.error(
> +                f"Not enough VFs configured on port {pf_port.identifier.pci} on node "
> +                f"{pf_port.identifier.node}. Need {num} but only {curr_num_vfs} are configured. "
> +                "DTS is unable to modify number of preexisting VFs."
> +            )
> +            raise InternalError("Failed to create VFs on port.")

This is not an InternalError. That exception represents a bug internal 
to DTS, it's not related to the node not satisfying requirements. I am 
guessing this requires a new kind of error.

> +        self.send_command(f"echo {num} > {sys_bus_path}", privileged=True, verify=True)
> +        return True
> +
> +    def get_pci_addr_of_vfs(self, pf_port: Port) -> list[str]:
> +        """Overrides :meth:`~.os_session.OSSession.get_pci_addr_of_vfs`."""
> +        sys_bus_path = f"/sys/bus/pci/devices/{pf_port.pci}".replace(":", "\\:")
> +        curr_num_vfs = int(self.send_command(f"cat {sys_bus_path}/sriov_numvfs").stdout)
> +        if curr_num_vfs > 0:
> +            pci_addrs = self.send_command(
> +                'awk -F "PCI_SLOT_NAME=" "/PCI_SLOT_NAME=/ {print \\$2}" '
> +                + f"{sys_bus_path}/virtfn*/uevent",

I like the use of awk. Using a TextWrapper may be overkill when we have 
easier options like this one.


> +    def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool:
> +        """Update the number of virtual functions (VFs) on a port.

Like Juraj mentioned, this docstring should say that it can be used for 
creating and removing VFs.