[PATCH 0/4] ci: fix hang of FreeBSD CI jobs

[PATCH 0/4] ci: fix hang of FreeBSD CI jobs

[Daniel P. Berrangé]

This addresses

  https://gitlab.com/qemu-project/qemu/-/issues/1882

Which turned out to be a genuine flaw which we missed during merge
as the patch hitting master co-incided with the FreeBSD CI job
having an temporary outage due to changed release image version.

Daniel P. Berrangé (4):
  microbit: add missing qtest_quit() call
  qtest: kill orphaned qtest QEMU processes on FreeBSD
  gitlab: make Cirrus CI timeout explicit
  gitlab: make Cirrus CI jobs gating

 .gitlab-ci.d/cirrus.yml       | 4 +++-
 .gitlab-ci.d/cirrus/build.yml | 2 ++
 tests/qtest/libqtest.c        | 7 +++++++
 tests/qtest/microbit-test.c   | 2 ++
 4 files changed, 14 insertions(+), 1 deletion(-)

-- 
2.41.0

[PATCH 1/4] microbit: add missing qtest_quit() call

[Daniel P. Berrangé]

Without this call, the QEMU process is being left running which on
FreeBSD 13.2 at least, makes meson think the test is still running,
and thus execution of "make check" continues forever.

This fixes the regression introduced in:

  commit a9c9bbee855877293683012942d3485d50f286af
  Author: Chris Laplante <chris@laplante.io>
  Date:   Tue Aug 22 17:31:02 2023 +0100

    qtest: microbit-test: add tests for nRF51 DETECT

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1882
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qtest/microbit-test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qtest/microbit-test.c b/tests/qtest/microbit-test.c
index 2abcad8e31..72190d38f7 100644
--- a/tests/qtest/microbit-test.c
+++ b/tests/qtest/microbit-test.c
@@ -434,6 +434,8 @@ static void test_nrf51_gpio_detect(void)
     g_assert_true(qtest_get_irq(qts, 0));
     qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 3, 0);
     g_assert_true(qtest_get_irq(qts, 0));
+
+    qtest_quit(qts);
 }
 
 static void timer_task(QTestState *qts, hwaddr task)
-- 
2.41.0

Re: [PATCH 1/4] microbit: add missing qtest_quit() call

[Richard Henderson]

On 9/12/23 11:41, Daniel P. Berrangé wrote:
> Without this call, the QEMU process is being left running which on
> FreeBSD 13.2 at least, makes meson think the test is still running,
> and thus execution of "make check" continues forever.
> 
> This fixes the regression introduced in:
> 
>    commit a9c9bbee855877293683012942d3485d50f286af
>    Author: Chris Laplante<chris@laplante.io>
>    Date:   Tue Aug 22 17:31:02 2023 +0100
> 
>      qtest: microbit-test: add tests for nRF51 DETECT
> 
> Fixes:https://gitlab.com/qemu-project/qemu/-/issues/1882
> Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> ---
>   tests/qtest/microbit-test.c | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

But I think that it's unfortunate that we have to remember this for each test.


r~

Re: [PATCH 1/4] microbit: add missing qtest_quit() call

[Daniel P. Berrangé]

On Tue, Sep 12, 2023 at 12:10:19PM -0700, Richard Henderson wrote:
> On 9/12/23 11:41, Daniel P. Berrangé wrote:
> > Without this call, the QEMU process is being left running which on
> > FreeBSD 13.2 at least, makes meson think the test is still running,
> > and thus execution of "make check" continues forever.
> > 
> > This fixes the regression introduced in:
> > 
> >    commit a9c9bbee855877293683012942d3485d50f286af
> >    Author: Chris Laplante<chris@laplante.io>
> >    Date:   Tue Aug 22 17:31:02 2023 +0100
> > 
> >      qtest: microbit-test: add tests for nRF51 DETECT
> > 
> > Fixes:https://gitlab.com/qemu-project/qemu/-/issues/1882
> > Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> > ---
> >   tests/qtest/microbit-test.c | 2 ++
> >   1 file changed, 2 insertions(+)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> But I think that it's unfortunate that we have to remember this for each test.

