[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