Re: [PATCH v3 08/12] dts: add NIC capability support

[Jeremy Spewock <jspewock@iol.unh.edu>]

On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
<snip>
>  @dataclass
>  class TestPmdPort(TextParser):
>      """Dataclass representing the result of testpmd's ``show port info`` command."""
> @@ -962,3 +1043,96 @@ def _close(self) -> None:
>          self.stop()
>          self.send_command("quit", "Bye...")
>          return super()._close()
> +
> +    """
> +    ====== Capability retrieval methods ======
> +    """
> +
> +    def get_capabilities_rxq_info(
> +        self,
> +        supported_capabilities: MutableSet["NicCapability"],
> +        unsupported_capabilities: MutableSet["NicCapability"],
> +    ) -> None:
> +        """Get all rxq capabilities and divide them into supported and unsupported.
> +
> +        Args:
> +            supported_capabilities: Supported capabilities will be added to this set.
> +            unsupported_capabilities: Unsupported capabilities will be added to this set.
> +        """
> +        self._logger.debug("Getting rxq capabilities.")
> +        command = f"show rxq info {self.ports[0].id} 0"
> +        rxq_info = TestPmdRxqInfo.parse(self.send_command(command))
> +        if rxq_info.rx_scattered_packets:
> +            supported_capabilities.add(NicCapability.SCATTERED_RX_ENABLED)
> +        else:
> +            unsupported_capabilities.add(NicCapability.SCATTERED_RX_ENABLED)
> +
> +    """
> +    ====== Decorator methods ======
> +    """
> +
> +    @staticmethod
> +    def config_mtu_9000(testpmd_method: TestPmdShellSimpleMethod) -> TestPmdShellDecoratedMethod:

It might be more valuable for me to make a method for configuring the
MTU of all ports so that you don't have to do the loops yourself, I
can add this to the MTU patch once I update that and rebase it on
main.

> +        """Configure MTU to 9000 on all ports, run `testpmd_method`, then revert.
> +
> +        Args:
> +            testpmd_method: The method to decorate.
> +
> +        Returns:
> +            The method decorated with setting and reverting MTU.
> +        """
> +
> +        def wrapper(testpmd_shell: Self):
> +            original_mtus = []
> +            for port in testpmd_shell.ports:
> +                original_mtus.append((port.id, port.mtu))
> +                testpmd_shell.set_port_mtu(port_id=port.id, mtu=9000, verify=False)
> +            testpmd_method(testpmd_shell)
> +            for port_id, mtu in original_mtus:
> +                testpmd_shell.set_port_mtu(port_id=port_id, mtu=mtu if mtu else 1500, verify=False)
> +
> +        return wrapper
<snip>
> diff --git a/dts/framework/testbed_model/capability.py b/dts/framework/testbed_model/capability.py
> index 8899f07f76..9a79e6ebb3 100644
> --- a/dts/framework/testbed_model/capability.py
> +++ b/dts/framework/testbed_model/capability.py
> @@ -5,14 +5,40 @@
>
>  This module provides a protocol that defines the common attributes of test cases and suites
>  and support for test environment capabilities.
> +
> +Many test cases are testing features not available on all hardware.
> +
> +The module also allows developers to mark test cases or suites a requiring certain

small typo: I think you meant " mark test cases or suites *as*
requiring certain..."

> +hardware capabilities with the :func:`requires` decorator.
> +
> +Example:
> +    .. code:: python
> +
> +        from framework.test_suite import TestSuite, func_test
> +        from framework.testbed_model.capability import NicCapability, requires
> +        class TestPmdBufferScatter(TestSuite):
> +            # only the test case requires the scattered_rx capability
> +            # other test cases may not require it
> +            @requires(NicCapability.scattered_rx)

Is it worth updating this to what the enum actually holds
(SCATTERED_RX_ENABLED) or not really since it is just an example in a
doc-string? I think it could do either way, but it might be better to
keep it consistent at least to start.

