[PATCH 0/2] remove 'requires_forwarding_restart' from framework

[PATCH 0/2] remove 'requires_forwarding_restart' from framework

[Nicholas Pratte]

While I initially thought that this would improve the cleanliness of
future test suite writing, because of the various nuances of each
testpmd run-time command, having the framework managing forwarding when
needed introduces far too many additional challenges, such that, it
simply is not worth the time implementing this.

Arguably, it also makes sense to have these mechanics (such as
forwarding restarts) written explicitly within the test suites.

Thank you Luca for pointing out this issue and its potential
side-effects.

Nicholas Pratte (2):
  dts: remove 'requires_forwarding_restart' from framework
  dts: rework run-time MTU adjustment test case

 dts/framework/remote_session/testpmd_shell.py | 26 +------------------
 dts/tests/TestSuite_mtu.py                    |  8 +++++-
 2 files changed, 8 insertions(+), 26 deletions(-)

-- 
2.47.1

[PATCH 1/2] dts: remove 'requires_forwarding_restart' from framework

[Nicholas Pratte]

This implementation conflicts with other bug fixes implemented in
earlier patches. The decorator was devised as a mean to make suite
writing a bit simpler, semantically, but the addition of this adds too
many complications for it to be worth implementing.

Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 26 +------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 9f07696aa2..e15fc0ee8f 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -1433,28 +1433,6 @@ def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs):
     return _wrapper
 
 
-def requires_forwarding_restart(func: TestPmdShellMethod) -> TestPmdShellMethod:
-    """Decorator for :class:`TestPmdShell` commands methods that requires forwarding restart.
-
-    If the decorated method is called while a :class:`TestPmdShell` is actively forwarding, then
-    forwarding is ceased, the wrapped function is executed, and forwarding is started again.
-
-    Args:
-        func: The :class:`TestPmdShell` method to decorate.
-    """
-
-    @functools.wraps(func)
-    def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs):
-        if self.currently_forwarding:
-            self._logger.debug("Forwarding needs to be restarted to continue.")
-            self.stop()
-            retval = func(self, *args, **kwargs)
-            self.start()
-            return retval
-
-    return _wrapper
-
-
 def add_remove_mtu(mtu: int = 1500) -> Callable[[TestPmdShellMethod], TestPmdShellMethod]:
     """Configure MTU to `mtu` on all ports, run the decorated function, then revert.
 
@@ -1503,7 +1481,6 @@ class TestPmdShell(DPDKShell):
     _command_extra_chars: ClassVar[str] = "\n"
 
     ports_started: bool
-    currently_forwarding: bool
 
     def __init__(
         self,
@@ -1528,7 +1505,6 @@ def __init__(
             name,
         )
         self.ports_started = not self._app_params.disable_device_start
-        self.currently_forwarding = not self._app_params.auto_start
         self._ports = None
 
     @property
@@ -1926,7 +1902,7 @@ def csum_set_hw(
                                                            {port_id}:\n{csum_output}"""
                     )
 
-    @requires_forwarding_restart
+    @requires_started_ports
     @requires_stopped_ports
     def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
         """Change the MTU of a port using testpmd.
-- 
2.47.1

Re: [PATCH 1/2] dts: remove 'requires_forwarding_restart' from framework

[Luca Vizzarro]

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

[PATCH 2/2] dts: rework run-time MTU adjustment test case

[Nicholas Pratte]

Forwarding restarts in the run-time MTU adjustment test case have been
explicitly added, given that the 'requires_forwarding_restart' decorator
from a previous patch was removed.

Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
---
 dts/tests/TestSuite_mtu.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/dts/tests/TestSuite_mtu.py b/dts/tests/TestSuite_mtu.py
index 3c96a36fc9..d1d48c2c13 100644
--- a/dts/tests/TestSuite_mtu.py
+++ b/dts/tests/TestSuite_mtu.py
@@ -164,20 +164,26 @@ def test_runtime_mtu_updating_and_forwarding(self) -> None:
             # Configure the new MTU.
 
             # Start packet capturing.
-            testpmd.start()
 
             testpmd.set_port_mtu_all(1500, verify=True)
+            testpmd.start()
             self.assess_mtu_boundary(testpmd, 1500)
 
+            testpmd.stop()
             testpmd.set_port_mtu_all(2400, verify=True)
+            testpmd.start()
             self.assess_mtu_boundary(testpmd, 1500)
             self.assess_mtu_boundary(testpmd, 2400)
 
+            testpmd.stop()
             testpmd.set_port_mtu_all(4800, verify=True)
+            testpmd.start()
             self.assess_mtu_boundary(testpmd, 1500)
             self.assess_mtu_boundary(testpmd, 4800)
 
+            testpmd.stop()
             testpmd.set_port_mtu_all(9000, verify=True)
+            testpmd.start()
             self.assess_mtu_boundary(testpmd, 1500)
             self.assess_mtu_boundary(testpmd, 9000)
 
-- 
2.47.1

Re: [PATCH 2/2] dts: rework run-time MTU adjustment test case

[Luca Vizzarro]

Looks good. Just some very small comments on polishing.

Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>

On 31/01/2025 19:38, Nicholas Pratte wrote:
> Forwarding restarts in the run-time MTU adjustment test case have been
> explicitly added, given that the 'requires_forwarding_restart' decorator
> from a previous patch was removed.
> 
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> ---
>   dts/tests/TestSuite_mtu.py | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/dts/tests/TestSuite_mtu.py b/dts/tests/TestSuite_mtu.py
> index 3c96a36fc9..d1d48c2c13 100644
> --- a/dts/tests/TestSuite_mtu.py
> +++ b/dts/tests/TestSuite_mtu.py
> @@ -164,20 +164,26 @@ def test_runtime_mtu_updating_and_forwarding(self) -> None:
>               # Configure the new MTU.
>   
>               # Start packet capturing.
> -            testpmd.start()
>   

I guess all of the lines above are no longer needed here.

>               testpmd.set_port_mtu_all(1500, verify=True)
> +            testpmd.start()
>               self.assess_mtu_boundary(testpmd, 1500)
>   
> +            testpmd.stop()
>               testpmd.set_port_mtu_all(2400, verify=True)
> +            testpmd.start()
>               self.assess_mtu_boundary(testpmd, 1500)
>               self.assess_mtu_boundary(testpmd, 2400)
>   
> +            testpmd.stop()
>               testpmd.set_port_mtu_all(4800, verify=True)
> +            testpmd.start()
>               self.assess_mtu_boundary(testpmd, 1500)
>               self.assess_mtu_boundary(testpmd, 4800)
>   
> +            testpmd.stop()
>               testpmd.set_port_mtu_all(9000, verify=True)
> +            testpmd.start()
>               self.assess_mtu_boundary(testpmd, 1500)
>               self.assess_mtu_boundary(testpmd, 9000)
>   

The start and stop could be grouped by forwarding sections:

   set_mtu()

   start()
   assess()
   assess()
   stop()

   set_mtu()
   ...