[v2,4/7] dts: enable copying directories to and from nodes

Message ID 20241021134935.1210500-5-luca.vizzarro@arm.com (mailing list archive)
State Superseded, archived
Delegated to: Paul Szczepanek
Headers
Series DTS external DPDK build |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Luca Vizzarro Oct. 21, 2024, 1:49 p.m. UTC
From: Tomáš Ďurovec <tomas.durovec@pantheon.tech>

Currently there is no support to transfer whole directories between the
DTS host and the nodes. This change adds this new feature.

Signed-off-by: Tomáš Ďurovec <tomas.durovec@pantheon.tech>
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
 dts/framework/testbed_model/os_session.py    | 120 ++++++++++++++++++-
 dts/framework/testbed_model/posix_session.py |  88 +++++++++++++-
 dts/framework/utils.py                       |  91 +++++++++++++-
 3 files changed, 287 insertions(+), 12 deletions(-)
  

Comments

Dean Marx Oct. 25, 2024, 6:12 p.m. UTC | #1
On Mon, Oct 21, 2024 at 9:50 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:

> From: Tomáš Ďurovec <tomas.durovec@pantheon.tech>
>
> Currently there is no support to transfer whole directories between the
> DTS host and the nodes. This change adds this new feature.
>
> Signed-off-by: Tomáš Ďurovec <tomas.durovec@pantheon.tech>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
>

Reviewed-by: Dean Marx <dmarx@iol.unh.edu>
  
Patrick Robb Oct. 29, 2024, 1:07 a.m. UTC | #2
On Mon, Oct 21, 2024 at 9:49 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:

>
> +
> +    @abstractmethod
> +    def remove_remote_file(self, remote_file_path: str | PurePath, force:
> bool = True) -> None:
> +        """Remove remote file, by default remove forcefully.
> +
> +        Args:
> +            remote_file_path: The file path to remove.
> +            force: If :data:`True`, ignore all warnings and try to remove
> at all costs.
> +        """
>

This is outside of the scope of this patch, but I figured I would comment
that we should use this to clean the dpdk-devbind.py file when we re-add
that functionality. I'm glad this method is added. :)