> +            @func_test
> +            def test_scatter_mbuf_2048(self):
<snip>
>
> @@ -96,6 +122,128 @@ def __hash__(self) -> int:
>          """The subclasses must be hashable so that they can be stored in sets."""
>
>
> +@dataclass
> +class DecoratedNicCapability(Capability):
> +    """A wrapper around :class:`~framework.remote_session.testpmd_shell.NicCapability`.
> +
> +    Some NIC capabilities are only present or listed as supported only under certain conditions,
> +    such as when a particular configuration is in place. This is achieved by allowing users to pass
> +    a decorator function that decorates the function that gets the support status of the capability.
> +
> +    New instances should be created with the :meth:`create_unique` class method to ensure
> +    there are no duplicate instances.
> +
> +    Attributes:
> +        nic_capability: The NIC capability that partly defines each instance.
> +        capability_decorator: The decorator function that will be passed the function associated
> +            with `nic_capability` when discovering the support status of the capability.
> +            Each instance is defined by `capability_decorator` along with `nic_capability`.
> +    """
> +
> +    nic_capability: NicCapability
> +    capability_decorator: TestPmdShellDecorator | None
> +    _unique_capabilities: ClassVar[
> +        dict[Tuple[NicCapability, TestPmdShellDecorator | None], Self]
> +    ] = {}
> +
> +    @classmethod
> +    def get_unique(
> +        cls, nic_capability: NicCapability, decorator_fn: TestPmdShellDecorator | None
> +    ) -> "DecoratedNicCapability":

This idea of get_unique really confused me at first. After reading
different parts of the code to learn how it is being used, I think I
understand now what it's for. My current understanding is basically
that you're using an uninstantiated class as essentially a factory
that stores a dictionary that you are using to hold singletons. It
might be confusing to me in general because I haven't really seen this
idea of dynamically modifying attributes of a class itself rather than
an instance of the class used this way. Understanding it now, it makes
sense what you are trying to do and how this is essentially a nice
cache/factory for singleton values for each capability, but It might
be helpful to document a little more somehow that _unique_capabilities
is really just a container for the singleton capabilities, and that
the top-level class is modified to keep a consistent state throughout
the framework.

Again, it could just be me having not really seen this idea used
before, but it was strange to wrap my head around at first since I'm
more used to class methods being used to read the state of attributes.

> +        """Get the capability uniquely identified by `nic_capability` and `decorator_fn`.
> +
> +        Args:
> +            nic_capability: The NIC capability.
> +            decorator_fn: The function that will be passed the function associated
> +                with `nic_capability` when discovering the support status of the capability.
> +
> +        Returns:
> +            The capability uniquely identified by `nic_capability` and `decorator_fn`.
> +        """
> +        if (nic_capability, decorator_fn) not in cls._unique_capabilities:
> +            cls._unique_capabilities[(nic_capability, decorator_fn)] = cls(
> +                nic_capability, decorator_fn
> +            )
> +        return cls._unique_capabilities[(nic_capability, decorator_fn)]
> +
> +    @classmethod
> +    def get_supported_capabilities(
> +        cls, sut_node: SutNode, topology: "Topology"
> +    ) -> set["DecoratedNicCapability"]:
> +        """Overrides :meth:`~Capability.get_supported_capabilities`.
> +
> +        The capabilities are first sorted by decorators, then reduced into a single function which
> +        is then passed to the decorator. This way we only execute each decorator only once.

This second sentence repeats the word "only" but I don't think it is
really necessary to and it might flow better with either one of them
instead of both.

> +        """
> +        supported_conditional_capabilities: set["DecoratedNicCapability"] = set()
> +        logger = get_dts_logger(f"{sut_node.name}.{cls.__name__}")
> +        if topology.type is Topology.type.no_link:
> +            logger.debug(
> +                "No links available in the current topology, not getting NIC capabilities."
> +            )
> +            return supported_conditional_capabilities
> +        logger.debug(
> +            f"Checking which NIC capabilities from {cls.capabilities_to_check} are supported."
> +        )
> +        if cls.capabilities_to_check:
> +            capabilities_to_check_map = cls._get_decorated_capabilities_map()
> +            with TestPmdShell(sut_node, privileged=True) as testpmd_shell:
> +                for conditional_capability_fn, capabilities in capabilities_to_check_map.items():
> +                    supported_capabilities: set[NicCapability] = set()
> +                    unsupported_capabilities: set[NicCapability] = set()
> +                    capability_fn = cls._reduce_capabilities(
> +                        capabilities, supported_capabilities, unsupported_capabilities
> +                    )

