nova: enable nested virt on Intel by bmwiedemann · Pull Request #1304 · crowbar/crowbar-openstack

bmwiedemann · 2017-09-22T11:19:05Z

because it defaults to off
but a lot of people rely on nested virt being available

While in https://fate.suse.com/320082 the virtualisation team
declined to promote nested virt to fully supported status for SLE12,
we are using this since 2012 in all kinds of places without problems.

same as #1261

vuntz

As said in the other pull request: I'm not really a big fan of enabling this by default. If anything, I'd prefer if it were an option.

My rationale is that enabling by default something that is tech preview leads people to believe it's not tech preview.

@vuntz

I think @vuntz has a point...

dirkmueller

make it a configurable, defaulting to off?

dirkmueller · 2018-01-15T09:51:40Z

+                  if grep -qw vmx /proc/cpuinfo ; then
+                    ! grep -q nested /etc/modprobe.d/80-kvm-intel.conf &&
+                      echo "options kvm_intel nested=1" > /etc/modprobe.d/80-kvm-intel.conf &&
+                      /sbin/modprobe -r kvm-intel


/sbin/modprobe -r kvm_intel

Either should work.

dirkmueller · 2018-01-15T09:51:52Z

+                    ! grep -q nested /etc/modprobe.d/80-kvm-intel.conf &&
+                      echo "options kvm_intel nested=1" > /etc/modprobe.d/80-kvm-intel.conf &&
+                      /sbin/modprobe -r kvm-intel
+                    /sbin/modprobe kvm-intel


/sbin/modprobe kvm_intel

nicolasbock · 2018-01-15T12:00:31Z

+                  if grep -qw vmx /proc/cpuinfo ; then
+                    ! grep -q nested /etc/modprobe.d/80-kvm-intel.conf &&
+                      echo "options kvm_intel nested=1" > /etc/modprobe.d/80-kvm-intel.conf &&
+                      /sbin/modprobe -r kvm-intel


Either should work.

houndci-bot · 2018-01-19T13:48:58Z

+                  ! grep -q N /sys/module/kvm_intel/parameters/nested ||
+                    /sbin/modprobe -r kvm_intel
+              EOF
+              only_if { node[:nova][:kvm][:nested_virt] && `uname -r`.include?("default") && system("grep -qw vmx /proc/cpuinfo") }


Metrics/LineLength: Line is too long. [131/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

houndci-bot · 2018-01-19T13:48:59Z

+                    echo "options kvm_intel nested=1" > /etc/modprobe.d/80-kvm-intel.conf
+                  ! grep -q N /sys/module/kvm_intel/parameters/nested ||
+                    /sbin/modprobe -r kvm_intel
+              EOF


Naming/HeredocDelimiterNaming: Use meaningful heredoc delimiters. (https://github.com/bbatsov/ruby-style-guide#heredoc-delimiters)

houndci-bot · 2018-01-19T13:51:14Z

+              EOF
+              only_if { node[:nova][:kvm][:nested_virt] &&
+                  `uname -r`.include?("default") &&
+                  system("grep -qw vmx /proc/cpuinfo") }


Layout/MultilineOperationIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylemultilineoperationindentation)
Layout/BlockEndNewline: Expression at 121, 56 should be on its own line.

houndci-bot · 2018-01-19T13:51:14Z

+                    /sbin/modprobe -r kvm_intel
+              EOF
+              only_if { node[:nova][:kvm][:nested_virt] &&
+                  `uname -r`.include?("default") &&


Layout/MultilineOperationIndentation: Use 2 (not 4) spaces for indenting an expression spanning multiple lines. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylemultilineoperationindentation)

houndci-bot · 2018-01-19T13:51:14Z

+                  ! grep -q N /sys/module/kvm_intel/parameters/nested ||
+                    /sbin/modprobe -r kvm_intel
+              EOF
+              only_if { node[:nova][:kvm][:nested_virt] &&


Style/BlockDelimiters: Avoid using {...} for multi-line blocks. (https://github.com/bbatsov/ruby-style-guide#single-line-blocks)
Layout/MultilineBlockLayout: Block body expression is on the same line as the block start.

bmwiedemann · 2018-01-19T13:54:05Z

updated it with an optional bool (without migration because it is not required and makes downgrades easier)

houndci-bot · 2018-01-19T14:00:56Z

+              only_if do
+                node[:nova][:kvm][:nested_virt] &&
+                `uname -r`.include?("default") &&
+                system("grep -qw vmx /proc/cpuinfo")


Layout/MultilineOperationIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylemultilineoperationindentation)

houndci-bot · 2018-01-19T14:00:57Z

+              SHELL
+              only_if do
+                node[:nova][:kvm][:nested_virt] &&
+                `uname -r`.include?("default") &&


Layout/MultilineOperationIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylemultilineoperationindentation)

nicolasbock · 2018-01-19T16:11:30Z

        "secret_uuid": ""
      },
      "kvm": {
+        "nested_virt": false,


Don't you need a migration?

no, the value is marked as optional (required false) in the .schema file

Ooh, that's a new one for me 😄 Ok, thanks this looks good then!

nicolasbock · 2018-01-19T16:16:03Z

        "secret_uuid": ""
      },
      "kvm": {
+        "nested_virt": false,


Ooh, that's a new one for me 😄 Ok, thanks this looks good then!

bmwiedemann · 2018-02-05T10:31:05Z

@vuntz @dirkmueller please re-review.

vuntz · 2018-04-03T17:57:12Z

+            execute "enable kvm intel nested virt" do
+              command <<-SHELL
+                  grep -q nested /etc/modprobe.d/80-kvm-intel.conf ||
+                    echo "options kvm_intel nested=1" > /etc/modprobe.d/80-kvm-intel.conf


Just wondering (because that's how I would have done it): why is this part not a cookbook_file resource?

vuntz · 2018-04-03T18:02:05Z

+                  `uname -r`.include?("default") &&
+                  system("grep -qw vmx /proc/cpuinfo")
+              end
+            end


If nested_virt is false, we also need code to disable nested virtualization.

vuntz · 2018-04-03T18:02:21Z

+                  grep -q nested /etc/modprobe.d/80-kvm-intel.conf ||
+                    echo "options kvm_intel nested=1" > /etc/modprobe.d/80-kvm-intel.conf
+                  ! grep -q N /sys/module/kvm_intel/parameters/nested ||
+                    /sbin/modprobe -r kvm_intel


Won't this cause failures if there's already a VM running?

the module cannot be unloaded then anymore

houndci-bot · 2018-04-04T08:02:20Z

  })
