-
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
Always use scratchspace when importing and converting #2832
Conversation
752c731
to
511e405
Compare
/lgtm |
pkg/importer/http-datasource.go
Outdated
return ProcessingPhaseTransferScratch, nil | ||
} | ||
if hs.readers.Archived || hs.customCA != "" { | ||
return ProcessingPhaseTransferDataFile, nil | ||
} | ||
hs.url, _ = url.Parse(fmt.Sprintf("nbd+unix:///?socket=%s", nbdkitSocket)) |
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.
looks like nbdkit will still be called for importing raw non compressed. How's the performance with that? should we get rid of it entirely?
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.
We need to keep it around for the multi stage import, which uses nbdkit. But you are right this will still be slow for uncompressed raw files since it uses nbdkit.
edc157b
to
5c80162
Compare
if err = hs.n.StartNbdkit(hs.endpoint.String()); err != nil { | ||
return ProcessingPhaseError, err | ||
if !hs.readers.Convert && (hs.readers.Archived || hs.customCA != "") { | ||
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 think we can technically get rid && (hs.readers.Archived || hs.customCA != "")
?
No need to write a raw/iso to scratch, right?
UNLESS we want to truly respect preallocation
settings. In which case, we should just always return ProcessingPhaseTransferScratch, nil
for kubevirt content type and preallocation=false
Signed-off-by: Alexander Wels <awels@redhat.com>
of ndbkit. Once we are able to get nbdkit 1.35.8 or newer we can revert this change since that will include improvements to the downloading speed. Signed-off-by: Alexander Wels <awels@redhat.com>
return total, and this means the metrics are disabled. Signed-off-by: Alexander Wels <awels@redhat.com>
Signed-off-by: Alexander Wels <awels@redhat.com>
Signed-off-by: Alexander Wels <awels@redhat.com>
5c80162
to
2060ecc
Compare
Fixed functional test that was not doing the right thing while running the test. Signed-off-by: Alexander Wels <awels@redhat.com>
2060ecc
to
ba11540
Compare
/test pull-containerized-data-importer-e2e-ceph |
@awels test failures may be related to changes? |
Yes, I think so, investigating what the problem is. |
pkg/importer/data-processor.go
Outdated
@@ -308,6 +308,8 @@ func (dp *DataProcessor) resize() (ProcessingPhase, error) { | |||
return ProcessingPhaseError, err | |||
} | |||
dp.preallocationApplied = dp.preallocation | |||
} else { |
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.
It's cleaner to leave dp.preallocationApplied = dp.preallocation
outside the condition and do it once, but it's not a must.
/test pull-containerized-data-importer-e2e-ceph
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.
Good point in my mind it was not logically equivalent because the stuff above could return a different phase, but that would not change outside the else.
writing to the device Signed-off-by: Alexander Wels <awels@redhat.com>
336e58a
to
5186dcd
Compare
/test pull-containerized-data-importer-e2e-nfs |
} | ||
dp.preallocationApplied = dp.preallocation |
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.
given the current implementation isn't preallocationApplied
always true?
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.
Only if the DV has the preallocation flag set, the value of dp.preallocation will be true in that case, false otherwise
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 was mistaken when preallocationApplied
should always be true. My understanding of the implementation in this PR is:
- Regardless of what the user
preallocation
setting the disk will always be preallocated when not "converting" so raw, iso, etc - User preallocation settings are respected when "converting" from scratch space.
I am wondering if case 1 above is accurately reflected in dp.preallocationApplied
?
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 am not sure I can know at this point in the code, this gets run for multiple different datasources, some of which may preallocate and some may not. I do know that if the flag was set on the importer pod, then the datasource would respect it, if it was not set, it may or may not have preallocated depending on the datsource.
Maybe we need to add something to the datasource API where the datasource can indicate it preallocated or not.
/lgtm |
Is #2743 still relevant after this merges? |
/approve |
[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 |
@awels: once the present PR merges, I will cherry-pick it on top of release-v1.57 in a new PR and assign it to you. 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. |
@awels: #2832 failed to apply on top of branch "release-v1.57":
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. |
* Remove older nbdkit Signed-off-by: Alexander Wels <awels@redhat.com> * When converting always use scratch space importing instead of ndbkit. Once we are able to get nbdkit 1.35.8 or newer we can revert this change since that will include improvements to the downloading speed. Signed-off-by: Alexander Wels <awels@redhat.com> * Disable metrics test for import because straight import doesn't return total, and this means the metrics are disabled. Signed-off-by: Alexander Wels <awels@redhat.com> * Fix broken functional tests Signed-off-by: Alexander Wels <awels@redhat.com> * Address review comments Signed-off-by: Alexander Wels <awels@redhat.com> * Additional review comments. Fixed functional test that was not doing the right thing while running the test. Signed-off-by: Alexander Wels <awels@redhat.com> * Always set preallocation on block devices when directly writing to the device Signed-off-by: Alexander Wels <awels@redhat.com> --------- Signed-off-by: Alexander Wels <awels@redhat.com>
* Remove older nbdkit Signed-off-by: Alexander Wels <awels@redhat.com> * When converting always use scratch space importing instead of ndbkit. Once we are able to get nbdkit 1.35.8 or newer we can revert this change since that will include improvements to the downloading speed. Signed-off-by: Alexander Wels <awels@redhat.com> * Disable metrics test for import because straight import doesn't return total, and this means the metrics are disabled. Signed-off-by: Alexander Wels <awels@redhat.com> * Fix broken functional tests Signed-off-by: Alexander Wels <awels@redhat.com> * Address review comments Signed-off-by: Alexander Wels <awels@redhat.com> * Additional review comments. Fixed functional test that was not doing the right thing while running the test. Signed-off-by: Alexander Wels <awels@redhat.com> * Always set preallocation on block devices when directly writing to the device Signed-off-by: Alexander Wels <awels@redhat.com> --------- Signed-off-by: Alexander Wels <awels@redhat.com>
* Remove older nbdkit * When converting always use scratch space importing instead of ndbkit. Once we are able to get nbdkit 1.35.8 or newer we can revert this change since that will include improvements to the downloading speed. * Disable metrics test for import because straight import doesn't return total, and this means the metrics are disabled. * Fix broken functional tests * Address review comments * Additional review comments. Fixed functional test that was not doing the right thing while running the test. * Always set preallocation on block devices when directly writing to the device --------- Signed-off-by: Alexander Wels <awels@redhat.com>
What this PR does / why we need it:
nbdkit is slow in combination with qemu-img when importing and converting a qcow2 image. For now when converting always use scratch space, this is significantly faster. Once nbdkit 1.35.8 is available we can revisit this because that version will improve nbdkit performance significantly.
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 #2809
Special notes for your reviewer:
Also removes old version of nbdkit left over in WORKSPACE file, as well as fixing invalid number of arguments in some error returns
Release note: