PR 1218 review
harvester-ui-extension PR 1218 (wheatdog) at head dfbc960, reviewed 2026-11-04.
It adds a Shut Down VM on Migration Failure control to the upgrade-config setting, with a banner that explains the selected behaviour, on v1.10.0 or later. It targets main and carries require backport/v1.10. Yu-Jack approved it, so it can merge at any time.
Preview: harvesterpr1218r.mf-created-dashboard.ui.rancher.space. Built from dfbc960, backend Harvester .118 (v1.9.0-rc4). Main baseline: the shared harvester-main preview, rebuilt today from 840f9de.
Verdict: the code works. Comment, and hold the merge for the backend. Grade A-
- No blockers. The control, default, saved value, reset and older-cluster path all work on the preview, and the 1.10 backport applies cleanly.
- One suggestion: the Enabled banner is out of date. After this PR was opened and approved, the author's backend commit
950e36f3dmade those VMs start again after the node upgrade, even with Restore VM disabled. The banner stops at "shut down" (finding 1). - Merge order: backend PR 12125 is still open, and all three of its approvals are dismissed. If this merges first, the default banner says the VM keeps running while the upgrade script still shuts it down (finding 2).
- Limit of this test: no real upgrade ran. It needs a multi-node v1.10 cluster, so the restart claim comes from reading the backend diff. The author ran the upgrade on the backend PR before adding the restart, and said they were testing the restart commit.
Since harvester PR 9326, a VM whose live migration fails in the upgrade pre-drain phase is always shut down. Backend PR 12125 makes that a choice through shutdownVMOnMigrationFailure, off by default, so the upgrade waits instead. This PR is the form for that field. The linked issue is a stub ("GUI Issue from 12036"), so I checked the form against the backend PR at 950e36f3d:
| Backend PR says | Form on this PR | Result |
|---|---|---|
Field defaults to false, and a missing field counts as false | Shows Disabled and saves false | Match |
Disabled: the VM keeps running and the upgrade waits. Unblock by fixing the migration, stopping the VM, or setting the field to true | The Disabled banner says the same, with the same three ways out | Match |
| Enabled: the VM is shut down so the drain can go on | The Enabled banner says this | Match |
Enabled: the VM is started again after the node is upgraded, regardless of restoreVM | Not mentioned | Gap, finding 1 |
| A non-boolean value is treated as "unknown", and the VM is not shut down | Shows Disabled, and Save writes false | Consistent |
Older versions ignore the field, so it can be set with kubectl before upgrading | Below v1.10.0 the control is hidden, and Save keeps a field that is already there | Match |
| The field can change while an upgrade waits, and applies within one loop | The form opens and Save sends its PUT (to my stub) while .118's own upgrade is in progress. The real webhook was not exercised | As far as tested |
Test it yourself
The control needs v1.10.0 and .118 runs v1.9.0-rc4, so lift the version in your browser. Log in as admin (HARVESTER_PASSWORD). I ran these steps.
- Open Settings on the PR preview.
- Run this in DevTools:
document.querySelector('#app').__vue_app__.config.globalProperties.$store.getters['harvester/byId']('harvesterhci.io.setting','server-version').value='v1.10.0' - On upgrade-config, open ⋮ → Edit Setting. Don't reload, because that drops the lift.
- Toggle the control and watch the banner. Don't press Save: unlike my test runs, this writes to the shared .118. Use Cancel.
None.
The Enabled banner doesn't say the VM comes back
What it's about
What I found
950e36f3d records the VM before stopping it and starts it again after the node is upgraded. The new comment in settings_helper.go says: "the vm is started again after the node is upgraded, regardless of RestoreVM." Restore VM sits directly above this control. With Restore VM Disabled, which is the default, a reader concludes the VM stays off, and the opposite is true.Suggested change
Evidence
restorevm.go, job_controller.go, migration_failed_vm.go) and not run, because it needs a multi-node v1.10 upgrade. So this stays a suggestion. Paused VMs and the upgrade repo VM are excluded from the restart, which is too much detail for the banner.Can this merge after backend PR 12125?
mergeable_state: blockedWhat it's about
What I found
upgrade_node.sh always shuts the VM down when its migration fails. A v1.10 build with this form and without that change would show the default as "the VM keeps running and the upgrade waits" and then stop the VM, which is the silent shutdown issue 12036 is about. The same applies to the backport into release-harvester-v1.10, already on 1.10.0-rc1. Merging second also lets finding 1 go in against the final backend.The save test doesn't go through the radio
What I found
should save the selected value sets parseDefaultValue and calls update() directly. I broke the template three ways and all 6 tests still passed each time: radio bound to restoreVM, its @update:value removed, and string options instead of booleans. The gate, default and banner logic are well covered: breaking those four ways each failed a test.Suggested change
update:value from the stubbed radio instead. I tried it: the spec stays green, and it now fails for the first two breaks.The new radio group has no accessible name
radiogroup with "Enabled" and "Disabled", because the <label> isn't tied to it. Restore VM has the same pattern and this copies it, so I left it out of the suggested review. RadioGroup reads aria-label or label, from its source, and I didn't run that fix. Your call if you want it raised.Run with Playwright on the two previews, with every write answered in the browser so nothing reached .118. Versions other than the real v1.9.0-rc4 are lifted by rewriting the server-version response, so hard reloads are covered too.
| Case | main (840f9de) | PR 1218 (dfbc960) |
|---|---|---|
| PR test steps: new field shown, Disabled by default, can be enabled and disabled | No control | All pass |
| Enable: radio and banner flip together. Save sends a real boolean | true, others intact | |
Reopen a saved true, turn it off, save | false, Restore VM stays true | |
| Use the default value, with the current default and with the backend PR's new default | Back to Disabled, saves false | |
Value customized on an older version (parallel + concurrency, manual + pauseNodes, logReadyTimeout) | Shows Disabled, nothing lost | |
| Enable it, then edit Restore VM | Both saved | |
Non-boolean "yes" left by an older version | Shows Disabled, saves false | |
| Real v1.9.0-rc4, nothing lifted | No control, no banner, field not added | |
Real v1.9.0-rc4, field already set with kubectl | Kept on Save | |
| Version strings: v1.9.1 hidden; v1.10.0-rc1, v1.10.0-dev-20261101, v1.10.1, v1.11.0, master-abcd1234-head shown | As expected | |
| Keyboard: Tab reaches the group, ArrowUp selects Enabled, the banner follows | Works | |
| Cancel | Sends nothing | |
| Width 700px | No horizontal scroll | |
Real upgrade-config on .118 after all of this | Unchanged, no value set | |
Value without nodeUpgradeOption | Form never renders | Same crash, so out of scope. Not raised |
| New spec | 6 of 6 | |
| Full jest suite | 158 passed, 1 skipped | |
| CI at dfbc960 | lint, unit-tests, DCO, Mergify green | |
| Merge with current main (840f9de) | No conflicts | |
Backport to release-harvester-v1.10 | Applies cleanly |
Not tested: a real upgrade with a failed migration, the webhook rejecting a non-boolean, the extension loaded through Rancher (it uses the same version getter), and the light theme.
| File | correctness | tests | a11y | i18n | conventions | errors | perf |
|---|---|---|---|---|---|---|---|
upgrade-config.vue | cleared | cleared | finding | cleared | cleared | cleared | cleared |
en-us.yaml | finding | n-a | n-a | cleared | cleared | n-a | n-a |
feature-flags.js | cleared | cleared | n-a | n-a | cleared | n-a | n-a |
upgrade-config.test.js | cleared | finding | n-a | n-a | cleared | n-a | n-a |
Per the repo's habits I didn't raise .js tests or the Options API. The method instead of a computed for the gate is justified: normalizeValue runs in data(), before computed properties exist, and the comment says so.
I tested this on a preview against a real Harvester, with the version gate lifted to v1.10.0 in the browser. The control, the default, the saved value and the older-cluster path all work, and the backport to release-harvester-v1.10 applies cleanly. One wording point inline.
Could this merge after harvester/harvester#12125? The banners describe what that PR does. Until it lands, the default banner says the VM keeps running while the upgrade script still shuts it down.
Inline: en-us.yaml line 1649 (attach img/pr-enabled.png)
Since this was opened, your commit 950e36f3d on the backend PR starts these VMs again after the node is upgraded, regardless of Restore VM. This text stops at "shut down", and Restore VM sits right above it, so with Restore VM disabled it reads as if the VM stays off. Could we add that it comes back? For example: "…the VM is shut down so that the upgrade can continue, and it is started again after the node is upgraded, even if Restore VM is disabled."
Inline: upgrade-config.test.js line 58
Nit: this sets parseDefaultValue and calls update() directly, so it never goes through the radio. I bound the radio to restoreVM, then separately removed its @update:value, and all 6 tests still passed each time. Emitting update:value from the stub instead catches both.