PR 1249: Upgrade Storage Class picker
harvester/harvester-ui-extension#1249 by pohanhuang, head 4325afd, opened today for harvester/harvester#12083. Reviewed 2026-11-11 with Cody's process (my-pr-full-review at 679c21e) and ours, then merged. Nothing is posted.
Preview: harvesterpr1249r built from 4325afd, backend the real Harvester .105 on v1.10.0-rc1. Baseline harvestermainr rebuilt from main 08e483e (the upstream tip), same backend. .105 has no Version to upgrade to, so the previews only enable Upgrade inside a browser that injects one. Every write was held in the browser; nothing was written to .105.
Comment: the air-gapped flow needs a fix, and this merges after the backendchanges needed
- Blocker, found by both passes: on the air-gapped page the picked class goes out as the annotation, but the page creates the ISO image on
longhorn-staticand the backend never reads the annotation whenspec.imageis set. The picker does nothing there. - The modal works at the request level: it preselects the default, blocks Upgrade without a class, and sends
harvesterhci.io/upgrade-storage-class, which is what #12123 reads. - Suggestions: the list offers per-image
lh-<uuid>and internal classes the other pickers hide; one test can't fail and the air-gapped save has none. - Nits and optional: stale hint and unused
{ name }, the annotation sent with the field hidden, the picker copied into two files, the hint not tied to the field. - Questions: merge after #12123 (still open; rc1 ignores the annotation), update #12083's text, and which classes are valid for the repository (node-local LVM?).
- UI/UX: no designer has seen it yet. Six notes for them below: the field doesn't say what it's for, the hint, silent empty states, the list, the air-gapped placement, the required marker.
See the flow
Three annotated clips on the real v1.10.0-rc1 server. The "Created Upgrade" toast at the end comes from the faked response: the request was held in the browser.
Before (main): the Upgrade modal has version, logging and the single-replica check, no StorageClass.
With this PR: the new field preselected, the 10-option list, the hint after another pick, and the held request with the annotation.
Air-gapped page:
lvm-backup-test picked, URL source; the image goes out on longhorn-static and the Upgrade with spec.image.Does it do what it says
Criteria from the PR body, #12082 and #12123, since #12083's body describes a different design.
| Criterion | Result | |
|---|---|---|
| AC1 | Online modal shows the field on v1.10+ | Met |
| AC2 | Air-gapped page shows the field | Met |
| AC3 | Preselects the default class | Met (harvester-longhorn) |
| AC4 | Pick another class that can hold the repository | Partly: every class is offered (finding 2) |
| AC5 | Saved as upgrade-storage-class | Met in both flows (request bodies) |
| AC6 | The picked class is the one the repository uses | Online: met by reading #12123. Air-gapped: not met (finding 1) |
| AC7 | No submit without a class | Met (no-default run) |
| AC8 | Hidden before v1.10 | Met for the field; annotation still sent (finding 5) |
| AC9 | Logging and skip-single-replica still sent | Met; the air-gapped page now keeps both annotations |
Findings after the merge
1
Air-gapped page: the picked StorageClass never reaches the repository volume
both ui + test
What happens
On the air-gapped page the page creates the ISO image itself, still with
targetStorageClassName: 'longhorn-static' (download and upload paths, both unchanged from main), and then sends the Upgrade with spec.image plus the new annotation. In harvester/harvester#12123 at 8665aac5 the controller only reads upgrade-storage-class inside CreateImageFromISO, which runs when spec.image is empty (upgrade_controller.go:190). With spec.image set it takes GetImage and uses the image as it is. With "Select Existing Image" the image already exists, so its class is already fixed.Why it matters
The PR says the picker works in both flows, and the backend redesign (#12082) exists for clusters that can't use
longhorn-static. On the air-gapped page those users pick a class, the request carries it, and the ISO still lands on longhorn-static.Simplest fix
On this page put the picked class on the image's
targetStorageClassName when the image is created (keep longhorn-static only when the feature is off), and don't offer the picker for "Select Existing Image". An upload starts as soon as the file is chosen, so the class has to be picked before that.Evidence
Pass A: held request bodies on the PR preview (picked
lvm-backup-test; image longhorn-static, Upgrade spec.image + annotation) and the clip below. Pass B: the same on its own run with longhorn, plus a jest probe calling save() with nfs-csi. Both read the controller at 8665aac5.
PR preview, air-gapped page:
lvm-backup-test picked, URL source. Clicking Upgrade sent the image on longhorn-static.
Pass B: the picker also shows for "Select Existing Image", where it can't apply.
2
The option list offers per-image and internal classes the other pickers hide
both ui
What happens
The list maps every StorageClass. On .105 that is 10 options, all selectable: five per-image
lh-<uuid> classes (they carry parameters.backingImage), the internal longhorn-static and vmstate-persistence, and a node-local LVM class.Why it matters
All 7 other StorageClass pickers in the extension that create something drop the per-image classes (
!s.parameters?.backingImage), and 5 of them show the internal ones as "(Internal)" and disabled. A repo volume on a VM image's backing image makes no sense, and the list grows by one cryptic entry per image.Open part
Which classes are valid for the repository volume is a backend call. CodeRabbit flagged on #12123 that a node-local class can leave the repository pod Pending once its node is drained; that's unverified.
longhorn-static is what the repository used before, so disabling it may not be right here.Evidence
Both passes opened the select on the PR preview and recorded the 10 options.
PR preview: the open list on .105.
3
"omits … when left blank" can't fail, and the air-gapped save has no test
ours test
What happens
The test asserts
expect.objectContaining({ spec: … }), which ignores metadata. Pass A changed the component to always send the annotation and the test still passed; Pass B ran the same assertion with a class set and it passed with the annotation present. Nothing tests the air-gapped save(), which is where finding 1 is.Simplest fix
Assert that
metadata.annotations is undefined there, and add a save() test for the air-gapped page that checks the image's class.How each pass got there
Pass A found it with a mutation test before its passes, then its rules dropped it: Cody's test-coverage pass isn't part of a default review. Pass B: our testing checklist, proved with a probe spec.
4
The hint is wrong after a change, and its { name } parameter is unused
both ui
What happens
"The default StorageClass is selected. You can choose a different one." stays after another class is picked. Both components pass
{ name: defaultStorageClassName }, but the string has no {name}. It also says "StorageClass" while the label says "Storage Class".Why only a nit
The data is right and nothing breaks; the copy is the designer's. The unused parameter is the code part.
PR preview:
lvm-backup-test picked, and the hint still says the default is selected.5
The annotation is sent even when the field is hidden
both ui + test
What happens
fetch() fills storageClassName with the default on every version, and the save only checks storageClassName. With server-version faked to v1.9 in the browser the field was hidden in both flows, but the Upgrade still carried upgrade-storage-class: harvester-longhorn.Why only a nit
A v1.9 controller ignores it. Gating on
repositoryStorageClassEnabled, as the skip-single-replica option already does, would match "hidden before v1.10".How each pass got there
Pass A saw it live and its refinement step dropped it (no user impact). Pass B kept it as a nit, live plus a probe.
Server version faked to v1.9.0: no field, but the held POST still had the annotation.
6
The same picker is copied into both files
both static
What happens
Five computeds (
storageClasses, defaultStorageClassName, storageClassOptions, canSubmitUpgrade, repositoryStorageClassEnabled), the template block and the annotation code are the same in both files. So findings 2, 4 and 5 each need fixing twice.Simplest fix
One small component holding the option list, the hint and the flag check.
7
The hint isn't tied to the select
ours ui
What happens
The
<p> above the field has no link to the combobox (no aria-describedby), so a screen reader doesn't read the help with the field. LabeledSelect passes aria-describedby through. No strict WCAG failure.Note
It goes away if the designer moves the help into the field's tooltip. Pass A looked at the same tree and didn't file it.
8
Merge order and the issue text
both
Merge order
Only the open backend PR harvester/harvester#12123 reads the annotation, added there today (
8665aac5). The UI flag turns on for every v1.10.0 build, so on v1.10.0-rc1 the field shows and does nothing. This should merge after #12123. The backend also calls the annotation a shortcut to revisit for an UpgradePlan v2 CRD.Issue text
harvester/harvester#12083 still asks for a dropdown on the
upgrade-config setting, and its only human comment (Oct 31) says #12082 needs no UI change. On Nov 10 the author pointed back at #12083 as the UI issue for this picker. The issue should describe what is built, so QA and docs follow it. The PR body also still has the harvester/harvester#<issue_number> placeholder.UI/UX notes for the designer
You asked to check this front because no designer has looked at the field. These are for them, not bugs, and I'd keep them out of the inline comments.
- What the field is for. The label "Upgrade Storage Class" doesn't say what goes there: the upgrade repository volume that holds the ISO for the length of the upgrade. The progress view already calls it "Upgrade Repository", and the i18n key is
repositoryStorageClass. A label or help text in those terms would let people choose on purpose. Most people should keep the default, so it could also sit in an advanced or collapsed area. - The hint. It says something already visible ("(Default)" is in the field), turns false once you change the class, disappears with no default, and sits between the version picker and the field, so it reads like a note on the version. Other Harvester forms put field help in a tooltip or under the field.
- Empty states say nothing. With no default class the required field is empty and Upgrade is disabled with no reason. With no classes at all the list says "No options". The backend's own message ("no default StorageClass found; configure one before upgrading") hints at the copy.
- What the list shows. Raw
lh-<uuid>names, internal and node-local classes, no hint about which suit the repository (finding 2 covers the data). Hide or disable-with-reason is the designer's call. The "(Default)" suffix and the preselection already match the other pickers. - Air-gapped page placement. The field is the first input, above the image Name and the File/URL source, and it also shows for "Select Existing Image". Once finding 1 is fixed it decides where the image goes, so it belongs with the image source and has to be chosen before an upload starts.
- Required marker. The class gets a red asterisk while the version picker, which you also can't skip, has none.
Default state in the modal.
No default class: empty field, no hint, Upgrade disabled with no reason.
Pass B: no classes at all.
The two passes side by side
| Finding | Cody's process (A) | Ours (B) | Merged |
|---|---|---|---|
| 1 Air-gapped choice ignored | yes, live + backend code | yes, live + jest probe + backend code | Blocker |
| 2 Per-image / internal classes in the list | yes | yes, plus the node-local question | Suggestion |
| 3 "omits" test can't fail; no air-gapped test | found by mutation, then dropped by its rules | yes | Suggestion |
4 Stale hint, unused { name } | stale hint | unused param; hint in the UX notes | Nit |
| 5 Annotation sent when hidden | seen live, dropped (no user impact) | nit, live + probe | Nit, optional |
| 6 Picker copied into both files | yes | yes, optional | Optional |
| 7 Hint not tied to the select | looked, not filed | optional | Optional |
| 8 Merge order, issue text | open questions | questions | Question |
| UX: no-classes state, field order vs upload | - | yes | UX notes |
Test style computed.x.call() | I flagged it mid-review: wrong | cleared (3 other specs use it) | dropped |
| Output | 3 clips, 9-criterion acceptance table, 4 comments | review, 7-dimension grid, checked table, 22 shots, jest probes | |
| Cost | in this session, about 1 h with the shared setup | 17 min, 65 tool calls, about 228k tokens, fresh agent |
What I'd post
Your shape: one intro line, one inline comment per point, no code, no preview link. Nothing is created on GitHub yet. Comments 1 and 2 carry a screenshot each; the before/after clips (URL band cropped) are ready if you want them in the body. Cody's process would post only comments 1, 2 and the hint, without the fix clauses.
Review body
Thanks! I tried this on a v1.10.0-rc1 cluster next to main. In the Upgrade modal the field preselects the default class and the pick goes out as the annotation, but on the air-gapped page it never reaches the repo volume. A few smaller points inline. I'd leave the wording and placement of the new field to the designer.
Inline on pkg/harvester/pages/c/_cluster/airgapupgrade/index.vue line 285
On the air-gapped page the picked class never reaches the repo volume. The page still creates the ISO image on longhorn-static (lines 264 and 332) and sends the Upgrade with spec.image, and harvester/harvester#12123 only reads this annotation when spec.image is empty. With Select Existing Image the image's class is already fixed, so the field can't apply there either. I think the picked class needs to go on the image's targetStorageClassName instead.
Attach this image.
Inline on pkg/harvester/components/HarvesterUpgrade.vue line 96
This offers every StorageClass. On our v1.10 cluster that's 10, including five per-image lh-<uuid> classes and the internal longhorn-static and vmstate-persistence. The other StorageClass pickers in the extension drop the per-image ones and most disable the internal ones. Is a node-local class like LVM OK for the repo volume? CodeRabbit raised a drain stall on #12123.
Attach this image.
Inline on pkg/harvester/components/__tests__/HarvesterUpgrade.test.js line 69
This passes even when the annotation is sent, because objectContaining only looks at spec. Nothing tests the air-gapped save, which is where the first comment is.
Inline on pkg/harvester/l10n/en-us.yaml line 1236
Nit: this keeps saying the default is selected after another class is picked, and both components pass { name } that the string doesn't use. I'd let the designer settle the wording.
Attach this image.
Inline on pkg/harvester/components/HarvesterUpgrade.vue line 158
Nit, optional: storageClassName gets the default even when the feature is off, so before v1.10 the field is hidden but the Upgrade still carries the annotation (same on the air-gapped page). Older backends ignore it, so it's harmless.
Inline on pkg/harvester/pages/c/_cluster/airgapupgrade/index.vue line 132
Optional: these computeds, the template block and the annotation code are the same in both files, so each fix above has to be made twice. A small shared component would keep them in one place.
Inline on pkg/harvester/components/HarvesterUpgrade.vue line 246
Optional: the hint isn't tied to the select, so a screen reader doesn't read it with the field. LabeledSelect passes aria-describedby through.
Inline on pkg/harvester/config/labels-annotations.js line 50
Question: only harvester/harvester#12123 reads this annotation and it's still open (rc1 ignores it), so I assume this merges after it? Also harvester/harvester#12083 still describes a dropdown on the upgrade-config setting. Could it be updated to this picker so QA and docs follow it?
Submit as Comment (or Request changes, your call). The UX notes stay on this page unless you want them as a collapsed block in the body.
What was and wasn't checked
- Both passes on the PR preview (4325afd) against the real Harvester .105 (v1.10.0-rc1), and main (08e483e, rebuilt today) for the before state. One fake Version (v1.10.1) was added to the versions list in the browser so the Upgrade button turns on.
- Every write was held in the browser and its body recorded. On .105 afterwards: still 3 Upgrades, newest image from 2026-08-06. The 14 browser sessions and the API tokens from this review were revoked.
- States: default, open list, another class picked, submit; no default class (default stripped in the browser); no classes at all (Pass B); server version faked to v1.9 (both flows); air-gapped URL source; Select Existing Image (render only).
- Backend: #12123 at 8665aac5 (controller, repo, webhook) read by both passes; issues #12083, #12082 and every comment; the human review comments on #12123.
- Unit tests: the PR's 12 pass; Pass A mutation test and Pass B probes as above; CI lint and unit tests green.
- Accessibility: AX tree and tab order of the modal. The inner combobox name "Search for option" is the shell's, the same on the existing version picker, so out of scope.
- Not checked: a real upgrade (shared cluster, and rc1 ignores the annotation anyway); what a real upgrade does with a per-image, internal or LVM class; the File upload path (from code it sets
longhorn-staticthe same way); a real pre-v1.10 cluster; Rancher-integrated mode; dark theme.