Hi Martin,
On 8/7/24 10:10, Michael Galaxy wrote:
Hi,
Answers below....
On 8/7/24 08:23, Martin Kletzander wrote:
On Tue, Aug 06, 2024 at 03:45:54PM -0500, Michael Galaxy wrote:
Thank you so much for the detailed comments. I'll work on those.
I believe you had a couple questions below on this one......inlined
.....
On 8/6/24 07:35, Martin Kletzander wrote:
On Wed, Jun 05, 2024 at 04:37:35PM -0400, mgalaxy@xxxxxxxxxx wrote:
From: Michael Galaxy <mgalaxy@xxxxxxxxxx>
+ _("Domain requesting configuration
for %1$lu NUMA nodes, but memory backing directory only has (%2$lu)
directory paths available. Either reduce this to one directory or
provide more paths to use."),
+ numa_node_count,
+ cfg->nb_memoryBackingDirs);
+ return -1;
+ }
+
+ path_index = virBitmapNextSetBit(numaBitmap, -1);
What if the single pinning is to the third host node, but there are
only
two memoryBackingDirs supplied in the config?
That case is totally supported (and expected). For example a *non*
vNUMA
guests (a single virtual NUMA node)
running on a standard 2-socket (2-NUMA-node) host in a typical pizza
box.
If I understood that correctly, that is supported without a problem.
The
error above is describing the case that
a virtual machine has *more* virtual NUMA nodes configured than the
host
has available to offer. In such a case,
either the user needs to provide more host nodes or turn this feature
off or use a single NUMA node (in which
case all the guest nodes would be placed in a single host node).
If the machine is only pinned to the third node and two
memoryBackingDirs are supplied then path_index will be 2 and later
accessing memoryBackingDirs[path_index] will crash. Or maybe I have
missed something. That's why I asked since I'm unsure myself, I have
not tested it.
Oh ok, now I understand the question. I don't believe I've tested that
either.
I'll see if libvirt lets the user do that or not and verify. Good catch.
{
+ virQEMUDriver *driver = priv->driver;
g_autofree char *path = NULL;
+ g_autoptr(virQEMUDriverConfig) cfg =
virQEMUDriverGetConfig(driver);
+ size_t i;
- if (qemuGetMemoryBackingPath(driver, vm->def, mem->info.alias,
&path) < 0)
- return -1;
+ for (i = 0; i < cfg->nb_memoryBackingDirs; i++) {
+ virDomainXMLPrivateDataCallbacks *privateData =
(virDomainXMLPrivateDataCallbacks *) priv;
+ if (qemuGetMemoryBackingPath(def, privateData, i,
mem->info.alias, &path) < 0)
+ return -1;
I wonder why are the paths not saved in the status XML of the running
domain. Since they can be changed during the VM's lifetime (while
restarting libvirt) it could cause issues during clean-up. Sure,
unlink() will set errno to ENOENT, but the proper paths will not be
cleaned up. It'd also make this part of the code easier and safer to
use from the callers. But that's pre-existing.
I'm not sure I'm following the question on this one. I do not think we
want the paths to be changing
during the VM's lifetime. Are you asking if we *want* to support that?
The current
libvirt XML schema does not support this. The actual path is kept
hidden
from the guest VM
and is only known internally to libvirt.
Can you clarify?
Exactly, we do not want the paths to change. But if you start the VM,
then stop virtqemud/libvirtd (totally supported), change the config file
to point to a different directory (or even different number of
directories, I haven't even thought of that), then start the daemon back
again, we'll have a problem when the VM is being cleaned up.
For these kinds of situations we are keeping more pieces of information
about running domains in a so called status XML. This is saved on disk,
but unavailable to the user so saving something there does not
explicitly expose it to the user. Having the paths saved there would
make it nicer to clean up and it should've probably been done even with
that one path that is supported nowadays. I can have a look at fixing
that at first and then your patches could be applied on top of that fix
if that's fine with you or you can have a look at tackling that
yourself, it should not be difficult, tests for that should be nice to
write as well, it just needs to be done in a back-compat way, of course.
See qemuDomainObjPrivateXMLFormat() and qemuDomainObjPrivateXMLParse()
for some starting points.
I've finished addressing most of the rest of the review comments here
and I'll
sent that out, but I'd like to avoid too many more revisions here.
IMHO, the problem above is really orthogonal to this feature and, as you
said,
applies to the original (single path) case, so I'd like to leave that as
a homework
assignment to the community.
Realistically, in a well-managed cloud, this is highly unlikely to
happen, so I'm
not too worried about it.
Would a ticket be helpful somewhere? I don't know if libvirt has a way
of tracking
these kinds of discoveries, I'd be happy to open a ticket somwhere and put
it in the libvirt backlog.
- Michael
