PR 1218, round three: Shut Down VM on Migration Failure
harvester/harvester-ui-extension#1218 by wheatdog, head d73d8aa (unchanged since round two), reviewed 2026-11-10 with Cody's process (my-pr-full-review at 5f9619b) and ours, then merged. Nothing is posted.
Preview: harvesterpr1218r built from d73d8aa, backend the real Harvester .105 on v1.10.0-rc1. Baseline harvestermainr rebuilt from main ee91524 (the upstream tip), same backend. Every save is held in the browser; nothing was written to .105.
Comment now, approve after the rebasegrade A-
The control does what the issue asks, on a real v1.10.0-rc1 server: Disabled by default, both choices save as real booleans with the rest of the setting kept, Use the default value resets it, and older clusters hide it and keep a value set with kubectl. Both passes found the same one real problem; ours adds an optional test nit. Nothing blocks.
- One suggestion, found by both passes: the new radio group has no accessible name, so a screen reader hears a second "Enabled / Disabled" right under Restore VM's.
- One optional nit, ours only: the save test never goes through the radio; two deliberate breakages still pass all 5 tests.
- Why not approve today: the PR conflicts with main in
feature-flags.js, so a push is coming, and this repo dismisses approvals on push (Yu-Jack's was dismissed at the last force-push). An approval now would be lost. Post the two comments, approve after the rebase. - Round two's main suggestion is gone: the docs PR now explains the option, and the form already links to that docs section.
- Nothing posted. Nothing written to the shared Harvester.
What moved since round two
- Docs: harvester/docs#1170 opened 11-06 09:58 (just after round two's check) and was approved by sushant-suse on 11-09. It documents the field, a caution that Disabled can hold an upgrade with no timeout, that the VM stays off even with restoreVM, and that the value is read once when node upgrades start. Issue 12036 now has
require/doc. - The form already points there: the setting's description links "settings/upgrade-config" to
docs.harvesterhci.io/v1.10/advanced/index#upgrade-config, which is the current docs page that PR edits. - .105 is now v1.10.0-rc1, so the v1.10.0 gate opened for real this round, no version faking. rc1 predates the backend change, and saves stayed in the browser anyway.
- Main moved four commits; the PR now conflicts in
feature-flags.js: main'scomponentHealthand this PR's flag land on the same line of the v1.10.0 list. Keep both. Mergify asked the author on 11-09. - QA passed the v1.9.1 backport (12172) on 11-09, after a first run that failed because the field was edited into
defaultinstead ofvalue. - You were requested as a reviewer, with houhoucoop and Vicente-Cheng. Head is still
d73d8aa.
See the flow
Three annotated clips on the same real v1.10.0-rc1 server, light theme, same click path: Advanced > Settings > upgrade-config > ⋮ > Edit Setting. The caption band says what each step shows.
Before (main, 13.5 s): Restore VM, then Node Upgrade Option. No way to choose what happens when a live migration fails.
With this PR (42 s): the new control, Disabled by default; the docs link at the top; Enabled; Save sends a real boolean; the card turns Modified with the field in the JSON; reopening keeps Enabled; Use the default value goes back to Disabled; Cancel sends nothing.
With this PR on a v1.9.1 cluster (20 s): version lowered in the browser, field set with kubectl as the v1.9.1 backport says. No control; changing Restore VM and saving keeps
"shutdownVMOnMigrationFailure": true.The same flow as stills:
1. Settings list
Advanced > Settings, the upgrade-config card. Its ⋮ menu has Edit Setting.
2. main: the form today
Restore VM, then Node Upgrade Option.
3. With the PR: the new control
Shut Down VM on Migration Failure, Disabled by default, between the two.
4. Enabled selected
Nothing else on the form changes.
5. After Save
The card is marked Modified and the stored JSON carries the field (save held in the browser).
6. On a v1.9.1 cluster
No control; a value set with kubectl is kept on save.
7. Light theme
Both options enabled.
8. Dark theme
Same state.
9. Keyboard focus
Tab lands on the group with a visible ring; arrow keys switch the value.
10. What the docs link will show
The field's entry on the docs#1170 preview, the page the form's description links to.
11. What a screen reader gets
Left: the PR's form, two unnamed groups. Right: the same component given its label elsewhere in the extension.
The two passes side by side
| Cody's process (Pass A) | Ours (Pass B) | |
|---|---|---|
| Verdict | His process ends at pending comments; the verdict is left to the reviewer. | Approve, A-. |
| Findings kept | 1: the unnamed radio group. | 2: the unnamed radio group, and the test gap (nit). Plus one question, dropped in the merge. |
| Checked by both | Default Disabled; Enabled and Disabled save booleans; other fields kept; Use the default value; hidden on v1.9.1 with a kubectl value kept; keyboard; the conflict. | Same. |
| Only this pass | Before/after demo pair plus the v1.9.1 clip; an 8-criterion acceptance table sourced from the thread; found that the page's docs link leads to the section docs#1170 adds; dark theme; his a11y probe in both themes; axe. | Seven mutants (the test gap); view mode is read-only; a hand-set string "true"; the aria-label fix proven with a unit test; lint; console errors on both builds; a 16-cell coverage grid. |
| Missed or wrong | The test gap: test coverage is not one of his default passes, by design. | Q1 rested on a misreading of QA's failure on 12172 (an edit to default, not a mid-upgrade change). |
| Output | 3 clips (13.5 s, 42 s, 20 s), 11 stills, criteria and verification files, 1 comment taken through his 7 comment phases. | review, coverage, checked and draft files; 1 clip (32 s); 9 images. |
| Cost | About 28 minutes in this session, after setup; it started warm with two earlier rounds of context. | 17 minutes, 76 tool calls, about 234k tokens; a fresh agent that read the rules and the code from zero. |
Findings after the merge
1
The new radio group has no accessible name
both
What happens
The bold
<label> above the group isn't tied to it, and the RadioGroup gets no label or aria-label. Chromium's accessibility tree on the PR build shows two radiogroups in a row with no name, each holding just "Enabled" and "Disabled": Restore VM's (already on main) and this new one.Why it matters
Someone on a screen reader can't tell the two apart, and this one decides whether VMs get shut down during an upgrade.
Simplest fix
Pass the same label text as
aria-label: RadioGroup forwards it to the role="radiogroup" element, and nothing changes visually. Pass B proved it with a unit test ([null,null] at the head, the label text with the attribute). The extension's rancher-cluster form already names its group this way, through label.How each pass got there
Cody's a11y pass: his probe flagged only shared-component noise; the missing name came from reading the AX tree, and his preexisting rule counts a pattern copied into new code as introduced. Ours: the a11y checklist item, checked live with
ariaSnapshot.
Pass B: what assistive tech gets for each group, the new one outlined.
Pass A: the AX tree of the PR's form, next to the same component named elsewhere.
2
The save test never goes through the radio group
ours
What happens
"should save the selected value" sets
parseDefaultValue and calls update() itself. Of seven deliberate breakages, two still pass all five tests: dropping @update:value on the new group (the choice would never reach the saved value) and binding it to restoreVM by a copy slip.Simplest fix
Emit
update:value from the group's stub and check the saved JSON: it passes on the real code and catches both (Pass B ran it).Why only ours
Cody's review never posts nits and doesn't run test coverage by default. Marcelo decides; it's marked optional.
Pass B's mutation run against the PR's spec.
3
Dropped: "should the form say the value is read once?"
Pass B, Q1 (optional question)
oursdropped
Why it was dropped
The behavior is real, but the support was wrong: Pass B said QA "ran into exactly this" on 12172. QA's failed run edited the field into
default, which the upgrade reset; the author's reply corrects that, and the re-run passed. Beyond that, docs#1170 says the value is read once and changes are ignored mid-upgrade, and the form already links to that section. Round two asked for a help line on the same grounds and is dropped for the same reason.Pass A: Cody's process, pass by pass
| Pass | Result |
|---|---|
| Demos | Before (main) and after (PR) on the same cluster and theme, plus the v1.9.1 case. The issue has no bug to reproduce; its "before" is the missing control. |
| Base review | Nothing beyond the passes. The method-not-computed comment is accurate (normalizeValue runs in data()). |
| 1. Acceptance criteria | 8 criteria from the thread: 3 stated, 3 agreed in discussion, 2 implied. Backend, restart and webhook marked not in this PR. |
| 2. Acceptance verify | 8 of 8 met, 11 live checks (the first three runs stopped on errors in my script, not the PR). |
| 3. Simplify | Nothing. |
| 4. Side effects | Nothing: the key, flag and method are read only by this form; the conflict is Mergify's to chase. |
| 5. Component reuse | No new component. |
| 6. Design fidelity | No design linked (the author's "latest design" isn't linked anywhere). |
| 7. Accessibility | His probe, both themes: only shared-component hits that Restore VM on main has too (its focus-outline hit is a false positive, a ring is drawn). AX tree: the new group has no name, filed. axe 4.10.2: 0 violations. |
| Comment phases | Verify (holds), refinement, final pass, remove suggestions (the fix pointer came out), condense (194 characters), preexisting (introduced: a copied pattern in new code). 1 comment survives. |
Acceptance criteria (from the issue thread) and what the PR build did
| Criterion | Asked for in | Verdict | |
|---|---|---|---|
| AC1 | Control shown on v1.10.0+ | backend PR: "the form needs a control" | Met |
| AC2 | Disabled when the field is missing | PR test step 3; backend default false | Met |
| AC3 | Toggles both ways, saves a boolean | PR test step 4; backend test plan step 5 | Met |
| AC4 | Other fields kept on save | docs#1170 and the author on 12172: a value replaces default entirely | Met |
| AC5 | Hidden before v1.10.0 | Author on 12172; docs#1170 | Met |
| AC6 | A kubectl value survives a save on older clusters | Author on 12172 (implied) | Met |
| AC7 | Use the default value resets to Disabled | Backend default (implied) | Met |
| AC8 | Light and dark | Implied for a visual change | Met |
Open question in the criteria file: whether the form should explain the option is not settled anywhere in the thread; the docs PR and the existing docs link carry it.
Pass B: our process
Approve, grade A-
Fresh agent, not shown Pass A. Our checklist, evidence kind on every finding, the coverage grid, and Marcelo's review rules. 2 suggestions, 1 optional question (dropped in the merge).
main and the PR on the same cluster.
Pass B's walkthrough (32 s): Enabled, Save with the saved value shown, then v1.9.1 with the control hidden.
Coverage grid
| File | correctness | tests | a11y | i18n | conventions | errors | perf |
|---|---|---|---|---|---|---|---|
| upgrade-config.vue | cleared | S2 | S1 | cleared | cleared | cleared | cleared |
| upgrade-config.test.js | cleared | S2 | n/a | n/a | cleared | n/a | n/a |
| feature-flags.js | cleared | cleared | n/a | n/a | cleared | n/a | n/a |
| en-us.yaml | cleared | n/a | n/a | cleared | cleared | n/a | n/a |
Also seen, out of scope (on main)
- Restore VM's group has no name either, and no
:mode, so it stays clickable in view mode; the PR's new group is read-only there. - Main's
normalizeValuereads the computednodesOptionsindata(), the trap the PR's comment explains.
View mode: the new group is disabled, Restore VM's is not (that one is on main).
Round two's findings, now
| Round two | Now |
|---|---|
| Suggestion: nothing explains what each choice does; keep one help line. | Dropped. docs#1170 (approved) explains all of it, and the form's description already links to that section. |
| Nit: the save test bypasses the radio. | Still open, re-measured by Pass B with seven mutants; kept as an optional nit. |
| Left out: the unnamed radio group, since it copies Restore VM. | Now in. Both passes flagged it on their own, and Cody's rule counts a pattern copied into new code as introduced. It's the second identical unnamed group in a row. |
| Outside note: backend PR 12125's description still lists "set the field to true" as an unblock. | Still stale there, but docs#1170 now says plainly that changing it mid-upgrade doesn't unblock. |
What I'd post
Two inline comments and a short body, in your shape. No preview links. GitHub copies of the clips have the URL band cropped off.
Review body
I tried this on a real v1.10.0-rc1 cluster next to main. The control is Disabled by default, Enabled and Disabled both save as booleans with the rest of the setting kept, Use the default value resets it, and on v1.9.1 the control is hidden and a value set with kubectl survives a save. One accessibility point and an optional test nit inline. Before this change: [before-main.mp4] With this change: [after-pr.mp4]
Inline on pkg/harvester/components/settings/upgrade-config.vue line 195
Screen readers get no name for this group. The bold label above it isn't tied to it, so it's announced as just "Enabled / Disabled", the same as the Restore VM group right above it, and the two can't be told apart. Passing the label text to this RadioGroup as its `aria-label` would name it without changing how it looks.
Attach this image.
Inline on pkg/harvester/components/settings/__tests__/upgrade-config.test.js line 46
Nit, optional: this test sets `parseDefaultValue` and calls `update()` itself, so it never goes through the radio group. Removing `@update:value` from the new group, or binding it to `restoreVM` by mistake, still passes all five tests. Emitting `update:value` from the group's stub would catch both.
Attach this image.
Submit as Comment, then approve once the conflict is fixed: the push that fixes it would dismiss an approval given now. The second comment is optional. Cody's process would post only the first one, without the fix clause.
What was and wasn't checked
- Live on both builds against the real v1.10.0-rc1, every write held in the browser: Pass A 11 acceptance checks, Pass B 13 scenarios (default, both saves, untouched save, v1.9.1 hidden, kubectl value kept, Use the default value, string "true", view mode, keyboard, console).
- Accessibility: Cody's probe in light and dark, Chromium's AX tree and
ariaSnapshoton both builds, axe 4.10.2 (0 violations), a real Tab walk to the group. - Unit tests: the PR's 5 plus feature-flags 7 pass; seven mutants, two survive; one extra test kills both; the aria-label fix passes a unit test. Lint clean on the source files; the local linter crashed on the test file, CI lint is green.
- Merge with main: one conflict,
feature-flags.js, keep both lines. - Baseline rebuilt before use: main
ee91524= upstream tip, traefik, 200 with TLS verified. - Not checked: a real upgrade with a failed migration (needs a multi-node v1.10 cluster with the backend change); a save against a backend that knows the field (.105 is shared and its rc1 predates the change); a real screen reader (the AX tree instead); Rancher-integrated mode; the webhook.