kubevirt
Fix VolumeImportSource DataVolume status tracking
What this PR does / why we need it:
This PR fixes a bug where DataVolumes using VolumeImportSource via spec.storage.dataSourceRef were immediately marked as Succeeded without tracking the
actual import progress.
When using CDI's own volume populators (VolumeImportSource, VolumeUploadSource, VolumeCloneSource) through the dataSourceRef field, the external-population-controller was treating them like external populators and immediately setting them to Succeeded status, instead of tracking their actual phase transitions (ImportScheduled → ImportInProgress → Succeeded) and progress percentages.
After the fix
kube-nfv focal-server-cloudimg-amd64-test-block ImportScheduled N/A 38s
kube-nfv focal-server-cloudimg-amd64-test-block ImportScheduled N/A 38s
kube-nfv focal-server-cloudimg-amd64-test-block ImportScheduled N/A 48s
kube-nfv focal-server-cloudimg-amd64-test-block ImportInProgress N/A 64s
kube-nfv focal-server-cloudimg-amd64-test-block ImportInProgress 0.17% 68s
kube-nfv focal-server-cloudimg-amd64-test-block ImportInProgress 0.65% 70s
...
kube-nfv focal-server-cloudimg-amd64-test-block ImportInProgress 10.03% 85s
...
kube-nfv focal-server-cloudimg-amd64-test-block ImportInProgress 19.65% 100s
...
kube-nfv focal-server-cloudimg-amd64-test-block ImportInProgress 99.46% 8m9s
kube-nfv focal-server-cloudimg-amd64-test-block Succeeded 100.0% 8m12s
kube-nfv focal-server-cloudimg-amd64-test-block Succeeded 100.0% 8m13s
kube-nfv focal-server-cloudimg-amd64-test-block Succeeded 100.0% 8m14s
Changes:
- Enable progress tracking in external-population-controller (set shouldUpdateProgress: true)
- Add updateCDIPopulatorStatus() to read PVC annotations set by populator controllers and update DataVolume phases accordingly
- Add helper functions isCDIVolumePopulator() and getCDIVolumePopulatorKind() to distinguish CDI's own populators from external ones
- Add comprehensive unit tests for all CDI populator types (Import, Upload, Clone) covering phase transitions, progress tracking, and failure scenarios
Special notes for your reviewer:
The fix keeps CDI volume populators routed to the external-population-controller (as designed) but adds special handling to read the PVC annotations
(cdi.kubevirt.io/storage.pod.phase and cdi.kubevirt.io/storage.populator.progress) that are set by the respective populator controllers (import-populator,
upload-populator, clone-populator). This mirrors the status update logic from import-controller but adapted for the populator pattern.
All existing tests pass (302/302), including 8 new tests that verify proper phase transitions and progress tracking for CDI volume populators.
Testing: A pre-built image with this fix is available at ghcr.io/kube-nfv/cdi-controller:v1.63-dv-rec-fix for testing purposes.
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign aglitke for approval. For more information see the Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Hi @DiMalovanyy. Thanks for your PR.
PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.
I understand the commands that are listed here.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.
Hi, thanks for the PR, I will review shortly. To get the dco check to pass, all you have to do is git commit --amend -s which will 'sign' the commit.
Thanks for the PR! would love to hear more about the use case, we generally only create the target PVCs with our own dataSourceRefs, not the DVs themselves.
/cc @alromeros
Hi @akalenyu, I'm using volumeimportsources as an image definition: "Let's say image ID". Later multiple image instances (eg. with different storageClasses might be instantiated from it), and finally the kubevirt VM will creates the bootable volume by cloning from it (with increased size). Also, I'd like to receive the status updates from DV, like progress, status, phases, etc.). So, I'm just using the DataSourceRef for DV, I saw it should be supported in documentation https://github.com/kubevirt/containerized-data-importer/blob/main/doc/cdi-populators.md#using-populators-with-datavolumes. Thanks, Dmytro
Hi @akalenyu, I'm using volumeimportsources as an image definition: "Let's say image ID". Later multiple image instances (eg. with different storageClasses might be instantiated from it), and finally the kubevirt VM will creates the bootable volume by cloning from it (with increased size). Also, I'd like to receive the status updates from DV, like progress, status, phases, etc.). So, I'm just using the DataSourceRef for DV, I saw it should be supported in documentation https://github.com/kubevirt/containerized-data-importer/blob/main/doc/cdi-populators.md#using-populators-with-datavolumes. Thanks, Dmytro
I see, you don't want to rely on us creating the volumeImportSource on your behalf?
Hi @akalenyu, I'm using volumeimportsources as an image definition: "Let's say image ID". Later multiple image instances (eg. with different storageClasses might be instantiated from it), and finally the kubevirt VM will creates the bootable volume by cloning from it (with increased size). Also, I'd like to receive the status updates from DV, like progress, status, phases, etc.). So, I'm just using the DataSourceRef for DV, I saw it should be supported in documentation https://github.com/kubevirt/containerized-data-importer/blob/main/doc/cdi-populators.md#using-populators-with-datavolumes. Thanks, Dmytro
I see, you don't want to rely on us creating the volumeImportSource on your behalf?
In my system, software image creation and image instantiation is two different processes. Also I want to have multiple instances (DataVolumes) that references the same VolumeImportSource.
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.
:memo: Please follow instructions in the contributing guide to update your commits with the DCO
Full details of the Developer Certificate of Origin can be found at developercertificate.org.
The list of commits missing DCO signoff:
- 84039bc Fix VolumeImportSource DataVolume status tracking
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.
Thanks @DiMalovanyy for the PR! The overall logic looks fine, but I have a couple of concerns.
In principle I’m not keen to add CDI populator logic into the external-population controller. That controller is intended as a generic way to support external populators, and including CDI-populator specific behavior introduces a level of knowledge that'd be better to avoid considering we have a native way to use those populators. If the use case justifies the change I’m ok with merging, but in my opinion creating a regular dv with an import source is probably just as simple at scale as the approach you are currently using. That said I'd also like to hear what other maintainers think.
Aside from that, I second @awels’ comment about signing the commits. It also looks like there is a leftover commit that should be removed: fd295de
Thanks for your comment. CDI already supports creating a DataVolume with a dataSourceRef pointing to CDI-managed resources such as VolumeImportSource. The import-populator controller correctly handles PVC creation, annotation patching, and other related operations.
However, the external-population-controller does not handle these annotations properly, even though it could. This might be confusing for users who try to use VolumeImportSource or other CDI resources as a dataSourceRef.
If you decide not to merge these changes, I think it should at least be documented that using a DataVolume with a dataSourceRef to CDI-managed resources may not work as expected.