Skip to content

Commit 0b9a801

Browse files
committed
fix(vm-disk): name the importer error in the disk's WorkloadsReady message
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]>
1 parent d7f3f0d commit 0b9a801

3 files changed

Lines changed: 112 additions & 4 deletions

File tree

‎internal/controller/workloadmonitor_controller.go‎

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,36 @@ func isDataVolumeReady(dv *unstructured.Unstructured) bool {
185185
return !dataVolumeInFlightPhases[phase]
186186
}
187187

188+
// dataVolumeFailureMessage returns why the DataVolume's pod is not running, or
189+
// "" when nothing says so. A source that keeps failing (404, DNS) never moves
190+
// the phase off ImportInProgress, because CDI only fails the import on a fatal
191+
// error and restarts the pod otherwise (updateStatusPhase in
192+
// pkg/controller/datavolume/import-controller.go). The Running condition holds
193+
// the clean error text: CDI keeps it through the back-off between restarts and
194+
// clears it while the pod runs, and Bound repeats it behind PVC details.
195+
// Unknown is not a failure, so only False is read. For a host-assisted clone
196+
// CDI joins the target and source messages as "<target> and <source>"
197+
// (updateWithTargetNotRunning in pkg/controller/datavolume/conditions.go), so
198+
// empty halves are dropped instead of reporting " and ".
199+
func dataVolumeFailureMessage(dv *unstructured.Unstructured) string {
200+
conditions, _, _ := unstructured.NestedSlice(dv.Object, "status", "conditions")
201+
for _, c := range conditions {
202+
cond, ok := c.(map[string]any)
203+
if !ok || cond["type"] != "Running" || cond["status"] != "False" {
204+
continue
205+
}
206+
message, _ := cond["message"].(string)
207+
var parts []string
208+
for _, part := range strings.Split(message, " and ") {
209+
if part != "" {
210+
parts = append(parts, part)
211+
}
212+
}
213+
return strings.Join(parts, " and ")
214+
}
215+
return ""
216+
}
217+
188218
// dataVolumeAPIServed treats a NoMatch as not served and returns any other
189219
// discovery error, so the caller retries instead of concluding CDI is absent.
190220
func dataVolumeAPIServed(mapper meta.RESTMapper) (bool, error) {
@@ -315,11 +345,14 @@ func (r *WorkloadMonitorReconciler) dataVolumesMessage(ctx context.Context, moni
315345
}
316346
notReady[dv.GetName()] = true
317347
phase, _, _ := unstructured.NestedString(dv.Object, "status", "phase")
348+
entry := fmt.Sprintf("DataVolume %s is %s", dv.GetName(), phase)
318349
if phase == "" {
319-
stuck = append(stuck, fmt.Sprintf("DataVolume %s has no phase", dv.GetName()))
320-
} else {
321-
stuck = append(stuck, fmt.Sprintf("DataVolume %s is %s", dv.GetName(), phase))
350+
entry = fmt.Sprintf("DataVolume %s has no phase", dv.GetName())
351+
}
352+
if cause := dataVolumeFailureMessage(dv); cause != "" {
353+
entry += ": " + cause
322354
}
355+
stuck = append(stuck, entry)
323356
}
324357
sort.Strings(stuck)
325358
return strings.Join(stuck, "; "), notReady, true, nil

‎internal/controller/workloadmonitor_controller_test.go‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1648,6 +1648,81 @@ func TestReconcile_DataVolumeMessageNamesTheDiskAndPhase(t *testing.T) {
16481648
}
16491649
}
16501650

