Re: [igt-dev] [PATCH i-g-t 1/2 v2] runner: Add support for aborting on network failure

Petri Latvala
2019-05-20T09:36:13Z

On Mon, May 20, 2019 at 09:17:58AM +0300, Arkadiusz Hiler wrote:
> On Thu, May 16, 2019 at 02:27:54PM +0300, Petri Latvala wrote:
> > If the network goes down while testing, CI tends to interpret that as
> > the device being down, cutting its power after a while. This causes an
> > incomplete to an innocent test, increasing noise in the results.
> >
> > A new flag to --abort-on-monitored-error, "ping", uses liboping to
> > ping a host configured in .igtrc with one ping after each test
> > execution and aborts the run if there is no reply in a hardcoded
> > amount of time.
> >
> > v2:
> > - Use a higher timeout
> > - Allow hostname configuration from environment
> >
> > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > Cc: Martin Peres <martin.peres@linux.intel.com>
> > Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> > ---
> > meson.build | 1 +
> > meson_options.txt | 6 ++
> > runner/executor.c | 135 +++++++++++++++++++++++++++++++++++++++++++++
> > runner/meson.build | 12 +++-
> > runner/settings.c | 4 ++
> > runner/settings.h | 5 +-
> > 6 files changed, 160 insertions(+), 3 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index a05e912c..aac67f1a 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -100,6 +100,7 @@ build_tests = get_option('build_tests')
> > with_libdrm = get_option('with_libdrm')
> > with_libunwind = get_option('with_libunwind')
> > build_runner = get_option('build_runner')
> > +with_oping = get_option('with_oping')
>
> I find it a bit distracting that we have this bit here, whereas
> everything else is in runner/meson.build. Is there any reason for that?

Oh, I thought we had all option fetches in toplevel. Will move that to
runner/.

>
> > _build_overlay = build_overlay != 'false'
> > _overlay_required = build_overlay == 'true'
> <SNIP>
> > diff --git a/runner/meson.build b/runner/meson.build
> > index b658f1d2..a54aaab4 100644
> > --- a/runner/meson.build
> > +++ b/runner/meson.build
> > @@ -1,4 +1,13 @@
> > jsonc = dependency('json-c', required: _runner_required)
> > +runner_deps = [jsonc, glib]
> > +have_oping = []
> > +if with_oping != 'false'
> > + oping = dependency('liboping', required: with_oping == 'true')
> > + if oping.found()
> > + runner_deps += oping
> > + have_oping = '-DHAVE_OPING=1'
> > + endif
> > +endif
>
> I think it is about time to:
> https://mesonbuild.com/Release-notes-for-0-47-0.html#new-type-of-build-option-for-features
>
> I'll check meson versions in various popular distros Today and see how
> the patch would look like.
>

Oh yes please!

> > runnerlib_sources = [ 'settings.c',
> > 'job_list.c',
> > @@ -21,7 +30,8 @@ if _build_runner and _build_tests and jsonc.found()
> >
> > runnerlib = static_library('igt_runner', runnerlib_sources,
> > include_directories : inc,
> > - dependencies : [jsonc, glib])
> > + c_args : have_oping,
>
> I know that we have just a single dep here, but can we name this
> runner_lib_c_args or something?

Sure.

>
> > + dependencies : runner_deps)
> >
> > runner = executable('igt_runner', runner_sources,
> > link_with : runnerlib,
>
> Other than that the C part looks ok. I share Daniel's sentiment on
> network and NIC possibly being be a bit too "best-efforty" for aborting
> on a single dropped ping.

Yes, I'll make it retry a few times in the next version.

> It also seems like the feature haven't been properly tested, as the HAX
> does not specify any IP address.

Why would it need a HAX? The .igtrc configuration from the time of v1
was still in place. See run.log, "abort on ping" doesn't appear on the
machines I tested, and build log (not publicly available) says
liboping was used.

--
Petri Latvala