We should use G_DEFINE_AUTOPTR_CLEANUP_FUNC for QTestState, and
then we can change tests to declare

   g_autoptr(QTestState) qts = qtest_init(....)

which will make it a bit more robust against forgotten cleanup.



We register an ABRT handler to kill off QEMU manually during
g_asserts().

The "death signal" code will give another layer of robustness
for exits too on Linux and now FreeBSD.

If we really wanted to we could add a 3rd layer of defence by
adding an atexit() handler, but I'm not sure this last one is
worth it for something we're not hitting frequently AFAIR.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH 2/4] qtest: kill orphaned qtest QEMU processes on FreeBSD

[Daniel P. Berrangé]

On Linux we use PR_SET_PDEATHSIG to kill orphaned QEMU processes
if we fail to call qtest_quit(), or the test program aborts/segvs.
This prevents meson from hanging forever due to the orphaned
process keeping stdout open.

On FreeBSD we can achieve the same using PROC_PDEATHSIG_CTL, which
gives us the equivalent protection against hangs.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qtest/libqtest.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 34b9c14b75..b1eba71ffe 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -24,6 +24,9 @@
 #ifdef __linux__
 #include <sys/prctl.h>
 #endif /* __linux__ */
+#ifdef __FreeBSD__
+#include <sys/procctl.h>
+#endif /* __FreeBSD__ */
 
 #include "libqtest.h"
 #include "libqmp.h"
@@ -414,6 +417,10 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
          */
         prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
 #endif /* __linux__ */
+#ifdef __FreeBSD__
+        int sig = SIGKILL;
+        procctl(P_PID, getpid(), PROC_PDEATHSIG_CTL, &sig);
+#endif /* __FreeBSD__ */
         if (!g_setenv("QEMU_AUDIO_DRV", "none", true)) {
             exit(1);
         }
-- 
2.41.0

Re: [PATCH 2/4] qtest: kill orphaned qtest QEMU processes on FreeBSD

[Richard Henderson]

On 9/12/23 11:41, Daniel P. Berrangé wrote:
> On Linux we use PR_SET_PDEATHSIG to kill orphaned QEMU processes
> if we fail to call qtest_quit(), or the test program aborts/segvs.
> This prevents meson from hanging forever due to the orphaned
> process keeping stdout open.
> 
> On FreeBSD we can achieve the same using PROC_PDEATHSIG_CTL, which
> gives us the equivalent protection against hangs.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qtest/libqtest.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 34b9c14b75..b1eba71ffe 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -24,6 +24,9 @@
>   #ifdef __linux__
>   #include <sys/prctl.h>
>   #endif /* __linux__ */
> +#ifdef __FreeBSD__
> +#include <sys/procctl.h>
> +#endif /* __FreeBSD__ */
>   
>   #include "libqtest.h"
>   #include "libqmp.h"
> @@ -414,6 +417,10 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
>            */
>           prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
>   #endif /* __linux__ */
> +#ifdef __FreeBSD__
> +        int sig = SIGKILL;
> +        procctl(P_PID, getpid(), PROC_PDEATHSIG_CTL, &sig);

We could use 0 for "current process", but this works too.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 3/4] gitlab: make Cirrus CI timeout explicit

[Daniel P. Berrangé]

On the GitLab side we're invoking the Cirrus CI job using the
cirrus-run tool which speaks to the Cirrus REST API. Cirrus
sometimes tasks 5-10 minutes to actually schedule the task,
and thus the execution time of 'cirrus-run' inside GitLab will
be slightly longer than the execution time of the Cirrus CI
task.

Setting the timeout in the GitLab CI job should thus be done
in relation to the timeout set for the Cirrus CI job. While
Cirrus CI defaults to 60 minutes, it is better to set this
explicitly, and make the relationship between the jobs
explicit

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 .gitlab-ci.d/cirrus.yml       | 3 +++
 .gitlab-ci.d/cirrus/build.yml | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/.gitlab-ci.d/cirrus.yml b/.gitlab-ci.d/cirrus.yml