This combines calling all of the capabilities into one function, but
if there are multiple capabilities that use the same underlying
testpmd function won't this call the same method multiple times? Or is
this handled by two Enum values in NicCapability that have the same
testpmd method as their value hashing to the same thing? For example,
if there are two capabilities that both require show rxq info and the
same decorator (scatter and some other capability X), won't this call
`show rxq info` twice even though you already know that the capability
is supported after the first call? It's not really harmful for this to
happen, but it would go against the idea of calling a method and
getting all of the capabilities that you can the first time. Maybe it
could be fixed with a conditional check which verifies if `capability`
is already in `supported_capabilities` or `unsupported_capabilities`
or not if it's a problem?

> +                    if conditional_capability_fn:
> +                        capability_fn = conditional_capability_fn(capability_fn)
> +                    capability_fn(testpmd_shell)
> +                    for supported_capability in supported_capabilities:
> +                        for capability in capabilities:
> +                            if supported_capability == capability.nic_capability:
> +                                supported_conditional_capabilities.add(capability)

I might be misunderstanding, but is this also achievable by just writing:

for capability in capabilities:
    if capability.nic_capability in supported_capabilities:
        supported_conditional_capabilities.add(capability)

I think that would be functionally the same, but I think it reads
easier than a nested loop.

> +
> +        logger.debug(f"Found supported capabilities {supported_conditional_capabilities}.")
> +        return supported_conditional_capabilities
> +
> +    @classmethod
> +    def _get_decorated_capabilities_map(
> +        cls,
> +    ) -> dict[TestPmdShellDecorator | None, set["DecoratedNicCapability"]]:
> +        capabilities_map: dict[TestPmdShellDecorator | None, set["DecoratedNicCapability"]] = {}
> +        for capability in cls.capabilities_to_check:
> +            if capability.capability_decorator not in capabilities_map:
> +                capabilities_map[capability.capability_decorator] = set()
> +            capabilities_map[capability.capability_decorator].add(capability)
> +
> +        return capabilities_map
> +
> +    @classmethod
> +    def _reduce_capabilities(
> +        cls,
> +        capabilities: set["DecoratedNicCapability"],
> +        supported_capabilities: MutableSet,
> +        unsupported_capabilities: MutableSet,
> +    ) -> TestPmdShellSimpleMethod:
> +        def reduced_fn(testpmd_shell: TestPmdShell) -> None:
> +            for capability in capabilities:
> +                capability.nic_capability(
> +                    testpmd_shell, supported_capabilities, unsupported_capabilities
> +                )
> +
> +        return reduced_fn

Would it make sense to put these two methods above
get_supported_capabilities since that is where they are used? I might
be in favor of it just because it would save you from having to look
further down in the diff to find what the method does and then go back
up, but I also understand that it looks like you might have been
sorting methods by private vs. public so if you think it makes more
sense to leave them here that is also viable.

> +
> +    def __hash__(self) -> int:
> +        """Instances are identified by :attr:`nic_capability` and :attr:`capability_decorator`."""
> +        return hash((self.nic_capability, self.capability_decorator))

I guess my question above is asking if `hash(self.nic_capability) ==
hash(self.nic_capability.value())` because, if they aren't, then I
think the map will contain multiple capabilities that use the same
testpmd function since the capabilities themselves are unique, and
then because the get_supported_capabilities() method above just calls
whatever is in this map, it would call it twice. I think the whole
point of the NoAliasEnum is making sure that they don't hash to the
same thing. I could be missing something, but, if I am, maybe some
kind of comment showing where this is handled would be helpful.