+  only_if do
+    `uname -r`.include?("default") &&
+    system("grep -qw vmx /proc/cpuinfo")


Layout/MultilineOperationIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylemultilineoperationindentation)

houndci-bot · 2018-04-04T08:02:20Z

+  source "kvm-intel-nested.conf.erb"
  variables({
-    kvm: node[:nova][:kvm]
+    kvm_nested_enabled: node[:nova][:kvm][:nested_virt]


Layout/IndentHash: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

houndci-bot · 2018-04-04T08:02:20Z

-template "/etc/default/qemu-kvm" do
-  source "qemu-kvm.erb"
+# set the nested KVM setting
+template "/etc/modprobe.d/80-kvm-intel.conf" do


Style/MultilineIfModifier: Favor a normal if-statement over a modifier clause in a multiline statement. (https://github.com/bbatsov/ruby-style-guide#no-multiline-if-modifiers)

houndci-bot · 2018-04-04T08:04:44Z

+  only_if do
+    node[:platform_family] == "suse" &&
+    `uname -r`.include?("default") &&
+    system("grep -qw vmx /proc/cpuinfo")


Layout/MultilineOperationIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylemultilineoperationindentation)

houndci-bot · 2018-04-04T08:04:44Z

+  )
+  only_if do
+    node[:platform_family] == "suse" &&
+    `uname -r`.include?("default") &&


Layout/MultilineOperationIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylemultilineoperationindentation)

rsalevsky · 2018-05-15T11:56:08Z

Needs a rebase.

because it defaults to off but a lot of people rely on nested virt being available While in https://fate.suse.com/320082 the virtualisation team declined to promote nested virt to fully supported status for SLE12, we are using this since 2012 in all kinds of places without problems.

bmwiedemann requested review from dirkmueller and vuntz September 22, 2017 11:19

nicolasbock previously approved these changes Sep 22, 2017

View reviewed changes

vuntz suggested changes Sep 23, 2017

View reviewed changes

dirkmueller requested changes Oct 16, 2017

View reviewed changes

dirkmueller reviewed Jan 15, 2018

View reviewed changes

nicolasbock previously approved these changes Jan 15, 2018

View reviewed changes

bmwiedemann dismissed nicolasbock’s stale review via 536fc30 January 19, 2018 13:48

houndci-bot reviewed Jan 19, 2018

View reviewed changes

nicolasbock suggested changes Jan 19, 2018

View reviewed changes

nicolasbock previously approved these changes Jan 19, 2018

View reviewed changes

dirkmueller previously approved these changes Apr 3, 2018

View reviewed changes

vuntz suggested changes Apr 3, 2018

View reviewed changes

dirkmueller dismissed stale reviews from nicolasbock and themself via ef50233 April 4, 2018 07:56

houndci-bot reviewed Apr 4, 2018

View reviewed changes

vuntz previously approved these changes Apr 4, 2018

View reviewed changes

dirkmueller dismissed vuntz’s stale review via a31d0f3 April 4, 2018 08:04

houndci-bot reviewed Apr 4, 2018

View reviewed changes

vuntz previously approved these changes Apr 4, 2018

View reviewed changes

toabctl previously approved these changes Apr 4, 2018

View reviewed changes

toabctl added this to the Cloud 8 GMC milestone Apr 4, 2018

nicolasbock previously approved these changes Apr 4, 2018

View reviewed changes

bmwiedemann dismissed stale reviews from nicolasbock, toabctl, and vuntz via afbcc5c May 16, 2018 09:11

rsalevsky approved these changes May 17, 2018

View reviewed changes

nicolasbock approved these changes May 17, 2018

View reviewed changes

rsalevsky merged commit 0da741a into crowbar:master May 17, 2018

rsalevsky deleted the nestedvirtm branch May 17, 2018 15:35

bmwiedemann mentioned this pull request May 17, 2018

nova: allow to enable nested virt on Intel #1671

Merged

Conversation

bmwiedemann commented Sep 22, 2017

Uh oh!

vuntz left a comment

Choose a reason for hiding this comment

Uh oh!

dirkmueller left a comment

Choose a reason for hiding this comment

Uh oh!

dirkmueller Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirkmueller Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmwiedemann commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmwiedemann commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsalevsky commented May 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

dirkmueller Jan 15, 2018 •

edited

Loading

dirkmueller Jan 15, 2018 •

edited

Loading