index 41d64d6680..816d89cc2a 100644
--- a/.gitlab-ci.d/cirrus.yml
+++ b/.gitlab-ci.d/cirrus.yml
@@ -15,6 +15,9 @@
   stage: build
   image: registry.gitlab.com/libvirt/libvirt-ci/cirrus-run:master
   needs: []
+  # 20 mins larger than "timeout_in" in cirrus/build.yml
+  # as there's often a 5-10 minute delay before Cirrus CI
+  # actually starts the task
   timeout: 80m
   allow_failure: true
   script:
diff --git a/.gitlab-ci.d/cirrus/build.yml b/.gitlab-ci.d/cirrus/build.yml
index a9444902ec..29d55c4aa3 100644
--- a/.gitlab-ci.d/cirrus/build.yml
+++ b/.gitlab-ci.d/cirrus/build.yml
@@ -16,6 +16,8 @@ env:
   TEST_TARGETS: "@TEST_TARGETS@"
 
 build_task:
+  # A little shorter than GitLab timeout in ../cirrus.yml
+  timeout_in: 60m
   install_script:
     - @UPDATE_COMMAND@
     - @INSTALL_COMMAND@ @PKGS@
-- 
2.41.0

Re: [PATCH 3/4] gitlab: make Cirrus CI timeout explicit

[Philippe Mathieu-Daudé]

On 12/9/23 20:41, Daniel P. Berrangé wrote:
> On the GitLab side we're invoking the Cirrus CI job using the
> cirrus-run tool which speaks to the Cirrus REST API. Cirrus
> sometimes tasks 5-10 minutes to actually schedule the task,
> and thus the execution time of 'cirrus-run' inside GitLab will
> be slightly longer than the execution time of the Cirrus CI
> task.
> 
> Setting the timeout in the GitLab CI job should thus be done
> in relation to the timeout set for the Cirrus CI job. While
> Cirrus CI defaults to 60 minutes, it is better to set this
> explicitly, and make the relationship between the jobs
> explicit
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   .gitlab-ci.d/cirrus.yml       | 3 +++
>   .gitlab-ci.d/cirrus/build.yml | 2 ++
>   2 files changed, 5 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 4/4] gitlab: make Cirrus CI jobs gating

[Daniel P. Berrangé]

The Cirrus CI jobs have been non-gating for a while to let us build
confidence in their reliability. Aside from periodic dependancy
problems when FreeBSD Ports switches to be based on a new FreeBSD
image version, the jobs have been reliable. It is thus worth making
them gating to prevent build failures being missed during merges.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 .gitlab-ci.d/cirrus.yml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/.gitlab-ci.d/cirrus.yml b/.gitlab-ci.d/cirrus.yml
index 816d89cc2a..e7f1f83c2c 100644
--- a/.gitlab-ci.d/cirrus.yml
+++ b/.gitlab-ci.d/cirrus.yml
@@ -19,7 +19,6 @@
   # as there's often a 5-10 minute delay before Cirrus CI
   # actually starts the task
   timeout: 80m
-  allow_failure: true
   script:
     - source .gitlab-ci.d/cirrus/$NAME.vars
     - sed -e "s|[@]CI_REPOSITORY_URL@|$CI_REPOSITORY_URL|g"
-- 
2.41.0

Re: [PATCH 0/4] ci: fix hang of FreeBSD CI jobs

[Stefan Hajnoczi]

On Tue, 12 Sept 2023 at 14:41, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> This addresses
>
>   https://gitlab.com/qemu-project/qemu/-/issues/1882
>
> Which turned out to be a genuine flaw which we missed during merge
> as the patch hitting master co-incided with the FreeBSD CI job
> having an temporary outage due to changed release image version.
>
> Daniel P. Berrangé (4):
>   microbit: add missing qtest_quit() call
>   qtest: kill orphaned qtest QEMU processes on FreeBSD
>   gitlab: make Cirrus CI timeout explicit
>   gitlab: make Cirrus CI jobs gating
>
>  .gitlab-ci.d/cirrus.yml       | 4 +++-
>  .gitlab-ci.d/cirrus/build.yml | 2 ++
>  tests/qtest/libqtest.c        | 7 +++++++
>  tests/qtest/microbit-test.c   | 2 ++
>  4 files changed, 14 insertions(+), 1 deletion(-)