> +
>      @abstractmethod
>      def remove_remote_dir(
>          self,
> @@ -213,11 +302,34 @@ def remove_remote_dir(
>          """Remove remote directory, by default remove recursively and
> forcefully.
>
>          Args:
> -            remote_dir_path: The path of the directory to remove.
> +            remote_dir_path: The directory path to remove.
>              recursive: If :data:`True`, also remove all contents inside
> the directory.
>              force: If :data:`True`, ignore all warnings and try to remove
> at all costs.
>          """
>
> +    @abstractmethod
> +    def create_remote_tarball(
> +        self,
> +        remote_dir_path: str | PurePath,
> +        compress_format: TarCompressionFormat = TarCompressionFormat.none,
> +        exclude: str | list[str] | None = None,
> +    ) -> PurePosixPath:
>

Does this have to be a PurePosixPath instead of a PurePath? I know adding
Windows support for DTS seems far out, but we should not add in barriers to
that now without good reason (though if there is a strong practical reason
why we want to do this now, then okay). I believe we will have a
PurePosixPath return in testbed_model/posix_session.py and a
PureWindowsPath return in testbed_model/windows_session.py (when it exists).

Otherwise, I know we discussed this at the DTS call on Thurs, but thanks
for remaining .gz .xz agnostic.

Reviewed-by: Patrick Robb <probb@iol.unh.edu>
  
Luca Vizzarro Oct. 29, 2024, noon UTC | #3
On 29/10/2024 01:07, Patrick Robb wrote:
> On Mon, Oct 21, 2024 at 9:49 AM Luca Vizzarro wrote:
>     +
>     +    @abstractmethod
>     +    def remove_remote_file(self, remote_file_path: str | PurePath,
>     force: bool = True) -> None:
>     +        """Remove remote file, by default remove forcefully.
>     +
>     +        Args:
>     +            remote_file_path: The file path to remove.
>     +            force: If :data:`True`, ignore all warnings and try to
>     remove at all costs.
>     +        """
> 
> 
> This is outside of the scope of this patch, but I figured I would 
> comment that we should use this to clean the dpdk-devbind.py file when 
> we re-add that functionality. I'm glad this method is added. :)

Sounds good to me!

>     +    @abstractmethod
>     +    def create_remote_tarball(
>     +        self,
>     +        remote_dir_path: str | PurePath,
>     +        compress_format: TarCompressionFormat =
>     TarCompressionFormat.none,
>     +        exclude: str | list[str] | None = None,
>     +    ) -> PurePosixPath:
> 
> 
> Does this have to be a PurePosixPath instead of a PurePath? I know 
> adding Windows support for DTS seems far out, but we should not add in 
> barriers to that now without good reason (though if there is a strong 
> practical reason why we want to do this now, then okay). I believe we 
> will have a PurePosixPath return in testbed_model/posix_session.py and a 
> PureWindowsPath return in testbed_model/windows_session.py (when it exists).

Excellent catch! Quite missed this, we should be able to amend this to 
PurePath under os_session.py and keep PurePosixPath under posix_session.py
> 
> Otherwise, I know we discussed this at the DTS call on Thurs, but thanks 
> for remaining .gz .xz agnostic.

The choice to zip the tarball is taken under 
framework.testbed_model.sut_node:_copy_dpdk_tree
  

Patch

diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py
index 1aac3659bf..6c3f84dec1 100644
--- a/dts/framework/testbed_model/os_session.py
+++ b/dts/framework/testbed_model/os_session.py
@@ -25,7 +25,7 @@ 
 from abc import ABC, abstractmethod
 from collections.abc import Iterable
 from ipaddress import IPv4Interface, IPv6Interface
-from pathlib import Path, PurePath
+from pathlib import Path, PurePath, PurePosixPath
 from typing import Union
 
 from framework.config import Architecture, NodeConfiguration, NodeInfo
@@ -38,7 +38,7 @@ 
 )
 from framework.remote_session.remote_session import CommandResult
 from framework.settings import SETTINGS
-from framework.utils import MesonArgs
+from framework.utils import MesonArgs, TarCompressionFormat
 
 from .cpu import LogicalCore
 from .port import Port
@@ -203,6 +203,95 @@  def copy_to(self, source_file: str | Path, destination_dir: str | PurePath) -> N
                 will be saved.
         """
 
+    @abstractmethod
+    def copy_dir_from(
+        self,
+        source_dir: str | PurePath,
+        destination_dir: str | Path,
+        compress_format: TarCompressionFormat = TarCompressionFormat.none,
+        exclude: str | list[str] | None = None,
+    ) -> None:
+        """Copy a directory from the remote node to the local filesystem.
+
+        Copy `source_dir` from the remote node associated with this remote session to
+        `destination_dir` on the local filesystem. The new local directory will be created
+        at `destination_dir` path.
+
+        Example:
+            source_dir = '/remote/path/to/source'
+            destination_dir = '/local/path/to/destination'
+            compress_format = TarCompressionFormat.xz
+
+            The method will:
+                1. Create a tarball from `source_dir`, resulting in:
+                    '/remote/path/to/source.tar.xz',
+                2. Copy '/remote/path/to/source.tar.xz' to
+                    '/local/path/to/destination/source.tar.xz',
+                3. Extract the contents of the tarball, resulting in:
+                    '/local/path/to/destination/source/',
+                4. Remove the tarball after extraction
+                    ('/local/path/to/destination/source.tar.xz').
+
+            Final Path Structure:
+                '/local/path/to/destination/source/'
+
+        Args:
+            source_dir: The directory on the remote node.
+            destination_dir: The directory path on the local filesystem.
+            compress_format: The compression format to use. Defaults to no compression.
+            exclude: Patterns for files or directories to exclude from the tarball.
+                These patterns are used with `tar`'s `--exclude` option.
+        """
+
+    @abstractmethod
+    def copy_dir_to(
+        self,
+        source_dir: str | Path,
+        destination_dir: str | PurePath,
+        compress_format: TarCompressionFormat = TarCompressionFormat.none,
+        exclude: str | list[str] | None = None,
+    ) -> None:
+        """Copy a directory from the local filesystem to the remote node.
+
+        Copy `source_dir` from the local filesystem to `destination_dir` on the remote node
+        associated with this remote session. The new remote directory will be created at
+        `destination_dir` path.
+
+        Example:
+            source_dir = '/local/path/to/source'
+            destination_dir = '/remote/path/to/destination'
+            compress_format = TarCompressionFormat.xz
+
+            The method will:
+                1. Create a tarball from `source_dir`, resulting in:
+                    '/local/path/to/source.tar.xz',
+                2. Copy '/local/path/to/source.tar.xz' to
+                    '/remote/path/to/destination/source.tar.xz',
+                3. Extract the contents of the tarball, resulting in:
+                    '/remote/path/to/destination/source/',
+                4. Remove the tarball after extraction
+                    ('/remote/path/to/destination/source.tar.xz').
+
+            Final Path Structure:
+                '/remote/path/to/destination/source/'
+
+        Args:
+            source_dir: The directory on the local filesystem.
+            destination_dir: The directory path on the remote node.
+            compress_format: The compression format to use. Defaults to no compression.
+            exclude: Patterns for files or directories to exclude from the tarball.
+                These patterns are used with `fnmatch.fnmatch` to filter out files.
+        """
+
+    @abstractmethod
+    def remove_remote_file(self, remote_file_path: str | PurePath, force: bool = True) -> None:
+        """Remove remote file, by default remove forcefully.
+
+        Args:
+            remote_file_path: The file path to remove.
+            force: If :data:`True`, ignore all warnings and try to remove at all costs.
+        """
+
     @abstractmethod
     def remove_remote_dir(
         self,
@@ -213,11 +302,34 @@  def remove_remote_dir(
         """Remove remote directory, by default remove recursively and forcefully.
 
         Args:
-            remote_dir_path: The path of the directory to remove.
+            remote_dir_path: The directory path to remove.
             recursive: If :data:`True`, also remove all contents inside the directory.
             force: If :data:`True`, ignore all warnings and try to remove at all costs.
         """
 
+    @abstractmethod
+    def create_remote_tarball(
+        self,
+        remote_dir_path: str | PurePath,
+        compress_format: TarCompressionFormat = TarCompressionFormat.none,
+        exclude: str | list[str] | None = None,
+    ) -> PurePosixPath:
+        """Create a tarball from the contents of the specified remote directory.
+
+        This method creates a tarball containing all files and directories
+        within `remote_dir_path`. The tarball will be saved in the directory of
+        `remote_dir_path` and will be named based on `remote_dir_path`.
+
+        Args:
+            remote_dir_path: The directory path on the remote node.
+            compress_format: The compression format to use. Defaults to no compression.
+            exclude: Patterns for files or directories to exclude from the tarball.
+                These patterns are used with `tar`'s `--exclude` option.
+
+        Returns:
+            The path to the created tarball on the remote node.
+        """
+
     @abstractmethod
     def extract_remote_tarball(
         self,
@@ -227,7 +339,7 @@  def extract_remote_tarball(
         """Extract remote tarball in its remote directory.
 
         Args:
-            remote_tarball_path: The path of the tarball on the remote node.
+            remote_tarball_path: The tarball path on the remote node.
             expected_dir: If non-empty, check whether `expected_dir` exists after extracting
                 the archive.
         """
diff --git a/dts/framework/testbed_model/posix_session.py b/dts/framework/testbed_model/posix_session.py
index 2449c0ab35..94e721da61 100644
--- a/dts/framework/testbed_model/posix_session.py
+++ b/dts/framework/testbed_model/posix_session.py
@@ -18,7 +18,13 @@ 
 from framework.config import Architecture, NodeInfo
 from framework.exception import DPDKBuildError, RemoteCommandExecutionError
 from framework.settings import SETTINGS
-from framework.utils import MesonArgs
+from framework.utils import (
+    MesonArgs,
+    TarCompressionFormat,
+    convert_to_list_of_string,
+    create_tarball,
+    extract_tarball,
+)
 
 from .os_session import OSSession
 
@@ -93,6 +99,48 @@  def copy_to(self, source_file: str | Path, destination_dir: str | PurePath) -> N
         """Overrides :meth:`~.os_session.OSSession.copy_to`."""
         self.remote_session.copy_to(source_file, destination_dir)
 
+    def copy_dir_from(
+        self,
+        source_dir: str | PurePath,
+        destination_dir: str | Path,
+        compress_format: TarCompressionFormat = TarCompressionFormat.none,
+        exclude: str | list[str] | None = None,
+    ) -> None:
+        """Overrides :meth:`~.os_session.OSSession.copy_dir_from`."""
+        source_dir = PurePath(source_dir)
+        remote_tarball_path = self.create_remote_tarball(source_dir, compress_format, exclude)
+
+        self.copy_from(remote_tarball_path, destination_dir)
+        self.remove_remote_file(remote_tarball_path)
+
+        tarball_path = Path(destination_dir, f"{source_dir.name}.{compress_format.extension}")
+        extract_tarball(tarball_path)
+        tarball_path.unlink()
+
+    def copy_dir_to(
+        self,
+        source_dir: str | Path,
+        destination_dir: str | PurePath,
+        compress_format: TarCompressionFormat = TarCompressionFormat.none,
+        exclude: str | list[str] | None = None,
+    ) -> None:
+        """Overrides :meth:`~.os_session.OSSession.copy_dir_to`."""
+        source_dir = Path(source_dir)
+        tarball_path = create_tarball(source_dir, compress_format, exclude=exclude)
+        self.copy_to(tarball_path, destination_dir)
+        tarball_path.unlink()
+
+        remote_tar_path = self.join_remote_path(
+            destination_dir, f"{source_dir.name}.{compress_format.extension}"
+        )
+        self.extract_remote_tarball(remote_tar_path)
+        self.remove_remote_file(remote_tar_path)
+
+    def remove_remote_file(self, remote_file_path: str | PurePath, force: bool = True) -> None:
+        """Overrides :meth:`~.os_session.OSSession.remove_remote_dir`."""
+        opts = PosixSession.combine_short_options(f=force)
+        self.send_command(f"rm{opts} {remote_file_path}")
+
     def remove_remote_dir(
         self,
         remote_dir_path: str | PurePath,
@@ -103,10 +151,42 @@  def remove_remote_dir(
         opts = PosixSession.combine_short_options(r=recursive, f=force)
         self.send_command(f"rm{opts} {remote_dir_path}")
 
-    def extract_remote_tarball(
+    def create_remote_tarball(
         self,
-        remote_tarball_path: str | PurePath,
-        expected_dir: str | PurePath | None = None,
+        remote_dir_path: str | PurePath,
+        compress_format: TarCompressionFormat = TarCompressionFormat.none,
+        exclude: str | list[str] | None = None,
+    ) -> PurePosixPath:
+        """Overrides :meth:`~.os_session.OSSession.create_remote_tarball`."""
+
+        def generate_tar_exclude_args(exclude_patterns) -> str:
+            """Generate args to exclude patterns when creating a tarball.
+
+            Args:
+                exclude_patterns: Patterns for files or directories to exclude from the tarball.
+                    These patterns are used with `tar`'s `--exclude` option.
+
+            Returns:
+                The generated string args to exclude the specified patterns.
+            """
+            if exclude_patterns:
+                exclude_patterns = convert_to_list_of_string(exclude_patterns)
+                return "".join([f" --exclude={pattern}" for pattern in exclude_patterns])
+            return ""
+
+        posix_remote_dir_path = PurePosixPath(remote_dir_path)
+        target_tarball_path = PurePosixPath(f"{remote_dir_path}.{compress_format.extension}")
+
+        self.send_command(
+            f"tar caf {target_tarball_path}{generate_tar_exclude_args(exclude)} "
+            f"-C {posix_remote_dir_path.parent} {posix_remote_dir_path.name}",
+            60,
+        )
+
+        return target_tarball_path
+
+    def extract_remote_tarball(
+        self, remote_tarball_path: str | PurePath, expected_dir: str | PurePath | None = None
     ) -> None:
         """Overrides :meth:`~.os_session.OSSession.extract_remote_tarball`."""
         self.send_command(
diff --git a/dts/framework/utils.py b/dts/framework/utils.py
index 1762d54e97..04b5813613 100644
--- a/dts/framework/utils.py
+++ b/dts/framework/utils.py
@@ -15,13 +15,16 @@ 
 """
 
 import atexit
+import fnmatch
 import json
 import os
 import random
 import subprocess
+import tarfile
 from enum import Enum, Flag
 from pathlib import Path
 from subprocess import SubprocessError
+from typing import Any, Callable
 
 from scapy.layers.inet import IP, TCP, UDP, Ether  # type: ignore[import-untyped]
 from scapy.packet import Packet  # type: ignore[import-untyped]
@@ -146,13 +149,17 @@  def __str__(self) -> str:
         return " ".join(f"{self._default_library} {self._dpdk_args}".split())
 
 
-class _TarCompressionFormat(StrEnum):
+class TarCompressionFormat(StrEnum):
     """Compression formats that tar can use.
 
     Enum names are the shell compression commands
     and Enum values are the associated file extensions.
+
+    The 'none' member represents no compression, only archiving with tar.
+    Its value is set to 'tar' to indicate that the file is an uncompressed tar archive.
     """
 
+    none = "tar"
     gzip = "gz"
     compress = "Z"
     bzip2 = "bz2"
@@ -162,6 +169,16 @@  class _TarCompressionFormat(StrEnum):
     xz = "xz"
     zstd = "zst"
 
+    @property
+    def extension(self):
+        """Return the extension associated with the compression format.
+
+        If the compression format is 'none', the extension will be in the format 'tar'.
+        For other compression formats, the extension will be in the format
+        'tar.{compression format}'.
+        """
+        return f"{self.value}" if self == self.none else f"{self.none.value}.{self.value}"
+
 
 class DPDKGitTarball:
     """Compressed tarball of DPDK from the repository.
@@ -175,7 +192,7 @@  class DPDKGitTarball:
     """
 
     _git_ref: str
-    _tar_compression_format: _TarCompressionFormat
+    _tar_compression_format: TarCompressionFormat
     _tarball_dir: Path
     _tarball_name: str
     _tarball_path: Path | None
@@ -184,7 +201,7 @@  def __init__(
         self,
         git_ref: str,
         output_dir: str,
-        tar_compression_format: _TarCompressionFormat = _TarCompressionFormat.xz,
+        tar_compression_format: TarCompressionFormat = TarCompressionFormat.xz,
     ):
         """Create the tarball during initialization.
 
@@ -205,7 +222,7 @@  def __init__(
         self._create_tarball_dir()
 
         self._tarball_name = (
-            f"dpdk-tarball-{self._git_ref}.tar.{self._tar_compression_format.value}"
+            f"dpdk-tarball-{self._git_ref}.{self._tar_compression_format.extension}"
         )
         self._tarball_path = self._check_tarball_path()
         if not self._tarball_path:
@@ -252,6 +269,72 @@  def __fspath__(self) -> str:
         return str(self._tarball_path)
 
 
+def convert_to_list_of_string(value: Any | list[Any]) -> list[str]:
+    """Convert the input to the list of strings."""
+    return list(map(str, value) if isinstance(value, list) else str(value))
+
+
+def create_tarball(
+    dir_path: Path,
+    compress_format: TarCompressionFormat = TarCompressionFormat.none,
+    exclude: Any | list[Any] | None = None,
+) -> Path:
+    """Create a tarball from the contents of the specified directory.
+
+    This method creates a tarball containing all files and directories within `dir_path`.
+    The tarball will be saved in the directory of `dir_path` and will be named based on `dir_path`.
+
+    Args:
+        dir_path: The directory path.
+        compress_format: The compression format to use. Defaults to no compression.
+        exclude: Patterns for files or directories to exclude from the tarball.
+                These patterns are used with `fnmatch.fnmatch` to filter out files.
+
+    Returns:
+        The path to the created tarball.
+    """
+
+    def create_filter_function(exclude_patterns: str | list[str] | None) -> Callable | None:
+        """Create a filter function based on the provided exclude patterns.
+
+        Args:
+            exclude_patterns: Patterns for files or directories to exclude from the tarball.
+                These patterns are used with `fnmatch.fnmatch` to filter out files.
+
+        Returns:
+            The filter function that excludes files based on the patterns.
+        """
+        if exclude_patterns:
+            exclude_patterns = convert_to_list_of_string(exclude_patterns)
+
+            def filter_func(tarinfo: tarfile.TarInfo) -> tarfile.TarInfo | None:
+                file_name = os.path.basename(tarinfo.name)
+                if any(fnmatch.fnmatch(file_name, pattern) for pattern in exclude_patterns):
+                    return None
+                return tarinfo
+
+            return filter_func
+        return None
+
+    target_tarball_path = dir_path.with_suffix(f".{compress_format.extension}")
+    with tarfile.open(target_tarball_path, f"w:{compress_format.value}") as tar:
+        tar.add(dir_path, arcname=dir_path.name, filter=create_filter_function(exclude))
+
+    return target_tarball_path
+
+
+def extract_tarball(tar_path: str | Path):
+    """Extract the contents of a tarball.
+
+    The tarball will be extracted in the same path as `tar_path` parent path.
+
+    Args:
+        tar_path: The path to the tarball file to extract.
+    """
+    with tarfile.open(tar_path, "r") as tar:
+        tar.extractall(path=Path(tar_path).parent)
+
+
 class PacketProtocols(Flag):
     """Flag specifying which protocols to use for packet generation."""