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

kvm: Passthrough host CPU in order to allow nesting #2555

Merged
merged 4 commits into from
Feb 22, 2018
Merged

kvm: Passthrough host CPU in order to allow nesting #2555

merged 4 commits into from
Feb 22, 2018

Conversation

fabiand
Copy link
Contributor

@fabiand fabiand commented Feb 16, 2018 •
edited
Loading

Before this patch the virtual CPU was the stock qemu CPU, in the sense that
the virtual CPU features were set according to the default qemu CPU.

With this change the CPU features of the host will be copied at start to the
domain definition. This includes features like svm and vmx, which in turn allow
to run nested virtualization if the host is configured accordingly i.e.
kvm_intel nested=y in /etc/modprobe.d/kvm.conf.

To turn on nesting, a user has to specify --kvm-cpu-model host-model when
creating the VM.

Resolves #2553

Signed-off-by: Fabian Deutsch fabiand@fedoraproject.org

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 16, 2018
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fabiand
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: luxas

Assign the PR to them by writing /assign @luxas in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 16, 2018
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 16, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 16, 2018
@fabiand
kvm: Passthrough host CPU in order to allow nesting
7a4babe 
Before this patch the virtual CPU was the stock qemu CPU, in the sense that
the virtual CPU features were set according to the default qemu CPU.

With this change the CPU features of the host will be copied at start to the
domain definition. This includes features like svm and vmx, which in turn allow
to run nested virtualization if the host is configured accordingly i.e.
`kvm_intel nested=y` in `/etc/modprobe.d/kvm.conf`.

To turn on nesting, a user has to specify `--kvm-cpu-model host-model` when
creating the VM.

Resolves kubernetes#2553

Signed-off-by: Fabian Deutsch <fabiand@fedoraproject.org>
@fabiand
iso: Enable KVM
5c5120d 
This patch enables KVM support inside the ISO to support nesting.

Signed-off-by: Fabian Deutsch <fabiand@fedoraproject.org>
@fabiand
Copy link
Contributor Author

fabiand commented Feb 16, 2018

Should be in a reviewable state now.

@fabiand
Copy link
Contributor Author

fabiand commented Feb 16, 2018

/assign @luxas

@fabiand
Copy link
Contributor Author

fabiand commented Feb 16, 2018

I cna confirm that nesting works:

$ minikube ssh
                         _             _            
            _         _ ( )           ( )           
  ___ ___  (_)  ___  (_)| |/')  _   _ | |_      __  
/' _ ` _ `\| |/' _ `\| || , <  ( ) ( )| '_`\  /'__`\
| ( ) ( ) || || ( ) || || |\`\ | (_) || |_) )(  ___/
(_) (_) (_)(_)(_) (_)(_)(_) (_)`\___/'(_,__/'`\____)

$ ls -shal /dev/kvm 
0 crw-rw---- 1 root root 10, 232 Feb 16 18:54 /dev/kvm

Achieved with this patchset as is.

@fabiand fabiand mentioned this pull request Feb 16, 2018
Closed
@r2d4
Copy link
Contributor

r2d4 commented Feb 16, 2018

@minikube-bot ok to test

@fabiand
Copy link
Contributor Author

fabiand commented Feb 17, 2018

Not sure why the tests fail.

zakame
zakame approved these changes Feb 18, 2018
View reviewed changes
Copy link
Contributor

@zakame zakame left a comment

Choose a reason for hiding this comment

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

LGTM

Is it ok that the DefaultCPUModel is custom though?

@fabiand
Copy link
Contributor Author

fabiand commented Feb 19, 2018

@zakame :)

I chose custom as it is compatible with the current settings.
We can choose to go with host-model which should also be safe (but different to the current settings), and lead to much better nesting performance.

Even if I favor to have host-model or host-passthrough by default, I tend to say that we can default to custom for a release or two, and then eventually do th switch if there was no negative feedback from host-* users.

@fabiand
Copy link
Contributor Author

fabiand commented Feb 20, 2018

/cc @r2d4 any thoughts on this PR?

dlorenc
dlorenc reviewed Feb 20, 2018
View reviewed changes
pkg/minikube/cluster/cluster_linux.go Outdated
@@ -71,6 +72,7 @@ func createKVM2Host(config MachineConfig) *kvmDriver {
},
Memory: config.Memory,
CPU: config.CPUs,
CPUModel: config.KvmCPUModel,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to actually pass this through as a config option? Could we hardcode it in the driver XML?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could hard code it if we are willing to make this change.

@michalskrivanek @berrange do you actually see a risk of defaulting to host-model? (Could you also confirm that host-model will enable nesting [assuming the host is configured accordingly])

Copy link

Choose a reason for hiding this comment

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

Since you dont care about migration you should use host-passthrough in preference to host-model.

@fabiand
Revert "kvm: Passthrough host CPU in order to allow nesting"
5b004de 
This reverts commit 7a4babe.

For now we just hard-code passthrough.

Signed-off-by: Fabian Deutsch <fabiand@fedoraproject.org>
@fabiand
kvm2: Switch to CPU passthrough mode by default
4bc479a 
With passthrough mode we can enable KVM nesting for guests.

Fixes kubernetes#2553

Signed-off-by: Fabian Deutsch <fabiand@fedoraproject.org>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 21, 2018
@fabiand
Copy link
Contributor Author

fabiand commented Feb 21, 2018

@dlorenc @zakame @berrange I updated the change set to set host-passthrough unconditionally/by default.

@fabiand
Copy link
Contributor Author

fabiand commented Feb 21, 2018

Any opinion on this PR?

I'd be fine in going with teh hard coded solution, there should be no drawback.

@dlorenc dlorenc merged commit ee58133 into kubernetes:master Feb 22, 2018
@fabiand
Copy link
Contributor Author

fabiand commented Feb 22, 2018

Awesome!

@fabiand fabiand deleted the hostCpu branch February 22, 2018 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berrange berrange berrange left review comments

@dlorenc dlorenc dlorenc approved these changes

@zakame zakame zakame approved these changes

Assignees

@luxas luxas

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@fabiand @minikube-bot @k8s-ci-robot @r2d4 @zakame @berrange @dlorenc @luxas