| Message ID | 20230602095037.97981-3-alexis.lothore@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | add failed test artifacts retriever | expand |
On Fri, 2023-06-02 at 11:50 +0200, Alexis Lothoré via lists.openembedded.org wrote: > Move target stop (for Qemu targets: qemu image stop) later in testimage > main function, especially _after_ having access to general test results, to > allow executing some other actions (like retrieving some files) on DUT > depending on test results (e.g. : retrieve some log files) > > Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> > --- > meta/classes-recipe/testimage.bbclass | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass > index 1bf0cb450ce4..6b10c1db09f9 100644 > --- a/meta/classes-recipe/testimage.bbclass > +++ b/meta/classes-recipe/testimage.bbclass > @@ -393,7 +393,6 @@ def testimage_main(d): > results = tc.results > finally: > signal.signal(signal.SIGTERM, orig_sigterm_handler) > - tc.target.stop() > > # Show results (if we have them) > if results: > @@ -404,6 +403,8 @@ def testimage_main(d): > dump_streams=d.getVar('TESTREPORT_FULLLOGS')) > results.logSummary(pn) > > + tc.target.stop() > + I've not really had a chance to think about this properly, sorry but there is an issue that jumps out at me in this patch. The stop is delibverately in a finally: clause so it always runs even if there is an exception in the code. You've moved it outside of this so the target would no longer be shut down any more in such an exception case. This would potentially leave images running in the case of exceptions and we want to avoid that. Cheers, Richard
Hello Richard, On 6/2/23 13:55, Richard Purdie wrote: > On Fri, 2023-06-02 at 11:50 +0200, Alexis Lothoré via > lists.openembedded.org wrote: >> Move target stop (for Qemu targets: qemu image stop) later in testimage >> main function, especially _after_ having access to general test results, to >> allow executing some other actions (like retrieving some files) on DUT >> depending on test results (e.g. : retrieve some log files) >> >> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> >> --- >> meta/classes-recipe/testimage.bbclass | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass >> index 1bf0cb450ce4..6b10c1db09f9 100644 >> --- a/meta/classes-recipe/testimage.bbclass >> +++ b/meta/classes-recipe/testimage.bbclass >> @@ -393,7 +393,6 @@ def testimage_main(d): >> results = tc.results >> finally: >> signal.signal(signal.SIGTERM, orig_sigterm_handler) >> - tc.target.stop() >> >> # Show results (if we have them) >> if results: >> @@ -404,6 +403,8 @@ def testimage_main(d): >> dump_streams=d.getVar('TESTREPORT_FULLLOGS')) >> results.logSummary(pn) >> >> + tc.target.stop() >> + > > I've not really had a chance to think about this properly, sorry but > there is an issue that jumps out at me in this patch. > > The stop is delibverately in a finally: clause so it always runs even > if there is an exception in the code. You've moved it outside of this > so the target would no longer be shut down any more in such an > exception case. This would potentially leave images running in the case > of exceptions and we want to avoid that. I felt the danger while touching this part (and tested at least with a Ctrl-C that everything was behaving correctly), but indeed I did not think enough about the "any exception other than KeyboardInterrupt or BlockingIOError". I'll then drop the patch moving this stop, and if it's ok, move the artifacts retrieval in the finally clause, just before the stop. > Cheers, > > Richard
diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass index 1bf0cb450ce4..6b10c1db09f9 100644 --- a/meta/classes-recipe/testimage.bbclass +++ b/meta/classes-recipe/testimage.bbclass @@ -393,7 +393,6 @@ def testimage_main(d): results = tc.results finally: signal.signal(signal.SIGTERM, orig_sigterm_handler) - tc.target.stop() # Show results (if we have them) if results: @@ -404,6 +403,8 @@ def testimage_main(d): dump_streams=d.getVar('TESTREPORT_FULLLOGS')) results.logSummary(pn) + tc.target.stop() + # Copy additional logs to tmp/log/oeqa so it's easier to find them targetdir = os.path.join(get_testimage_json_result_dir(d), d.getVar("PN")) os.makedirs(targetdir, exist_ok=True) @@ -415,6 +416,7 @@ def testimage_main(d): if not results.wasSuccessful(): bb.fatal('%s - FAILED - also check the logs in %s' % (pn, d.getVar("LOG_DIR")), forcelog=True) + def get_runtime_paths(d): """ Returns a list of paths where runtime test must reside.
Move target stop (for Qemu targets: qemu image stop) later in testimage main function, especially _after_ having access to general test results, to allow executing some other actions (like retrieving some files) on DUT depending on test results (e.g. : retrieve some log files) Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> --- meta/classes-recipe/testimage.bbclass | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)