On 11/16/20 5:33 AM, Andrea Bolognani wrote:
On Sun, 2020-11-15 at 19:19 -0500, Laine Stump wrote:
On 11/15/20 3:43 PM, Andrea Bolognani wrote:
We are generating a fresh UUID and storing it in the XML for the
default network, but this is unnecessary because the network
driver will automatically generate one if it's missing from the
XML;
But that automatically generated uuid will not be stored in the original
xml in /etc/libvirt, so a new and different uuid will be generated every
time libvirt is restarted.
That doesn't seem to be the case:
$ sudo cat /etc/libvirt/qemu/networks/default.xml
<network>
<name>default</name>
<forward mode='nat'/>
<bridge name='virbr0'/>
<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>
$ sudo systemctl start libvirtd
$ sudo systemctl stop libvirtd
$ sudo cat /etc/libvirt/qemu/networks/default.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 net-edit default
or other application using the libvirt API.
-->
<network>
<name>default</name>
<uuid>212a684a-4ca4-4c42-b389-aecd676868f8</uuid>
<forward mode='nat'/>
<bridge name='virbr0' stp='on' delay='0'/>
<mac address='52:54:00:ff:27:85'/>
<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>
$ sudo systemctl start libvirtd
$ sudo systemctl stop libvirtd
$ sudo cat /etc/libvirt/qemu/networks/default.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 net-edit default
or other application using the libvirt API.
-->
<network>
<name>default</name>
<uuid>212a684a-4ca4-4c42-b389-aecd676868f8</uuid>
<forward mode='nat'/>
<bridge name='virbr0' stp='on' delay='0'/>
<mac address='52:54:00:ff:27:85'/>
<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>
But... I did a nearly identical experiment last night before posting my
reply, and came up with different results, which is why I posted...
Ah! So the difference is that in your example, you have no MAC address
either, and the code you point out down below is noticing that change,
and causing the entire network config to be re-written to the disk. I
hadn't removed the MAC address, so in my case it didn't re-write the config.
(...and now I've read to the bottom of your reply and see that you've
already figured that out :-P)
I guess if we're okay with re-writing the config at daemon start to add
a MAC address, then we should be okay with doing that to add a uuid
(they're really just differently-used examples of the same thing). But
if we're going to do that, the check to see if the config should be
modified to check both uuid and mac address (even if in current practice
both are absent from the proto-default.xml, that may not be the case in
the future).
Alternately, if we're *not* okay with rewriting the config at daemon
start time (even though, as you've pointed out, we've been doing that
for 6 years already), then we would need to *stop* doing that for MAC
address. (Which would mean we would need to figure out a different way
to get a fixed MAC address into the config file, and seeing that the
commit you reference actually *removes* my original code that added it
in at install time, we can't really go back there (also because that's
moving the code in the wrong direction - Fedora SilverBlue people said
in the BZ I referenced that they don't want %post_install scripts
modifying the files that are installed).
I think I'm okay with the former (adding a check for changed uuid onto
your patches); it gives us a stable uuid (as long as the modified disk
contents are saved for the next reboot) while eliminating another bit of
the undesirable code that runs during %post_install.
Is there anything else wrt. pre-built images that we're not taking into
consideration though?
I don't know if the solution to this is to modify libvirt so that it
rewrites the XML for a network if it has to auto-generate any
attributes[*], or to put a canned/static uuid value in the installed
xml, or just to declare that we don't care if the uuid changes from one
run of libvirtd to the next. But definitely these patches change
behavior, so we probably need to point that out and maybe discuss it.
[*] I have a vague memory that there is (or was?) at least one case
where we would automatically update the persistent definition of a
domain or network on disk when libvirt started (it was needed due to a
change in some attribute or something), but I don't recall the details,
don't feel like looking for it right now, and it anyway is not the norm.
AFAICT the behavior you're talking about, and which can be seen in
action in the transcript above, is caused by the following snippet in
virNetworkLoadConfig():
switch ((virNetworkForwardType) def->forward.type) {
case VIR_NETWORK_FORWARD_NONE:
case VIR_NETWORK_FORWARD_NAT:
case VIR_NETWORK_FORWARD_ROUTE:
case VIR_NETWORK_FORWARD_OPEN:
if (!def->mac_specified) {
virNetworkSetBridgeMacAddr(def);
virNetworkSaveConfig(configDir, def, xmlopt);
}
break;
which can be tracked down to
https://gitlab.com/libvirt/libvirt/-/commit/a47ae7c004e92f959b45808ca82326e8559c2f61
So, it's far from a new behavior.
Note that we only persist the new configuration if the bridge's MAC
address has been generated, not if the network UUID has, which is
something that we might want to change. But, for the use case at
hand, we know that the XML template for the default network will have
neither of those, so in practice they will both be generated during
the first startup and the result will be persisted to disk.