Thank you!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [PATCH 0/4] ci: fix hang of FreeBSD CI jobs

[Thomas Huth]

On 12/09/2023 20.41, Daniel P. Berrangé wrote:
> This addresses
> 
>    https://gitlab.com/qemu-project/qemu/-/issues/1882
> 
> Which turned out to be a genuine flaw which we missed during merge
> as the patch hitting master co-incided with the FreeBSD CI job
> having an temporary outage due to changed release image version.
> 
> Daniel P. Berrangé (4):
>    microbit: add missing qtest_quit() call
>    qtest: kill orphaned qtest QEMU processes on FreeBSD
>    gitlab: make Cirrus CI timeout explicit
>    gitlab: make Cirrus CI jobs gating
> 
>   .gitlab-ci.d/cirrus.yml       | 4 +++-
>   .gitlab-ci.d/cirrus/build.yml | 2 ++
>   tests/qtest/libqtest.c        | 7 +++++++
>   tests/qtest/microbit-test.c   | 2 ++
>   4 files changed, 14 insertions(+), 1 deletion(-)
> 

Series
Reviewed-by: Thomas Huth <thuth@redhat.com>

Alex, will you pick these up or shall I take them for my next PR?
Or Stefan, do you want to apply these directly as a CI fix?

  Thomas

Re: [PATCH 0/4] ci: fix hang of FreeBSD CI jobs

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> On 12/09/2023 20.41, Daniel P. Berrangé wrote:
>> This addresses
>>    https://gitlab.com/qemu-project/qemu/-/issues/1882
>> Which turned out to be a genuine flaw which we missed during merge
>> as the patch hitting master co-incided with the FreeBSD CI job
>> having an temporary outage due to changed release image version.
>> Daniel P. Berrangé (4):
>>    microbit: add missing qtest_quit() call
>>    qtest: kill orphaned qtest QEMU processes on FreeBSD
>>    gitlab: make Cirrus CI timeout explicit
>>    gitlab: make Cirrus CI jobs gating
>>   .gitlab-ci.d/cirrus.yml       | 4 +++-
>>   .gitlab-ci.d/cirrus/build.yml | 2 ++
>>   tests/qtest/libqtest.c        | 7 +++++++
>>   tests/qtest/microbit-test.c   | 2 ++
>>   4 files changed, 14 insertions(+), 1 deletion(-)
>> 
>
> Series
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
> Alex, will you pick these up or shall I take them for my next PR?

Queued to testing/next, thanks.

Do you have a patch to disable the borked avacado tests? Or maybe I
should just include Philippe's fix?

> Or Stefan, do you want to apply these directly as a CI fix?
>
>  Thomas


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/4] ci: fix hang of FreeBSD CI jobs

[Daniel P. Berrangé]

On Wed, Sep 13, 2023 at 09:48:34AM +0100, Alex Bennée wrote:
> 
> Thomas Huth <thuth@redhat.com> writes:
> 
> > On 12/09/2023 20.41, Daniel P. Berrangé wrote:
> >> This addresses
> >>    https://gitlab.com/qemu-project/qemu/-/issues/1882
> >> Which turned out to be a genuine flaw which we missed during merge
> >> as the patch hitting master co-incided with the FreeBSD CI job
> >> having an temporary outage due to changed release image version.
> >> Daniel P. Berrangé (4):
> >>    microbit: add missing qtest_quit() call
> >>    qtest: kill orphaned qtest QEMU processes on FreeBSD
> >>    gitlab: make Cirrus CI timeout explicit
> >>    gitlab: make Cirrus CI jobs gating
> >>   .gitlab-ci.d/cirrus.yml       | 4 +++-
> >>   .gitlab-ci.d/cirrus/build.yml | 2 ++
> >>   tests/qtest/libqtest.c        | 7 +++++++
> >>   tests/qtest/microbit-test.c   | 2 ++
> >>   4 files changed, 14 insertions(+), 1 deletion(-)
> >> 
> >
> > Series
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> >
> > Alex, will you pick these up or shall I take them for my next PR?
> 
> Queued to testing/next, thanks.
> 
> Do you have a patch to disable the borked avacado tests? Or maybe I
> should just include Philippe's fix?

