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.

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

Domain type 'kvm' should be configurable and use qemu:/// uri. #182

Merged
merged 1 commit into from May 6, 2014

Conversation

purpleidea
Copy link
Contributor

Previous versions simply defaulted to 'kvm'. Now that they default to
'qemu', there is typically a 5x performance decrease that makes vagrant
unusably slow.

Previous versions simply defaulted to 'kvm'. Now that they default to
'qemu', there is typically a 5x performance decrease that makes vagrant
unusably slow.
@purpleidea
Copy link
Contributor Author

Yikes! No wonder my virtual machines were so slow when testing 1.5.1.
This is the fix. Review and merge ASAP please :)
I think this calls for a new release!

purpleidea added a commit to purpleidea/puppet-gluster that referenced this pull request May 5, 2014
This wasn't necessary in previous versions of vagrant-libvirt because
this option specified the uri, and the domain type defaulted to kvm. In
newer releases, you have to specify this for performance, otherwise it
will default to qemu software only virtualization. This depends on:
vagrant-libvirt/vagrant-libvirt#182
@purpleidea
Copy link
Contributor Author

Let me know when this is merged so that I can push:
https://github.com/purpleidea/puppet-gluster/tree/kvm-fix
to master.

Cheers
James

@sciurus
Copy link
Contributor

sciurus commented May 5, 2014

Hi James, what commit(s) caused the problem?

purpleidea added a commit to purpleidea/puppet-gluster that referenced this pull request May 5, 2014
This wasn't necessary in previous versions of vagrant-libvirt because
this option specified the uri, and the domain type defaulted to kvm. In
newer releases, you have to specify this for performance, otherwise it
will default to qemu software only virtualization. This depends on:
vagrant-libvirt/vagrant-libvirt#182
@purpleidea
Copy link
Contributor Author

@sciurus

I didn't bisect it, but in (vagrant-libvirt 0.0.14 i think) this worked. In action/create_domain.rb there is:

      # TODO get type from driver config option
      @domain_type = 'kvm'

So since it was hardcoded in, this always worked.
Now that this is different, new virtual machines don't get the 'kvm' type in the domain.xml.erb template, and they are slow.

Does this help?

PS: Thanks for the fast response...

@purpleidea
Copy link
Contributor Author

I should note that it's fine that we can actually use this property now (I personally don't need to, but whatever) however if we specify type kvm, it should actually get passed through and used. Without my patch, you can't specify the kvm domain type.

(Unless I misunderstood this problem somehow, however this patch works great for me!)

pronix added a commit that referenced this pull request May 6, 2014
Domain type 'kvm' should be configurable and use qemu:/// uri.
@pronix pronix merged commit f138076 into vagrant-libvirt:master May 6, 2014
@sciurus
Copy link
Contributor

sciurus commented May 11, 2014

Sorry for being slow to grasp what is going on here.

@purpleidea what you are saying is that e382687 disabled hardware acceleration by default? That's a serious regression. I guess the easy fix is to change the default in config.rb (and the README and sample vagrantfile) to kvm.

lib/vagrant-libvirt/config.rb
16:      attr_accessor :driver
62:        @driver            = UNSET_VALUE
126:      @driver = 'qemu' if @driver == UNSET_VALUE

lib/vagrant-libvirt/action/create_domain.rb
44:          @domain_type = config.driver

lib/vagrant-libvirt/templates/domain.xml.erb
1:<domain type='<%= @domain_type %>'>

@purpleidea
Copy link
Contributor Author

@sciurus Yeah, it was a pretty serious regression. Keep in mind that my patch is still important (with or without the e382687 regression) because we need to do correct uri parsing to still use qemu:/// as there's no "kvm:///" uri :)

I don't know if there are other implications to changing the default to 'kvm'. Also, this all "works for me", but I haven't tested in other environments.

@purpleidea
Copy link
Contributor Author

BTW, nice catch finding that patch :) If we really had time, we'd review their other patches for regressions, or find out more about why qemu was a better default for them...

@sciurus
Copy link
Contributor

sciurus commented May 11, 2014

Right, I get what you're saying about the URI. PR as soon as I finish testing.

@purpleidea
Copy link
Contributor Author

@pradels @pronix @sciurus cough release :)

@sciurus
Copy link
Contributor

sciurus commented May 11, 2014

Martin was working on xen support, so the hard-coded kvm value didn't work for him.

For anyone else who was confused: although qemu/kvm is documented as being a single driver and always uses qemu in the uri, you have to specify qemu or kvm as the domain type in the xml depending on whether or not you want hardware acceleration. It's easy to miss the problem since afaik for all the other drivers there is a only one domain type.

@purpleidea
Copy link
Contributor Author

On Sun, May 11, 2014 at 12:01 AM, Brian Pitts notifications@github.comwrote:

Martin was working on xen support, so the hard-coded kvm value didn't work
for him.

Ah!

For anyone else who was confused: although qemu/vkmhttp://libvirt.org/drvqemu.htmlis documented as being a single driver and always uses
qemu in the uri, you have to specify qemu or kvm as the domain type
in the xml depending on whether or not you want hardware acceleration. It's
easy to miss the problem since afaik for all the other drivers there is a
only one domain type.

Yep :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants