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

set cpu model only for custom cpu mode #678

Merged
merged 4 commits into from Oct 29, 2016
Merged

set cpu model only for custom cpu mode #678

merged 4 commits into from Oct 29, 2016

Conversation

evgeni
Copy link
Contributor

@evgeni evgeni commented Oct 23, 2016

according to https://libvirt.org/formatdomain.html#elementsCPU setting the model is not supported when using "host-model" and with recent libvirt this actually results in errors like this:

There was an error talking to Libvirt. The error message is shown
below:

Call to virDomainCreateWithFlags failed: the CPU is incompatible with host CPU: Host CPU does not provide required features: svm

@evgeni
only set the cpu model when using a custom cpu mode
517da85 
according to https://libvirt.org/formatdomain.html#elementsCPU
setting the model is not supported when using "host-model" and
with recent libvirt this actually results in errors like this:
 Call to virDomainCreateWithFlags failed:
  the CPU is incompatible with host CPU:
  Host CPU does not provide required features: svm
@evgeni
also set the cpu_model in the config accordingly to the cpu_mode
7ec7b8e
@evgeni
adjust README.md to reflect latest defaults changes
7f11cb1
pronix
pronix reviewed Oct 23, 2016
View reviewed changes
lib/vagrant-libvirt/config.rb
@@ -502,7 +502,8 @@ def finalize!
@memory = 512 if @memory == UNSET_VALUE
@cpus = 1 if @cpus == UNSET_VALUE
@cpu_mode = 'host-model' if @cpu_mode == UNSET_VALUE
@cpu_model = 'qemu64' if @cpu_model == UNSET_VALUE
@cpu_model = 'qemu64' if (@cpu_model == UNSET_VALUE and @cpu_mode == 'custom')
@cpu_model = '' if (@cpu_mode != 'custom')
Copy link
Member

Choose a reason for hiding this comment

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

twice assigned variable.

Copy link
Contributor Author

@evgeni evgeni Oct 23, 2016 •
edited

Choose a reason for hiding this comment

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

would you prefer:

if (@cpu_model == UNSET_VALUE and @cpu_mode == 'custom')
  @cpu_model = 'qemu64'
elsif (@cpu_model == UNSET_VALUE)
  @cpu_model = ''
end

or even

if (@cpu_model == UNSET_VALUE and @cpu_mode == 'custom')
  @cpu_model = 'qemu64'
elsif (@cpu_mode != 'custom')
  @cpu_model = ''
end

instead?

Copy link
Member

Choose a reason for hiding this comment

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

@cpu_model = if ... elsif .... else raise.... end

lib/vagrant-libvirt/templates/domain.xml.erb
@@ -7,7 +7,7 @@

<cpu mode='<%= @cpu_mode %>'>
<% if @cpu_mode != 'host-passthrough' %>
<model fallback='<%= @cpu_fallback %>'><%= @cpu_model %></model>
<model fallback='<%= @cpu_fallback %>'><% if @cpu_mode == 'custom' %><%= @cpu_model %><% end %></model>
Copy link
Member

Choose a reason for hiding this comment

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

if custom then @cpu_model also custom

Copy link
Contributor Author

@evgeni evgeni Oct 23, 2016 •
edited

Choose a reason for hiding this comment

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

not sure I understand your comment :(

Copy link
Member

Choose a reason for hiding this comment

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

you are right. ignore it.

pronix
pronix reviewed Oct 28, 2016
View reviewed changes
lib/vagrant-libvirt/config.rb
@@ -502,7 +502,11 @@ def finalize!
@memory = 512 if @memory == UNSET_VALUE
@cpus = 1 if @cpus == UNSET_VALUE
@cpu_mode = 'host-model' if @cpu_mode == UNSET_VALUE
@cpu_model = 'qemu64' if @cpu_model == UNSET_VALUE
@cpu_model = if (@cpu_model == UNSET_VALUE and @cpu_mode == 'custom')
'qemu64'
Copy link
Member

Choose a reason for hiding this comment

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

previous default value was qemu64
right now for keep same settings require define cpu_mode to "custom"
i think default settings and behaviour should not be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the point of this PR ;-)

While it is okay to set "random" values for cpu_model when cpu_mode is custom, but when set to host-model or host-passthrough it is not really supported (https://libvirt.org/formatdomain.html#elementsCPU) and may even make the VM fail to start (https://www.redhat.com/archives/libvir-list/2016-May/msg01940.html).

Thus the idea was to to keep the qemu64 model while in custom mode, but reset it to an empty string in any other mode.

@pronix pronix merged commit 780b6b9 into vagrant-libvirt:master Oct 29, 2016
@pronix
Copy link
Member

pronix commented Oct 29, 2016

@evgeni thanks

@lelutin
Copy link

lelutin commented Nov 23, 2016

I'm still experiencing this issue on latest release (which was done before this PR was merged). I'd encourage project maintainers to make a new release in order to get this fix out the door :)

@ekohl ekohl mentioned this pull request Jan 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pronix pronix pronix left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@evgeni @pronix @lelutin