Build kselftests the same way we do in kernel CI#58
Build kselftests the same way we do in kernel CI#58roxanan1996 wants to merge 2 commits intomainlinefrom
Conversation
Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>
Useful when subtests can be disabled. Note: This needs root permission Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>
There was a problem hiding this comment.
Pull request overview
This PR updates how kselftests are built and executed to align more closely with kernel CI, including pre-building BPF tooling/samples and using the installed run_kselftest.sh harness.
Changes:
- Run
kernel_kselftest.shwithsudofrom the VM runner. - Make
virt.start()idempotent by ignoring “already active” failures. - Rework
kernel_kselftest.shto build bpftool + BPF samples, install selftests to a fixed path, and run via the installed runner.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| kt/ktlib/vm.py | Runs the kselftest script under sudo in the VM. |
| kt/ktlib/virt.py | Makes virsh start tolerate already-running domains. |
| kernel_kselftest.sh | Aligns kselftest build/run flow with kernel CI (bpftool/samples build + install + run). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| script = str(config.base_path / Path("kernel-src-tree-tools") / Path("kernel_kselftest.sh")) | ||
| output_file = self.kernel_workspace.folder.absolute() / Path(f"kselftest-{self.current_head_sha_short()}.log") | ||
| ssh_cmd = f"cd {self.kernel_workspace.src_worktree.folder.absolute()} && {script}" | ||
| ssh_cmd = f"cd {self.kernel_workspace.src_worktree.folder.absolute()} && sudo {script}" |
There was a problem hiding this comment.
sudo without non-interactive flags can hang indefinitely in CI/SSH contexts if a password is required. Consider using sudo -n (fail fast) and quoting the cd path/script (e.g., via shlex.quote) to avoid breaking when paths contain spaces or shell-special characters.
| if "Domain is already active" in str(e): | ||
| pass | ||
| else: | ||
| raise e |
There was a problem hiding this comment.
raise e resets the traceback and makes debugging harder. Use a bare raise to re-raise the original exception with its original traceback.
| raise e | |
| raise |
| @@ -68,7 +68,13 @@ def install( | |||
|
|
|||
| @classmethod | |||
| def start(cls, vm_name: str) -> str: | |||
There was a problem hiding this comment.
The method is annotated to return str but it doesn’t return a value (it implicitly returns None). Either return something meaningful (e.g., vm_name or command output) or change the return annotation to None to match behavior.
| def start(cls, vm_name: str) -> str: | |
| def start(cls, vm_name: str) -> None: |
| try: | ||
| cls.run(command_type=VmCommandType.VIRSH, command=["start", vm_name]) | ||
| except RuntimeError as e: | ||
| if "Domain is already active" in str(e): | ||
| pass | ||
| else: | ||
| raise e | ||
|
|
There was a problem hiding this comment.
Matching on an exact error message string is brittle (message text can vary across libvirt/virsh versions and locales). Prefer checking a structured error indicator (exit code / stderr pattern from virsh, or a narrower exception type if cls.run exposes one) so “already active” is handled reliably.
| try: | |
| cls.run(command_type=VmCommandType.VIRSH, command=["start", vm_name]) | |
| except RuntimeError as e: | |
| if "Domain is already active" in str(e): | |
| pass | |
| else: | |
| raise e | |
| state = cls.dominfo(vm_name).get("State", "").lower() | |
| if state == "running": | |
| return | |
| cls.run(command_type=VmCommandType.VIRSH, command=["start", vm_name]) |
| run_kselftest() { | ||
| SUDO_TARGETS=$1 | ||
| SKIP_TARGETS=$2 | ||
| mkdir -p $KSELFTEST_LOG_DIR |
There was a problem hiding this comment.
Unquoted variable expansions can break on spaces/globs and may lead to surprising behavior. Quote these paths (e.g., mkdir -p "$KSELFTEST_LOG_DIR" and tee "$KSELFTEST_LOG_DIR/...") to make the script robust.
| for run in $(seq 1 $runs) ; do | ||
| make -C tools/testing/selftests SKIP_TARGETS="$SUDO_TARGETS $SKIP_TARGETS" run_tests | tee $KSELFTEST_LOG_DIR/selftest-$(uname -r)-$run.log | ||
| sudo make -C tools/testing/selftests TARGETS="$SUDO_TARGETS" run_tests | tee -a $KSELFTEST_LOG_DIR/selftest-$(uname -r)-$run.log | ||
| "$KSELFTEST_PATH/run_kselftest.sh" | tee $KSELFTEST_LOG_DIR/selftest-$(uname -r)-$run.log |
There was a problem hiding this comment.
Unquoted variable expansions can break on spaces/globs and may lead to surprising behavior. Quote these paths (e.g., mkdir -p "$KSELFTEST_LOG_DIR" and tee "$KSELFTEST_LOG_DIR/...") to make the script robust.
| make -j$(nproc) samples/bpf/ | ||
|
|
||
| export BPFTOOL=$(pwd)/tools/bpf/bpftool/bpftool | ||
| export KSELFTEST_PATH=/var/kselftests |
There was a problem hiding this comment.
Hardcoding KSELFTEST_PATH reduces portability and makes it harder to run locally or in alternate CI environments. Consider allowing an override via environment (e.g., defaulting only if unset) so callers can choose a different install/run directory when needed.
| export KSELFTEST_PATH=/var/kselftests | |
| KSELFTEST_PATH=${KSELFTEST_PATH:-/var/kselftests} | |
| export KSELFTEST_PATH |
Note: There is still a difference in the number of tests we run.