On 7/19/22 12:01 PM, Laine Stump wrote:
On 7/19/22 11:09 AM, Eugenio Perez Martin wrote:
On Tue, Jul 19, 2022 at 4:02 PM Laine Stump <laine@xxxxxxxxx> wrote:
On 7/18/22 11:15 AM, Jiri Denemark wrote:
On Mon, Jul 18, 2022 at 10:40:56 +0200, Eugenio Perez Martin wrote:
On Mon, Jul 18, 2022 at 10:25 AM Jiri Denemark
<jdenemar@xxxxxxxxxx> wrote:
Which in ideal case would mean only a QMP command (such as
hotplugging a non-migratable device) is the only way to add migration
blocker. If this is true, than we're safe as libvirt does not
allow such
commands between qemuMigrationSrcIsAllowed and migration start.
Ok, that rules out a few bad use cases. I can do a fast lookup to
check if blockers can be added without the knowledge of libvirt.
That said, is there a reason for not implementing the correct
solution
right away as a separate patch?
I was not sure if libvirt already had another way to check, for
example, if the vhost device didn't have VHOST_F_LOG_ALL feature.
I'm not aware of such check, but even if it exists, checking for
migration blockers looks like the right way of doing things anyway.
Actually that's been on my todo list for a long time - for any qemu that
supports the QMP command that checks for migratability, we should be
calling this command rather than checking against our own internal list
(which is really just an "informed guess") of what can't be migrated.
This way we'll always get the right answer (or at least what QEMU
believes to be the right answer :-)). Fixing it this way will also mean
that migration of VFIO devices will just "magically" start working once
a migration-supporting driver is written for the device, and the correct
vfio driver is bound to the device (this latter item is also on my
list).
So if you're up for making the patch to call the QMP command, I'd be
happy to review it!
Thanks! Actually I'd need some guidance first, I'm not very used to
libvirt code.
As I understand I should create a function in qemu_agent.h/c, a getter
similar to qemuAgentGetFSInfo. How can I get a qemuAgent from
qemuMigrationSrcIsAllowed? I only have a virQEMUDriver there.
qemu_agent.c is only for functions that require calling to the QEMU
guest agent, which is a process running inside the guest. You just need
to run a simple QMP command. There are some good examples of this in
qemu_monitor_json.c
For now it should be enough to delete vdpa hardcoded negation, and
then other parts of libvirt can delete other hardcoded checks, isn't
it?
There's just a single function that checks for migratability
(qemuMigrationSrcIsAllowed()). In theory *everything* in that function
should be deprecated by just calling qemu to ask. In practice there may
be / probably are things that qemu doesn't count as "can't migrate" that
really should be counted that way. Certainly the VDPA and hostdev checks
should be removable immediately though (although of course this should
still be checked before pushing!)
What I would do is this:
1) a patch that adds code to the qemu_capabilities to set a flag if the
desired field in the "query-migrate" QMP command would be filled in by
this qemu binary.
Just to permanently document live discussions from IRC:
jjongsma pointed us to a patch he wrote a year ago (but never pushed
upstream) that implements (1):
https://gitlab.com/jjongsma/libvirt/-/commit/4003b7047058a17465083178d6c0a0f62c900c74
2) a patch that adds a function to qemu_monitor_json.c to call that
command and return migratable/not.
Thinking about this more, I guess a function that returns the full text
of "blocked-reason" would be more useful (that way it could be easily
logged).
3) a patch that adds a call to that function to
qemuMigrationSrcIsAllowed().
4) additional patches that remove specific hardcoded checks *only if the
new field in query-migrate is available (as indicated by the new
capabilities flag) and returned a definite yes/no (otherwise the checks
still need to be done, to account for older qemu binaries that don't
have the qmp command).
I had thought there was a bugzilla somewhere that was tracking this for
libvirt, but I can't find it.
