diff mbox series

[2/2] oeqa/utils/postactions: transfer whole archive over ssh instead of doing individual copies

Message ID 20240705144638.441976-3-alexis.lothore@bootlin.com
State Accepted, archived
Commit 4aeb10aa38efc6768928fbb74985e36e972b8e46
Headers show
Series oeqa/utils/postactions: fix disk storage consumption | expand

Commit Message

Alexis Lothoré July 5, 2024, 2:46 p.m. UTC 
From: Alexis Lothoré <alexis.lothore@bootlin.com>

Fixes [YOCTO 15536]

The postactions retrieval actions currently rely on scp executed
individually on any file or directory expanded from
TESTIMAGE_FAILED_QA_ARTIFACTS. Unfortunately, symlinks are not preserved
with this mechanism, which lead to big storage space consumption. Things
may go even worse if those symlinks create some circular chains. This
mechanism then needs to be updated to preserve symlinks instead of
following them during copy. There are multiple ways to do it:
- create a local archive on the target and execute scp on this file
- use rsync instead of scp for all files
- create an archive and pipe it to ssh instead of storing it onto the
  target

The first solution may create pressure on targets storage space, while the
second assumes that rsync is installed on the target, which may not be
true. So the third one is a compromise: tar is very likely present, at
least through busybox, and no disk space is used on the target.

Replace the current per-file scp call by a single call to tar run on the
target. Retrieve the generated compressed archive directly from SSH output,
and feed it to another tar process but on host, to uncompress and extract
it at the same place as before.

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 meta/lib/oeqa/utils/postactions.py | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

patchtest@automation.yoctoproject.org July 5, 2024, 3:06 p.m. UTC | #1 
Thank you for your submission. Patchtest identified one
or more issues with the patch. Please see the log below for
more information:

---
Testing patch /home/patchtest/share/mboxes/2-2-oeqa-utils-postactions-transfer-whole-archive-over-ssh-instead-of-doing-individual-copies.patch

FAIL: test bugzilla entry format: Bugzilla issue ID is not correctly formatted - specify it with format: "[YOCTO #<bugzilla ID>]" (test_mbox.TestMbox.test_bugzilla_entry_format)

PASS: pretest pylint (test_python_pylint.PyLint.pretest_pylint)
PASS: test Signed-off-by presence (test_mbox.TestMbox.test_signed_off_by_presence)
PASS: test author valid (test_mbox.TestMbox.test_author_valid)
PASS: test commit message presence (test_mbox.TestMbox.test_commit_message_presence)
PASS: test max line length (test_metadata.TestMetadata.test_max_line_length)
PASS: test mbox format (test_mbox.TestMbox.test_mbox_format)
PASS: test non-AUH upgrade (test_mbox.TestMbox.test_non_auh_upgrade)
PASS: test pylint (test_python_pylint.PyLint.test_pylint)
PASS: test shortlog format (test_mbox.TestMbox.test_shortlog_format)
PASS: test shortlog length (test_mbox.TestMbox.test_shortlog_length)

SKIP: pretest src uri left files: No modified recipes, skipping pretest (test_metadata.TestMetadata.pretest_src_uri_left_files)
SKIP: test CVE check ignore: No modified recipes or older target branch, skipping test (test_metadata.TestMetadata.test_cve_check_ignore)
SKIP: test CVE tag format: No new CVE patches introduced (test_patch.TestPatch.test_cve_tag_format)
SKIP: test Signed-off-by presence: No new CVE patches introduced (test_patch.TestPatch.test_signed_off_by_presence)
SKIP: test Upstream-Status presence: No new CVE patches introduced (test_patch.TestPatch.test_upstream_status_presence_format)
SKIP: test lic files chksum modified not mentioned: No modified recipes, skipping test (test_metadata.TestMetadata.test_lic_files_chksum_modified_not_mentioned)
SKIP: test lic files chksum presence: No added recipes, skipping test (test_metadata.TestMetadata.test_lic_files_chksum_presence)
SKIP: test license presence: No added recipes, skipping test (test_metadata.TestMetadata.test_license_presence)
SKIP: test series merge on head: Merge test is disabled for now (test_mbox.TestMbox.test_series_merge_on_head)
SKIP: test src uri left files: No modified recipes, skipping pretest (test_metadata.TestMetadata.test_src_uri_left_files)
SKIP: test summary presence: No added recipes, skipping test (test_metadata.TestMetadata.test_summary_presence)
SKIP: test target mailing list: Series merged, no reason to check other mailing lists (test_mbox.TestMbox.test_target_mailing_list)

