Re: [PATCH i-g-t v3 3/5] scripts/hooks: Example guc log copy script and allowlist

[Gustavo Sousa <gustavo.sousa@intel.com>]

Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> writes:

> We need to explicitly extract guc logs for failed tests so add hook
> script which does it after subtest/dynsubtest execution. As copying to
> attachments takes a lot of time do it only for failing subtests.
>
> Adding this hook script to CI will extend execution time much and
> using allowlist is necessary. Instead of allowlist argument in the
> runner (previous attempt) now script filters such list on its own and
> decides when to move forward and do some post-test actions. Such
> approach is more flexible but moves decision to script what causes
> it is more prone buggy.
>
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Ryszard Knop <ryszard.knop@intel.com>
> Cc: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
> v2: install hook to datadir/hooks
> v3: migrate allowlist from runner to script
> ---
>  scripts/hooks/guc_copy.allowlist  |  2 ++
>  scripts/hooks/guc_copy_on_fail.sh | 47 +++++++++++++++++++++++++++++++
>  scripts/meson.build               |  3 ++
>  3 files changed, 52 insertions(+)
>  create mode 100644 scripts/hooks/guc_copy.allowlist
>  create mode 100755 scripts/hooks/guc_copy_on_fail.sh
>
> diff --git a/scripts/hooks/guc_copy.allowlist b/scripts/hooks/guc_copy.allowlist
> new file mode 100644
> index 0000000000..04cb3f8298
> --- /dev/null
> +++ b/scripts/hooks/guc_copy.allowlist

Do we want really want the allowlist to be part distributed with IGT? I
think we should default it to no allowlist and then CI (or whoever is
using the hook) defines it if that's desired.

> @@ -0,0 +1,2 @@
> +igt@xe_compute@compute-square
> +igt@xe_exec_basic@once-basic@ccs0
> diff --git a/scripts/hooks/guc_copy_on_fail.sh b/scripts/hooks/guc_copy_on_fail.sh
> new file mode 100755
> index 0000000000..595cf5205e
> --- /dev/null
> +++ b/scripts/hooks/guc_copy_on_fail.sh

Maybe guc_log_on_fail.sh? I think it is more descriptive than
guc_copy_*, since the latter is a bit vague about what is being copied.

> @@ -0,0 +1,47 @@
> +#!/bin/bash
> +
> +# Hook script for copying guc.log.
> +# Suggested usage with:
> +# --hook 'post-subtest:scripts/hooks/guc_copy_on_fail.sh' --hook 'post-dyn-subtest:scripts/hooks/guc_copy_on_fail.sh'
> +
> +# Copy only for failed subtests as this is time-consuming
> +if [ "${IGT_HOOK_RESULT}" != "FAIL" ]; then
> +	exit 0
> +fi
> +
> +# Process and copy guc logs only for those specified in allowlist
> +ALLOWLIST="guc_copy.allowlist"
> +scriptdir=$(dirname $(realpath $0))

It is safer to use "$0" above.

> +
> +if [ -z "$IGT_RUNNER_ATTACHMENTS_DIR" ]; then
> +	echo "Missing IGT_RUNNER_ATTACHMENTS_DIR env"
> +	exit 0
> +fi

Well, we could allow the hook user to define the output and default it
to "$IGT_RUNNER_ATTACHMENTS_DIR/guc_log" if not passed and
$IGT_RUNNER_ATTACHMENTS_DIR is defined.  Otherwise, we should error out
IMO (i.e. exit 1).

As an example, someone could be calling a single test binary directly
and want to use this hook.

We could do some simple argument parsing here or, to make it simpler,
establish that IGT_HOOKS_GUC_LOG_DEST would be such an input variable
(and document it in the doc comment at the start of the script).

> +
> +cd "$IGT_RUNNER_ATTACHMENTS_DIR"
> +
> +# Look for allowlist in following places:
> +# 1. Try in IGT_HOOK_ALLOWLIST_DIR if this environment exists
> +# 2. Try in <scriptdir>
> +
> +if [ ! -z "$IGT_HOOK_ALLOWLIST_DIR" ]; then
> +	ALLOWLIST_PATH="${IGT_HOOK_ALLOWLIST_DIR}/${ALLOWLIST}"
> +else
> +	ALLOWLIST_PATH="${scriptdir}/${ALLOWLIST}"
> +fi

I would just receive the allowlist path from CLI args or, to make it
simpler, from an environment like IGT_HOOKS_GUC_LOG_ALLOWLIST; and I
think we should default to not using any allowlist if none was passed.

> +
> +if [ ! -e "${ALLOWLIST_PATH}" ]; then
> +	exit 0
> +fi
> +
> +STNAME="${IGT_HOOK_TEST_FULLNAME}"
> +echo "${STNAME}" | grep -q -f "${ALLOWLIST_PATH}"
> +if [ $? -ne 0 ]; then
> +	exit 0
> +fi

This can simplified with:

     if ! echo "$IGT_HOOK_TEST_FULLNAME" | grep -q -f "$ALLOWLIST_PATH"; then
        exit 0;
     fi

> +
> +for log in $(find /sys/kernel/debug/dri -iname 'guc_log'); do

Suggestion: use $(cd /sys/kernel/debug/dri && find -name guc_log)...

> +	attout=$(echo ${log:23} | sed -e 's/\//_/g')

...and then here we don't need this line...

> +	mkdir -p "${STNAME}"
> +	cp "$log" "${STNAME}/${attout}"

...and here we can simply do: cp "$log" "$STNAME/${log////_}".

One issue with the find command above is that we could potentially
collect GuC log for other device (not the one under test).  I know that
IGT supports selecting a device via the environment variable
IGT_DEVICE.  I wonder if we should make it set an environment for the
device under test; we could use a different variable name to avoid
issues with the existing one.

--
Gustavo Sousa

> +done
> diff --git a/scripts/meson.build b/scripts/meson.build
> index 6e64065c5e..2ce961898d 100644
> --- a/scripts/meson.build
> +++ b/scripts/meson.build
> @@ -15,3 +15,6 @@ endif
>  igt_doc_script = find_program('igt_doc.py', required : build_testplan)
>  gen_rst_index = find_program('gen_rst_index', required : build_sphinx)
>  generate_iga64_codes = find_program('generate_iga64_codes')
> +
> +install_data('hooks/guc_copy_on_fail.sh', install_dir : datadir / 'hooks')
> +install_data('hooks/guc_copy.allowlist', install_dir : datadir / 'hooks')
> -- 
> 2.43.0