diff mbox series

[v2,2/2] testimage: implement test artifacts retriever for failing tests

Message ID 20230607083015.20760-3-alexis.lothore@bootlin.com
State New
Headers show
Series add failed tests artifacts retriever | expand

Commit Message

Alexis Lothoré June 7, 2023, 8:30 a.m. UTC 
Add a basic artifacts retrievers in testimage class which:
- triggers when at least one runtime test fails but tests execution
  encountered no major issue
- reads a list of paths to retrieve from TESTIMAGE_FAILED_QA_ARTIFACTS
- checks for artifacts presence on target
- retrieve those files over scp thanks to existing ssh class
- store those files in an "artifacts" directory in "tmp/log/oeqa/<image>"

This implementation assumes that the SSH or Qemu target has run and
finished gracefully. If tests do not finish because of an exception,
artifacts will not be retrieved

Bring partial solution to [YOCTO #14901]

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
Changes since v1:
- only gather artifacts in nominal case (ie qemu runs without any raised
  exception)
- list artifacts directly in variable instead of using external file
- use standard variables in artifacts paths
- allow glob patterns usage in artifacts paths
- expand/filter artifacts list on target before retrieving them
- tune default artifacts list
---
 meta/classes-recipe/testimage.bbclass | 49 +++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

Comments

Richard Purdie June 7, 2023, 8:49 a.m. UTC | #1 
On Wed, 2023-06-07 at 10:30 +0200, Alexis Lothoré via
lists.openembedded.org wrote:
> Add a basic artifacts retrievers in testimage class which:
> - triggers when at least one runtime test fails but tests execution
>   encountered no major issue
> - reads a list of paths to retrieve from TESTIMAGE_FAILED_QA_ARTIFACTS
> - checks for artifacts presence on target
> - retrieve those files over scp thanks to existing ssh class
> - store those files in an "artifacts" directory in "tmp/log/oeqa/<image>"
> 
> This implementation assumes that the SSH or Qemu target has run and
> finished gracefully. If tests do not finish because of an exception,
> artifacts will not be retrieved
> 
> Bring partial solution to [YOCTO #14901]
> 
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> ---
> Changes since v1:
> - only gather artifacts in nominal case (ie qemu runs without any raised
>   exception)
> - list artifacts directly in variable instead of using external file
> - use standard variables in artifacts paths
> - allow glob patterns usage in artifacts paths
> - expand/filter artifacts list on target before retrieving them
> - tune default artifacts list
> ---
>  meta/classes-recipe/testimage.bbclass | 49 +++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass
> index b48cd96575d2..c6ce74a9e7a8 100644
> --- a/meta/classes-recipe/testimage.bbclass
> +++ b/meta/classes-recipe/testimage.bbclass
> @@ -18,6 +18,16 @@ inherit image-artifact-names
>  
>  TESTIMAGE_AUTO ??= "0"
>  
> +# When any test fails, TESTIMAGE_FAILED_QA ARTIFACTS will be parsed and for
> +# each entry in it, if artifact pointed by path description exists on target,
> +# it will be retrieved onto host
> +
> +TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\
> +    ${localstatedir}/log \
> +    ${sysconfdir}/version \
> +    ${sysconfdir}/os-release \
> +    ${libdir}/*/ptest"

I know Alex Kanavin asked for this but I'm really not sure 
${libdir}/*/ptest is a great idea. This will effectively copy the
entire ptest package files for every ptest on the image. Now we split
ptests to different images it is less of an issue but it is still
potentially rather large. Could we at least limit it to failing ptests?

Cheers,

Richard
Alexander Kanavin June 7, 2023, 9:20 a.m. UTC | #2 
What might work better without code complications is testimage.bbclass
setting only the minimum set (no ptests):

+TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\
+    ${localstatedir}/log \
+    ${sysconfdir}/version \
+    ${sysconfdir}/os-release \
"


Then core-image-ptest.bb should append "${libdir}/{MCNAME}/ptest" to
that. As that same image recipe installs only a single ptest via
IMAGE_INSTALL:append, and the artifacts are retrieved only if that
ptest would fail, this would achieve the same outcome. Other images
(which can potentially include many ptests) can then set what
artifacts to retrieve themselves as they please.

Alex

On Wed, 7 Jun 2023 at 10:49, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Wed, 2023-06-07 at 10:30 +0200, Alexis Lothoré via
> lists.openembedded.org wrote:
> > Add a basic artifacts retrievers in testimage class which:
> > - triggers when at least one runtime test fails but tests execution
> >   encountered no major issue
> > - reads a list of paths to retrieve from TESTIMAGE_FAILED_QA_ARTIFACTS
> > - checks for artifacts presence on target
> > - retrieve those files over scp thanks to existing ssh class
> > - store those files in an "artifacts" directory in "tmp/log/oeqa/<image>"
> >
> > This implementation assumes that the SSH or Qemu target has run and
> > finished gracefully. If tests do not finish because of an exception,
> > artifacts will not be retrieved
> >
> > Bring partial solution to [YOCTO #14901]
> >
> > Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> > ---
> > Changes since v1:
> > - only gather artifacts in nominal case (ie qemu runs without any raised
> >   exception)
> > - list artifacts directly in variable instead of using external file
> > - use standard variables in artifacts paths
> > - allow glob patterns usage in artifacts paths
> > - expand/filter artifacts list on target before retrieving them
> > - tune default artifacts list
> > ---
> >  meta/classes-recipe/testimage.bbclass | 49 +++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass
> > index b48cd96575d2..c6ce74a9e7a8 100644
> > --- a/meta/classes-recipe/testimage.bbclass
> > +++ b/meta/classes-recipe/testimage.bbclass
> > @@ -18,6 +18,16 @@ inherit image-artifact-names
> >
> >  TESTIMAGE_AUTO ??= "0"
> >
> > +# When any test fails, TESTIMAGE_FAILED_QA ARTIFACTS will be parsed and for
> > +# each entry in it, if artifact pointed by path description exists on target,
> > +# it will be retrieved onto host
> > +
> > +TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\
> > +    ${localstatedir}/log \
> > +    ${sysconfdir}/version \
> > +    ${sysconfdir}/os-release \
> > +    ${libdir}/*/ptest"
>
> I know Alex Kanavin asked for this but I'm really not sure
> ${libdir}/*/ptest is a great idea. This will effectively copy the
> entire ptest package files for every ptest on the image. Now we split
> ptests to different images it is less of an issue but it is still
> potentially rather large. Could we at least limit it to failing ptests?
>
> Cheers,
>
> Richard
>
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#182462): https://lists.openembedded.org/g/openembedded-core/message/182462
> Mute This Topic: https://lists.openembedded.org/mt/99380468/1686489
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alex.kanavin@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Alexis Lothoré June 7, 2023, 12:24 p.m. UTC | #3 
Hello Alexander, Richard,

Thanks for the feedback. Indeed my testing setup is quite minimal, so it may not
reflect how many files may be pulled in real cases.

On 6/7/23 11:20, Alexander Kanavin wrote:
> What might work better without code complications is testimage.bbclass
> setting only the minimum set (no ptests):
> 
> +TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\
> +    ${localstatedir}/log \
> +    ${sysconfdir}/version \
> +    ${sysconfdir}/os-release \
> "
> 
> 
> Then core-image-ptest.bb should append "${libdir}/{MCNAME}/ptest" to
> that. As that same image recipe installs only a single ptest via
> IMAGE_INSTALL:append, and the artifacts are retrieved only if that
> ptest would fail, this would achieve the same outcome. Other images
> (which can potentially include many ptests) can then set what
> artifacts to retrieve themselves as they please.

I am not familiar with multiconfig, so I'll have to document myself before
trying what you suggest, especially to make sure it will not make the retriever
too dependant on a proper multiconfig definition.

Thanks,
Alexis
> 
> Alex
> 
> On Wed, 7 Jun 2023 at 10:49, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
>>
>> On Wed, 2023-06-07 at 10:30 +0200, Alexis Lothoré via
>> lists.openembedded.org wrote:
>>> Add a basic artifacts retrievers in testimage class which:
>>> - triggers when at least one runtime test fails but tests execution
>>>   encountered no major issue
>>> - reads a list of paths to retrieve from TESTIMAGE_FAILED_QA_ARTIFACTS
>>> - checks for artifacts presence on target
>>> - retrieve those files over scp thanks to existing ssh class
>>> - store those files in an "artifacts" directory in "tmp/log/oeqa/<image>"
>>>
>>> This implementation assumes that the SSH or Qemu target has run and
>>> finished gracefully. If tests do not finish because of an exception,
>>> artifacts will not be retrieved
>>>
>>> Bring partial solution to [YOCTO #14901]
>>>
>>> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
>>> ---
>>> Changes since v1:
>>> - only gather artifacts in nominal case (ie qemu runs without any raised
>>>   exception)
>>> - list artifacts directly in variable instead of using external file
>>> - use standard variables in artifacts paths
>>> - allow glob patterns usage in artifacts paths
>>> - expand/filter artifacts list on target before retrieving them
>>> - tune default artifacts list
>>> ---
>>>  meta/classes-recipe/testimage.bbclass | 49 +++++++++++++++++++++++++++
>>>  1 file changed, 49 insertions(+)
>>>
>>> diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass
>>> index b48cd96575d2..c6ce74a9e7a8 100644
>>> --- a/meta/classes-recipe/testimage.bbclass
>>> +++ b/meta/classes-recipe/testimage.bbclass
>>> @@ -18,6 +18,16 @@ inherit image-artifact-names
>>>
>>>  TESTIMAGE_AUTO ??= "0"
>>>
>>> +# When any test fails, TESTIMAGE_FAILED_QA ARTIFACTS will be parsed and for
>>> +# each entry in it, if artifact pointed by path description exists on target,
>>> +# it will be retrieved onto host
>>> +
>>> +TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\
>>> +    ${localstatedir}/log \
>>> +    ${sysconfdir}/version \
>>> +    ${sysconfdir}/os-release \
>>> +    ${libdir}/*/ptest"
>>
>> I know Alex Kanavin asked for this but I'm really not sure
>> ${libdir}/*/ptest is a great idea. This will effectively copy the
>> entire ptest package files for every ptest on the image. Now we split
>> ptests to different images it is less of an issue but it is still
>> potentially rather large. Could we at least limit it to failing ptests?
>>
>> Cheers,
>>
>> Richard
>>
>>
>>
>> -=-=-=-=-=-=-=-=-=-=-=-
>> Links: You receive all messages sent to this group.
>> View/Reply Online (#182462): https://lists.openembedded.org/g/openembedded-core/message/182462
>> Mute This Topic: https://lists.openembedded.org/mt/99380468/1686489
>> Group Owner: openembedded-core+owner@lists.openembedded.org
>> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alex.kanavin@gmail.com]
>> -=-=-=-=-=-=-=-=-=-=-=-
>>
Alexander Kanavin June 7, 2023, 12:29 p.m. UTC | #4 
On Wed, 7 Jun 2023 at 14:23, Alexis Lothoré <alexis.lothore@bootlin.com> wrote:
> > Then core-image-ptest.bb should append "${libdir}/{MCNAME}/ptest" to
> > that. As that same image recipe installs only a single ptest via
> > IMAGE_INSTALL:append, and the artifacts are retrieved only if that
> > ptest would fail, this would achieve the same outcome. Other images
> > (which can potentially include many ptests) can then set what
> > artifacts to retrieve themselves as they please.
>
> I am not familiar with multiconfig, so I'll have to document myself before
> trying what you suggest, especially to make sure it will not make the retriever
> too dependant on a proper multiconfig definition.

core-image-ptest.bb is already set for this, so it just adds another
usage of MCNAME variable into it to get the name of the ptest from the
mcextend variant and substitute that into the path that gets added to
TESTIMAGE_FAILED_QA_ARTIFACTS. The retriever won't know anything about
it as it happens at the recipe parsing step.

Alex
Richard Purdie June 7, 2023, 12:32 p.m. UTC | #5 
On Wed, 2023-06-07 at 14:29 +0200, Alexander Kanavin wrote:
> On Wed, 7 Jun 2023 at 14:23, Alexis Lothoré <alexis.lothore@bootlin.com> wrote:
> > > Then core-image-ptest.bb should append "${libdir}/{MCNAME}/ptest" to
> > > that. As that same image recipe installs only a single ptest via
> > > IMAGE_INSTALL:append, and the artifacts are retrieved only if that
> > > ptest would fail, this would achieve the same outcome. Other images
> > > (which can potentially include many ptests) can then set what
> > > artifacts to retrieve themselves as they please.
> > 
> > I am not familiar with multiconfig, so I'll have to document myself before
> > trying what you suggest, especially to make sure it will not make the retriever
> > too dependant on a proper multiconfig definition.
> 
> core-image-ptest.bb is already set for this, so it just adds another
> usage of MCNAME variable into it to get the name of the ptest from the
> mcextend variant and substitute that into the path that gets added to
> TESTIMAGE_FAILED_QA_ARTIFACTS. The retriever won't know anything about
> it as it happens at the recipe parsing step.

It does seem like a neat way to handle things to me FWIW.

Cheers,

Richard
diff mbox series

Patch

diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass
index b48cd96575d2..c6ce74a9e7a8 100644
--- a/meta/classes-recipe/testimage.bbclass
+++ b/meta/classes-recipe/testimage.bbclass
@@ -18,6 +18,16 @@  inherit image-artifact-names
 
 TESTIMAGE_AUTO ??= "0"
 
+# When any test fails, TESTIMAGE_FAILED_QA ARTIFACTS will be parsed and for
+# each entry in it, if artifact pointed by path description exists on target,
+# it will be retrieved onto host
+
+TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\
+    ${localstatedir}/log \
+    ${sysconfdir}/version \
+    ${sysconfdir}/os-release \
+    ${libdir}/*/ptest"
+
 # You can set (or append to) TEST_SUITES in local.conf to select the tests
 # which you want to run for your target.
 # The test names are the module names in meta/lib/oeqa/runtime/cases.
@@ -192,6 +202,39 @@  def get_testimage_boot_patterns(d):
                 boot_patterns[flag] = flagval.encode().decode('unicode-escape')
     return boot_patterns
 
+def get_artifacts_list(target, raw_list):
+    result = []
+    # Passed list may contains patterns in paths, expand them directly on target
+    for raw_path in raw_list.split():
+        cmd = f"for p in {raw_path}; do if [ -e $p ]; then echo $p; fi; done"
+        try:
+            status, output = target.run(cmd)
+            if status != 0 or not output:
+                raise Exception()
+            result += output.split()
+        except:
+            bb.warn(f"No file/directory matching path {raw_path}")
+
+    return result
+
+def retrieve_test_artifacts(target, artifacts_list, target_dir):
+    import shutil
+
+    local_artifacts_dir = os.path.join(target_dir, "artifacts")
+    if os.path.isdir(local_artifacts_dir):
+        shutil.rmtree(local_artifacts_dir)
+
+    os.makedirs(local_artifacts_dir)
+    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:
+            bb.warn(f"Can not retrieve {artifact_path} from test target")
 
 def testimage_main(d):
     import os
@@ -383,6 +426,12 @@  def testimage_main(d):
             pass
         results = tc.runTests()
         complete = True
+        if not results.wasSuccessful():
+            artifacts_list = get_artifacts_list(tc.target, d.getVar("TESTIMAGE_FAILED_QA_ARTIFACTS"))
+            if not artifacts_list:
+                bb.warn("Could not load artifacts list, skip artifacts retrieval")
+            else:
+                retrieve_test_artifacts(tc.target, artifacts_list, get_testimage_json_result_dir(d))
     except (KeyboardInterrupt, BlockingIOError) as err:
         if isinstance(err, KeyboardInterrupt):
             bb.error('testimage interrupted, shutting down...')