---

Please address the issues identified and
submit a new revision of the patch, or alternatively, reply to this
email with an explanation of why the patch should be accepted. If you
believe these results are due to an error in patchtest, please submit a
bug at https://bugzilla.yoctoproject.org/ (use the 'Patchtest' category
under 'Yocto Project Subprojects'). For more information on specific
failures, see: https://wiki.yoctoproject.org/wiki/Patchtest. Thank
you!
Richard Purdie July 7, 2024, 8:10 a.m. UTC | #2 
On Fri, 2024-07-05 at 16:46 +0200, Alexis Lothoré via lists.openembedded.org wrote:
> From: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> Fixes [YOCTO 15536]
> 
> The postactions retrieval actions currently rely on scp executed
> individually on any file or directory expanded from
> TESTIMAGE_FAILED_QA_ARTIFACTS. Unfortunately, symlinks are not preserved
> with this mechanism, which lead to big storage space consumption. Things
> may go even worse if those symlinks create some circular chains. This
> mechanism then needs to be updated to preserve symlinks instead of
> following them during copy. There are multiple ways to do it:
> - create a local archive on the target and execute scp on this file
> - use rsync instead of scp for all files
> - create an archive and pipe it to ssh instead of storing it onto the
>   target
> 
> The first solution may create pressure on targets storage space, while the
> second assumes that rsync is installed on the target, which may not be
> true. So the third one is a compromise: tar is very likely present, at
> least through busybox, and no disk space is used on the target.
> 
> Replace the current per-file scp call by a single call to tar run on the
> target. Retrieve the generated compressed archive directly from SSH output,
> and feed it to another tar process but on host, to uncompress and extract
> it at the same place as before.
> 
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> ---
>  meta/lib/oeqa/utils/postactions.py | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/meta/lib/oeqa/utils/postactions.py b/meta/lib/oeqa/utils/postactions.py
> index ecdddd2d40e3..f7ce25f2af8e 100644
> --- a/meta/lib/oeqa/utils/postactions.py
> +++ b/meta/lib/oeqa/utils/postactions.py
> @@ -62,17 +62,16 @@ def get_artifacts_list(target, raw_list):
>      return result
>  
>  def retrieve_test_artifacts(target, artifacts_list, target_dir):
> +    import io, subprocess
>      local_artifacts_dir = os.path.join(target_dir, "artifacts")
> -    for artifact_path in artifacts_list:
> -        if not os.path.isabs(artifact_path):
> -            bb.warn(f"{artifact_path} is not an absolute path")
> -            continue
> -        try:
> -            dest_dir = os.path.join(local_artifacts_dir, os.path.dirname(artifact_path[1:]))
> -            os.makedirs(dest_dir, exist_ok=True)
> -            target.copyFrom(artifact_path, dest_dir)
> -        except Exception as e:
> -            bb.warn(f"Can not retrieve {artifact_path} from test target: {e}")
> +    try:
> +        cmd = f"tar zcf - {" ".join(artifacts_list)}"

Thanks for the patch. The syntax above causes failures:

https://valkyrie.yoctoproject.org/#/builders/95/builds/59/steps/14/logs/stdio

I've put a fix on master-next which I can squash in but it does make me
wonder how it was tested.

Cheers,

Richard


> +        (status, output) = target.run(cmd, raw = True)
> +        if status != 0 or not output:
> +            raise Exception("Error while fetching compressed artifacts")
> +        p = subprocess.run(["tar", "zxf", "-", "-C", local_artifacts_dir], input=output)
> +    except Exception as e:
> +        bb.warn(f"Can not retrieve {artifact_path} from test target: {e}")
>  
>  def list_and_fetch_failed_tests_artifacts(d, tc):
>      artifacts_list = get_artifacts_list(tc.target, d.getVar("TESTIMAGE_FAILED_QA_ARTIFACTS"))
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#201610): https://lists.openembedded.org/g/openembedded-core/message/201610
> Mute This Topic: https://lists.openembedded.org/mt/107054834/1686473
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [richard.purdie@linuxfoundation.org]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Alexis Lothoré July 8, 2024, 9:10 p.m. UTC | #3 
Hello Richard,

On 7/7/24 10:10, Richard Purdie wrote:
> On Fri, 2024-07-05 at 16:46 +0200, Alexis Lothoré via lists.openembedded.org wrote:
>> From: Alexis Lothoré <alexis.lothore@bootlin.com>
>> +    try:
>> +        cmd = f"tar zcf - {" ".join(artifacts_list)}"
> 
> Thanks for the patch. The syntax above causes failures:
> 
> https://valkyrie.yoctoproject.org/#/builders/95/builds/59/steps/14/logs/stdio
> 
> I've put a fix on master-next which I can squash in but it does make me
> wonder how it was tested.

Sorry about that. This error made me doubt about my testing setup, so I ran new
tests, and I confirm that this line is being executed and returns a correct tar
command on my machine, which confused me even more because it indeed looks broken.

My best guess about those differences is a mismatch on python versions between
the autobuilder and my machine. I am running python 3.12, and it appears that it
has brought many improvements to python f-strings, especially "quote reuse",
which makes python not raise an exception anymore when the expression inside an
f-string reuses the same kind of quotes as for the parent f-string (see [1]). I
ran a quick test with Python 3.11 instead of 3.12, and I now I see the exception.

I guess it would be better for me to keep working with the same python version
as in the autobuilder. I see mentions of python 3.6 in README-WALKTHROUGHS.md
from the autobuilder2 repository, but I am not sure how up-to-date it is. Could
you please confirm whether this is the correct version or not?

Thanks,

Alexis

[1]
https://docs.python.org/3/whatsnew/3.12.html#pep-701-syntactic-formalization-of-f-strings
Richard Purdie July 8, 2024, 9:37 p.m. UTC | #4 
On Mon, 2024-07-08 at 23:10 +0200, Alexis Lothoré wrote:
> Hello Richard,
> 
> On 7/7/24 10:10, Richard Purdie wrote:
> > On Fri, 2024-07-05 at 16:46 +0200, Alexis Lothoré via lists.openembedded.org wrote:
> > > From: Alexis Lothoré <alexis.lothore@bootlin.com>
> > > +    try:
> > > +        cmd = f"tar zcf - {" ".join(artifacts_list)}"
> > 
> > Thanks for the patch. The syntax above causes failures:
> > 
> > https://valkyrie.yoctoproject.org/#/builders/95/builds/59/steps/14/logs/stdio
> > 
> > I've put a fix on master-next which I can squash in but it does make me
> > wonder how it was tested.
> 
> Sorry about that. This error made me doubt about my testing setup, so I ran new
> tests, and I confirm that this line is being executed and returns a correct tar
> command on my machine, which confused me even more because it indeed looks broken.
> 
> My best guess about those differences is a mismatch on python versions between
> the autobuilder and my machine. I am running python 3.12, and it appears that it
> has brought many improvements to python f-strings, especially "quote reuse",
> which makes python not raise an exception anymore when the expression inside an
> f-string reuses the same kind of quotes as for the parent f-string (see [1]). I
> ran a quick test with Python 3.11 instead of 3.12, and I now I see the exception.

That explains things. I'm very surprised any version of python accepts
that formatting to be honest!

> I guess it would be better for me to keep working with the same python version
> as in the autobuilder. I see mentions of python 3.6 in README-WALKTHROUGHS.md
> from the autobuilder2 repository, but I am not sure how up-to-date it is. Could
> you please confirm whether this is the correct version or not?

The docs are out of date and the definitive version is the one checked in bitbake:

https://git.openembedded.org/bitbake/tree/lib/bb/__init__.py#n15

"Sorry, python 3.8.0 or later is required for this version of bitbake"

so 3.8+ is our current requirement.

Cheers,

Richard
Alexis Lothoré July 9, 2024, 8:06 a.m. UTC | #5 
On 7/8/24 23:37, Richard Purdie wrote:
> On Mon, 2024-07-08 at 23:10 +0200, Alexis Lothoré wrote:
>> Hello Richard,

[...]

>> My best guess about those differences is a mismatch on python versions between
>> the autobuilder and my machine. I am running python 3.12, and it appears that it
>> has brought many improvements to python f-strings, especially "quote reuse",
>> which makes python not raise an exception anymore when the expression inside an
>> f-string reuses the same kind of quotes as for the parent f-string (see [1]). I
>> ran a quick test with Python 3.11 instead of 3.12, and I now I see the exception.
> 
> That explains things. I'm very surprised any version of python accepts
> that formatting to be honest!
> 
>> I guess it would be better for me to keep working with the same python version
>> as in the autobuilder. I see mentions of python 3.6 in README-WALKTHROUGHS.md
>> from the autobuilder2 repository, but I am not sure how up-to-date it is. Could
>> you please confirm whether this is the correct version or not?
> 
> The docs are out of date and the definitive version is the one checked in bitbake:
> 
> https://git.openembedded.org/bitbake/tree/lib/bb/__init__.py#n15
> 
> "Sorry, python 3.8.0 or later is required for this version of bitbake"
> 
> so 3.8+ is our current requirement.

ACK, I'll then align with this version for any future development.

Thanks,

Alexis
diff mbox series

Patch

diff --git a/meta/lib/oeqa/utils/postactions.py b/meta/lib/oeqa/utils/postactions.py
index ecdddd2d40e3..f7ce25f2af8e 100644
--- a/meta/lib/oeqa/utils/postactions.py
+++ b/meta/lib/oeqa/utils/postactions.py
@@ -62,17 +62,16 @@  def get_artifacts_list(target, raw_list):
     return result
 
 def retrieve_test_artifacts(target, artifacts_list, target_dir):
+    import io, subprocess
     local_artifacts_dir = os.path.join(target_dir, "artifacts")
-    for artifact_path in artifacts_list:
-        if not os.path.isabs(artifact_path):
-            bb.warn(f"{artifact_path} is not an absolute path")
-            continue
-        try:
-            dest_dir = os.path.join(local_artifacts_dir, os.path.dirname(artifact_path[1:]))
-            os.makedirs(dest_dir, exist_ok=True)
-            target.copyFrom(artifact_path, dest_dir)
-        except Exception as e:
-            bb.warn(f"Can not retrieve {artifact_path} from test target: {e}")
+    try:
+        cmd = f"tar zcf - {" ".join(artifacts_list)}"
+        (status, output) = target.run(cmd, raw = True)
+        if status != 0 or not output:
+            raise Exception("Error while fetching compressed artifacts")
+        p = subprocess.run(["tar", "zxf", "-", "-C", local_artifacts_dir], input=output)
+    except Exception as e:
+        bb.warn(f"Can not retrieve {artifact_path} from test target: {e}")
 
 def list_and_fetch_failed_tests_artifacts(d, tc):
     artifacts_list = get_artifacts_list(tc.target, d.getVar("TESTIMAGE_FAILED_QA_ARTIFACTS"))