[WIP] Revert to using nbdkit for http conversion imports

[alromeros]

What this PR does / why we need it:

Due to some performance issues discussed in #2809, we stopped using nbdkit for most http imports. This new behavior introduced minor differences in some specific flows, such as stopping the conversion of uncompressed raw images.

The conversion process made the actual size of raw images significantly smaller, probably because of qemu's handling of sparse images. The import of raw images without conversion ended up causing failures in some tests, as imported images had a significant increase in size.

Since nbdkit performance issues have been addressed in v1.35.8 (and now we use v1.36.2), this pull request aims to revert to the old behavior.

Example:

Fresh image import before this PR:

sh-5.1$ qemu-img info disk.img 
image: disk.img
file format: raw
virtual size: 70 GiB (75161927680 bytes)
disk size: 55 GiB

Fresh image import after this PR (due to convert):

$ qemu-img info disk.img 
image: disk.img
file format: raw
virtual size: 70 GiB (75161927680 bytes)
disk size: 9.95 GiB

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # https://issues.redhat.com/browse/CNV-36026

Special notes for your reviewer:

Check #2809 and #2832 for more context about the original change and why it's safe to revert now.

If we prefer to keep this behavior and avoid using nbdkit, an alternative would be to use scratch for uncompressed raw images.

Release note:

Bugfix: Use nbdkit for http conversion imports

[kubevirt-bot]

[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 akalenyu for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[alromeros]

/test pull-cdi-unit-test

[kubevirt-bot]

@alromeros: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-containerized-data-importer-e2e-nfs	3abfa19	link	true	/test pull-containerized-data-importer-e2e-nfs

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/test-infra repository. I understand the commands that are listed here.

[mhenriks]

Should return ProcessingPhaseTransferScratch here if we want to have proper preallocation

[alromeros]

I saw this comment earlier and understood the opposite #2832 (comment), should we return transferScratch when preallocation is false or true? Anyway, I'm thinking of simplifying this PR to just address this bug so we can backport it safely and reconsider the usage of nbdkit again in a follow-up.

[akalenyu]

So

containerized-data-importer/pkg/importer/data-processor.go

Line 271 in 3abfa19

// convert is called when convert the image from the url to a RAW disk image. Source formats include RAW/QCOW2 (Raw to raw conversion is a copy)

is not true? raw->raw conversion is not just a copy?

I also think we have some testing of sparse images in e2e so worth to see if we can reduce this from the Windows case

[akalenyu]

So

containerized-data-importer/pkg/importer/data-processor.go

Line 271 in 3abfa19

// convert is called when convert the image from the url to a RAW disk image. Source formats include RAW/QCOW2 (Raw to raw conversion is a copy)

is not true? raw->raw conversion is not just a copy?
I also think we have some testing of sparse images in e2e so worth to see if we can reduce this from the Windows case

So looking into this further, I think our error is that we have flows where we don't use qemu-img convert to seal the deal before declaring a VM disk image is "ready" to be used.
qemu-img encapsulates a lot of knowledge in itself and this is just one manifestation of something we were missing from it, which is that io.Copy doesn't preserve sparseness just like a regular cp call wouldn't if one doesn't specify --sparse=always.

I think we should always go through with qemu-img convert before marking a (content_type=kubevirt) image ready.

Regarding e2e, we have the setup to test this today with images like tinycore.iso and cirros.raw.
This is because running a dummy conversion (qemu-img convert -O qcow2 orig.qcow2 sparse.qcow2, or any conversion I think) sparsifies the image.
I was wondering why tests don't catch this. After some digging, I found out our sparseness verification in tests is simply wrong, #3213.

EDIT:

• We should also consider how backportable this is. Maybe older downstream setups would not be able to pull in the newer, "fixed" nbdkit?
• This problem has existed in upload flows too for a while if I'm not mistaken

[alromeros]

So looking into this further, I think our error is that we have flows where we don't use qemu-img convert to seal the deal before declaring a VM disk image is "ready" to be used. qemu-img encapsulates a lot of knowledge in itself and this is just one manifestation of something we were missing from it, which is that io.Copy doesn't preserve sparseness just like a regular cp call wouldn't if one doesn't specify --sparse=always.

I think we should always go through with qemu-img convert before marking a (content_type=kubevirt) image ready.

Thanks for the analysis, @akalenyu. Yeah, at least according to this flowchart we should be converting all kubevirt imgs, something we aren't doing. I think the safest way to accomplish this PR in a backportable way is to just avoid conversion of archived files and transfer to scratch every other.

I think I'll first do that in this PR so we can backport it safely and then consider using nbdkit again in a follow-up.

[akalenyu]

So looking into this further, I think our error is that we have flows where we don't use qemu-img convert to seal the deal before declaring a VM disk image is "ready" to be used. qemu-img encapsulates a lot of knowledge in itself and this is just one manifestation of something we were missing from it, which is that io.Copy doesn't preserve sparseness just like a regular cp call wouldn't if one doesn't specify --sparse=always.
I think we should always go through with qemu-img convert before marking a (content_type=kubevirt) image ready.

Thanks for the analysis, @akalenyu. Yeah, at least according to this flowchart we should be converting all kubevirt imgs, something we aren't doing. I think the safest way to accomplish this PR in a backportable way is to just avoid conversion of archived files and transfer to scratch every other.

I think I'll first do that in this PR so we can backport it safely and then consider using nbdkit again in a follow-up.

Yeah but then using scratch for raw is also painful (2x windows image)... let's see what others think

[alromeros]

Closing this PR following the conversation during the sig-storage meeting: For consistency, we've decided to prioritize the scratch space flow for most imports as it's reliable and will allow us to keep using qemu-img convert. Will open a follow-up with a simple fix in the importer flow.