1651+
func withRunningCondition(dv *unstructured.Unstructured, status, reason, message string) *unstructured.Unstructured {
1652+
_ = unstructured.SetNestedSlice(dv.Object, []any{map[string]any{
1653+
"type": "Running",
1654+
"status": status,
1655+
"reason": reason,
1656+
"message": message,
1657+
}}, "status", "conditions")
1658+
return dv
1659+
}
1660+
1661+
func TestReconcile_DataVolumeMessageCarriesTheImporterError(t *testing.T) {
1662+
selected := map[string]string{"app.kubernetes.io/instance": "vm-disk-test"}
1663+
phase := func(p string) *string { return &p }
1664+
// Running conditions CDI v1.64 wrote for importers that kept failing; only
1665+
// the host name is changed. v1.66 builds the condition the same way for a
1666+
// single-container importer.
1667+
const (
1668+
http404 = "Unable to connect to http data source: expected status code 200, got 404. Status: 404 Not Found"
1669+
noHost = `Unable to connect to http data source: HTTP request errored: Get "http://image.example.invalid/disk.img": dial tcp: lookup image.example.invalid on 10.96.0.10:53: no such host`
1670+
)
1671+
cases := []struct {
1672+
name string
1673+
phase *string
1674+
status, reason, message string
1675+
want string
1676+
}{
1677+
{"404 from the source", phase("ImportInProgress"), "False", "Error", http404,
1678+
"DataVolume vm-disk-test is ImportInProgress: " + http404},
1679+
{"unresolvable host", phase("ImportInProgress"), "False", "Error", noHost,
1680+
"DataVolume vm-disk-test is ImportInProgress: " + noHost},
1681+
{"unschedulable importer", phase("ImportScheduled"), "False", "Unschedulable", "Importer pod cannot be scheduled",
1682+
"DataVolume vm-disk-test is ImportScheduled: Importer pod cannot be scheduled"},
1683+
{"fatal checksum error", phase("Failed"), "False", "ChecksumError", "checksum mismatch",
1684+
"DataVolume vm-disk-test is Failed: checksum mismatch"},
1685+
{"no phase", nil, "False", "Error", http404,
1686+
"DataVolume vm-disk-test has no phase: " + http404},
1687+
{"importer not started yet", phase("ImportScheduled"), "False", "", "",
1688+
"DataVolume vm-disk-test is ImportScheduled"},
1689+
{"importer running", phase("ImportInProgress"), "True", "Pod is running", "transferring",
1690+
"DataVolume vm-disk-test is ImportInProgress"},
1691+
{"source state unknown", phase("CloneInProgress"), "Unknown", "Pending and Unknown", "Pod Pending and source state unknown",
1692+
"DataVolume vm-disk-test is CloneInProgress"},
1693+
{"clone pods both waiting without a message", phase("CloneInProgress"), "False", "ContainerCreating and ContainerCreating", " and ",
1694+
"DataVolume vm-disk-test is CloneInProgress"},
1695+
{"only the source pod has a message", phase("CloneInProgress"), "False", "ContainerCreating and Error", " and source unreachable",
1696+
"DataVolume vm-disk-test is CloneInProgress: source unreachable"},
1697+
{"both clone pods have a message", phase("CloneInProgress"), "False", "Pending and Pending", "Pod Pending and Pod Pending",
1698+
"DataVolume vm-disk-test is CloneInProgress: Pod Pending and Pod Pending"},
1699+
{"populated", phase("Succeeded"), "False", "Completed", "Import Complete", ""},
1700+
}
1701+
for _, tc := range cases {
1702+
t.Run(tc.name, func(t *testing.T) {
1703+
dv := withRunningCondition(newDataVolume("vm-disk-test", selected, tc.phase), tc.status, tc.reason, tc.message)
1704+
got := reconcileDataVolumeMonitor(t, true, nil, dv)
1705+
if got.Status.Message != tc.want {
1706+
t.Errorf("Message = %q, want %q", got.Status.Message, tc.want)
1707+
}
1708+
})
1709+
}
1710+
1711+
t.Run("only the Running condition is read", func(t *testing.T) {
1712+
dv := withRunningCondition(newDataVolume("vm-disk-test", selected, phase("ImportInProgress")), "True", "Pod is running", "")
1713+
conditions, _, _ := unstructured.NestedSlice(dv.Object, "status", "conditions")
1714+
conditions = append([]any{map[string]any{
1715+
"type": "Bound", "status": "False", "reason": "Pending",
1716+
"message": "target PVC vm-disk-test Pending and [prime-1] : " + http404,
1717+
}}, conditions...)
1718+
_ = unstructured.SetNestedSlice(dv.Object, conditions, "status", "conditions")
1719+
got := reconcileDataVolumeMonitor(t, true, nil, dv)
1720+
if want := "DataVolume vm-disk-test is ImportInProgress"; got.Status.Message != want {
1721+
t.Errorf("Message = %q, want %q", got.Status.Message, want)
1722+
}
1723+
})
1724+
}
1725+
16511726
func TestReconcile_PopulatedDataVolumeClearsTheLastVerdict(t *testing.T) {
16521727
s := newTestScheme()
16531728
selector := map[string]string{"app.kubernetes.io/instance": "vm-disk-test"}

‎packages/apps/vm-disk/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ A Virtual Machine Disk
44

55
> `storageClass` and `source` are annotated as immutable in the chart schema — see [`docs/storage-immutability.md`](../../../docs/storage-immutability.md) for the contract and which consumers enforce it. `DataVolume.spec` is immutable in CDI, so the chart also fails the release on an attempted `storageClass`/`source` edit of an existing disk rather than silently reusing the old spec; delete and recreate the disk to change either field.
66
7-
> Whether CDI has finished populating the disk shows up in the `WorkloadsReady` condition, not in `Ready`, which follows the Helm release and does not wait on the DataVolume: `WorkloadsReady` stays `False` while an import, clone or upload is in flight and when the DataVolume fails. Its message names the DataVolume and the phase it is in, `kubectl get vmdisks` shows the condition in the `WORKLOADS` column, and the console badge reads `DataVolumeNotReady` until it is `True`. A disk created before this behaviour was introduced keeps its old release settings, under which `Ready` may wait on the DataVolume, until its application is next updated. A `source.upload` disk reads `False` from the moment it is created until `virtctl image-upload` completes, because CDI reports waiting for the upload and receiving it as one phase. A disk counts as ready while it waits for its consumer with `bindImmediately` off on a `WaitForFirstConsumer` class, because that is a state such a disk is supposed to rest in.
7+
> Whether CDI has finished populating the disk shows up in the `WorkloadsReady` condition, not in `Ready`, which follows the Helm release and does not wait on the DataVolume: `WorkloadsReady` stays `False` while an import, clone or upload is in flight and when the DataVolume fails. Its message names the DataVolume and the phase it is in, followed by the error CDI reports for the pod populating the disk when there is one, such as a 404 or an unresolvable host on an HTTP source: CDI keeps retrying such an import, so its phase stays `ImportInProgress`, and the error is shown between retries rather than while a retry is running. `kubectl get vmdisks` shows the condition in the `WORKLOADS` column, and the console badge reads `DataVolumeNotReady` until it is `True`. A disk created before this behaviour was introduced keeps its old release settings, under which `Ready` may wait on the DataVolume, until its application is next updated. A `source.upload` disk reads `False` from the moment it is created until `virtctl image-upload` completes, because CDI reports waiting for the upload and receiving it as one phase. A disk counts as ready while it waits for its consumer with `bindImmediately` off on a `WaitForFirstConsumer` class, because that is a state such a disk is supposed to rest in.
88

99
> `bindImmediately` is on by default, so a disk's volume is provisioned when the disk is created rather than deferred to the first consumer. That default exists because a VMDisk is a standalone object with a lifecycle of its own: on a `WaitForFirstConsumer` StorageClass a disk that no VM has attached yet would otherwise never populate at all. It costs something on a node-pinned class such as `local`, where the CDI worker pod becomes the first consumer and the volume lands where that pod was scheduled rather than where the VM will run, so a VM whose placement is constrained (a `nodeSelector`, a resource request only one node satisfies, or the Windows affinity applied by `_cluster.scheduling.dedicatedNodesForWindowsVMs`) can stay unschedulable against a node-affinity conflict, and `storageClass` is immutable so the only exit is deleting and recreating the disk. Turn `bindImmediately` off for a disk a VMInstance will consume on such a class: KubeVirt then renders a temporary launcher pod carrying the VM's own affinity and node selector, and the volume binds where the VM can actually run. The chart default `replicated` binds `Immediate` regardless, so neither setting changes anything there.
1010

0 commit comments

Comments
 (0)