Repository navigation
fix(vm-disk): name the importer error in the disk's WorkloadsReady message - #4815
Conversation
…ssage A disk whose HTTP source answers 404 or does not resolve never leaves ImportInProgress: CDI fails an import only on a fatal error and restarts the importer pod otherwise, so the condition only ever said the disk was still importing. The reason it will not finish was visible only in the importer logs and on the DataVolume, whose Running condition carries the error through the back-off between restarts. Appending that message lets the user see why the disk is stuck from the application status and the console, which already shows the WorkloadsReady message for a disk. Assisted-by: LLM Signed-off-by: Alexey Artamonov <[email protected]>
The table test builds its Running conditions by hand, so it can only confirm the shape it already assumes. A DataVolume read off a cluster where the importer had failed on a 404 for days carries a Bound condition with the same error, which the message must not pick up, and proves the error sits where the controller looks for it. Assisted-by: LLM Signed-off-by: Alexey Artamonov <[email protected]>
github-actions
Bot
added
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe workload monitor now includes errors from failed DataVolume ChangesDataVolume status messages
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Disk status messages will now show the importer error, such as an HTTP 404 or an unresolvable host. The change is small, documented and tested, and no merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. The importer error CDI already keeps on the DataVolume now reaches WorkloadsReady, and ready disks get no extra text.
Business context: a VMDisk whose HTTP source keeps failing sits in ImportInProgress forever, and the reason was visible only in the importer logs.
scooby87 here is what I checked.
- CDI behaves the way the comment says, at v1.66.1 which the platform ships.
setAnnotationsFromPodWithPrefixinpkg/controller/util.gocopies the importer's termination message into the running-condition annotation, keeps it through CrashLoopBackOff and clears it once every container runs. Clone messages are joined with"%s and %s"inupdateWithTargetNotRunning. The phase goes toFailedonly withAnnImportFatalError(seeupdateStatusPhase), so a 404 staysImportInProgress. - Healthy disks don't change. Ready DataVolumes are skipped before the new code runs. The DataVolume watch has no predicate, so a Running flip does reconcile the monitor. The lastTransitionTime of WorkloadsReady comes from the monitor's creationTimestamp, so the extra status writes on each importer restart don't move it.
- Nothing sensitive reaches the tenant. The vm-disk chart renders
source.httpwithurlonly, withoutsecretRefor a cert ConfigMap. The connect error comes from Go's HTTP client, which strips a password from the URL. What is left is the tenant's own URL and CDI's error, in a WorkloadMonitor in the tenant's namespace. The fixture has only example names and URLs. - Tests pass on a trial merge with current main (
go test ./internal/controller/...). I broke the append, theRunningtype check, theFalsestatus check and the empty-half filter one at a time, and each mutant fails the new tests.
Nit, not blocking: the dataVolumesMessage doc comment still says it names each DataVolume "with its phase". It now appends CDI's error too.
2 participants
What this PR does
A VMDisk whose HTTP source answers 404 or does not resolve never finishes importing, and since #4380 its
WorkloadsReadycondition says onlyDataVolume vm-disk-x is ImportInProgress. That reads the same as a large download that is still running. CDI fails an import only on a fatal error (a checksum mismatch) and restarts the importer pod otherwise, so the phase never moves, and the reason the disk will not finish was visible only in the importer logs.CDI already keeps that reason on the DataVolume: the
Runningcondition goesFalsewith the importer's termination message, and keeps it through the back-off between restarts. The WorkloadMonitor now appends it, so the condition readsDataVolume vm-disk-x is ImportInProgress: Unable to connect to http data source: expected status code 200, got 404. Status: 404 Not Found. The console already shows theWorkloadsReadymessage on the disk's overview page, so no UI change is needed. The same text covers an unschedulable importer, a failed image pull and a checksum failure, since CDI reports all of them throughRunning=False.Only
Running=Falseis read:Unknownis not a failure, andBoundrepeats the same error behind PVC details. For a host-assisted clone CDI joins the target and source pod messages as<target> and <source>, which comes out asandwhen both pods are still being created, so empty halves are dropped.Behaviour to be aware of: while the importer pod is briefly running between restarts, CDI flips
RunningtoTrueand clears the message, so the condition shows the bare phase for those seconds. This is not a terminal state either: a disk with a broken source still staysImportInProgressand keeps retrying. Deciding when such an import should count as failed is left for a separate change.Tests
A table test covers a 404, an unresolvable host, an unschedulable importer, a fatal checksum error, a DataVolume without a phase and joined clone messages with one or both halves empty, plus the cases that must not add text: an importer that has not started, a running importer, an
Unknowncondition, a populated disk and an error that only sits onBound. Each conjunct of the condition check and the append itself were mutated and each mutant fails the test. A second test runs against a DataVolume captured from a test cluster after its importer had restarted about two thousand times on a 404, with only names and the URL changed, so the parsing is pinned against what CDI actually writes rather than a hand-built shape.Screenshots
No UI code changes: the console renders the
WorkloadsReadymessage it already receives. A message that carries a long source URL is not wrapped on the overview card; that is a styling follow-up for the console and is left out of this change.Downstream repositories
The only non-Go change is a sentence in the
packages/apps/vm-disk/README.mdnotes; no values, schema, API kind or default changes. The website reference page for vm-disk is regenerated from that README by the release bot.Release note
Summary by CodeRabbit