[libvirt PATCH 1/2] network: Drop UUID handling for default network
Laine Stump
laine at redhat.com
Tue Nov 17 18:39:09 UTC 2020
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...
More information about the libvir-list
mailing list