-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Qemu 2.10 requires -U flag to read volume metadata
#4567
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
Qemu 2.10 requires -U flag to read volume metadata
#4567
Conversation
5f49669 to
849035f
Compare
|
@GutoVeronezi what error/issue do you see and on which KVM distro? |
|
@rhtyd I checked this one with him. The problem happens on Qemu 2.10+ (running on Ubuntu). The release notes from Qemu have the following message. The problem is happening when ACS tries to get some info from a volume using qemu-img. Then, one would get the following error. |
|
Okay but which specific version KVM/Linux distro is it reproducible on? I'll want to test this. |
|
@rhtyd It has been tested over Ubuntu 18.04 with KVM 2.11.1 |
weizhouapache
left a comment
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.
code lgtm
@DaanHoogland @rhtyd we need this in 4.15.0.0 as well.
|
Looks like RC4 has been cut, given Ubuntu 18.04 has been around for quite some time should this be considered a blocker? |
|
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2526 |
|
Pkging was kicked manually BO pkging integration for some reason is not working (I'll check tomorrow) |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
@GutoVeronezi @weizhouapache probably too late now - RC4 is cut; in order to get the PR into 4.15, the base branch needs to be changed |
|
Thanks for the review guys. To us, it is fine if it only goes to master. |
ok, let's merge this after 4.15.0.0 release |
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.
Code LGTM...
I think we need Unit tests to cover these changes... But to do this you'll need to do some changes in many methods.
Maybe you can do this in another PR?
Another point ... maybe you can better explain how this branch can be tested.
It has been tested locally.
Your explanation does not tell us anything related to the tests.
|
@RodrigoDLopez to test this enhancement I had used the |
GabrielBrascher
left a comment
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.
Code LGTM.
However, considering that this impacts versions that have at least ubuntu 18.04 (if I am not mistaken Ubuntu 18.04 gets by default qemu-kvm 2.11). It would be nice to have this on prior LTSs.
Any comment on what branches/releases should we target this one? @rhtyd @weizhouapache @rafaelweingartner and others
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2615 |
|
@blueorangutan package |
|
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2617 |
|
@blueorangutan test |
|
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Thanks, @GutoVeronezi! |
GabrielBrascher
left a comment
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.
Tests indicate that all LibvirtException have been handled. LGTM.
|
[S] Trillian test result (tid-181)
|
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests [S] |
|
[S] Trillian test result (tid-188)
|
|
[S] Trillian test result (tid-189)
|
@GabrielBrascher I merged based on this comment of yours, but then we had some internal discussion on whether it was tested enough. I don't think any of the failures above are due to this change, but still, can you comment a bit more on testing please? cc @GutoVeronezi |
…e#4567)" Co-authored-by: Daniel Augusto Veronezi Salvador <[email protected]> * Conflicts: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptor.java * Fixed conflict by removing the class above, which is introduced only on future releases. Signed-off-by: gabriel <[email protected]>
Description
Qemu 2.10 added the requirement of a --force-share flag to 'qemu-img info' command when reading information about a disk that is in use by a guest. After this Qemu release, to obtain the attached disk metadata, we need to use a flag to force information reading.This PR intends to add the required flag to the
qemu-img infocommand if the qemu version is 2.10+. We can get the Qemu version in the LibvirtConnection.Fixes: #4024
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
It has been tested locally.