> +
> +    def __repr__(self) -> str:
> +        """Easy to read string of :attr:`nic_capability` and :attr:`capability_decorator`."""
> +        condition_fn_name = ""
> +        if self.capability_decorator:
> +            condition_fn_name = f"{self.capability_decorator.__qualname__}|"
> +        return f"{condition_fn_name}{self.nic_capability}"
> +
> +
>  class TestProtocol(Protocol):
>      """Common test suite and test case attributes."""
>
> @@ -116,6 +264,34 @@ def get_test_cases(cls, test_case_sublist: Sequence[str] | None = None) -> tuple
>          raise NotImplementedError()
>
>
> +def requires(
> +    *nic_capabilities: NicCapability,
> +    decorator_fn: TestPmdShellDecorator | None = None,
> +) -> Callable[[type[TestProtocol]], type[TestProtocol]]:
> +    """A decorator that adds the required capabilities to a test case or test suite.
> +
> +    Args:
> +        nic_capabilities: The NIC capabilities that are required by the test case or test suite.
> +        decorator_fn: The decorator function that will be used when getting
> +            NIC capability support status.
> +        topology_type: The topology type the test suite or case requires.
> +
> +    Returns:
> +        The decorated test case or test suite.
> +    """
> +
> +    def add_required_capability(test_case_or_suite: type[TestProtocol]) -> type[TestProtocol]:
> +        for nic_capability in nic_capabilities:
> +            decorated_nic_capability = DecoratedNicCapability.get_unique(
> +                nic_capability, decorator_fn
> +            )
> +            decorated_nic_capability.add_to_required(test_case_or_suite)
> +
> +        return test_case_or_suite
> +
> +    return add_required_capability
> +
> +
>  def get_supported_capabilities(
>      sut_node: SutNode,
>      topology_config: Topology,
> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> index 178a40385e..713549a5b2 100644
> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> @@ -25,6 +25,7 @@
>  from framework.params.testpmd import SimpleForwardingModes
>  from framework.remote_session.testpmd_shell import TestPmdShell
>  from framework.test_suite import TestSuite, func_test
> +from framework.testbed_model.capability import NicCapability, requires
>
>
>  class TestPmdBufferScatter(TestSuite):
> @@ -123,6 +124,7 @@ def pmd_scatter(self, mbsize: int) -> None:
>                      f"{offset}.",
>                  )
>
> +    @requires(NicCapability.SCATTERED_RX_ENABLED, decorator_fn=TestPmdShell.config_mtu_9000)

Is it possible to instead associate the required decorator with the
scattered_rx capability itself? Since the configuration is required to
check the capability, I don't think there will ever be a case where
`decorator_fn` isn't required here, or a case where it is ever
anything other than modifying the MTU. Maybe it is more clear from the
reader's perspective this way that there are other things happening
under-the-hood, but it also saves developers from having to specify
something static when we already know beforehand what they need to
specify.

Doing so would probably mess up some of what you have written in the
way of DecoratedNicCapability and it might be more difficult to do it
in a way that only calls the decorator method once if there are
multiple capabilities that require the same decorator.

Maybe something that you could do is make the NicCapability class in
Testpmd have values that are tuples of (decorator_fn | None,
get_capabilities_fn), and then you can still have the
DecoratedNicCapabilitity class and the methods wouldn't really need to
change. I think the main thing that would change is just that the
decorator_fn is collected from the capability/enum instead of the
requires() method. You could potentially make get_unique easier as
well since you can just rely on the enum values since already know
what is required. Then you could take the pairs from that enum and
create a mapping like you have now of which ones require which
decorators and keep the same idea.

>      @func_test
>      def test_scatter_mbuf_2048(self) -> None:
>          """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
> --
> 2.34.1
>