Add support for volume populators

[alromeros]

What this PR does / why we need it:

This Pull Request enables the use of volume populators in CDI, so datavolume-owned PVCs can be populated using custom logic.

Populators can be now specified in the DV spec using the new DataSourceRef field, so, when a DataVolume is created with a populated DataSourceRef, the datavolume-controller creates the corresponding PVC accordingly, skipping all the population-related steps and controllers.

You can check volume-populators-redesigned and volume-populators-beta for more information about external population of volumes.

Release note:

Support for external volume populators in DataVolumes

[kubevirt-bot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[mhenriks]

Looks great! Couple higher level comments for your consideration

[mhenriks]

I'm not saying this is wrong, but it feels weird to me that the datavolume spec.source will be unset when using this API. My assumption was that DataVolumeSource would have a Populator field.

[alromeros]

I also like the idea of having a Populator field but in practice I found it to be redundant. I have another semi-finished implementation where I add the new source so changing it wouldn't really be a problem, but I found some drawbacks:

• Since we are embedding the PVC spec into our Storage API, it makes sense to also embed the DataSourceRef field (it's already supported in the PVC API). We should then allow users to populate it, which would leave the source field useless.
• By having a Populator source, we could allow users to specify the populator there in the same way as they'd do it on the DataSourceRef field, and then copy it to the PVC spec. However, this would require additional validation in case both the DataSourceRef and source.Populator fields are set without really providing any advantage.
• The biggest drawback for me: We would basically allow to do the same thing in two different ways without any justification or real advantages, which in my opinion is an ugly practice. Having two ways to do the same thing would also require some slightly ugly logic in the datavolume-controller.

I still see some advantages: Having a Populator source is easier to understand for users, and in practice, they'd probably be using that field instead of DataSourceRef. Eventually, when we adopt populators as the main population method in CDI, we could just use that single Populator source for all cases, which in my opinion is prettier than DataSourceRef. Let me know what you think.

[akalenyu]

/test pull-containerized-data-importer-fossa

[mhenriks]

/hold

/approve cancel

let's talk about how cross-namespace datasource support [1] affects this feature.

Main problem I see is that when the DataVolume controller creates the PVC, the user context is lost and ReferenceGrants will be checked against DataVolume service account.

[1] https://kubernetes.io/blog/2023/01/02/cross-namespace-data-sources-alpha/

[mhenriks]

per discussion in 01/16/2023 kubevirt-sig-storage meeting - we should explicitly forbid cross-namespace datasources until we figure out how to do authorization with user context in webhook. However, that cannot be implemented at the moment because it requires 1.26 k8s api (currently at 1.23). Updating to 1.26 may not be trivial and I don't think this PR should be gated on that though, since namespace datasource is alpha in 1.26.

/hold cancel

[mhenriks]

/approve

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [mhenriks]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mhenriks]

@arnongilboa or @akalenyu want to make a final pass?

[akalenyu]

/lgtm

[alromeros]

/test pull-containerized-data-importer-e2e-nfs

[alromeros]

/hold cancel