[RFC PATCH 0/1] dts: add SUT cleanup to framework

[RFC PATCH 0/1] dts: add SUT cleanup to framework

[Dean Marx]

Adds/modifies methods in sut_node, node, and runner to
remove dpdk tarball/tree from remote server after execution ends.
If the user specifies the remote option in the configuration, the
removal step is skipped. This attempts to fix the problem of
multiple users running on the same server, where shared names
of tarballs/trees can cause permission errors.

Dean Marx (1):
  dts: add SUT cleanup to framework

 dts/framework/runner.py                 |  2 +-
 dts/framework/testbed_model/sut_node.py | 23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 4 deletions(-)

-- 
2.44.0

[RFC PATCH 1/1] dts: add SUT cleanup to framework

[Dean Marx]

Add method cleanup_sut to framework that removes a
DPDK source from a SUT node at the conclusion of a testrun.
This will only run when the DPDK source is being copied
from the DTS engine node during the testrun
(when remote=false in the conf.yaml).

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/runner.py                 |  2 +-
 dts/framework/testbed_model/sut_node.py | 23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/dts/framework/runner.py b/dts/framework/runner.py
index f91c462ce5..7f58cd01ce 100644
--- a/dts/framework/runner.py
+++ b/dts/framework/runner.py
@@ -340,7 +340,7 @@ def _run_test_run(
         finally:
             try:
                 self._logger.set_stage(DtsStage.test_run_teardown)
-                sut_node.tear_down_test_run()
+                sut_node.tear_down_test_run(test_run_config.dpdk_config)
                 tg_node.tear_down_test_run()
                 test_run_result.update_teardown(Result.PASS)
             except Exception as e:
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 14d77c50a6..d39c1bd632 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -175,6 +175,21 @@ def path_to_devbind_script(self) -> PurePath | str:
             )
         return self._path_to_devbind_script
 
+    def cleanup_sut(
+        self, dpdk_build_config: DPDKBuildConfiguration, remote_tree: str | PurePath | None
+    ) -> None:
+        """Removes the DPDK tree and/or build directory/tarball depending on the configuration."""
+        match dpdk_build_config.dpdk_location:
+            case LocalDPDKTreeLocation(dpdk_tree=dpdk_tree):
+                tree_path = self.main_session.join_remote_path(self._remote_tmp_dir, dpdk_tree.name)
+                self.main_session.remove_remote_dir(tree_path)
+            case LocalDPDKTarballLocation(tarball=tarball):
+                self.main_session.remove_remote_dir(str(remote_tree))
+                tarball_path = self.main_session.join_remote_path(
+                    self._remote_tmp_dir, tarball.name
+                )
+                self.main_session.remove_remote_file(tarball_path)
+
     def get_dpdk_build_info(self) -> DPDKBuildInfo:
         """Get additional DPDK build information.
 
@@ -203,11 +218,11 @@ def set_up_test_run(
             self.virtual_devices.append(VirtualDevice(vdev))
         self._set_up_dpdk(dpdk_build_config)
 
-    def tear_down_test_run(self) -> None:
+    def tear_down_test_run(self, dpdk_build_config: DPDKBuildConfiguration) -> None:
         """Extend the test run teardown with virtual device teardown and DPDK teardown."""
         super().tear_down_test_run()
         self.virtual_devices = []
-        self._tear_down_dpdk()
+        self._tear_down_dpdk(dpdk_build_config)
 
     def _set_up_dpdk(
         self,
@@ -243,14 +258,16 @@ def _set_up_dpdk(
 
         self.bind_ports_to_driver()
 
-    def _tear_down_dpdk(self) -> None:
+    def _tear_down_dpdk(self, dpdk_build_config: DPDKBuildConfiguration) -> None:
         """Reset DPDK variables and bind port driver to the OS driver."""
         self._env_vars = {}
+        remote_tree = self.__remote_dpdk_tree_path
         self.__remote_dpdk_tree_path = None
         self._remote_dpdk_build_dir = None
         self._dpdk_version = None
         self.compiler_version = None
         self.bind_ports_to_driver(for_dpdk=False)
+        self.cleanup_sut(dpdk_build_config, remote_tree)
 
     def _set_remote_dpdk_tree_path(self, dpdk_tree: PurePath):
         """Set the path to the remote DPDK source tree based on the provided DPDK location.
-- 
2.44.0

Re: [RFC PATCH 1/1] dts: add SUT cleanup to framework

[Patrick Robb]

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

On Mon, Dec 9, 2024 at 2:58 PM Dean Marx <dmarx@iol.unh.edu> wrote:

>
> -                sut_node.tear_down_test_run()
> +                sut_node.tear_down_test_run(test_run_config.dpdk_config)
>

My first question was why is there a need to pass all of dpdk_config
through from here all the way to cleanup_sut()? Is it sufficient to extract
dpdk_location from dpdk_config at the beginning here, and pass along just
dpdk_location?


>                  tg_node.tear_down_test_run()
>                  test_run_result.update_teardown(Result.PASS)
>              except Exception as e:
> diff --git a/dts/framework/testbed_model/sut_node.py
> b/dts/framework/testbed_model/sut_node.py
> index 14d77c50a6..d39c1bd632 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -175,6 +175,21 @@ def path_to_devbind_script(self) -> PurePath | str:
>              )
>          return self._path_to_devbind_script
>
> +    def cleanup_sut(
> +        self, dpdk_build_config: DPDKBuildConfiguration, remote_tree: str
> | PurePath | None
>

I understand the need for str or PurePath, but can you explain why None is
included in the union?


> --
> 2.44.0
>
>
There was also discussion at the previous DTS meeting about appending the
datetime to the dpdk artifacts when they're copied over to the SUT (a way
to create artifact uniqueness). It's not really within the scope of this
patchseries, but it wouldn't hurt for us to touch base - maybe you can do
this next.

Looks good to me overall. Once you submit the v2 I will leave a little time
for other comments, otherwise I will merge.

Thanks Dean.

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

Re: [RFC PATCH 1/1] dts: add SUT cleanup to framework

[Patrick Robb]

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

Tested-by: Patrick Robb <probb@iol.unh.edu>

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

Re: [RFC PATCH 1/1] dts: add SUT cleanup to framework

[Dean Marx]

> My first question was why is there a need to pass all of dpdk_config through from here all the way to cleanup_sut()? Is it sufficient to extract dpdk_location from dpdk_config at the beginning here, and pass along just dpdk_location?

I can do that too if it's preferred, wasn't sure what the best way to
handle that was my apologies

> I understand the need for str or PurePath, but can you explain why None is included in the union?

I believe it was just so the linter wouldn't throw errors in the check
format script, but I can double check that.

> There was also discussion at the previous DTS meeting about appending the datetime to the dpdk artifacts when they're copied over to the SUT (a way to create artifact uniqueness). It's not really within the scope of this patchseries, but it wouldn't hurt for us to touch base - maybe you can do this next.

Yes, that's in progress currently but I've put it on the backburner
for now. Will send out a separate RFC for that sometime soon.

> Looks good to me overall. Once you submit the v2 I will leave a little time for other comments, otherwise I will merge.
>
> Thanks Dean.

Sounds good, I'll get that out asap

[PATCH v2] dts: add SUT cleanup to framework

[Dean Marx]

Add method cleanup_sut to framework that removes a
DPDK source from a SUT node at the conclusion of a testrun.
This will only run when the DPDK source is being copied
from the DTS engine node during the testrun
(when remote=false in the conf.yaml).

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/runner.py                 |  2 +-
 dts/framework/testbed_model/sut_node.py | 21 ++++++++++++++++++---
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/dts/framework/runner.py b/dts/framework/runner.py
index 9f9789cf49..367034e038 100644
--- a/dts/framework/runner.py
+++ b/dts/framework/runner.py
@@ -336,7 +336,7 @@ def _run_test_run(
         finally:
             try:
                 self._logger.set_stage(DtsStage.test_run_teardown)
-                sut_node.tear_down_test_run()
+                sut_node.tear_down_test_run(test_run_config.dpdk_config.dpdk_location)
                 tg_node.tear_down_test_run()
                 test_run_result.update_teardown(Result.PASS)
             except Exception as e:
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 483733cede..c73500b146 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -188,6 +188,19 @@ def path_to_devbind_script(self) -> PurePath | str:
             )
         return self._path_to_devbind_script
 
+    def cleanup_sut(self, dpdk_build_location, remote_tree: str | PurePath | None) -> None:
+        """Removes the DPDK tree and/or build directory/tarball depending on the configuration."""
+        match dpdk_build_location:
+            case LocalDPDKTreeLocation(dpdk_tree=dpdk_tree):
+                tree_path = self.main_session.join_remote_path(self._remote_tmp_dir, dpdk_tree.name)
+                self.main_session.remove_remote_dir(tree_path)
+            case LocalDPDKTarballLocation(tarball=tarball):
+                self.main_session.remove_remote_dir(str(remote_tree))
+                tarball_path = self.main_session.join_remote_path(
+                    self._remote_tmp_dir, tarball.name
+                )
+                self.main_session.remove_remote_file(tarball_path)
+
     def get_dpdk_build_info(self) -> DPDKBuildInfo:
         """Get additional DPDK build information.
 
@@ -216,11 +229,11 @@ def set_up_test_run(
             self.virtual_devices.append(VirtualDevice(vdev))
         self._set_up_dpdk(dpdk_build_config)
 
-    def tear_down_test_run(self) -> None:
+    def tear_down_test_run(self, dpdk_build_location=None) -> None:
         """Extend the test run teardown with virtual device teardown and DPDK teardown."""
         super().tear_down_test_run()
         self.virtual_devices = []
-        self._tear_down_dpdk()
+        self._tear_down_dpdk(dpdk_build_location)
 
     def _set_up_dpdk(
         self,
@@ -256,14 +269,16 @@ def _set_up_dpdk(
 
         self.bind_ports_to_driver()
 
-    def _tear_down_dpdk(self) -> None:
+    def _tear_down_dpdk(self, dpdk_build_location) -> None:
         """Reset DPDK variables and bind port driver to the OS driver."""
         self._env_vars = {}
+        remote_tree = self.__remote_dpdk_tree_path
         self.__remote_dpdk_tree_path = None
         self._remote_dpdk_build_dir = None
         self._dpdk_version = None
         self.compiler_version = None
         self.bind_ports_to_driver(for_dpdk=False)
+        self.cleanup_sut(dpdk_build_location, remote_tree)
 
     def _set_remote_dpdk_tree_path(self, dpdk_tree: PurePath):
         """Set the path to the remote DPDK source tree based on the provided DPDK location.
-- 
2.48.1