Not at this time. My hope was that this patch might address at least
some of the broken tests:

https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg02797.html

but I ran out of time to test that yesterday.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/4] ci: fix hang of FreeBSD CI jobs

[Thomas Huth]

On 13/09/2023 10.48, Alex Bennée wrote:
> 
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 12/09/2023 20.41, Daniel P. Berrangé wrote:
>>> This addresses
>>>     https://gitlab.com/qemu-project/qemu/-/issues/1882
>>> Which turned out to be a genuine flaw which we missed during merge
>>> as the patch hitting master co-incided with the FreeBSD CI job
>>> having an temporary outage due to changed release image version.
>>> Daniel P. Berrangé (4):
>>>     microbit: add missing qtest_quit() call
>>>     qtest: kill orphaned qtest QEMU processes on FreeBSD
>>>     gitlab: make Cirrus CI timeout explicit
>>>     gitlab: make Cirrus CI jobs gating
>>>    .gitlab-ci.d/cirrus.yml       | 4 +++-
>>>    .gitlab-ci.d/cirrus/build.yml | 2 ++
>>>    tests/qtest/libqtest.c        | 7 +++++++
>>>    tests/qtest/microbit-test.c   | 2 ++
>>>    4 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>
>> Series
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>> Alex, will you pick these up or shall I take them for my next PR?
> 
> Queued to testing/next, thanks.
> 
> Do you have a patch to disable the borked avacado tests? Or maybe I
> should just include Philippe's fix?

I thought that Philippe mentioned that he wanted to provide a patch that 
disables the broken tests?

  Thomas

Re: [PATCH 0/4] ci: fix hang of FreeBSD CI jobs

[Philippe Mathieu-Daudé]

On 13/9/23 11:02, Thomas Huth wrote:
> On 13/09/2023 10.48, Alex Bennée wrote:
>>
>> Thomas Huth <thuth@redhat.com> writes:
>>
>>> On 12/09/2023 20.41, Daniel P. Berrangé wrote:
>>>> This addresses
>>>>     https://gitlab.com/qemu-project/qemu/-/issues/1882
>>>> Which turned out to be a genuine flaw which we missed during merge
>>>> as the patch hitting master co-incided with the FreeBSD CI job
>>>> having an temporary outage due to changed release image version.
>>>> Daniel P. Berrangé (4):
>>>>     microbit: add missing qtest_quit() call
>>>>     qtest: kill orphaned qtest QEMU processes on FreeBSD
>>>>     gitlab: make Cirrus CI timeout explicit
>>>>     gitlab: make Cirrus CI jobs gating
>>>>    .gitlab-ci.d/cirrus.yml       | 4 +++-
>>>>    .gitlab-ci.d/cirrus/build.yml | 2 ++
>>>>    tests/qtest/libqtest.c        | 7 +++++++
>>>>    tests/qtest/microbit-test.c   | 2 ++
>>>>    4 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>
>>> Series
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>
>>> Alex, will you pick these up or shall I take them for my next PR?
>>
>> Queued to testing/next, thanks.
>>
>> Do you have a patch to disable the borked avacado tests? Or maybe I
>> should just include Philippe's fix?

As we discussed with Richard yesterday, the fix is not correct.

> I thought that Philippe mentioned that he wanted to provide a patch that 
> disables the broken tests?

Yes, I'm still discussing a bit more on the other thread:
https://lore.kernel.org/all/4e335f86-d075-4cc0-af5a-9dca9b3bf261@linaro.org/