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

Adds qemu wrapper to disable pmu in cpu flags #199

Merged
merged 1 commit into from
Nov 15, 2018

Conversation

dcode
Copy link
Contributor

@dcode dcode commented Nov 9, 2018

When guestfish is called under nested VMware virtualization, it uses the cpu flag of host. This alone causes issues under nested VMware virtualization. This PR checks if gf-oemid is running under a VMware hypervisor using the same check used by virt-what. If yes, a qemu wrapper is written that adds pmu=off to the cpu flags for qemu-kvm.

Fixes #198

src/gf-oemid Outdated

# Work around for https://github.com/coreos/coreos-assembler/issues/198
## Borrowed from virt-what
hv=$(dmidecode 2>&1 | grep 'Manufacturer: VMware')
Copy link
Member

Choose a reason for hiding this comment

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

This is only going to work in a --privileged container. At some point I think we shoud consider only supporting the unprivileged mode introduced by: #190

But honestly, I can't think of a downside to using -cpu host unconditionally for libguestfs. It makes sense. As far as pmu=off, I am less certain of all of the implications of that, but offhand, we don't care much about power management for these transient VMs, so I'm fine enabling that unconditionally too.

One messy thing here is...right now we're using LIBGUESTFS_BACKEND=direct because we hit race conditions otherwise I think. If we weren't libguestfs would use libvirt and I suspect that changes the qemu flags.

In the future we're likely to drop the virt-install codepath and so we'll more consistently not use libvirt and speak qemu directly.

It's probably worth filing a libguestfs bug (bugzilla.redhat.com or upstream) and see what the maintainer thinks about adding these flags by default.

In the meantime...let's basically drop the dmidecode and use the wrapper unconditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requested changes made. I'll file the libguestfs bug too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgwalters
Copy link
Member

One thing I'd missed originally is this isn't adding -cpu host, looks like libguestfs already does that. So it's just the pmu=off. Which seems benign to me; if we go with what https://bugzilla.redhat.com/show_bug.cgi?id=1648403#c1 suggested it'd be a huge perf hit.

Anyways, needs a rebase now that the shellcheck PR landed, I think we can merge after that.

@dustymabe dustymabe self-requested a review November 12, 2018 16:28
@dustymabe
Copy link
Member

will review this today

src/gf-oemid Outdated Show resolved Hide resolved
@dustymabe
Copy link
Member

hey @dcode - can you rebase this and address the code review comments?

@dcode dcode changed the title Adds qemu wrapper to modify cpu flags Adds qemu wrapper to disable pmu in cpu flags Nov 15, 2018
@dcode dcode force-pushed the nested-vmware-workaround branch 3 times, most recently from a382d0f to ff35236 Compare November 15, 2018 14:19
When guestfish is called under nested VMware virtualization, it uses the cpu flag of `host`. This alone causes issues under nested VMware virtaulization in the default configuration. This commit removes the default host flags and replaces with `host,pmu=off`.

Fixes coreos#198
@dcode
Copy link
Contributor Author

dcode commented Nov 15, 2018

Requested changes made. CI checks passed. 😃

@dustymabe
Copy link
Member

this works for me. I think some of it might need to be moved around so that other calls to libguestfs will use the wrapper. I'll open a follow up PR for that and ask @dcode to review it.

@dustymabe dustymabe merged commit 31621c3 into coreos:master Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PMU causes guestfish to fail on VMware hypervisors
4 participants