Translate all provider/base/units/stress pxus (Infra) - #2817
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2817 +/- ##
==========================================
+ Coverage 61.32% 61.33% +0.01%
==========================================
Files 499 499
Lines 50363 50392 +29
Branches 8848 8856 +8
==========================================
+ Hits 30886 30910 +24
Misses 18637 18637
- Partials 840 845 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4fc82a6 to
192c2b8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Several migrated YAML job command fields contain embedded newlines inside quoted scalars, which will split shell commands at runtime and break the affected jobs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR migrates the providers/base/units/stress provider units from legacy .pxu format to YAML, aiming to keep the same test-plan behavior while removing invalid keys and improving related documentation.
Changes:
- Converted stress test plans, jobs, and templates from
.pxuto.yaml, removing the old.pxusources. - Updated the suspend-cycles reboot flow documentation (
suspend_cycles_reboot.md) to better describe execution order and generated job relationships. - Normalized/cleaned up some YAML fields (e.g., quoting of
estimated_duration, manifest field naming/formatting) and added packaging metadata forstress-ng.
File summaries
| File | Description |
|---|---|
| providers/base/units/stress/test-plan.yaml | New YAML test-plan definitions replacing the removed PXU plan. |
| providers/base/units/stress/test-plan.pxu | Removed legacy PXU test-plan source after YAML migration. |
| providers/base/units/stress/suspend_cycles_reboot.yaml | New YAML resource/template/job units for suspend cycles with reboot flow. |
| providers/base/units/stress/suspend_cycles_reboot.pxu | Removed legacy PXU suspend/reboot units after YAML migration. |
| providers/base/units/stress/suspend_cycles_reboot.md | Updated documentation describing generated jobs and their dependency flow. |
| providers/base/units/stress/stress-ng.yaml | Minor YAML normalization (e.g., estimated_duration quoting). |
| providers/base/units/stress/s3s4.yaml | New YAML definitions for S3/S4 stress categories/resources/templates replacing PXU. |
| providers/base/units/stress/s3s4.pxu | Removed legacy PXU S3/S4 units after YAML migration. |
| providers/base/units/stress/packaging.yaml | Added packaging metadata to express required stress-ng dependency. |
| providers/base/units/stress/manifest.yaml | Updated manifest entry field naming and made hidden-reason a block scalar. |
| providers/base/units/stress/jobs.yaml | New YAML job definitions replacing PXU jobs for stress/power-management tasks. |
| providers/base/units/stress/jobs.pxu | Removed legacy PXU jobs after YAML migration. |
Review details
Suppressed comments (2)
providers/base/units/stress/suspend_cycles_reboot.yaml:227
- This
commandis a single-quoted multi-line YAML scalar, so the embedded newline is preserved and will split the shell command in two. Keep it on one line (or use a folded scalar) so--s/--rremain arguments tosleep_time_check.py.
command:
'[ -e "$PLAINBOX_SESSION_SHARE"/suspend_cycles_with_reboot_total.log ] && sleep_time_check.py "$PLAINBOX_SESSION_SHARE"/suspend_cycles_with_reboot_total.log
--s {sleep_threshold} --r {resume_threshold}'
providers/base/units/stress/s3s4.yaml:164
- The
commandvalue is a single-quoted multi-line YAML scalar; the newline is preserved and will split the shell command, causing-t all ...to be executed separately. Keep the command on one line (or use a folded scalar).
command:
'[ -e "$PLAINBOX_SESSION_SHARE"/hibernate_{s4_iterations}_cycles.log ] && sleep_test_log_check.py -v --ignore-warning
-t all "$PLAINBOX_SESSION_SHARE"/hibernate_{s4_iterations}_cycles.log'
- Files reviewed: 11/12 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Several newly introduced YAML units contain invalid command: scalars (missing |/>-) and one multi-line shell command is missing a line-continuation, which will break parsing/runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
providers/base/units/stress/jobs.yaml:505
command:is missing a YAML block scalar (|/>-) so the following indented lines are not a valid scalar value, which will break YAML parsing for this unit.
command:
pm_log_check.py --log-level=notset "$PLAINBOX_SESSION_SHARE"/pm_test.reboot.10.log
"$PLAINBOX_SESSION_SHARE"/pm_log_check_reboot.10.log
providers/base/units/stress/jobs.yaml:528
command:is missing a YAML block scalar (|/>-) so the following indented lines are not a valid scalar value, which will break YAML parsing for this unit.
command:
pm_log_check.py --log-level=notset "$PLAINBOX_SESSION_SHARE"/pm_test.poweroff.30.log
"$PLAINBOX_SESSION_SHARE"/pm_log_check_poweroff.30.log
providers/base/units/stress/jobs.yaml:551
command:is missing a YAML block scalar (|/>-) so the following indented lines are not a valid scalar value, which will break YAML parsing for this unit.
command:
pm_log_check.py --log-level=notset "$PLAINBOX_SESSION_SHARE"/pm_test.poweroff.10.log
"$PLAINBOX_SESSION_SHARE"/pm_log_check_poweroff.10.log
providers/base/units/stress/jobs.yaml:574
command:is missing a YAML block scalar (|/>-) so the following indented lines are not a valid scalar value, which will break YAML parsing for this unit.
command:
pm_log_check.py --log-level=notset "$PLAINBOX_SESSION_SHARE"/pm_test.reboot.100.log
"$PLAINBOX_SESSION_SHARE"/pm_log_check_reboot.100.log
providers/base/units/stress/jobs.yaml:597
command:is missing a YAML block scalar (|/>-) so the following indented lines are not a valid scalar value, which will break YAML parsing for this unit.
command:
pm_log_check.py --log-level=notset "$PLAINBOX_SESSION_SHARE"/pm_test.poweroff.100.log
"$PLAINBOX_SESSION_SHARE"/pm_log_check_poweroff.100.log
- Files reviewed: 11/12 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The migration can leave the watchdog disabled and prevent Checkbox from restarting after reboot jobs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
providers/base/units/stress/test-plan.yaml:306
restore_watchdogis no longer selected by this plan. The stressor templates'beforerelation only orders the restore job when it is already in the selected job map; it does not pull that job in (checkbox-ng/plainbox/impl/ctrl.py:148-154). Theafterrelation does pull the disable job, so this sequence can leave the systemd watchdog disabled after the stress run. Keep the explicit wrapper jobs in the plan.
providers/base/units/stress/suspend_cycles_reboot.md:29
- These generated-job examples have the indices and ranges reversed. This template produces the first suspend for reboot cycles 2 through K, while the next template produces suspend cycles 2 through N for every reboot cycle. As written, the documentation duplicates
S_{1,1}, omitsS_{n,1}, and labelsS_{1,k}asS_{k,1}.
- `suspend_cycles_1_reboot{{suspend_reboot_id}}`: $S_{k,1}$, where $k = 1,2,3$
- `suspend_cycles_{{suspend_id}}_reboot{{suspend_reboot_id}}`: $S_{n, k}$, where $n = 2,3,4,5$ and $k=2,3$
- Files reviewed: 11/12 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The suspend-cycle documentation currently gives incorrect generated-job indices and ranges.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
providers/base/units/stress/suspend_cycles_reboot.md:18
- Correct the grammar and duplicated word in this definition.
providers/base/units/stress/suspend_cycles_reboot.md:29
- These ranges do not match the generators:
stress_s3_cycles_iterations_1emits only reboot IDs 2 through K, whilestress_s3_cycles_iterations_multipleemits every reboot ID 1 through K. The first template is also$S_{1,k}$ , not$S_{k,1}$ , so the example currently misdocuments which jobs are generated.
- `suspend_cycles_1_reboot{{suspend_reboot_id}}`: $S_{k,1}$, where $k = 1,2,3$
- `suspend_cycles_{{suspend_id}}_reboot{{suspend_reboot_id}}`: $S_{n, k}$, where $n = 2,3,4,5$ and $k=2,3$
- Files reviewed: 11/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Two cold-boot siblings retain incorrect inherited reboot purposes because their overrides use description instead of purpose.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
providers/base/units/stress/jobs.yaml:126
- This sibling inherits the parent job's reboot
purpose, becausedescriptionis a separate legacy field and does not overridepurpose. The generated cold-boot job therefore exposes a purpose describing a reboot rather than a poweroff. Usepurposehere to preserve the PXU override semantics.
providers/base/units/stress/jobs.yaml:205 - This sibling inherits the parent attachment's reboot
purpose; settingdescriptiondoes not replace that field. As a result, the generated cold-boot attachment has a poweroff description but still exposes the reboot purpose. Overridepurposedirectly.
providers/base/units/stress/suspend_cycles_reboot.md:62 - Use the plural verb to agree with “jobs.”
- Files reviewed: 11/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
tomli380576
changed the title
provider/base/units/stress pxus (New)provider/base/units/stress pxus (Infra)
Hook25
left a comment
Hook25
left a comment
There was a problem hiding this comment.
+1, Than ks for all the fixes
Description
This PR translates all pxu files under
provider/base/units/stressand removes all invalid keys.Resolved issues
Documentation
Jinja2
In
docs/how-to/migrate-away-jinja2.rstit explicitly mentions that the use of jinja2 is very dangerous in thecommandsection. However, to keep this PR as close to thecheckbox-cli translateoutput as possible (it's already pretty big), I left the jinja2 pieces in. The impacted jobs still work as shown in the C3 submission, but I will have a follow-up PR specifically to remove all uses of jinja2 after this PR is merged.Impacted jobs
stress-tests/suspend_s3_iterations_cycles. This one is especially sketchy because it uses jinja2 as a conditional checkThese jobs only use
{{}}substitution syntax from jinja2 so it should be easy to migrate.stress-tests/suspend_cycles_1_rebootsuspend_reboot_idstress-tests/suspend_cycles_suspend_id_rebootsuspend_reboot_idstress-tests/suspend_cycles_rebootsuspend_reboot_idstress-tests/hibernate_s4_iterations_cycles(Seemingly) orphaned jobs
When doing the translation copilot found these jobs that doesn't seem to be included in any test plan (I searched around the repo to validate this and made sure copilot wasn't making things up)
not going to remove them in this PR because I'm not sure if any custom test plans uses them. They are listed here for documentation.
Tests
Successful run of
suspend-cycles-stress: https://certification.canonical.com/hardware/202601-38351/submission/508978/Compared the output of
checkbox-cli list-bootstrappedbefore and after the PR for these test plans and no jobs were lost