RE: [RFC PATCH v2 03/10] dts: add dpdk build on sut

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

[-- Attachment #1: Type: text/plain, Size: 6644 bytes --]

Apologies for removing recipients in my previous reply.

From: Owen Hilyard <ohilyard@iol.unh.edu>
Sent: Monday, November 21, 2022 1:35 PM
To: Juraj Linkeš <juraj.linkes@pantheon.tech>
Subject: Re: [RFC PATCH v2 03/10] dts: add dpdk build on sut

On Fri, Nov 18, 2022 at 7:24 AM Juraj Linkeš <juraj.linkes@pantheon.tech<mailto:juraj.linkes@pantheon.tech>> wrote:
A note: If I'm not mistaken, review should be done in plain text. I've formatted this as plain text and prefixed my replies with [Juraj].

+    @abstractmethod
+    def build_dpdk(
+        self,
+        env_vars: EnvVarsDict,
+        meson_args: str,
+        remote_dpdk_dir: str | PurePath,
+        target_name: str,
+        rebuild: bool = False,
+        timeout: float = SETTINGS.compile_timeout,
+    ) -> PurePath:

I think that we should consider having a MesonArgs type which implements the builder pattern. That way common things like static vs dynamic linking, enabling lto, setting the optimization level, et can be handled via dedicated methods, and then we can add a method on that which is "add this string onto the end". This would also allow defining additional methods for DPDK-specific meson arguments, like only enabling certain drivers/applications/tests or forcing certain vector widths. I would also like to see an option to make use of ccache, because currently the only way I see to do that is via environment variables, which will make creating a test matrix that includes multiple compilers difficult.

[Juraj] The MesonArgs type is a good suggestion, I'll do that.
[Juraj] We don't necessarily need ccache at this point, but it is very useful and it doesn't look like that big of an addition. How exactly should the implementation look like? Do we want configure something in the conf.yaml file? What to I need to add to meson invocation?

[Owen] I think that we probably want to have a setting in the conf.yaml file that creates a "compiler wrapper". You can either declare one for all compilers or declare one for some subset of compilers. I think putting it into the conf.yaml file makes sense.

executions:
    - build_targets:
        - arch: x86_64
          os: linux
          cpu: native
          compiler: gcc
          compiler_wrapper: ccache
        - arch: x86_64
          os: linux
          cpu: native
          compiler: icc
          compiler_wrapper: /usr/local/bin/my_super_special_compiler_wrapper
        - arch: x86_64
          os: linux
          cpu: native
          compiler: clang # clang doesn't need a wrapper for some reason



The only way that I know of to easily set the compiler in Meson is to set CC="<compiler_wrapper> <compiler>" for "meson setup". Also, you will need to completely wipe out the build directory between build targets due to meson not actually reconfiguring properly.

Ok, I'll modify the CC variable when compiler_wrapper is defined. It seems to be working, but may not be the cleanest implementation.
The current DPDK build works this way: The first DPDK build per build target is done from scratch and subsequent builds (currently application building) is done on top of that, so we should be fine on this front.

<snip>

+    @abstractmethod
+    def copy_file(
+        self, source_file: str, destination_file: str, source_remote: bool = False
+    ) -> None:
+        """
+        Copy source_file from local storage to destination_file on the remote Node

This should clarify that local storage means inside of the DTS container, not the system it is running on.

[Juraj] Ack. The local storage (I really should've said filesystem) could be any place where DTS is running, be it a container, a VM or a baremetal host. I think just changing local storage to local filesystem should be enough. If not, please propose an alternative wording.

[Juraj] And a related note - should we split copy_file into copy_file_to and copy_file_from?

<snip>

+    @skip_setup
+    def _copy_dpdk_tarball(self) -> None:
+        """
+        Copy to and extract DPDK tarball on the SUT node.
+        """
+        # check local path
+        assert SETTINGS.dpdk_ref.exists(), f"Package {SETTINGS.dpdk_ref} doesn't exist."
+
+        http://self.logger.info("Copying DPDK tarball to SUT.")
+        self.main_session.copy_file(SETTINGS.dpdk_ref, self._remote_tmp_dir)
+
+        # construct remote tarball path
+        # the basename is the same on local host and on remote Node
+        remote_tarball_path = self.main_session.join_remote_path(
+            self._remote_tmp_dir, os.path.basename(SETTINGS.dpdk_ref)
+        )
+
+        # construct remote path after extracting
+        with tarfile.open(SETTINGS.dpdk_ref) as dpdk_tar:
+            dpdk_top_dir = dpdk_tar.getnames()[0]
+        self._remote_dpdk_dir = self.main_session.join_remote_path(
+            self._remote_tmp_dir, dpdk_top_dir
+        )
+
+        http://self.logger.info("Extracting DPDK tarball on SUT.")

Can we add a path to this log message?

[Juraj] Absolutely, I'll add it. If there are more logs that would be useful to you, I'll add those as well (maybe as debugs).

<snip>

+class EnvVarsDict(dict):
+    def __str__(self) -> str:
+        return " ".join(["=".join(item) for item in self.items()])

This needs to make sure it doesn't silently run over the line length limitations in posix sh/bash (4096 chars) or cmd (8191 chars). That would be a VERY frustrating bug to track down and it can easily be stopped by checking that this is a reasonable length (< 2k characters) and emitting a warning if something goes over that.

[Juraj] Interesting, I didn't know about this. Would a warning be enough?
Also, Allowing less than 2k characters leaves us with at least 2k characters for the rest of the command and that should be plenty, but do we want to check that as well? If so, we may want to do the check when sending a commad. Another thing to consider is that we're going to switch to Fabric and we won't need to worry about this - it would be up to the underlying RemoteSession implementations to check this.

[Owen] A warning would probably be enough. "Another thing to consider is that we're going to switch to Fabric and we won't need to worry about this" We will need to worry about this if we are still exposing a bash shell in any way to the user.

Ok, I think we should note this and consider it when implementing Farbric. I don't think we'll be exposing shell at this point, but maybe that'll change when we need to handle DPDK applications - we should address this then I think.

[-- Attachment #2: Type: text/html, Size: 13422 bytes --]