Fix loading of kvm/kvm_intel/kvm_amd modules by praveenkumar · Pull Request #1454 · crc-org/crc

praveenkumar · 2020-08-11T09:13:48Z

Fixes: Issue #1449

Solution/Idea

Enable the kvm_intel and kvm_amd kernel module according to processor.

Testing

With current master following will fail to start the crc

$ sudo modprobe -r kvm_intel
$ $ lsmod | grep kvm
$ crc setup
DEBU Trying to load kvm module                    
DEBU exit status 1 : modprobe: ERROR: could not insert 'kvm': Operation not permitted 
FATA Failed to load kvm module

If you manually execute the sudo modprobe kvm then the start will fail

$ sudo modprobe kvm
$ $ lsmod | grep kvm
kvm                   815104  0
irqbypass              16384  1 kvm
$ ./crc setup
$ crc start
...
DEBU Checking if /dev/kvm exists                  
DEBU kvm kernel module is not loaded              
FATA kvm kernel module is not loaded

openshift-ci-robot · 2020-08-11T09:13:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: praveenkumar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [praveenkumar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

guillaumerose · 2020-08-11T09:20:51Z

pkg/crc/preflight/preflight_checks_linux.go

+	}
+	re := regexp.MustCompile(`flags.*:.*`)
+
+	flags := re.FindString(string(out))


Maybe use this library https://godoc.org/github.com/shirou/gopsutil/cpu#InfoStat ?

@guillaumerose this is what I also suggested to @cfergeau but since our usecase is not that much to include this complete library as dependency so I went without it.

if we merge #1396, we will have this library included 👼

@guillaumerose Ah, may be we will merge it soon :)

Praveen mentioned it, but it's indeed a bit overkill for what we are doing. No strong feeling either way if it's already an indirect dependency.

cfergeau · 2020-08-11T14:56:44Z

pkg/crc/preflight/preflight_checks_linux.go

 func fixKvmEnabled() error {
 	logging.Debug("Trying to load kvm module")
-	stdOut, stdErr, err := crcos.RunWithPrivilege("Load kvm kernel module","modprobe", "kvm")
+	stdOut, stdErr, err := crcos.RunWithPrivilege("Load kvm kernel module", "modprobe", "kvm")


Actually you could remove loading of kvm (contrary to what the commit log is saying), kvm_intel/kvm_amd will trigger the load of the kvm module if needed.

cfergeau · 2020-08-11T15:01:25Z

pkg/crc/preflight/preflight_checks_linux.go

 	}

-	re = regexp.MustCompile(`(vmx|svm)`)
+	re := regexp.MustCompile(`(vmx|svm)`)


This could almost use strings.Contains() to be consistent with the fixup method.

@cfergeau I then need to use 2 different strings.Contains since it doesn't allow the regex :(

cfergeau · 2020-08-11T15:01:49Z

pkg/crc/preflight/preflight_checks_linux.go

+			return fmt.Errorf("Failed to load kvm amd module: %s %v: %s", stdOut, err, stdErr)
+		}
+	default:
+		logging.Debug("Not able to detect processor details")


cfergeau · 2020-08-11T15:07:49Z

pkg/crc/preflight/preflight_checks_linux.go

+	}
+	re := regexp.MustCompile(`flags.*:.*`)
+
+	flags := re.FindString(string(out))


Praveen mentioned it, but it's indeed a bit overkill for what we are doing. No strong feeling either way if it's already an indirect dependency.

cfergeau · 2020-08-13T12:03:00Z

