On 11/17/20 12:45 PM, Andrea Bolognani wrote:
On Tue, 2020-11-17 at 10:47 -0500, Laine Stump wrote:
On 11/16/20 11:43 AM, Andrea Bolognani wrote:
I don't see how we could possibly not be okay with writing back the
updated configuration: filling in whatever blanks are present in the
user-provided configuration and then writing the result back to disk
is quite central to how we handle domains and other objects, and I
see no reason networks should behave any differently.
If anything, I think we should not extend the check to UUIDs, but
just drop it completely and unconditionally write the (possibly)
updated file file to disk. I'll try cooking a patch for that.
I understand why you would consider doing this (low/no maintenance,
easier to verify correct results), but I don't think it's a generally
good idea to rewrite every config file every time we restart libvirtd.
Some media has slow write speed and a limited number of writes in its
lifetime (sure, it's not 1982 and we're not talking about a 2716 EPROM,
but still :-)) and some people may rely on /etc/libvirt to be in a
read-only filesystem for some reason (I haven't tried that, but am just
assuming that it would work).
I would absolutely expect that to *not* be the case, if nothing else
because I don't think anyone is actually QA-ing that scenario.
Even assuming it does indeed work, you'd end up with a pretty limited
version of libvirt, where features like 'virsh define' don't work.
Honestly I would be very surprised if anyone actually used libvirt
this way.
I never presume to know the depths of... er... "uniqueness" that users
will plumb. (half /s) But yeah, sure - *I* would never do that, and
would never recommend anyone to do that.
Hmm, although - maybe a copy-on-write overlay filesystem would be
useful, and I recall (very possibly *incorrectly*) about some sort of
copy-on-write overlay being used for a "live CD" (maybe even some
incarnation of the Fedora Live CD?) that would keep track of all updates
to the filesystem in a separate file/partition/whatever that would
eventually fill up. (I may be mixing that up with some even-more-ancient
memory of a filesystem for a write-once CD-R though - with my brain you
just never know what you're gonna get!)
Except for three cases (that I can think of; I'm sure there are others),
what is read from libvirt's config files in /etc is exactly what would
be written if we were to re-write it immediately after a read-parse-format:
1) the first read of an xml file that was generated outside libvirt, and
has been manually put into /etc/libvirt, e.g. xml files that are a part
of a package installation/upgrade. NB: all other cases of externally
generated files being placed in /etc/libvirt are expressly forbidden
(well, at least "discouraged and declared unsupported") by libvirt
documentation.
2) a user manually edits a file in /etc/libvirt - see (1)
Okay, but from libvirt's point of view there's really no way to tell
apart a file that's been dropped into /etc/libvirt by the package
manager as opposed to the admin, is there? So they will ultimately
have to be treated the same way.
Yep. That's why the "see (1)". I'd already said "3", and couldn't go
back on that (just pretend my up-arrow doesn't work, okay?), but
realized while typing that (1) and (2) were essentially the same thing.
3) upgrading libvirt causes something to be parsed differently than it
was before. This should never happen, as there should always be enough
info in the xml we write to the disk after its initial arrival via the
libvirt API that any future read/write cycles are completely idempotent.
Except for randomly-generated data such as, I don't know, MAC
addresses or UUIDs :)
Note that I said *should*.
So, I played with this a bit to understand the state of the art.
>
$ for f in storage/default qemu/networks/default \
qemu/cirros nwfilter/allow-arp; do
f="/etc/libvirt/$f.xml"
echo "### $f"
sudo cat "$f"
done
### /etc/libvirt/storage/default.xml
<pool type='dir'>
<name>default</name>
<target>
<path>/var/lib/libvirt/images</path>
</target>
</pool>
### /etc/libvirt/qemu/networks/default.xml
<network>
<name>default</name>
<forward mode='nat'/>
<bridge name='virbr0'/>
<mac address='52:54:00:42:5a:e7'/>
<ip address='192.168.122.1' netmask='255.255.255.0'>
<dhcp>
<range start='192.168.122.2' end='192.168.122.254'/>
</dhcp>
</ip>
</network>
### /etc/libvirt/qemu/cirros.xml
<domain type='kvm'>
<name>cirros</name>
<memory unit='KiB'>131072</memory>
<vcpu placement='static'>1</vcpu>
<os>
<type arch='x86_64' machine='q35'>hvm</type>
</os>
<features>
<acpi/>
<apic/>
</features>
<cpu mode='host-passthrough'/>
<devices>
<emulator>/usr/bin/qemu-system-x86_64</emulator>
<disk type='file' device='disk'>
<driver name='qemu' type='qcow2'/>
<source file='/var/lib/libvirt/images/cirros.qcow2'/>
<target dev='vda' bus='virtio'/>
</disk>
<controller type='usb' model='none'/>
<controller type='pci' model='pcie-root'/>
<serial type='pty'>
<target type='isa-serial'/>
</serial>
<console type='pty'>
<target type='serial'/>
</console>
<memballoon model='none'/>
<rng model='virtio'>
<backend model='random'>/dev/urandom</backend>
</rng>
</devices>
</domain>
### /etc/libvirt/nwfilter/allow-arp.xml
<filter name='allow-arp' chain='arp'>
<rule action='accept' direction='inout'/>
</filter>
$ for f in storage/default qemu/networks/default \
qemu/cirros nwfilter/allow-arp; do
f="/etc/libvirt/$f.xml"
sudo md5sum "$f"
done
be781432b877fa98092bc45610060fff /etc/libvirt/storage/default.xml
1233c7b5bd043a8d209e18abdccbb251 /etc/libvirt/qemu/networks/default.xml
2976a665897bf8ce64a7dbf36af3adbf /etc/libvirt/qemu/cirros.xml
5fdb011f22de738feae3189f5f6b2e91 /etc/libvirt/nwfilter/allow-arp.xml
$ sudo systemctl start libvirtd
$ sudo systemctl stop libvirtd 2>/dev/null
$ for f in storage/default qemu/networks/default \
qemu/cirros nwfilter/allow-arp; do
f="/etc/libvirt/$f.xml"
sudo md5sum "$f"
done
be781432b877fa98092bc45610060fff /etc/libvirt/storage/default.xml
1233c7b5bd043a8d209e18abdccbb251 /etc/libvirt/qemu/networks/default.xml
2976a665897bf8ce64a7dbf36af3adbf /etc/libvirt/qemu/cirros.xml
7aff93debdd3e646492d8763d03d6b62 /etc/libvirt/nwfilter/allow-arp.xml
Okay, so everything is the same except for the nwfilter. Let's see
what changed there:
$ sudo cat /etc/libvirt/nwfilter/allow-arp.xml
<!--
WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
OVERWRITTEN AND LOST. Changes to this xml configuration should be made using:
virsh nwfilter-edit allow-arp
or other application using the libvirt API.
-->
<filter name='allow-arp' chain='arp' priority='-500'>
<uuid>e4487541-6b88-415c-9aaf-a7f1d3d4263a</uuid>
<rule action='accept' direction='inout' priority='500'/>
</filter>
This behavior is implemented in virNWFilterObjListLoadConfig():
/* We generated a UUID, make it permanent by saving the config to disk */
if (!def->uuid_specified &&
virNWFilterSaveConfig(configDir, def) < 0)
goto error;
Looks reasonable enough to me.
Of course, since other objects don't have similar snippets in the
relevant places, they will get a different UUID every single time the
daemon is restarted:
$ sudo systemctl stop libvirtd 2>/dev/null
$ for i in $(seq 1 3); do
sleep 1; sudo systemctl start libvirtd
sleep 1; sudo virsh pool-dumpxml --inactive default | grep uuid
sudo systemctl stop libvirtd 2>/dev/null
done
<uuid>2bfe8afb-0366-443a-96cd-b706df4e5221</uuid>
<uuid>6804e364-e3a9-494d-8054-93ebe85970a6</uuid>
<uuid>6be460c9-8cfb-403c-a791-fdc1d505a0ee</uuid>
$ sudo systemctl stop libvirtd 2>/dev/null
$ for i in $(seq 1 3); do
sleep 1; sudo systemctl start libvirtd
sleep 1; sudo virsh net-dumpxml --inactive default | grep uuid
sudo systemctl stop libvirtd 2>/dev/null
done
<uuid>22c61987-43a0-4499-a54d-d1247d36c881</uuid>
<uuid>8360afda-4b5c-445d-97f8-abe81d877e7d</uuid>
<uuid>821ddd59-c2b0-47c6-9d2f-0708e4e2a9bc</uuid>
$ sudo systemctl stop libvirtd 2>/dev/null
$ for i in $(seq 1 3); do
sleep 1; sudo systemctl start libvirtd
sleep 1; sudo virsh dumpxml --inactive cirros | grep uuid
sudo systemctl stop libvirtd 2>/dev/null
done
<uuid>b74581ea-a215-4fe9-8365-c6cfceb2b18e</uuid>
<uuid>d00d902b-ebca-4213-9411-d27f14363fdb</uuid>
<uuid>74d50bb0-b54a-4a7e-82eb-410ccc12fd9b</uuid>
That's not cool, right?
The fact that you took the time to do such a nicely documented
experiment *is* cool. The results are not, though.
The configuration files might not have
reached /etc/libvirt the Proper Way™, but we stop so very short
of dealing with them reasonably...
The most bothersome part of this to me is that the treatment is
inconsistent. If we're going to re-write the config when parsing causes
a change, then we should do that for all types of config, and all
changes. And that goes double for changes to things that are supposed to
remain stable (which, I guess, is "everything").
If writing the file back to disk every single time is a concern due
to performance reasons, which I can actually see being the case for
large enough hosts, can we at least agree that when UUIDs and MAC
addresses have been generated by libvirt then we need to store them
persistently?
Yep. I'm right there with you on this - I was only objecting to the idea
of always re-writing no matter what.
If nothing else, I would argue we definitely need this to be the case
for at least nwfilters, networks and storage pools, which are all
things for which some configuration can be reasonably provided
through the package manager - before the corresponding APIs are even
available.
I think it should be done consistently across all config objects. I
would suggest making some sort of pseudo-standardized function/table for
every config object type that would maintain a list of those items that
need to be checked, but that just seems like over-engineering that would
lead to lots of code that not everybody would know about and/or
remember, and so it wouldn't be maintained anyway...
