* [PATCH bpf-next v4 0/2] BPF selftest helper script
@ 2021-02-02 22:15 KP Singh
2021-02-02 22:15 ` [PATCH bpf-next v4 1/2] bpf: Helper script for running BPF presubmit tests KP Singh
2021-02-02 22:15 ` [PATCH bpf-next v4 2/2] bpf/selftests: Add a short note about run_in_vm.sh in README.rst KP Singh
0 siblings, 2 replies; 6+ messages in thread
From: KP Singh @ 2021-02-02 22:15 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Brendan Jackman
# v3 -> v4
- Fix logic for updating kernel config to not download the file
if there are no upstream modifications and avoid extraneous
kernel compilation as suggested by Andrii.
- This also removes the need for the -k flag.
# v2 -> v3
- Fixes to silence verbose commands
- Fixed output buffering without being teed out
- Fixed the clobbered error code of the script
- Other fixes suggested by Andrii
# v1 -> v2
- The script now compiles the kernel by default, and the -k option
implies "keep the kernel"
- Pointer to the script in the docs.
- Some minor simplifications.
Allow developers and contributors to understand if their changes would
end up breaking the BPF CI and avoid the back and forth required for
fixing the test cases in the CI environment. The series also adds a
pointer in tools/testing/selftests/bpf/README.rst.
KP Singh (2):
bpf: Helper script for running BPF presubmit tests
bpf/selftests: Add a short note about run_in_vm.sh in README.rst
tools/testing/selftests/bpf/README.rst | 24 ++
tools/testing/selftests/bpf/run_in_vm.sh | 368 +++++++++++++++++++++++
2 files changed, 392 insertions(+)
create mode 100755 tools/testing/selftests/bpf/run_in_vm.sh
--
2.30.0.365.g02bc693789-goog
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next v4 1/2] bpf: Helper script for running BPF presubmit tests
2021-02-02 22:15 [PATCH bpf-next v4 0/2] BPF selftest helper script KP Singh
@ 2021-02-02 22:15 ` KP Singh
2021-02-04 4:52 ` Andrii Nakryiko
2021-02-02 22:15 ` [PATCH bpf-next v4 2/2] bpf/selftests: Add a short note about run_in_vm.sh in README.rst KP Singh
1 sibling, 1 reply; 6+ messages in thread
From: KP Singh @ 2021-02-02 22:15 UTC (permalink / raw)
To: bpf
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Brendan Jackman
The script runs the BPF selftests locally on the same kernel image
as they would run post submit in the BPF continuous integration
framework.
The goal of the script is to allow contributors to run selftests locally
in the same environment to check if their changes would end up breaking
the BPF CI and reduce the back-and-forth between the maintainers and the
developers.
Tested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
tools/testing/selftests/bpf/run_in_vm.sh | 368 +++++++++++++++++++++++
1 file changed, 368 insertions(+)
create mode 100755 tools/testing/selftests/bpf/run_in_vm.sh
diff --git a/tools/testing/selftests/bpf/run_in_vm.sh b/tools/testing/selftests/bpf/run_in_vm.sh
new file mode 100755
index 000000000000..4473b3dedba4
--- /dev/null
+++ b/tools/testing/selftests/bpf/run_in_vm.sh
@@ -0,0 +1,368 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+set -u
+set -e
+
+# This script currently only works for x86_64, as
+# it is based on the VM image used by the BPF CI which is
+# x86_64.
+QEMU_BINARY="${QEMU_BINARY:="qemu-system-x86_64"}"
+X86_BZIMAGE="arch/x86/boot/bzImage"
+DEFAULT_COMMAND="./test_progs"
+MOUNT_DIR="mnt"
+ROOTFS_IMAGE="root.img"
+OUTPUT_DIR="$HOME/.bpf_selftests"
+KCONFIG_URL="https://raw.githubusercontent.com/libbpf/libbpf/master/travis-ci/vmtest/configs/latest.config"
+KCONFIG_API_URL="https://api.github.com/repos/libbpf/libbpf/contents/travis-ci/vmtest/configs/latest.config"
+INDEX_URL="https://raw.githubusercontent.com/libbpf/libbpf/master/travis-ci/vmtest/configs/INDEX"
+NUM_COMPILE_JOBS="$(nproc)"
+
+usage()
+{
+ cat <<EOF
+Usage: $0 [-i] [-d <output_dir>] -- [<command>]
+
+<command> is the command you would normally run when you are in
+tools/testing/selftests/bpf. e.g:
+
+ $0 -- ./test_progs -t test_lsm
+
+If no command is specified, "${DEFAULT_COMMAND}" will be run by
+default.
+
+If you build your kernel using KBUILD_OUTPUT= or O= options, these
+can be passed as environment variables to the script:
+
+ O=<kernel_build_path> $0 -- ./test_progs -t test_lsm
+
+or
+
+ KBUILD_OUTPUT=<kernel_build_path> $0 -- ./test_progs -t test_lsm
+
+Options:
+
+ -i) Update the rootfs image with a newer version.
+ -d) Update the output directory (default: ${OUTPUT_DIR})
+ -j) Number of jobs for compilation, similar to -j in make
+ (default: ${NUM_COMPILE_JOBS})
+EOF
+}
+
+unset URLS
+populate_url_map()
+{
+ if ! declare -p URLS &> /dev/null; then
+ # URLS contain the mapping from file names to URLs where
+ # those files can be downloaded from.
+ declare -gA URLS
+ while IFS=$'\t' read -r name url; do
+ URLS["$name"]="$url"
+ done < <(curl -Lsf ${INDEX_URL})
+ fi
+}
+
+download()
+{
+ local file="$1"
+
+ if [[ ! -v URLS[$file] ]]; then
+ echo "$file not found" >&2
+ return 1
+ fi
+
+ echo "Downloading $file..." >&2
+ curl -Lsf "${URLS[$file]}" "${@:2}"
+}
+
+newest_rootfs_version()
+{
+ {
+ for file in "${!URLS[@]}"; do
+ if [[ $file =~ ^libbpf-vmtest-rootfs-(.*)\.tar\.zst$ ]]; then
+ echo "${BASH_REMATCH[1]}"
+ fi
+ done
+ } | sort -rV | head -1
+}
+
+download_rootfs()
+{
+ local rootfsversion="$1"
+ local dir="$2"
+
+ if ! which zstd &> /dev/null; then
+ echo 'Could not find "zstd" on the system, please install zstd'
+ exit 1
+ fi
+
+ download "libbpf-vmtest-rootfs-$rootfsversion.tar.zst" |
+ zstd -d | sudo tar -C "$dir" -x
+}
+
+recompile_kernel()
+{
+ local kernel_checkout="$1"
+ local make_command="$2"
+
+ cd "${kernel_checkout}"
+
+ ${make_command} olddefconfig
+ ${make_command}
+}
+
+mount_image()
+{
+ local rootfs_img="${OUTPUT_DIR}/${ROOTFS_IMAGE}"
+ local mount_dir="${OUTPUT_DIR}/${MOUNT_DIR}"
+
+ sudo mount -o loop "${rootfs_img}" "${mount_dir}"
+}
+
+unmount_image()
+{
+ local mount_dir="${OUTPUT_DIR}/${MOUNT_DIR}"
+
+ sudo umount "${mount_dir}" &> /dev/null
+}
+
+update_selftests()
+{
+ local kernel_checkout="$1"
+ local selftests_dir="${kernel_checkout}/tools/testing/selftests/bpf"
+
+ cd "${selftests_dir}"
+ ${make_command}
+
+ # Mount the image and copy the selftests to the image.
+ mount_image
+ sudo rm -rf "${mount_dir}/root/bpf"
+ sudo cp -r "${selftests_dir}" "${mount_dir}/root"
+ unmount_image
+}
+
+update_init_script()
+{
+ local init_script_dir="${OUTPUT_DIR}/${MOUNT_DIR}/etc/rcS.d"
+ local init_script="${init_script_dir}/S50-startup"
+ local command="$1"
+ local log_file="$2"
+
+ mount_image
+
+ if [[ ! -d "${init_script_dir}" ]]; then
+ cat <<EOF
+Could not find ${init_script_dir} in the mounted image.
+This likely indicates a bad rootfs image, Please download
+a new image by passing "-i" to the script
+EOF
+ exit 1
+
+ fi
+
+ sudo bash -c "cat >${init_script}" <<EOF
+#!/bin/bash
+
+{
+ cd /root/bpf
+ echo ${command}
+ stdbuf -oL -eL ${command}
+} 2>&1 | tee /root/${log_file}
+poweroff -f
+EOF
+
+ sudo chmod a+x "${init_script}"
+ unmount_image
+}
+
+create_vm_image()
+{
+ local rootfs_img="${OUTPUT_DIR}/${ROOTFS_IMAGE}"
+ local mount_dir="${OUTPUT_DIR}/${MOUNT_DIR}"
+
+ rm -rf "${rootfs_img}"
+ touch "${rootfs_img}"
+ chattr +C "${rootfs_img}" >/dev/null 2>&1 || true
+
+ truncate -s 2G "${rootfs_img}"
+ mkfs.ext4 -q "${rootfs_img}"
+
+ mount_image
+ download_rootfs "$(newest_rootfs_version)" "${mount_dir}"
+ unmount_image
+}
+
+run_vm()
+{
+ local kernel_bzimage="$1"
+ local rootfs_img="${OUTPUT_DIR}/${ROOTFS_IMAGE}"
+
+ if ! which "${QEMU_BINARY}" &> /dev/null; then
+ cat <<EOF
+Could not find ${QEMU_BINARY}
+Please install qemu or set the QEMU_BINARY environment variable.
+EOF
+ exit 1
+ fi
+
+ ${QEMU_BINARY} \
+ -nodefaults \
+ -display none \
+ -serial mon:stdio \
+ -cpu kvm64 \
+ -enable-kvm \
+ -smp 4 \
+ -m 2G \
+ -drive file="${rootfs_img}",format=raw,index=1,media=disk,if=virtio,cache=none \
+ -kernel "${kernel_bzimage}" \
+ -append "root=/dev/vda rw console=ttyS0,115200"
+}
+
+copy_logs()
+{
+ local mount_dir="${OUTPUT_DIR}/${MOUNT_DIR}"
+ local log_file="${mount_dir}/root/$1"
+
+ mount_image
+ sudo cp ${log_file} "${OUTPUT_DIR}"
+ sudo rm -f ${log_file}
+ unmount_image
+}
+
+is_rel_path()
+{
+ local path="$1"
+
+ [[ ${path:0:1} != "/" ]]
+}
+
+update_kconfig()
+{
+ local kconfig_file="$1"
+ local update_command="curl -sLf ${KCONFIG_URL} -o ${kconfig_file}"
+ # Github does not return the "last-modified" header when retrieving the
+ # raw contents of the file. Use the API call to get the last-modified
+ # time of the kernel config and only update the config if it has been
+ # updated after the previously cached config was created. This avoids
+ # unnecessarily compiling the kernel and selftests.
+ if [[ -f "${kconfig_file}" ]]; then
+ local last_modified_date="$(curl -sL -D - "${KCONFIG_API_URL}" -o /dev/null | \
+ grep "last-modified" | awk -F ': ' '{print $2}')"
+ local remote_modified_timestamp="$(date -d "${last_modified_date}" +"%s")"
+ local local_creation_timestamp="$(stat -c %W "${kconfig_file}")"
+
+ if [[ "${remote_modified_timestamp}" -gt "${local_creation_timestamp}" ]]; then
+ ${update_command}
+ fi
+ else
+ ${update_command}
+ fi
+}
+
+main()
+{
+ local script_dir="$(cd -P -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd -P)"
+ local kernel_checkout=$(realpath "${script_dir}"/../../../../)
+ local log_file="$(date +"bpf_selftests.%Y-%m-%d_%H-%M-%S.log")"
+ # By default the script searches for the kernel in the checkout directory but
+ # it also obeys environment variables O= and KBUILD_OUTPUT=
+ local kernel_bzimage="${kernel_checkout}/${X86_BZIMAGE}"
+ local command="${DEFAULT_COMMAND}"
+ local update_image="no"
+
+ while getopts 'hkid:j:' opt; do
+ case ${opt} in
+ i)
+ update_image="yes"
+ ;;
+ d)
+ OUTPUT_DIR="$OPTARG"
+ ;;
+ j)
+ NUM_COMPILE_JOBS="$OPTARG"
+ ;;
+ h)
+ usage
+ exit 0
+ ;;
+ \? )
+ echo "Invalid Option: -$OPTARG"
+ usage
+ exit 1
+ ;;
+ : )
+ echo "Invalid Option: -$OPTARG requires an argument"
+ usage
+ exit 1
+ ;;
+ esac
+ done
+ shift $((OPTIND -1))
+
+ if [[ $# -eq 0 ]]; then
+ echo "No command specified, will run ${DEFAULT_COMMAND} in the vm"
+ else
+ command="$@"
+ fi
+
+ local kconfig_file="${OUTPUT_DIR}/latest.config"
+ local make_command="make -j ${NUM_COMPILE_JOBS} KCONFIG_CONFIG=${kconfig_file}"
+
+ # Figure out where the kernel is being built.
+ # O takes precedence over KBUILD_OUTPUT.
+ if [[ "${O:=""}" != "" ]]; then
+ if is_rel_path "${O}"; then
+ O="$(realpath "${PWD}/${O}")"
+ fi
+ kernel_bzimage="${O}/${X86_BZIMAGE}"
+ make_command="${make_command} O=${O}"
+ elif [[ "${KBUILD_OUTPUT:=""}" != "" ]]; then
+ if is_rel_path "${KBUILD_OUTPUT}"; then
+ KBUILD_OUTPUT="$(realpath "${PWD}/${KBUILD_OUTPUT}")"
+ fi
+ kernel_bzimage="${KBUILD_OUTPUT}/${X86_BZIMAGE}"
+ make_command="${make_command} KBUILD_OUTPUT=${KBUILD_OUTPUT}"
+ fi
+
+ populate_url_map
+
+ local rootfs_img="${OUTPUT_DIR}/${ROOTFS_IMAGE}"
+ local mount_dir="${OUTPUT_DIR}/${MOUNT_DIR}"
+
+ echo "Output directory: ${OUTPUT_DIR}"
+
+ mkdir -p "${OUTPUT_DIR}"
+ mkdir -p "${mount_dir}"
+ update_kconfig "${kconfig_file}"
+
+ recompile_kernel "${kernel_checkout}" "${make_command}"
+
+ if [[ "${update_image}" == "no" && ! -f "${rootfs_img}" ]]; then
+ echo "rootfs image not found in ${rootfs_img}"
+ update_image="yes"
+ fi
+
+ if [[ "${update_image}" == "yes" ]]; then
+ create_vm_image
+ fi
+
+ update_selftests "${kernel_checkout}" "${make_command}"
+ update_init_script "${command}" "${log_file}"
+ run_vm "${kernel_bzimage}"
+ copy_logs "${log_file}"
+ echo "Logs saved in ${OUTPUT_DIR}/${log_file}"
+}
+
+catch()
+{
+ local exit_code=$1
+ # This is just a cleanup and the directory may
+ # have already been unmounted. So, don't let this
+ # clobber the error code we intend to return.
+ unmount_image || true
+ exit ${exit_code}
+}
+
+trap 'catch "$?"' EXIT
+
+main "$@"
--
2.30.0.365.g02bc693789-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next v4 2/2] bpf/selftests: Add a short note about run_in_vm.sh in README.rst
2021-02-02 22:15 [PATCH bpf-next v4 0/2] BPF selftest helper script KP Singh
2021-02-02 22:15 ` [PATCH bpf-next v4 1/2] bpf: Helper script for running BPF presubmit tests KP Singh
@ 2021-02-02 22:15 ` KP Singh
1 sibling, 0 replies; 6+ messages in thread
From: KP Singh @ 2021-02-02 22:15 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Brendan Jackman
Add a short note to make contributors aware of the existence of the
script. The documentation does not intentionally document all the
options of the script to avoid mentioning it in two places (it's
available in the usage / help message of the script).
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
tools/testing/selftests/bpf/README.rst | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/tools/testing/selftests/bpf/README.rst b/tools/testing/selftests/bpf/README.rst
index ca064180d4d0..80ddcce8df02 100644
--- a/tools/testing/selftests/bpf/README.rst
+++ b/tools/testing/selftests/bpf/README.rst
@@ -6,6 +6,30 @@ General instructions on running selftests can be found in
__ /Documentation/bpf/bpf_devel_QA.rst#q-how-to-run-bpf-selftests
+=========================
+Running Selftests in a VM
+=========================
+
+It's now possible to run the selftests using ``tools/testing/selftests/bpf/run_in_vm.sh``.
+The script tries to ensure that the tests are run with the same environment as they
+would be run post-submit in the CI used by the Maintainers.
+
+This script downloads a suitable Kconfig and VM userspace image from the system used by
+the CI. It builds the kernel (without overwriting your existing Kconfig), recompiles the
+bpf selftests, runs them (by default ``tools/testing/selftests/bpf/test_progs``) and
+saves the resulting output (by default in ``~/.bpf_selftests``).
+
+For more information on about using the script, run:
+
+.. code-block:: console
+
+ $ tools/testing/selftests/bpf/run_in_vm.sh -h
+
+.. note:: The script uses pahole and clang based on host environment setting.
+ If you want to change pahole and llvm, you can change `PATH` environment
+ variable in the beginning of script.
+
+.. note:: The script currently only supports x86_64.
Additional information about selftest failures are
documented here.
--
2.30.0.365.g02bc693789-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf: Helper script for running BPF presubmit tests
2021-02-02 22:15 ` [PATCH bpf-next v4 1/2] bpf: Helper script for running BPF presubmit tests KP Singh
@ 2021-02-04 4:52 ` Andrii Nakryiko
2021-02-04 18:59 ` KP Singh
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2021-02-04 4:52 UTC (permalink / raw)
To: KP Singh
Cc: bpf, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Florent Revest, Brendan Jackman
On Tue, Feb 2, 2021 at 2:16 PM KP Singh <kpsingh@kernel.org> wrote:
>
> The script runs the BPF selftests locally on the same kernel image
> as they would run post submit in the BPF continuous integration
> framework.
>
> The goal of the script is to allow contributors to run selftests locally
> in the same environment to check if their changes would end up breaking
> the BPF CI and reduce the back-and-forth between the maintainers and the
> developers.
>
> Tested-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
I almost applied it :) But found two problems still, which ruins
experience in my environment, see below.
Also, do you mind renaming the script (and updating the doc in patch
#2)to vmtest.sh for a shorter name without underscores?
First problem is that it still doesn't propagate exit codes properly.
Try ./run_in_vm.sh -- false, followed by echo $? It should print 1,
but currently it prints zero.
> tools/testing/selftests/bpf/run_in_vm.sh | 368 +++++++++++++++++++++++
> 1 file changed, 368 insertions(+)
> create mode 100755 tools/testing/selftests/bpf/run_in_vm.sh
>
[...]
> +
> +update_kconfig()
> +{
> + local kconfig_file="$1"
> + local update_command="curl -sLf ${KCONFIG_URL} -o ${kconfig_file}"
> + # Github does not return the "last-modified" header when retrieving the
> + # raw contents of the file. Use the API call to get the last-modified
> + # time of the kernel config and only update the config if it has been
> + # updated after the previously cached config was created. This avoids
> + # unnecessarily compiling the kernel and selftests.
> + if [[ -f "${kconfig_file}" ]]; then
> + local last_modified_date="$(curl -sL -D - "${KCONFIG_API_URL}" -o /dev/null | \
> + grep "last-modified" | awk -F ': ' '{print $2}')"
> + local remote_modified_timestamp="$(date -d "${last_modified_date}" +"%s")"
> + local local_creation_timestamp="$(-c %W "${kconfig_file}")"
> +
%W breaks the entire experience for me. stat -c %W returns 0 in my
environment, don't know why. But it's also not clear why %W (file
creation time) was used instead of %Y (file modification time)? When
we overwrite latest.config, it will get updated modification time, but
old creation time, so this whole idea with %W seems wrong?
So, do you mind switching to local_modification_timestamp with %Y? I
checked locally, it finally allowed to skip rebuilding both the kernel
and selftests.
> + if [[ "${remote_modified_timestamp}" -gt "${local_creation_timestamp}" ]]; then
> + ${update_command}
> + fi
> + else
> + ${update_command}
> + fi
> +}
> +
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf: Helper script for running BPF presubmit tests
2021-02-04 4:52 ` Andrii Nakryiko
@ 2021-02-04 18:59 ` KP Singh
2021-02-04 19:22 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: KP Singh @ 2021-02-04 18:59 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Florent Revest, Brendan Jackman
On Thu, Feb 4, 2021 at 5:52 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Feb 2, 2021 at 2:16 PM KP Singh <kpsingh@kernel.org> wrote:
> >
> > The script runs the BPF selftests locally on the same kernel image
> > as they would run post submit in the BPF continuous integration
> > framework.
> >
> > The goal of the script is to allow contributors to run selftests locally
> > in the same environment to check if their changes would end up breaking
> > the BPF CI and reduce the back-and-forth between the maintainers and the
> > developers.
> >
> > Tested-by: Jiri Olsa <jolsa@redhat.com>
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
>
> I almost applied it :) But found two problems still, which ruins
> experience in my environment, see below.
>
> Also, do you mind renaming the script (and updating the doc in patch
> #2)to vmtest.sh for a shorter name without underscores?
Sure, I like vmtest.sh better too.
>
> First problem is that it still doesn't propagate exit codes properly.
> Try ./run_in_vm.sh -- false, followed by echo $? It should print 1,
> but currently it prints zero.
So propagating the error from the script that ran in the VM would I
think be a little
tricky. This is just the error from the wrapper script.
I can take a stab at it in a later patch (hope that's okay for now) as it's
not trivial [at least in my head] as we might have to save the status in a file,
copy the file back to the host and then use that status code instead or
do something socket / SSH.
>
> > tools/testing/selftests/bpf/run_in_vm.sh | 368 +++++++++++++++++++++++
> > 1 file changed, 368 insertions(+)
> > create mode 100755 tools/testing/selftests/bpf/run_in_vm.sh
> >
>
> [...]
>
> > +
> > +update_kconfig()
> > +{
> > + local kconfig_file="$1"
> > + local update_command="curl -sLf ${KCONFIG_URL} -o ${kconfig_file}"
> > + # Github does not return the "last-modified" header when retrieving the
> > + # raw contents of the file. Use the API call to get the last-modified
> > + # time of the kernel config and only update the config if it has been
> > + # updated after the previously cached config was created. This avoids
> > + # unnecessarily compiling the kernel and selftests.
> > + if [[ -f "${kconfig_file}" ]]; then
> > + local last_modified_date="$(curl -sL -D - "${KCONFIG_API_URL}" -o /dev/null | \
> > + grep "last-modified" | awk -F ': ' '{print $2}')"
> > + local remote_modified_timestamp="$(date -d "${last_modified_date}" +"%s")"
> > + local local_creation_timestamp="$(-c %W "${kconfig_file}")"
> > +
>
> %W breaks the entire experience for me. stat -c %W returns 0 in my
> environment, don't know why. But it's also not clear why %W (file
> creation time) was used instead of %Y (file modification time)? When
> we overwrite latest.config, it will get updated modification time, but
> old creation time, so this whole idea with %W seems wrong?
>
> So, do you mind switching to local_modification_timestamp with %Y? I
> checked locally, it finally allowed to skip rebuilding both the kernel
> and selftests.
Sure, I can switch to %Y. Both seem to work for me.
>
> > + if [[ "${remote_modified_timestamp}" -gt "${local_creation_timestamp}" ]]; then
> > + ${update_command}
> > + fi
> > + else
> > + ${update_command}
> > + fi
> > +}
> > +
>
> [...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf: Helper script for running BPF presubmit tests
2021-02-04 18:59 ` KP Singh
@ 2021-02-04 19:22 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2021-02-04 19:22 UTC (permalink / raw)
To: KP Singh
Cc: bpf, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Florent Revest, Brendan Jackman
On Thu, Feb 4, 2021 at 10:59 AM KP Singh <kpsingh@kernel.org> wrote:
>
> On Thu, Feb 4, 2021 at 5:52 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Feb 2, 2021 at 2:16 PM KP Singh <kpsingh@kernel.org> wrote:
> > >
> > > The script runs the BPF selftests locally on the same kernel image
> > > as they would run post submit in the BPF continuous integration
> > > framework.
> > >
> > > The goal of the script is to allow contributors to run selftests locally
> > > in the same environment to check if their changes would end up breaking
> > > the BPF CI and reduce the back-and-forth between the maintainers and the
> > > developers.
> > >
> > > Tested-by: Jiri Olsa <jolsa@redhat.com>
> > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > > ---
> >
> > I almost applied it :) But found two problems still, which ruins
> > experience in my environment, see below.
> >
> > Also, do you mind renaming the script (and updating the doc in patch
> > #2)to vmtest.sh for a shorter name without underscores?
>
> Sure, I like vmtest.sh better too.
>
> >
> > First problem is that it still doesn't propagate exit codes properly.
> > Try ./run_in_vm.sh -- false, followed by echo $? It should print 1,
> > but currently it prints zero.
>
> So propagating the error from the script that ran in the VM would I
> think be a little
> tricky. This is just the error from the wrapper script.
>
> I can take a stab at it in a later patch (hope that's okay for now) as it's
> not trivial [at least in my head] as we might have to save the status in a file,
> copy the file back to the host and then use that status code instead or
> do something socket / SSH.
>
Yeah, follow up is ok. Storing in file and returning that seems ok,
similar to what you do with logs.
> >
> > > tools/testing/selftests/bpf/run_in_vm.sh | 368 +++++++++++++++++++++++
> > > 1 file changed, 368 insertions(+)
> > > create mode 100755 tools/testing/selftests/bpf/run_in_vm.sh
> > >
> >
> > [...]
> >
> > > +
> > > +update_kconfig()
> > > +{
> > > + local kconfig_file="$1"
> > > + local update_command="curl -sLf ${KCONFIG_URL} -o ${kconfig_file}"
> > > + # Github does not return the "last-modified" header when retrieving the
> > > + # raw contents of the file. Use the API call to get the last-modified
> > > + # time of the kernel config and only update the config if it has been
> > > + # updated after the previously cached config was created. This avoids
> > > + # unnecessarily compiling the kernel and selftests.
> > > + if [[ -f "${kconfig_file}" ]]; then
> > > + local last_modified_date="$(curl -sL -D - "${KCONFIG_API_URL}" -o /dev/null | \
> > > + grep "last-modified" | awk -F ': ' '{print $2}')"
> > > + local remote_modified_timestamp="$(date -d "${last_modified_date}" +"%s")"
> > > + local local_creation_timestamp="$(-c %W "${kconfig_file}")"
> > > +
> >
> > %W breaks the entire experience for me. stat -c %W returns 0 in my
> > environment, don't know why. But it's also not clear why %W (file
> > creation time) was used instead of %Y (file modification time)? When
> > we overwrite latest.config, it will get updated modification time, but
> > old creation time, so this whole idea with %W seems wrong?
> >
> > So, do you mind switching to local_modification_timestamp with %Y? I
> > checked locally, it finally allowed to skip rebuilding both the kernel
> > and selftests.
>
> Sure, I can switch to %Y. Both seem to work for me.
>
> >
> > > + if [[ "${remote_modified_timestamp}" -gt "${local_creation_timestamp}" ]]; then
> > > + ${update_command}
> > > + fi
> > > + else
> > > + ${update_command}
> > > + fi
> > > +}
> > > +
> >
> > [...]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-04 19:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 22:15 [PATCH bpf-next v4 0/2] BPF selftest helper script KP Singh
2021-02-02 22:15 ` [PATCH bpf-next v4 1/2] bpf: Helper script for running BPF presubmit tests KP Singh
2021-02-04 4:52 ` Andrii Nakryiko
2021-02-04 18:59 ` KP Singh
2021-02-04 19:22 ` Andrii Nakryiko
2021-02-02 22:15 ` [PATCH bpf-next v4 2/2] bpf/selftests: Add a short note about run_in_vm.sh in README.rst KP Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).