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

libvirt: use host-passthrough CPU type for libvirt VMs #977

Conversation

mykaul
Copy link

@mykaul mykaul commented Jan 1, 2019

libvirt: use host-passthrough CPU type

Should be slightly faster, I hope.

From https://wiki.openstack.org/wiki/LibvirtXMLCPUModel :
host-passthrough" - this causes libvirt to tell KVM to passthrough the host CPU with no modifications.
The difference to host-model, instead of just matching feature flags, every last detail of the host CPU is matched.
This gives absolutely best performance, and can be important to some apps which check low level CPU details, but it comes at a cost wrt migration.
The guest can only be migrated to an exactly matching host CPU.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 1, 2019
@openshift-ci-robot
Copy link
Contributor

Hi @mykaul. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 1, 2019
@cgwalters
Copy link
Member

Looks reasonable to me.

@abhinavdahiya
Copy link
Contributor

can we keep these changes in two separate commits?
Do you have pointers to some docs that can help understand the effect?

@mykaul
Copy link
Author

mykaul commented Jan 2, 2019

@abhinavdahiya - I guess we can have them in two commits, not sure it's worth the effort though.
I have enough experience with libvirt and KVM to know that both are useful and provide better performance - certainly the CPU pass-through - it allows the guest to utilize the full potential of the host CPU capabilities.
From https://wiki.openstack.org/wiki/LibvirtXMLCPUModel :

"host-passthrough" - this causes libvirt to tell KVM to passthrough the host CPU with no modifications. The difference to host-model, instead of just matching feature flags, every last detail of the host CPU is matched. This gives absolutely best performance, and can be important to some apps which check low level CPU details, but it comes at a cost wrt migration. The guest can only be migrated to an exactly matching host CPU.

@cgwalters
Copy link
Member

One useful thing for example is this guarantees that if the host has RDRAND so does the guest.

@mykaul mykaul force-pushed the mykaul-patch-libvirt-host-passthrough branch from 2e6346d to 51b497d Compare January 2, 2019 18:40
@mykaul
Copy link
Author

mykaul commented Jan 2, 2019

I've left out the virtio console part (it's less important), and just have the host-passthrough CPU type.

@mykaul mykaul changed the title Use host-passthrough CPU type, virtio console Use host-passthrough CPU type for libvirt VMs Jan 2, 2019
@abhinavdahiya
Copy link
Contributor

51b497d has the commit title Use host-passthrough CPU type, virtio console i think we can drop text regarding the virtio.

Also you can include

From https://wiki.openstack.org/wiki/LibvirtXMLCPUModel :

"host-passthrough" - this causes libvirt to tell KVM to passthrough the host CPU with no modifications. The difference to host-model, instead of just matching feature flags, every last detail of the host CPU is matched. This gives absolutely best performance, and can be important to some apps which check low level CPU details, but it comes at a cost wrt migration. The guest can only be migrated to an exactly matching host CPU.

in commit message body (https://github.com/openshift/installer/blob/master/CONTRIBUTING.md#commit-message-format)

Thanks! 😇

@mykaul mykaul force-pushed the mykaul-patch-libvirt-host-passthrough branch from 51b497d to 4de101c Compare January 2, 2019 19:22
@mykaul mykaul changed the title Use host-passthrough CPU type for libvirt VMs libvirt: use host-passthrough CPU type for libvirt VMs Jan 2, 2019
@mykaul
Copy link
Author

mykaul commented Jan 2, 2019

Updated.

@abhinavdahiya
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 2, 2019
@wking
Copy link
Member

wking commented Jan 2, 2019

Why is this just for the bootstrap node, and not for the master domains too? Also, if this is something folks want to set on libvirt, does it make sense to add it to the cluster-API provider too, either as a configurable or hard-coded setting?

@mykaul
Copy link
Author

mykaul commented Jan 2, 2019

@wking - it should be for the master node as well! Will add.
I'm not sure what the cluster-api-provider-libvirt project is... I can look at it, but it's unrelated to this PR, I assume?

For both bootstrap and master VMs.
Should be slightly faster, I hope.
From https://wiki.openstack.org/wiki/LibvirtXMLCPUModel :

"host-passthrough" - this causes libvirt to tell KVM to passthrough the host CPU with no modifications.
The difference to host-model, instead of just matching feature flags, every last detail of the host CPU is matched.
This gives absolutely best performance, and can be important to some apps which check low level CPU details,
but it comes at a cost wrt migration.
The guest can only be migrated to an exactly matching host CPU.
@mykaul mykaul force-pushed the mykaul-patch-libvirt-host-passthrough branch from 4de101c to 529114a Compare January 2, 2019 20:36
@wking
Copy link
Member

wking commented Jan 2, 2019

I'm not sure what the cluster-api-provider-libvirt project is... I can look at it, but it's unrelated to this PR, I assume?

The libvirt provider is in charge of creating nodes to fulfil machine and machine-set configuration. At the moment, that means it creates the worker nodes to fulfil the worker machine-set. Eventually it may also be in charge of creating master nodes (if we work out a way to handle etcd bootstrapping and new-master joining). #792 is also related to this, using the cluster-API machine configurations to figure out the Terraform configuration.

@crawford
Copy link
Contributor

crawford commented Jan 3, 2019

/lgtm

This looks fine for now. We can sort out the actuator stuff later.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, mykaul

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 3, 2019
@openshift-merge-robot openshift-merge-robot merged commit 713289e into openshift:master Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants