Re: [PATCH v1] dts: remove the OS UDP test suite

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

On Tue, Apr 23, 2024 at 2:00 PM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> On Tue, Apr 23, 2024 at 12:39 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
> >
> > On 19/04/2024 14:52, Juraj Linkeš wrote:
> > > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> > > <snip>
> > > -    def configure_testbed_ipv4(self, restore: bool = False) -> None:
> > > -        """Configure IPv4 addresses on all testbed ports.
> > > -
> > > -        The configured ports are:
> > > -
> > > -        * SUT ingress port,
> > > -        * SUT egress port,
> > > -        * TG ingress port,
> > > -        * TG egress port.
> > > -
> > > -        Args:
> > > -            restore: If :data:`True`, will remove the configuration instead.
> > > -        """
> > > -        delete = True if restore else False
> > > -        enable = False if restore else True
> > > -        self._configure_ipv4_forwarding(enable)
> > > -        self.sut_node.configure_port_ip_address(
> > > -            self._sut_ip_address_egress, self._sut_port_egress, delete
> > > -        )
> > > -        self.sut_node.configure_port_state(self._sut_port_egress, enable)
> > > -        self.sut_node.configure_port_ip_address(
> > > -            self._sut_ip_address_ingress, self._sut_port_ingress, delete
> > > -        )
> > > -        self.sut_node.configure_port_state(self._sut_port_ingress, enable)
> > > -        self.tg_node.configure_port_ip_address(
> > > -            self._tg_ip_address_ingress, self._tg_port_ingress, delete
> > > -        )
> > > -        self.tg_node.configure_port_state(self._tg_port_ingress, enable)
> > > -        self.tg_node.configure_port_ip_address(
> > > -            self._tg_ip_address_egress, self._tg_port_egress, delete
> > > -        )
> > > -        self.tg_node.configure_port_state(self._tg_port_egress, enable)
> > > -
> > > -    def _configure_ipv4_forwarding(self, enable: bool) -> None:
> > > -        self.sut_node.configure_ipv4_forwarding(enable)
> > > -
> > > <snip>
> > > diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py
> > > <snip>
> > > -    def configure_port_state(self, port: Port, enable: bool) -> None:
> > > -        """Overrides :meth:`~.os_session.OSSession.configure_port_state`."""
> > > -        state = "up" if enable else "down"
> > > -        self.send_command(f"ip link set dev {port.logical_name} {state}", privileged=True)
> > > -
> > > -    def configure_port_ip_address(
> > > -        self,
> > > -        address: Union[IPv4Interface, IPv6Interface],
> > > -        port: Port,
> > > -        delete: bool,
> > > -    ) -> None:
> > > -        """Overrides :meth:`~.os_session.OSSession.configure_port_ip_address`."""
> > > -        command = "del" if delete else "add"
> > > -        self.send_command(
> > > -            f"ip address {command} {address} dev {port.logical_name}",
> > > -            privileged=True,
> > > -            verify=True,
> > > -        )
> > > -
> >  > <snip>
> > > @@ -205,8 +185,3 @@ def configure_port_mtu(self, mtu: int, port: Port) -> None:
> >  > <snip>
> > > -
> > > -    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)
> > > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> > > <snip>
> > > -    def configure_port_state(self, port: Port, enable: bool = True) -> None:
> > > -        """Enable/disable `port`.
> > > -
> > > -        Args:
> > > -            port: The port to enable/disable.
> > > -            enable: :data:`True` to enable, :data:`False` to disable.
> > > -        """
> > > -        self.main_session.configure_port_state(port, enable)
> > > -
> > > -    def configure_port_ip_address(
> > > -        self,
> > > -        address: Union[IPv4Interface, IPv6Interface],
> > > -        port: Port,
> > > -        delete: bool = False,
> > > -    ) -> None:
> > > -        """Add an IP address to `port` on this node.
> > > -
> > > -        Args:
> > > -            address: The IP address with mask in CIDR format. Can be either IPv4 or IPv6.
> > > -            port: The port to which to add the address.
> > > -            delete: If :data:`True`, will delete the address from the port instead of adding it.
> > > -        """
> > > -        self.main_session.configure_port_ip_address(address, port, delete)
> > > -
> > > <snip>
> > > diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py
> > > <snip>
> > > -    @abstractmethod
> > > -    def configure_port_state(self, port: Port, enable: bool) -> None:
> > > -        """Enable/disable `port` in the operating system.
> > > -
> > > -        Args:
> > > -            port: The port to configure.
> > > -            enable: If :data:`True`, enable the port, otherwise shut it down.
> > > -        """
> > > -
> > > -    @abstractmethod
> > > -    def configure_port_ip_address(
> > > -        self,
> > > -        address: Union[IPv4Interface, IPv6Interface],
> > > -        port: Port,
> > > -        delete: bool,
> > > -    ) -> None:
> > > -        """Configure an IP address on `port` in the operating system.
> > > -
> > > -        Args:
> > > -            address: The address to configure.
> > > -            port: The port to configure.
> > > -            delete: If :data:`True`, remove the IP address, otherwise configure it.
> > > -        """
> > > -
> > > <snip>
> > > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
> > > <snip>
> > > -    def configure_ipv4_forwarding(self, enable: bool) -> None:
> > > -        """Enable/disable IPv4 forwarding on the node.
> > > -
> > > -        Args:
> > > -            enable: If :data:`True`, enable the forwarding, otherwise disable it.
> > > -        """
> > > -        self.main_session.configure_ipv4_forwarding(enable)
> > > -
> >
> > While I agree to remove unused code, especially code that will never be
> > used again. Is it worthwhile removing already implemented features?
> > Unless we don't foresee using them ever again...
>
> My thinking was that we don't need these for DPDK apps, but maybe we
> need some of them for Scapy (or any other tg using the OS networking
> stack). I'll verify this just to make sure we're not removing
> something that Scapy needs.

From what I can tell, we don't need any of these for Scapy, but we
should test this in other environments as well (my virtual environment
doesn't play nice with port rebinding and running testpmd - I have to
start Scapy after starting testpmd to get it to work. This may be a
nightmare to debug.). Can you, Luca and Patrick, run the scatter test
suite with this patch to verify this?