pkg/crc/preflight/preflight_checks_linux.go

 	if err != nil {
-		logging.Debugf("%v : %s", err, buf.String())
-		return fmt.Errorf("Failed to load kvm module")
+		return fmt.Errorf("Failed to load kvm module: %s %v: %s", stdOut, err, stdErr)


Fwiw, what would be nice would be to define an RunError type

type RunError struct { stdOut string stdErr string err Error }

and make use of return fmt.Errorf("Failed to load kvm module: %w", runError)
(see https://blog.golang.org/go1.13-errors )
This way we keep a relatively structured error, in case higher layers don't want to show stdOut/stdErr on errors, or on the contrary, want to show it, but split on multiple lines, .. This probably also means the stdErr return value can be removed.

@cfergeau I think then this should go as part of runCmd where we can wrap err and stderr as part of struct.

cfergeau · 2020-08-13T12:05:26Z

pkg/crc/preflight/preflight_checks_linux.go

 	// #nosec G204
-	cmd = exec.Command("virsh", "--connect", "qemu:///system", "net-undefine", libvirt.DefaultNetwork)
-	_ = cmd.Run()
+	_,_,_ = crcos.RunWithDefaultLocale("virsh", "--connect", "qemu:///system", "net-undefine", libvirt.DefaultNetwork)


Can you make the spacing consistent with the previous call?

cfergeau · 2020-08-13T12:07:38Z

pkg/crc/preflight/preflight_checks_linux.go

 func fixKvmEnabled() error {
 	logging.Debug("Trying to load kvm module")
-	stdOut, stdErr, err := crcos.RunWithPrivilege("Load kvm kernel module","modprobe", "kvm")
+	stdOut, stdErr, err := crcos.RunWithPrivilege("Load kvm kernel module", "modprobe", "kvm")


Can you move this to the initial commit, since this is when the missing space was introduced.

cfergeau · 2020-08-13T12:08:22Z

pkg/crc/preflight/preflight_checks_linux.go

+	_, _, _ = crcos.RunWithDefaultLocale("virsh", "--connect", "qemu:///system", "net-destroy", libvirt.DefaultNetwork)
 	// #nosec G204
-	_,_,_ = crcos.RunWithDefaultLocale("virsh", "--connect", "qemu:///system", "net-undefine", libvirt.DefaultNetwork)
+	_, _, _ = crcos.RunWithDefaultLocale("virsh", "--connect", "qemu:///system", "net-undefine", libvirt.DefaultNetwork)


Ah, this belongs to the previous commit where I asked to make the spacing consistent 👍

we should use our own implementation of command execution when possible it provide us stdout, stderr and err instead using exec.Command where we need to create buffer for stderr and stdout.

For intel we need to run `modprobe kvm_intel` to get the file `/dev/kvm` and for amd `modprobe kvm_amd`, it will also enable the `kvm` module.

With centos-8 there is no `kvm` package so remove it

openshift-ci-robot requested review from anjannath and cfergeau August 11, 2020 09:13

openshift-ci-robot added the approved label Aug 11, 2020

guillaumerose reviewed Aug 11, 2020

View reviewed changes

cfergeau reviewed Aug 13, 2020

View reviewed changes

cfergeau mentioned this pull request Aug 13, 2020

crc setup fails with setting up KVM on vanilla CentOS 7 #1449

Closed

cfergeau changed the title ~~Issue 1449~~ Fix loading of kvm/kvm_intel/kvm_amd modules Aug 13, 2020

praveenkumar added 4 commits August 17, 2020 12:10

Use RunWithDefaultLocale instead exec.Command when possible

c4147d8

we should use our own implementation of command execution when possible it provide us stdout, stderr and err instead using exec.Command where we need to create buffer for stderr and stdout.

Use sudo for modprobe command

182b6da

Add processor specific modprobe command

88e8ae2

For intel we need to run `modprobe kvm_intel` to get the file `/dev/kvm` and for amd `modprobe kvm_amd`, it will also enable the `kvm` module.

[Centos-CI] Remove kvm from package list

f1a1471

With centos-8 there is no `kvm` package so remove it

praveenkumar merged commit 2f8acc0 into crc-org:master Aug 17, 2020

Conversation

praveenkumar commented Aug 11, 2020

Solution/Idea

Testing

Uh oh!

openshift-ci-robot commented Aug 11, 2020

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!

Choose a reason for hiding this comment

Uh oh!

cfergeau Aug 13, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cfergeau Aug 13, 2020 •

edited

Loading