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

Arkadiusz Hiler
2019-05-20T06:18:02Z

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?

> _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.

> 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?

> + 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.

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

--
Cheers,
Arek