Re: [PATCH v1 1/1] dts: bind to DPDK driver before running test suites

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Fri, Oct 27, 2023 at 12:03 AM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Modifies the current process so that we bind to os_driver_for_dpdk from
> the configuration file before running test suites and bind back to the
> os_driver afterwards. This allows test suites to assume that the ports
> are bound to a DPDK supported driver or bind to either driver as needed.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>  dts/framework/remote_session/linux_session.py |  3 ++
>  dts/framework/remote_session/os_session.py    |  6 ++++
>  dts/framework/testbed_model/sut_node.py       | 34 +++++++++++++++++++
>  dts/tests/TestSuite_os_udp.py                 |  4 +++
>  dts/tests/TestSuite_smoke_tests.py            |  6 ++--
>  5 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/dts/framework/remote_session/linux_session.py b/dts/framework/remote_session/linux_session.py
> index a3f1a6bf3b..7f2453c44c 100644
> --- a/dts/framework/remote_session/linux_session.py
> +++ b/dts/framework/remote_session/linux_session.py
> @@ -199,3 +199,6 @@ def configure_port_ip_address(
>      def configure_ipv4_forwarding(self, enable: bool) -> None:
>          state = 1 if enable else 0
>          self.send_command(f"sysctl -w net.ipv4.ip_forward={state}", privileged=True)
> +
> +    def probe_driver(self, driver_name: str) -> None:
> +        self.send_command(f"modprobe {driver_name}", verify=True)

This may not be something we want to do in DTS, but rather assume it's
already been configured and maybe add a smoke test that would test
whether the driver is there.

> diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/remote_session/os_session.py
> index 8a709eac1c..719e815ac8 100644
> --- a/dts/framework/remote_session/os_session.py
> +++ b/dts/framework/remote_session/os_session.py
> @@ -282,3 +282,9 @@ def configure_ipv4_forwarding(self, enable: bool) -> None:
>          """
>          Enable IPv4 forwarding in the underlying OS.
>          """
> +
> +    @abstractmethod
> +    def probe_driver(self, driver_name: str) -> None:
> +        """
> +        Load the module for the driver.
> +        """
> diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
> index 202aebfd06..5a7dd91cac 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -89,6 +89,7 @@ class SutNode(Node):
>      _dpdk_version: str | None
>      _node_info: NodeInfo | None
>      _compiler_version: str | None
> +    _path_to_devbind: PurePath | None

I'd add script to the variable so that it's clearer:
self._path_to_devbind_script

>
>      def __init__(self, node_config: SutNodeConfiguration):
>          super(SutNode, self).__init__(node_config)
> @@ -105,6 +106,7 @@ def __init__(self, node_config: SutNodeConfiguration):
>          self._dpdk_version = None
>          self._node_info = None
>          self._compiler_version = None
> +        self._path_to_devbind = None
>          self._logger.info(f"Created node: {self.name}")
>
>      @property
> @@ -155,6 +157,14 @@ def compiler_version(self) -> str:
>                  return ""
>          return self._compiler_version
>
> +    @property
> +    def path_to_devbind(self) -> PurePath:
> +        if self._path_to_devbind is None:
> +            self._path_to_devbind = self.main_session.join_remote_path(
> +                self._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
> +            )
> +        return self._path_to_devbind
> +
>      def get_build_target_info(self) -> BuildTargetInfo:
>          return BuildTargetInfo(
>              dpdk_version=self.dpdk_version, compiler_version=self.compiler_version
> @@ -176,6 +186,14 @@ def _set_up_build_target(
>          self._configure_build_target(build_target_config)
>          self._copy_dpdk_tarball()
>          self._build_dpdk()
> +        self.bind_ports_to_driver()

This only binds the ports on the SUT node, should we also do this on
the TG node? We may not have access to the devbind script on the TG
node, but we can just copy it there.

> +
> +    def _tear_down_build_target(self) -> None:
> +        """
> +        This method exists to be optionally overwritten by derived classes and
> +        is not decorated so that the derived class doesn't have to use the decorator.
> +        """
> +        self.bind_ports_to_driver(for_dpdk=False)

There are three possibilities here:
1. We unbind the port
2. We bind the port to the OS driver (like you're suggesting)
3. We bind the port to the state before self.bind_ports_to_driver()

I don't think it's that big of a deal if we just bind it to the OS
driver, but 3 is probably the best.

>
>      def _configure_build_target(
>          self, build_target_config: BuildTargetConfiguration
> @@ -389,3 +407,19 @@ def create_interactive_shell(
>          return super().create_interactive_shell(
>              shell_cls, timeout, privileged, str(eal_parameters)
>          )
> +
> +    def bind_ports_to_driver(self, for_dpdk: bool = True) -> None:
> +        """Bind all ports on the SUT to a driver.
> +
> +        Args:
> +            for_dpdk: Boolean that, when True, binds ports to os_driver_for_dpdk
> +            or, when False, binds to os_driver. Defaults to True.
> +        """
> +        for port in self.ports:
> +            driver = port.os_driver_for_dpdk if for_dpdk else port.os_driver
> +            self.main_session.probe_driver(driver)
> +            self.main_session.send_command(
> +                f"{self.path_to_devbind} -b {driver} --force {port.pci}",
> +                privileged=True,
> +                verify=True,
> +            )

Just a note: I was thinking about creating a dependency on the devbind
script which is outside DTS and it actually looks like a benefit. Not
only we don't need to implement anything, we're also testing the tool.

> diff --git a/dts/tests/TestSuite_os_udp.py b/dts/tests/TestSuite_os_udp.py
> index 9b5f39711d..bf6b93deb5 100644
> --- a/dts/tests/TestSuite_os_udp.py
> +++ b/dts/tests/TestSuite_os_udp.py
> @@ -19,6 +19,8 @@ def set_up_suite(self) -> None:
>              Configure SUT ports and SUT to route traffic from if1 to if2.
>          """
>
> +        # This test uses kernel drivers
> +        self.sut_node.bind_ports_to_driver(for_dpdk=False)

This should be in the test suite setup.

>          self.configure_testbed_ipv4()
>
>      def test_os_udp(self) -> None:
> @@ -43,3 +45,5 @@ def tear_down_suite(self) -> None:
>              Remove the SUT port configuration configured in setup.
>          """
>          self.configure_testbed_ipv4(restore=True)
> +        # Assume other suites will likely need dpdk driver
> +        self.sut_node.bind_ports_to_driver(for_dpdk=True)

Looks like the whole change is needed just because of this test suite.
If so, we can just remove the suite, as that was only a demonstration
of traffic generator code.

> diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
> index 4a269df75b..0b839cba4e 100644
> --- a/dts/tests/TestSuite_smoke_tests.py
> +++ b/dts/tests/TestSuite_smoke_tests.py
> @@ -84,9 +84,7 @@ def test_device_bound_to_driver(self) -> None:
>              Ensure that all drivers listed in the config are bound to the correct
>              driver.
>          """
> -        path_to_devbind = self.sut_node.main_session.join_remote_path(
> -            self.sut_node._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
> -        )
> +        path_to_devbind = self.sut_node.path_to_devbind

Now that I'm looking at this, there doesn't seem that much of a reason
to access the path directly. Just having methods that would return
structured output from the script makes way more sense to me. The
methods should also be usable on the TG node, there's not really a
reason to limit this to the SUT node.

>
>          all_nics_in_dpdk_devbind = self.sut_node.main_session.send_command(
>              f"{path_to_devbind} --status | awk '{REGEX_FOR_PCI_ADDRESS}'",
> @@ -108,7 +106,7 @@ def test_device_bound_to_driver(self) -> None:
>              # We know this isn't None, but mypy doesn't
>              assert devbind_info_for_nic is not None
>              self.verify(
> -                devbind_info_for_nic.group(1) == nic.os_driver,
> +                devbind_info_for_nic.group(1) == nic.os_driver_for_dpdk,
>                  f"Driver for device {nic.pci} does not match driver listed in "
>                  f"configuration (bound to {devbind_info_for_nic.group(1)})",
>              )
> --
> 2.42.0
>