core: Preallocate blank image disks as well #1561
Conversation
This fixes a bug where blank image disks were not preallocated. This is a cherry-pick from master to v28, including all dependencies required for functional tests but without new functionality. Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1914177 Signed-off-by: Tomasz Baranski <tbaransk@redhat.com>
kubevirt-bot
added
release-note-none
|
/rerun |
| if shouldPreallocate { | ||
| return ProcessingPhasePreallocate, nil | ||
| } | ||
| dp.preallocationApplied = "skipped" // qemu did not preallicate space for a resized file |
| if contentType == common.FilesystemCloneContentType { | ||
| return filesystemCloneProcessor(stream, common.ImporterVolumePath) | ||
| return "false", filesystemCloneProcessor(stream, common.ImporterVolumePath) |
There was a problem hiding this comment.
Is there a strong reason to use strings that are booleans? it's a bummer to lose the benefit of type-checking
There was a problem hiding this comment.
It's "true", "false" or "skipped".
I could do a type redefinition here (like type PreallocationStatus string), but it seemed to me like an overkill.
There was a problem hiding this comment.
In Go, in general it is very simple to do the redefinition, and it adds some type safety, in particular with typos in the strings. maybe you can do:
type Preallocation string
const PreallocationYes Preallocation="true"
const PreallocationNo Preallocation="false"
const PreallocationSkip Preallocation="skipped"And you can pass the Yes/No instead of the booleans, and return Yes/No/Skip for the applied. I think that would make it easier to read. I had to re-read a few times before I found the place where the value was set to none.
There was a problem hiding this comment.
This is a cherry-pick for 1.28. I don't want to change this here, let me create a new PR for that.
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awels 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 |
kubevirt-bot
added
the
approved
|
/cherrypick release-v1.28 |
|
@awels: once the present PR merges, I will cherry-pick it on top of release-v1.28 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: base branch (release-v1.28) needs to differ from target branch (release-v1.28) 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. |
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1914177
Signed-off-by: Tomasz Baranski tbaransk@redhat.com
What this PR does / why we need it:
This fixes a bug where blank image disks were not preallocated.
This is a cherry-pick from master to v28, including all dependencies
required for functional tests but without new functionality.
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 #
Special notes for your reviewer:
Release note: