[v2,4/5] dts: add ability to start/stop testpmd ports
Checks
Commit Message
Add testpmd commands to start and stop all the ports, so that they can
be configured. Because there is a distinction of commands that require
the ports to be stopped and started, also add decorators for commands
that require a specific state, removing this logic from the test
writer's duty.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
dts/framework/remote_session/testpmd_shell.py | 86 ++++++++++++++++++-
1 file changed, 84 insertions(+), 2 deletions(-)
Comments
I tried to address this very same problem in the scatter expansion
patch series but, admittedly, I like your approach better. There is
one thing that Juraj and I discussed on that thread however that is
probably worth noting here regarding verification of the stopping and
starting ports. The main idea behind that being if the user calls a
testpmd method and specifically specifies they don't want to verify
the outcome of that method, but then we still verify whether we
stopped the ports or not, that could cause confusion. The compromise
we ended up settling for was just to make a parameter to the decorator
that allows you to statically decide which decorated methods should
verify the ports stopping and which shouldn't. It was mainly because
of the ability to specify "verify" as a kwarg or an arg and the types
being messy. The discussion we had about it starts here if you were
interested in reading it:
http://inbox.dpdk.org/dev/dff89e16-0173-4069-878c-d1c34df1ae13@pantheon.tech/
On Tue, Aug 6, 2024 at 8:49 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add testpmd commands to start and stop all the ports, so that they can
> be configured. Because there is a distinction of commands that require
> the ports to be stopped and started, also add decorators for commands
> that require a specific state, removing this logic from the test
> writer's duty.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
<snip>
> +P = ParamSpec("P")
> +TestPmdShellMethod = Callable[Concatenate["TestPmdShell", P], Any]
Agggh, I spent so much time trying to figure out how to do exactly
this when I was working on the scatter series but I couldn't get a
nice typehint while specifying that I wanted a TestPmdShell first. I
had no idea that Concatenate was a type but I will definitely keep
that one in mind.
> +
>
> class TestPmdDevice:
> """The data of a device that testpmd can recognize.
> @@ -577,12 +581,51 @@ class TestPmdPortStats(TextParser):
> tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
>
>
> +def requires_stopped_ports(func: TestPmdShellMethod) -> TestPmdShellMethod:
> + """Decorator for :class:`TestPmdShell` commands methods that require stopped ports.
> +
> + If the decorated method is called while the ports are started, then these are stopped before
> + continuing.
> + """
> +
> + @functools.wraps(func)
> + def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs):
> + if self.ports_started:
> + self._logger.debug("Ports need to be stopped to continue")
> + self.stop_all_ports()
> +
> + return func(self, *args, **kwargs)
> +
> + return _wrapper
> +
> +
> +def requires_started_ports(func: TestPmdShellMethod) -> TestPmdShellMethod:
> + """Decorator for :class:`TestPmdShell` commands methods that require started ports.
> +
> + If the decorated method is called while the ports are stopped, then these are started before
> + continuing.
> + """
> +
> + @functools.wraps(func)
> + def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs):
> + if not self.ports_started:
> + self._logger.debug("Ports need to be started to continue")
> + self.start_all_ports()
> +
> + return func(self, *args, **kwargs)
> +
> + return _wrapper
> +
I'm not sure how much it is necessary since it is pretty clear what
these decorators are trying to do with the parameter, but since these
are public methods I think not having an "Args:" section for the func
might trip up the doc generation.
> +
> class TestPmdShell(DPDKShell):
> """Testpmd interactive shell.
>
> The testpmd shell users should never use
> the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
> call specialized methods. If there isn't one that satisfies a need, it should be added.
> +
> + Attributes:
> + ports_started: Indicates whether the ports are started.
> """
>
> _app_params: TestPmdParams
> @@ -619,6 +662,9 @@ def __init__(
> name,
> )
>
> + self.ports_started = not self._app_params.disable_device_start
> +
It might be useful to add this attribute as a stub in the class just
to be clear on the typing. It also helps me understand things at more
of a glance personally.
> + @requires_started_ports
> def start(self, verify: bool = True) -> None:
> """Start packet forwarding with the current configuration.
<snip>
> --
> 2.34.1
>
As Jeremy mentioned, adding the verify argument may be worthwhile, but
maybe only if we actually identify a usecase where we wouldn't want to
do the verification.
I also have two other points:
1. Do we want a decorator that does both - starting and stopping? Is the
idea to have all methods that require a certain state to be decorated
and in that case we don't need this? E.g. if there are multiple
configuration steps, only the first one stops the ports (if started) and
we start the ports only when a method needs that (such as starting pkt
fwd)? I think this workflow should be documented in the class docstring
(the important part being that starting and stopping of ports is done
automatically).
2. Do we want decorators that start/stop just one port? I guess this is
basically useless with the above workflow.
On 09/08/2024 16:13, Jeremy Spewock wrote:
> I tried to address this very same problem in the scatter expansion
> patch series but, admittedly, I like your approach better. There is
> one thing that Juraj and I discussed on that thread however that is
> probably worth noting here regarding verification of the stopping and
> starting ports. The main idea behind that being if the user calls a
> testpmd method and specifically specifies they don't want to verify
> the outcome of that method, but then we still verify whether we
> stopped the ports or not, that could cause confusion. The compromise
> we ended up settling for was just to make a parameter to the decorator
> that allows you to statically decide which decorated methods should
> verify the ports stopping and which shouldn't. It was mainly because
> of the ability to specify "verify" as a kwarg or an arg and the types
> being messy. The discussion we had about it starts here if you were
> interested in reading it:
> http://inbox.dpdk.org/dev/dff89e16-0173-4069-878c-d1c34df1ae13@pantheon.tech/
When it comes to starting and stopping ports, we may need to ensure that
the operations were done correctly. Surely we don't want to start
forwarding thinking that the ports are initialised, when they could be
not... I guess the point here is that it may be important to save the
correct state. Not sure, I guess verify could always be added later if
needed.
> On Tue, Aug 6, 2024 at 8:49 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>>
>> Add testpmd commands to start and stop all the ports, so that they can
>> be configured. Because there is a distinction of commands that require
>> the ports to be stopped and started, also add decorators for commands
>> that require a specific state, removing this logic from the test
>> writer's duty.
>>
>> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
>> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
>> ---
> <snip>
>> +P = ParamSpec("P")
>> +TestPmdShellMethod = Callable[Concatenate["TestPmdShell", P], Any]
>
> Agggh, I spent so much time trying to figure out how to do exactly
> this when I was working on the scatter series but I couldn't get a
> nice typehint while specifying that I wanted a TestPmdShell first. I
> had no idea that Concatenate was a type but I will definitely keep
> that one in mind.
>
Glad I could help! :D
>> +def requires_started_ports(func: TestPmdShellMethod) -> TestPmdShellMethod:
>> + """Decorator for :class:`TestPmdShell` commands methods that require started ports.
>> +
>> + If the decorated method is called while the ports are stopped, then these are started before
>> + continuing.
>> + """
>> +
>> + @functools.wraps(func)
>> + def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs):
>> + if not self.ports_started:
>> + self._logger.debug("Ports need to be started to continue")
>> + self.start_all_ports()
>> +
>> + return func(self, *args, **kwargs)
>> +
>> + return _wrapper
>> +
>
> I'm not sure how much it is necessary since it is pretty clear what
> these decorators are trying to do with the parameter, but since these
> are public methods I think not having an "Args:" section for the func
> might trip up the doc generation.
We have several methods that have one-liner docstrings... so not really
sure. I was hoping to have the doc generation merged as testing what's
correct and what's not is currently complicated.
>>
>> + self.ports_started = not self._app_params.disable_device_start
>> +
>
> It might be useful to add this attribute as a stub in the class just
> to be clear on the typing. It also helps me understand things at more
> of a glance personally.
You make a fair point here.
On 23/08/2024 13:16, Juraj Linkeš wrote:
> As Jeremy mentioned, adding the verify argument may be worthwhile, but
> maybe only if we actually identify a usecase where we wouldn't want to
> do the verification.
Yeah, as I pointed out, it feels unlikely to pretend that they are
started (or stopped). I think that could cause my unpredicted behaviour
and confusion. But also as said already, it can always be added on demand.
> I also have two other points:
> 1. Do we want a decorator that does both - starting and stopping? Is the
> idea to have all methods that require a certain state to be decorated
> and in that case we don't need this? E.g. if there are multiple
> configuration steps, only the first one stops the ports (if started) and
> we start the ports only when a method needs that (such as starting pkt
> fwd)? I think this workflow should be documented in the class docstring
> (the important part being that starting and stopping of ports is done
> automatically).
The way I envisioned these decorators is by using a lazy approach. I
don't think it may be right to eagerly restart ports after stopping
them, because maybe we want to run another command after that will stop
them again. So only start and stop as needed. Ideally every port that
requires a specific state of the ports need to be decorated. I went
through all the methods in the class to check which would and which
wouldn't, and it seems alright like this.
> 2. Do we want decorators that start/stop just one port? I guess this is
> basically useless with the above workflow.
Would it actually matter? We are not really using the other ports in
parallel here I believe. May bring unnecessary complexity. I am thinking
that maybe if needed it could be added later.
On 6. 9. 2024 18:46, Luca Vizzarro wrote:
> On 23/08/2024 13:16, Juraj Linkeš wrote:
>> As Jeremy mentioned, adding the verify argument may be worthwhile, but
>> maybe only if we actually identify a usecase where we wouldn't want to
>> do the verification.
>
> Yeah, as I pointed out, it feels unlikely to pretend that they are
> started (or stopped). I think that could cause my unpredicted behaviour
> and confusion. But also as said already, it can always be added on demand.
>
>> I also have two other points:
>> 1. Do we want a decorator that does both - starting and stopping? Is
>> the idea to have all methods that require a certain state to be
>> decorated and in that case we don't need this? E.g. if there are
>> multiple configuration steps, only the first one stops the ports (if
>> started) and we start the ports only when a method needs that (such as
>> starting pkt fwd)? I think this workflow should be documented in the
>> class docstring (the important part being that starting and stopping
>> of ports is done automatically).
>
> The way I envisioned these decorators is by using a lazy approach. I
> don't think it may be right to eagerly restart ports after stopping
> them, because maybe we want to run another command after that will stop
> them again. So only start and stop as needed. Ideally every port that
> requires a specific state of the ports need to be decorated. I went
> through all the methods in the class to check which would and which
> wouldn't, and it seems alright like this.
>
I agree. I was initially thinking about this from the point of view of
making sure we don't change things for other users, but that doesn't
apply here, as we control the whole testpmd workflow. Your approach is
actually great - simple and effective.
>> 2. Do we want decorators that start/stop just one port? I guess this
>> is basically useless with the above workflow.
>
> Would it actually matter? We are not really using the other ports in
> parallel here I believe. May bring unnecessary complexity. I am thinking
> that maybe if needed it could be added later.
I think the only benefit would be that we're sending less commands so
it'll be a bit faster. Maybe we just make a mental note of this for the
future.
On Fri, Sep 6, 2024 at 12:46 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 23/08/2024 13:16, Juraj Linkeš wrote:
> > As Jeremy mentioned, adding the verify argument may be worthwhile, but
> > maybe only if we actually identify a usecase where we wouldn't want to
> > do the verification.
>
> Yeah, as I pointed out, it feels unlikely to pretend that they are
> started (or stopped). I think that could cause my unpredicted behaviour
> and confusion. But also as said already, it can always be added on demand.
>
I agree that we can save this as something to add as we need it and
that there could be some confusion to pretending the ports are in a
certain state. Since we actually save the state in testpmd, we
definitely want that to be accurate. The main thing I was thinking
about this for however was less about the state tracking and more
about what the expectation is for developers when they specifically
specify they do not want a method to be verified. There aren't many
cases where you wouldn't want to verify the outcome of a command, but
I would imagine the two reasons you would do so is either you don't
care about the result, or you specifically don't want to be disruptive
to the rest of the run if this command fails (like if the framework
has a testpmd shell running for some kind of process but doesn't want
to end the run if it fails). When we enforce the verification of some
part of the process like stopping the port in this case, that
assumption about not verifying changes. The second reason I mentioned
is why I believe capabilities checking specifies no-verify when it
updates the MTU, for example. I imagine we don't want the test run to
end if a testpmd method fails its check with capabilities, we would
want the capability to just be unsupported. Of course the developer
could catch the exception themselves, but in that case I think it
would make more sense for catching the exception to be the primary way
to avoid verification and we could just remove the parameter all
together so there isn't this edge-case for avoiding verification with
decorated methods. I guess even the static parameter that specifies
whether or not to verify doesn't really address this problem either,
haha. Maybe it would even make more sense for the decorator to assume
that it shouldn't throw an exception if the ports state changing
failed (but still avoid updating the port state) and let the verifying
process be completely down to if the method being decorated succeeded
or not with the ports in whatever state they were at the time since, I
would assume, if the ports fail to update their state the method will
also fail anyway.
All-in-all however, there likely won't be many cases where the ports
actually fail to stop in the first place, and not verifying in general
is uncommon. If what I mentioned above makes sense and we're fine with
the slight change in thinking, I think it is fine to merge this and
just keep in the back of our minds that this is the case, and maybe
either document somewhere that you have to catch the exception with
decorated methods, or just remove the verify parameter from decorated
methods all together and enforce that they must be verified. As
mentioned, it's not like we are forcing ourselves to stick with it,
adding the parameter is pretty easy to do on demand.
> > I also have two other points:
> > 1. Do we want a decorator that does both - starting and stopping? Is the
> > idea to have all methods that require a certain state to be decorated
> > and in that case we don't need this? E.g. if there are multiple
> > configuration steps, only the first one stops the ports (if started) and
> > we start the ports only when a method needs that (such as starting pkt
> > fwd)? I think this workflow should be documented in the class docstring
> > (the important part being that starting and stopping of ports is done
> > automatically).
>
> The way I envisioned these decorators is by using a lazy approach. I
> don't think it may be right to eagerly restart ports after stopping
> them, because maybe we want to run another command after that will stop
> them again. So only start and stop as needed. Ideally every port that
> requires a specific state of the ports need to be decorated. I went
> through all the methods in the class to check which would and which
> wouldn't, and it seems alright like this.
>
> > 2. Do we want decorators that start/stop just one port? I guess this is
> > basically useless with the above workflow.
>
> Would it actually matter? We are not really using the other ports in
> parallel here I believe. May bring unnecessary complexity. I am thinking
> that maybe if needed it could be added later.
On 9. 9. 2024 16:20, Jeremy Spewock wrote:
> On Fri, Sep 6, 2024 at 12:46 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>>
>> On 23/08/2024 13:16, Juraj Linkeš wrote:
>>> As Jeremy mentioned, adding the verify argument may be worthwhile, but
>>> maybe only if we actually identify a usecase where we wouldn't want to
>>> do the verification.
>>
>> Yeah, as I pointed out, it feels unlikely to pretend that they are
>> started (or stopped). I think that could cause my unpredicted behaviour
>> and confusion. But also as said already, it can always be added on demand.
>>
>
> I agree that we can save this as something to add as we need it and
> that there could be some confusion to pretending the ports are in a
> certain state. Since we actually save the state in testpmd, we
> definitely want that to be accurate. The main thing I was thinking
> about this for however was less about the state tracking and more
> about what the expectation is for developers when they specifically
> specify they do not want a method to be verified. There aren't many
> cases where you wouldn't want to verify the outcome of a command, but
> I would imagine the two reasons you would do so is either you don't
> care about the result, or you specifically don't want to be disruptive
> to the rest of the run if this command fails (like if the framework
> has a testpmd shell running for some kind of process but doesn't want
> to end the run if it fails). When we enforce the verification of some
> part of the process like stopping the port in this case, that
> assumption about not verifying changes. The second reason I mentioned
> is why I believe capabilities checking specifies no-verify when it
> updates the MTU, for example. I imagine we don't want the test run to
> end if a testpmd method fails its check with capabilities, we would
> want the capability to just be unsupported. Of course the developer
> could catch the exception themselves, but in that case I think it
> would make more sense for catching the exception to be the primary way
> to avoid verification and we could just remove the parameter all
> together so there isn't this edge-case for avoiding verification with
> decorated methods. I guess even the static parameter that specifies
> whether or not to verify doesn't really address this problem either,
> haha. Maybe it would even make more sense for the decorator to assume
> that it shouldn't throw an exception if the ports state changing
> failed (but still avoid updating the port state) and let the verifying
> process be completely down to if the method being decorated succeeded
> or not with the ports in whatever state they were at the time since, I
> would assume, if the ports fail to update their state the method will
> also fail anyway.
>
> All-in-all however, there likely won't be many cases where the ports
> actually fail to stop in the first place, and not verifying in general
> is uncommon. If what I mentioned above makes sense and we're fine with
> the slight change in thinking, I think it is fine to merge this and
> just keep in the back of our minds that this is the case, and maybe
> either document somewhere that you have to catch the exception with
> decorated methods, or just remove the verify parameter from decorated
> methods all together and enforce that they must be verified. As
> mentioned, it's not like we are forcing ourselves to stick with it,
> adding the parameter is pretty easy to do on demand.
>
These are pretty good points. It makes sense to remove the verify
parameter and if we don't care about the result (or the result should
not be disrupting), we just catch the exception.
This patchset is not the place to settle this, so please open a bugzilla
ticket for this. In the worst case scenario we just close the ticket,
but it's good to have these thoughts in the ticket as well.
>>> I also have two other points:
>>> 1. Do we want a decorator that does both - starting and stopping? Is the
>>> idea to have all methods that require a certain state to be decorated
>>> and in that case we don't need this? E.g. if there are multiple
>>> configuration steps, only the first one stops the ports (if started) and
>>> we start the ports only when a method needs that (such as starting pkt
>>> fwd)? I think this workflow should be documented in the class docstring
>>> (the important part being that starting and stopping of ports is done
>>> automatically).
>>
>> The way I envisioned these decorators is by using a lazy approach. I
>> don't think it may be right to eagerly restart ports after stopping
>> them, because maybe we want to run another command after that will stop
>> them again. So only start and stop as needed. Ideally every port that
>> requires a specific state of the ports need to be decorated. I went
>> through all the methods in the class to check which would and which
>> wouldn't, and it seems alright like this.
>>
>>> 2. Do we want decorators that start/stop just one port? I guess this is
>>> basically useless with the above workflow.
>>
>> Would it actually matter? We are not really using the other ports in
>> parallel here I believe. May bring unnecessary complexity. I am thinking
>> that maybe if needed it could be added later.
@@ -14,16 +14,17 @@
testpmd_shell.close()
"""
+import functools
import re
import time
from dataclasses import dataclass, field
from enum import Flag, auto
from pathlib import PurePath
-from typing import ClassVar
+from typing import Any, Callable, ClassVar, Concatenate, ParamSpec
from typing_extensions import Self, Unpack
-from framework.exception import InteractiveCommandExecutionError
+from framework.exception import InteractiveCommandExecutionError, InternalError
from framework.params.testpmd import SimpleForwardingModes, TestPmdParams
from framework.params.types import TestPmdParamsDict
from framework.parser import ParserFn, TextParser
@@ -33,6 +34,9 @@
from framework.testbed_model.sut_node import SutNode
from framework.utils import StrEnum
+P = ParamSpec("P")
+TestPmdShellMethod = Callable[Concatenate["TestPmdShell", P], Any]
+
class TestPmdDevice:
"""The data of a device that testpmd can recognize.
@@ -577,12 +581,51 @@ class TestPmdPortStats(TextParser):
tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
+def requires_stopped_ports(func: TestPmdShellMethod) -> TestPmdShellMethod:
+ """Decorator for :class:`TestPmdShell` commands methods that require stopped ports.
+
+ If the decorated method is called while the ports are started, then these are stopped before
+ continuing.
+ """
+
+ @functools.wraps(func)
+ def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs):
+ if self.ports_started:
+ self._logger.debug("Ports need to be stopped to continue")
+ self.stop_all_ports()
+
+ return func(self, *args, **kwargs)
+
+ return _wrapper
+
+
+def requires_started_ports(func: TestPmdShellMethod) -> TestPmdShellMethod:
+ """Decorator for :class:`TestPmdShell` commands methods that require started ports.
+
+ If the decorated method is called while the ports are stopped, then these are started before
+ continuing.
+ """
+
+ @functools.wraps(func)
+ def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs):
+ if not self.ports_started:
+ self._logger.debug("Ports need to be started to continue")
+ self.start_all_ports()
+
+ return func(self, *args, **kwargs)
+
+ return _wrapper
+
+
class TestPmdShell(DPDKShell):
"""Testpmd interactive shell.
The testpmd shell users should never use
the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
call specialized methods. If there isn't one that satisfies a need, it should be added.
+
+ Attributes:
+ ports_started: Indicates whether the ports are started.
"""
_app_params: TestPmdParams
@@ -619,6 +662,9 @@ def __init__(
name,
)
+ self.ports_started = not self._app_params.disable_device_start
+
+ @requires_started_ports
def start(self, verify: bool = True) -> None:
"""Start packet forwarding with the current configuration.
@@ -723,6 +769,42 @@ def set_forward_mode(self, mode: SimpleForwardingModes, verify: bool = True):
f"Test pmd failed to set fwd mode to {mode.value}"
)
+ def stop_all_ports(self, verify: bool = True) -> None:
+ """Stops all the ports.
+
+ Args:
+ verify: If :data:`True`, the output of the command will be checked for a successful
+ execution.
+
+ Raises:
+ InteractiveCommandExecutionError: If `verify` is :data:`True` and the ports were not
+ stopped successfully.
+ """
+ self._logger.debug("Stopping all the ports...")
+ output = self.send_command("port stop all")
+ if verify and not output.strip().endswith("Done"):
+ raise InteractiveCommandExecutionError("Ports were not stopped successfully")
+
+ self.ports_started = False
+
+ def start_all_ports(self, verify: bool = True) -> None:
+ """Starts all the ports.
+
+ Args:
+ verify: If :data:`True`, the output of the command will be checked for a successful
+ execution.
+
+ Raises:
+ InteractiveCommandExecutionError: If `verify` is :data:`True` and the ports were not
+ started successfully.
+ """
+ self._logger.debug("Starting all the ports...")
+ output = self.send_command("port start all")
+ if verify and not output.strip().endswith("Done"):
+ raise InteractiveCommandExecutionError("Ports were not started successfully")
+
+ self.ports_started = True
+
def show_port_info_all(self) -> list[TestPmdPort]:
"""Returns the information of all the ports.