-
Notifications
You must be signed in to change notification settings - Fork 258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve http import flow to decide whether to use scratch space or not #3219
Conversation
case "xz": | ||
r, err = fr.xzReader() | ||
if err == nil { | ||
fr.Archived = true | ||
fr.ArchiveXz = true | ||
} | ||
case "qcow2": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewer: Just moving this to group archived/converted formats together.
pkg/importer/http-datasource.go
Outdated
@@ -133,7 +133,7 @@ func (hs *HTTPDataSource) Info() (ProcessingPhase, error) { | |||
if hs.contentType == cdiv1.DataVolumeArchive { | |||
return ProcessingPhaseTransferDataDir, nil | |||
} | |||
if !hs.readers.Convert { | |||
if hs.readers.Archived { | |||
return ProcessingPhaseTransferDataFile, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say we transfer to scratch for archived as well so I think we can delete this if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear we might uncover issues like the one this PR addressed (#2845), but I'm ok with the change. Let's see how tests behave.
wonder if the number of cases in which we don't use scratch for import (I think just registry node pull?) is so limited that we should get rid of the "recreate pod with scratch space" flow? import controller can be in charge of deciding when scratch is NOT needed |
1e0ef53
to
684ba92
Compare
Since we dropped the use of nbdkit we've started prioritizing the use of scratch space for most import flows. However, this new behavior introduced minor differences such as stop converting raw images, which caused inconsistencies in some tests. This commit improves the importer flow to better determine whether to use scratch space or not. Signed-off-by: Alvaro Romero <alromero@redhat.com>
684ba92
to
db3c151
Compare
/retest-required |
@@ -133,19 +133,13 @@ func (hs *HTTPDataSource) Info() (ProcessingPhase, error) { | |||
if hs.contentType == cdiv1.DataVolumeArchive { | |||
return ProcessingPhaseTransferDataDir, nil | |||
} | |||
if !hs.readers.Convert { | |||
return ProcessingPhaseTransferDataFile, nil | |||
} | |||
if pullMethod, _ := util.ParseEnvVar(common.ImporterPullMethod, false); pullMethod == string(cdiv1.RegistryPullNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Planning to try getting rid of nbdkit here? I don't think it's necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The major change in that PR is returning "convert" for registry node pull. Can still do that. I just don't think we need nbdkit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have much time to test this today but I added a commit to get rid of nbdkit for pull node. Let's see how tests behave and I'll do more testing tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhenriks seems the version of qemu-img we use doesn't support running convert/info on http endpoints, that's why we need either scratch space or nbdkit. It works on my local version though (odd since I'm using an older version), but since we expect to backport this I think we should keep using nbdkit here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does not sound correct to me. What is the error you see? Can you check the logs to see what args we are we passing to qemu-img?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a simple qemu-img info --output=json http://localhost:8100/tinyCore.iso
returns qemu-img: Could not open 'http://localhost:8100/tinyCore.iso': Unknown protocol 'http'
. I've tried creating a debugging pod and manually running the command inside and it happens the same. However, running it locally works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah the container must not have qemu-block-curl
package.
Since updating rpms in old branches is annoying. We can hold off getting rid of nbdkit in this pr but I think we should get rid of it in a subsequent pr that is not backported
f332dfc
to
db3c151
Compare
I like that idea, other than archive imports, vddk and registry node pull I think we can use scratch for every import type. We can leave that for other PR but good thing to consider for sure. |
/lgtm Thanks @alromeros, this PR is something that can be backported. For the furure we should:
|
[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:
Approvers can indicate their approval by writing |
/cherrypick release-v1.57 |
@alromeros: new pull request created: #3222 In response to this:
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. |
/cherrypick release-v1.58 |
@alromeros: new pull request created: #3223 In response to this:
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. |
/cherrypick release-v1.59 |
@alromeros: new pull request created: #3224 In response to this:
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. |
What this PR does / why we need it:
Follow up for #3212.
Due to some performance issues discussed in #2809, we stopped using nbdkit for most http imports and decided to use our custom scratch space method for most 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.
This PR aims to fix this behavior by using scratch space with more import flows: All archived kubevirt imgs will be transfered direcly to file, while all non-archived kubevirt imgs will be imported to scratch.
Example:
Fresh image import before this PR:
Fresh image import after this PR (due to convert):
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.
Release note: