Skip to content
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.

Sign up for GitHub

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

Jump to bottom

core: Preallocate blank block volumes #1559

Merged
merged 1 commit into from Jan 18, 2021
Merged

core: Preallocate blank block volumes #1559

merged 1 commit into from Jan 18, 2021

Conversation

tomob
Copy link
Contributor

@tomob tomob commented Jan 12, 2021

Signed-off-by: Tomasz Baranski tbaransk@redhat.com

What this PR does / why we need it:
Importer pod needs to be started for blank block volumes and it needs to
handle the case.

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:

NONE

core: Preallocate blank block volumes
86a8e9e 
Importer pod needs to be started for blank block volumes and it needs to
handle the case.

Signed-off-by: Tomasz Baranski <tbaransk@redhat.com>
@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Jan 12, 2021
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 73.73% when pulling 86a8e9e on tomob:preallocation-blank-block-disk into d31591d on kubevirt:master.

@tomob
Copy link
Contributor Author

tomob commented Jan 12, 2021

/retest

@maya-r
Copy link
Contributor

maya-r commented Jan 13, 2021

Sorry to be asking this question rather late -- why do we want to preallocate block disks?
the code looks good to me.

@maya-r
Copy link
Contributor

maya-r commented Jan 13, 2021

OK, I can answer my own question: because if someone asks for preallocation it's surprising if it doesn't do that in some scenarios.
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2021
awels
awels reviewed Jan 15, 2021
View reviewed changes
pkg/image/qemu.go Show resolved Hide resolved
tests/import_test.go
Skip("Storage Class for block volume is not available")
}

return utils.NewDataVolumeForBlankRawImageBlock("import-dv", "100Mi", f.BlockSCName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A full alloc should be pretty slow, so there should be enough time to verify that a pod actually was created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default 270 seconds should be enough for anybod^W writing 100Mi of zeros.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the new part here is that for a block device we DO create a pod if preallocation is set to true. So our test should actually verify the pod is created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test verifies that the preallocation annotation has been created, and it's only created when the pod started AND finished successfully.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, didn't realize the annotation was created only if the pod started and finished.

awels
awels reviewed Jan 18, 2021
View reviewed changes
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@kubevirt-bot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2021
@kubevirt-bot kubevirt-bot merged commit 9668eb7 into kubevirt:master Jan 18, 2021
@tomob
Copy link
Contributor Author

tomob commented Feb 16, 2021

/cherry-pick release-v1.28

@kubevirt-bot
Copy link
Contributor

@tomob: #1559 failed to apply on top of branch "release-v1.28":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	cmd/cdi-importer/importer.go
M	pkg/controller/import-controller.go
M	tests/import_test.go
Falling back to patching base and 3-way merge...
Auto-merging tests/import_test.go
Auto-merging pkg/controller/import-controller.go
Auto-merging cmd/cdi-importer/importer.go
CONFLICT (content): Merge conflict in cmd/cdi-importer/importer.go
Patch failed at 0001 core: Preallocate blank block volumes

In response to this:

/cherry-pick release-v1.28

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.

tomob added a commit to tomob/containerized-data-importer that referenced this pull request Feb 16, 2021
@tomob
core: Preallocate blank block volumes (kubevirt#1559)
fc03ef8 
Importer pod needs to be started for blank block volumes and it needs to
handle the case.

Signed-off-by: Tomasz Baranski <tbaransk@redhat.com>
@tomob tomob mentioned this pull request Feb 16, 2021
Merged
tomob added a commit to tomob/containerized-data-importer that referenced this pull request Feb 17, 2021
@tomob
core: Preallocate blank block volumes (kubevirt#1559)
5b847fc 
Importer pod needs to be started for blank block volumes and it needs to
handle the case.

Signed-off-by: Tomasz Baranski <tbaransk@redhat.com>

Signed-off-by: Tomasz Barański <tomob@users.noreply.github.com>
tomob added a commit to tomob/containerized-data-importer that referenced this pull request Feb 23, 2021
@tomob
core: Preallocate blank block volumes (kubevirt#1559)
f452cfc 
Importer pod needs to be started for blank block volumes and it needs to
handle the case.

Signed-off-by: Tomasz Baranski <tbaransk@redhat.com>
kubevirt-bot pushed a commit that referenced this pull request Mar 11, 2021
@tomob
core: Preallocate blank block volumes (#1559) (#1664)
5153a13 
Importer pod needs to be started for blank block volumes and it needs to
handle the case.

Signed-off-by: Tomasz Baranski <tbaransk@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awels awels awels left review comments

@aglitke aglitke Awaiting requested review from aglitke

Assignees

@awels awels

@maya-r maya-r

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tomob @coveralls @maya-r @kubevirt-bot @awels