Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the L2 Dobby test runner and CI workflow to improve robustness on CI (notably cgroupv2/userns environments), by hardening bundle handling, adjusting platform-specific expectations, and regenerating/normalizing bundle configs used by tests.
Changes:
- Add bundle extraction/validation tracking (
untar_bundle.valid) and update most test groups to fail gracefully when bundle extraction/structure is invalid. - Improve CI resilience: DobbyDaemon readiness probing, retry container start on D-Bus races, and cgroupv1/v2 PID limit path detection.
- Add CI step + script to regenerate bundles for cgroupv2 compatibility; update OCI templates to remove
swappiness.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/L2_testing/test_runner/thunder_plugin.py | Sanitizes thunder bundle config and makes tests platform-conditional; handles invalid bundle extraction. |
| tests/L2_testing/test_runner/test_utils.py | Enhances untar_bundle validation/cleanup, adds D-Bus readiness wait, adds start retries, and improves error logging. |
| tests/L2_testing/test_runner/start_from_bundle.py | Uses untar_bundle.valid to fail early; adds small delay to allow log flush before validation. |
| tests/L2_testing/test_runner/runner.py | Avoids loading test_results.json for empty test groups; simplifies indexing. |
| tests/L2_testing/test_runner/pid_limit_tests.py | Adds cgroup v1/v2 PID limit discovery and uses DobbyTool info to resolve cgroup paths. |
| tests/L2_testing/test_runner/network_tests.py | Uses launch_container and validates bundle extraction before running. |
| tests/L2_testing/test_runner/memcr_tests.py | Adds PID parsing normalization and makes PID validation optional when info is unavailable. |
| tests/L2_testing/test_runner/basic_sanity_tests.py | Reworks async log read to select()-based timeout; changes stop verification strategy. |
| tests/L2_testing/test_runner/annotation_tests.py | Starts container from Dobby spec and ensures container is stopped after test. |
| tests/L2_testing/test_runner/bundle/regenerate_bundles.py | New script to patch/repack bundles for cgroupv2/userns compatibility. |
| tests/L2_testing/test_runner/bundle_generation.py | Switches to normalized config.json comparison and adds rootfs existence check. |
| bundle/lib/source/templates/OciConfigJsonVM1.0.2-dobby.template | Removes memory swappiness from generated OCI config. |
| bundle/lib/source/templates/OciConfigJson1.0.2-dobby.template | Removes memory swappiness from generated OCI config. |
| .github/workflows/L2-tests.yml | Runs bundle regeneration in CI; adjusts lcov invocation to ignore “unused” errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pids = wait_for_container_pids(container_id) | ||
| skip_pid_checks = not bool(pids) | ||
| if skip_pid_checks: | ||
| test_utils.print_log("No pids reported by DobbyTool info; skipping memcr pid checkpoint validation", | ||
| test_utils.Severity.warning) |
There was a problem hiding this comment.
untar_bundle.__enter__() now returns None when extraction/validation fails, but this test still assumes bundle_path is always a valid string path. If extraction fails, bundle_path becomes None and the subsequent DobbyTool start command construction will raise a TypeError before the test can report a clean failure. Update this to use a bundle_ctx = test_utils.untar_bundle(...) pattern and bail out early when not bundle_ctx.valid (similar to other updated tests).
| pids = wait_for_container_pids(container_id) | ||
| skip_pid_checks = not bool(pids) | ||
| if skip_pid_checks: | ||
| test_utils.print_log("No pids reported by DobbyTool info; skipping memcr pid checkpoint validation", | ||
| test_utils.Severity.warning) |
There was a problem hiding this comment.
Same issue as in basic_memcr_test: this function uses with ... untar_bundle(...) as bundle_path without checking .valid. If bundle extraction/validation fails, bundle_path will be None, and the DobbyTool start invocation will error out instead of producing a structured test failure. Switch to the bundle_ctx pattern and return a failing result when not bundle_ctx.valid.
| message = "Successfully received message '%s' from container" % nc_message | ||
|
|
||
| output = test_utils.create_simple_test_output(test, result, message, status.stderr) | ||
| output = test_utils.create_simple_test_output(test, result, message) |
There was a problem hiding this comment.
This test no longer passes any launch error output into log_content. create_simple_test_output() will fall back to get_container_log(), but this container doesn’t necessarily produce /tmp/<container>.log, so failures may end up with an empty/irrelevant log and be hard to diagnose. Consider including the DobbyTool/launch output (stdout/stderr) in log_content at least when launch_result is false (or adjust launch_container to return details).
| # Reject members that escape the target directory via absolute paths | ||
| # or '..' components to prevent path-traversal attacks. | ||
| for member in tar.getmembers(): | ||
| member_path = (bundle_dir / member.name).resolve() | ||
| if not str(member_path).startswith(str(bundle_dir.resolve())): | ||
| raise RuntimeError( | ||
| f"Tarball member '{member.name}' would escape extraction " | ||
| f"directory '{bundle_dir}' — aborting for safety" | ||
| ) | ||
| tar.extractall(path=bundle_dir) |
There was a problem hiding this comment.
tarfile.extractall() is still vulnerable to symlink/hardlink based path traversal even with the current member.name path prefix check (a tar can create a symlink inside bundle_dir and then extract subsequent members through it). If these tarballs are ever untrusted (or accidentally corrupted), this can write outside the intended directory. Consider using the stdlib’s safe extraction filter (where available) and/or explicitly rejecting symlink/hardlink members before extraction.
| # Reject members that escape the target directory via absolute paths | |
| # or '..' components to prevent path-traversal attacks. | |
| for member in tar.getmembers(): | |
| member_path = (bundle_dir / member.name).resolve() | |
| if not str(member_path).startswith(str(bundle_dir.resolve())): | |
| raise RuntimeError( | |
| f"Tarball member '{member.name}' would escape extraction " | |
| f"directory '{bundle_dir}' — aborting for safety" | |
| ) | |
| tar.extractall(path=bundle_dir) | |
| # Reject members that escape the intended bundle extraction directory | |
| # and reject link entries, which can be used to redirect later writes | |
| # outside the destination even when member names look safe. | |
| extract_root = extract_path.resolve() | |
| for member in tar.getmembers(): | |
| member_path = (bundle_dir / member.name).resolve() | |
| if not str(member_path).startswith(str(extract_root)): | |
| raise RuntimeError( | |
| f"Tarball member '{member.name}' would escape extraction " | |
| f"directory '{extract_path}' — aborting for safety" | |
| ) | |
| if member.issym() or member.islnk(): | |
| raise RuntimeError( | |
| f"Tarball member '{member.name}' is a symbolic/hard link " | |
| f"and is not allowed during extraction" | |
| ) | |
| try: | |
| tar.extractall(path=bundle_dir, filter='data') | |
| except TypeError: | |
| # Older Python versions do not support extraction filters. | |
| # Fall back to extraction only after the explicit validation above. | |
| tar.extractall(path=bundle_dir) |
|
|
||
| # Backup original | ||
| if backup: | ||
| backup_path = bundle_tarball.with_suffix('.tar.gz.bak') |
There was a problem hiding this comment.
Path.with_suffix() only replaces the last suffix, so for a bundle named *_bundle.tar.gz this will produce a backup name like *_bundle.tar.tar.gz.bak (duplicated .tar). If you want *_bundle.tar.gz.bak (or *_bundle.bak), build the backup filename explicitly (e.g., using with_name(...)).
| backup_path = bundle_tarball.with_suffix('.tar.gz.bak') | |
| backup_path = bundle_tarball.with_name(bundle_tarball.name + '.bak') |
Description
What does this PR change/fix and why?
If there is a corresponding JIRA ticket, please ensure it is in the title of the PR.
Test Procedure
How to test this PR (if applicable)
Type of Change
Requires Bitbake Recipe changes?
meta-rdk-ext/recipes-containers/dobby/dobby.bb) must be modified to support the changes in this PR (beyond updatingSRC_REV)