[PATCH 0/7] network: implement automatic subnet selection for virtual networks

The problem this is solving has a very long history. with a simple bugzilla search I found reports all the way back to 2012. The issue is that sometimes when a libvirt virtual network is started, it could be on exactly the same subnet as another interface already active on the host at libvirt install time (which was fixed long ago), or as another interface that isn't active in the install environment, but will be active when libvirt is actually started at a later time, possibly in a completely different environment than the install (this *wasn't* fixed. until now). At one point (which I've located in this bugzilla comment in 2014 - https://bugzilla.redhat.com/1146232#c17) I thought of having a config knob in virtual networks that would look for an unused network at runtime, and start the network with that subnet. Of course that would have solved the problem where the conflicting network was already active when libvirt started its networks, but wouldn't do anything about the case where the conflicting network was started *after* libvirt had started its networks. And anyway it would have required config settings in /etc/libvirt/network.conf which didn't exist at the time. Several years later (around 2020) I learned about NetworkManager dispatcher scripts, which are called any time NM brings an interface up or down, and wrote a python script that would use this to destroy any libvirt network that had a conflict with a newly activated NM interface. After sending it as an RFC patch, I promptly forgot about it (except for being on my "list of things I should get to someday"). A few weeks ago I realized that the "there is no network.conf to store config items" problem was recently solved (I had to add a network.conf file for the knob that selects nftables vs iptables), which prompted me to go back and find the branch with the NM dispatcher script, and *finally* implement the runtime auto address selection. Combining this with enhancing the NM dispatcher script to not only destroy conflicting networks, but to also then *restart* them if they have autoaddr set, gives what I think is finally a full solution to the problem - no matter what order interfaces are started vs. libvirt networks being started, a network with "autoaddr='yes'" set will *always* end up with all networks being usable. (Well, the one issue that still remains is that if there are already guests attached to the autoaddr='yes' network when its address is changed, those guests will then be disconnected, and once reconnected they will have stale IP addresses. That's another hill for another day though; I don't think anyone has complained about that. Yet.) I think this may break the record for "oldest bug to be fixed", which was previously set when danpb fixed the firewalling issue with guests on two different NATed networks being able to communicate with each other. :-) Laine Stump (7): conf: add XML config for autoaddr networks network: add configurable network autoaddr items to driver config network: reorganize the check for route collisions network: turn on autoaddr selection in bridge driver network: NetworkManager script to monitor/resolve conflicts with new interfaces network: turn on autoaddr in default network spec: stop trying to find unused network during deamon-network-config %post docs/formatnetwork.rst | 42 +++- libvirt.spec.in | 38 +--- meson.build | 14 +- meson_options.txt | 4 + src/conf/network_conf.c | 75 +++++-- src/conf/network_conf.h | 7 + src/conf/schemas/network.rng | 5 + src/network/bridge_driver.c | 201 +++++++++++++++++- src/network/bridge_driver_conf.c | 61 ++++++ src/network/bridge_driver_conf.h | 4 + src/network/bridge_driver_linux.c | 132 +++++++----- src/network/bridge_driver_nop.c | 22 +- src/network/bridge_driver_platform.h | 5 +- src/network/default.xml.in | 2 +- src/network/libvirtd_network.aug | 8 +- src/network/meson.build | 12 ++ src/network/network.conf.in | 11 + src/network/nm-dispatcher-check-nets.py | 196 +++++++++++++++++ src/network/test_libvirtd_network.aug.in | 3 + .../networkxml2xmlin/nat-network-autoaddr.xml | 11 + .../nat-network-autoaddr.xml | 11 + tests/networkxml2xmltest.c | 1 + 22 files changed, 744 insertions(+), 121 deletions(-) create mode 100755 src/network/nm-dispatcher-check-nets.py create mode 100644 tests/networkxml2xmlin/nat-network-autoaddr.xml create mode 100644 tests/networkxml2xmlout/nat-network-autoaddr.xml -- 2.45.2

If the <ip> element of a network has the attribute autoaddr='yes', any specified IP address, netmask, prefix, or dhcp range will be used as a hint, but if the current network is already present in the host's routing table (i.e. it is in use) then libvirt will instead automatically find an unused subnet "somewhere", and use that instead. Since this patch is just the XML config bits, it has none of the details about how an unused subnet is found. That is coming later. autoaddr='yes' is currently only supported for IPv4. IPv6 autoaddr should work differently (rather than selecting from a manually configured range of networks, I *think* it should semi-randomly select a network ala RFC 4193, and anyway conflicting IPv6 networks hasn't been an issue up to now). Signed-off-by: Laine Stump <laine@redhat.com> --- docs/formatnetwork.rst | 42 ++++++++++- src/conf/network_conf.c | 75 ++++++++++++++----- src/conf/network_conf.h | 7 ++ src/conf/schemas/network.rng | 5 ++ .../networkxml2xmlin/nat-network-autoaddr.xml | 11 +++ .../nat-network-autoaddr.xml | 11 +++ tests/networkxml2xmltest.c | 1 + 7 files changed, 132 insertions(+), 20 deletions(-) create mode 100644 tests/networkxml2xmlin/nat-network-autoaddr.xml create mode 100644 tests/networkxml2xmlout/nat-network-autoaddr.xml diff --git a/docs/formatnetwork.rst b/docs/formatnetwork.rst index 9b4ecbf31d..9c5e974002 100644 --- a/docs/formatnetwork.rst +++ b/docs/formatnetwork.rst @@ -805,8 +805,27 @@ of 'route' or 'nat'. divisible by 4 for IPv6) libvirt may be unable to compute the PTR domain automatically. The ``ip`` element is supported :since:`since 0.3.0`. IPv6, multiple addresses on a single network, ``family``, and ``prefix`` are - supported :since:`since 0.8.7`. The ``ip`` element may contain the following - elements: + supported :since:`since 0.8.7`. + + Instead of (or in addition to) an IPv4 address and prefix (or + netmask), the attribute ``autoaddr`` can be set to ``yes``, and in + this case libvirt will search through a range of IPv4 subnets + (configured with the autoaddr_* settings in + /etc/libvirt/network.conf) to find an unused subnet :since: `since + 10.7.0`. If an address has been specified in addition to + ``autoaddr='yes'`` then that address will be tried first, before + looking to the range given in network.conf. In any case, once an + unused subnet has been found, any dhcp range, static IP addresses, + or tftp boot servers will have the network part of their address + changed to the new subnet (if no ``address`` was given in the + config, the address will be set to the ".1" address on the newly + chosen network, and if there is an empty ``<dhcp/>`` subelement, it + will be given a range equal to the entire subnet exceot the ".1" + address). Once this is done, the updated network config is saved by + libvirt so that libvirt can attempt to use the same subnet the next + time the network is started. + + The ``ip`` element may contain the following sub-elements: ``tftp`` The optional ``tftp`` element and its mandatory ``root`` attribute enable @@ -859,6 +878,11 @@ of 'route' or 'nat'. except when setting an infinite lease time (``expiry='0'``). :since:`Since 6.3.0` + If the ``ip`` element has ``autoaddr='yes'``, then the ``dhcp`` + element can be empty, and in this case libvirt automatically + adds a ``range`` equivalent to the entire subnet (minus the .1 + address for the bridge device itself). :since: `Since 10.7.0` + Network namespaces ~~~~~~~~~~~~~~~~~~ @@ -928,6 +952,20 @@ definition. </ip> </network> +And here is a NAT network that has its subnet (and thus its ip address +and dhcp range) chosen automatically by libvirt from the range of +"autoaddr" subnets configured in network.conf: + +:: + + <network> + <name>automagic</name> + <forward mode="nat"/> + <ip autoaddr="yes"> + <dhcp/> + </ip> + </network> + IPv6 NAT based network ~~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3af4e1d036..2a6b95e7ac 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -599,7 +599,8 @@ virNetworkDHCPHostDefParseXML(const char *networkName, static int virNetworkDHCPDefParseXML(const char *networkName, xmlNodePtr node, - virNetworkIPDef *def) + virNetworkIPDef *def, + bool isIPv4) { g_autoptr(GPtrArray) rangeNodes = virXMLNodeGetSubelementList(node, "range"); @@ -607,6 +608,7 @@ virNetworkDHCPDefParseXML(const char *networkName, xmlNodePtr bootp = virXMLNodeGetSubelement(node, "bootp"); size_t i; + def->dhcp = true; for (i = 0; i < rangeNodes->len; i++) { virNetworkDHCPRangeDef range = { 0 }; @@ -629,8 +631,7 @@ virNetworkDHCPDefParseXML(const char *networkName, VIR_APPEND_ELEMENT(def->hosts, def->nhosts, host); } - if (bootp && - VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET)) { + if (bootp && isIPv4) { g_autofree char *server = virXMLPropString(bootp, "server"); if (!(def->bootfile = virXMLPropStringRequired(bootp, "file"))) @@ -991,24 +992,28 @@ virNetworkIPDefParseXML(const char *networkName, g_autofree char *address = NULL; g_autofree char *netmask = NULL; int ret = -1; + bool isIPv4 = true; ctxt->node = node; /* grab raw data from XML */ def->family = virXPathString("string(./@family)", ctxt); + if (STREQ_NULLABLE(def->family, "ipv6")) + isIPv4 = false; - address = virXPathString("string(./@address)", ctxt); - if (!address) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required address attribute in network '%1$s'"), - networkName); - goto cleanup; - } - if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid address '%1$s' in network '%2$s'"), - address, networkName); + if (virXMLPropTristateBool(node, "autoaddr", VIR_XML_PROP_NONE, &def->autoaddr) < 0) goto cleanup; + + address = virXPathString("string(./@address)", ctxt); + + if (address) { + if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid address '%1$s' in network '%2$s'"), + address, networkName); + goto cleanup; + } + isIPv4 = VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET); } netmask = virXPathString("string(./@netmask)", ctxt); @@ -1028,6 +1033,27 @@ virNetworkIPDefParseXML(const char *networkName, &def->localPTR) < 0) goto cleanup; + switch (def->autoaddr) { + case VIR_TRISTATE_BOOL_NO: + case VIR_TRISTATE_BOOL_ABSENT: + case VIR_TRISTATE_BOOL_LAST: + if (!address) { + virReportError(VIR_ERR_XML_ERROR, + _("Missing required address attribute in network '%1$s'"), + networkName); + goto cleanup; + } + break; + case VIR_TRISTATE_BOOL_YES: + if (!isIPv4) { + virReportError(VIR_ERR_XML_ERROR, + _("autoaddr='yes' is only supported for IPv4 in network '%1$s'"), + networkName); + goto cleanup; + } + break; + } + /* validate address, etc. for each family */ if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) { if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) || @@ -1057,7 +1083,8 @@ virNetworkIPDefParseXML(const char *networkName, goto cleanup; } } else if (STREQ(def->family, "ipv6")) { - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { + if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6) || + VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Family 'ipv6' specified for non-IPv6 address '%1$s' in network '%2$s'"), address, networkName); @@ -1083,11 +1110,11 @@ virNetworkIPDefParseXML(const char *networkName, } if ((dhcp = virXPathNode("./dhcp[1]", ctxt)) && - virNetworkDHCPDefParseXML(networkName, dhcp, def) < 0) + virNetworkDHCPDefParseXML(networkName, dhcp, def, isIPv4) < 0) goto cleanup; if (virXPathNode("./tftp[1]", ctxt)) { - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET)) { + if (!isIPv4) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported <tftp> element in an IPv6 element in network '%1$s'"), networkName); @@ -2045,6 +2072,11 @@ virNetworkIPDefFormat(virBuffer *buf, if (def->family) virBufferAsprintf(buf, " family='%s'", def->family); + + if (def->autoaddr != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " autoaddr='%s'", + virTristateBoolTypeToString(def->autoaddr)); + if (VIR_SOCKET_ADDR_VALID(&def->address)) { g_autofree char *addr = virSocketAddrFormat(&def->address); if (!addr) @@ -2072,7 +2104,14 @@ virNetworkIPDefFormat(virBuffer *buf, virBufferEscapeString(buf, "<tftp root='%s'/>\n", def->tftproot); } - if ((def->nranges || def->nhosts)) { + + if (!(def->nranges || def->nhosts)) { + /* in case an empty <dhcp/> (with no subelements) + * was specified + */ + if (def->dhcp) + virBufferAddLit(buf, "<dhcp/>\n"); + } else { size_t i; virBufferAddLit(buf, "<dhcp>\n"); virBufferAdjustIndent(buf, 2); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index c2a4198abc..d44984e71a 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -158,6 +158,12 @@ struct _virNetworkDNSDef { typedef struct _virNetworkIPDef virNetworkIPDef; struct _virNetworkIPDef { char *family; /* ipv4 or ipv6 - default is ipv4 */ + + /* automatically determine IP address when the network is started. + * default is 'no' + */ + virTristateBool autoaddr; + virSocketAddr address; /* Bridge IP address */ /* One or the other of the following two will be used for a given @@ -171,6 +177,7 @@ struct _virNetworkIPDef { virTristateBool localPTR; + bool dhcp; /* true if there is a <dhcp> element */ size_t nranges; /* Zero or more dhcp ranges */ virNetworkDHCPRangeDef *ranges; diff --git a/src/conf/schemas/network.rng b/src/conf/schemas/network.rng index b7c8551fad..64d4818e96 100644 --- a/src/conf/schemas/network.rng +++ b/src/conf/schemas/network.rng @@ -354,6 +354,11 @@ <optional> <attribute name="address"><ref name="ipAddr"/></attribute> </optional> + <optional> + <attribute name="autoaddr"> + <ref name="virYesNo"/> + </attribute> + </optional> <optional> <choice> <attribute name="netmask"><ref name="ipv4Addr"/></attribute> diff --git a/tests/networkxml2xmlin/nat-network-autoaddr.xml b/tests/networkxml2xmlin/nat-network-autoaddr.xml new file mode 100644 index 0000000000..71167285e6 --- /dev/null +++ b/tests/networkxml2xmlin/nat-network-autoaddr.xml @@ -0,0 +1,11 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr0"/> + <forward mode="nat"/> + <ip family='ipv4' autoaddr='yes'> + <dhcp/> + </ip> + <ip family="ipv4" autoaddr='yes' address="10.24.10.1"> + </ip> +</network> diff --git a/tests/networkxml2xmlout/nat-network-autoaddr.xml b/tests/networkxml2xmlout/nat-network-autoaddr.xml new file mode 100644 index 0000000000..629710175c --- /dev/null +++ b/tests/networkxml2xmlout/nat-network-autoaddr.xml @@ -0,0 +1,11 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip family='ipv4' autoaddr='yes'> + <dhcp/> + </ip> + <ip family='ipv4' autoaddr='yes' address='10.24.10.1'> + </ip> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 0783d84915..de8abefe42 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -137,6 +137,7 @@ mymain(void) DO_TEST("nat-network-forward-nat-address"); DO_TEST("nat-network-forward-nat-no-address"); DO_TEST("nat-network-mtu"); + DO_TEST("nat-network-autoaddr"); DO_TEST("8021Qbh-net"); DO_TEST("direct-net"); DO_TEST("host-bridge-net"); -- 2.45.2

These options are added to network.conf and virNetworkDriverConfig object: autoaddr_start - start of the range of subnets to search (def: "192.168.122.0") autoaddr_end - end of the range of subnets (def: "192.168.255.0") autoaddr_prefix - prefix of these subnets (def: 24) They will be used by the network driver when looking for unused subnets to assign to networks that have "autoaddr='yes'" in one of their <ip> elements. Signed-off-by: Laine Stump <laine@redhat.com> --- meson.build | 14 +++++- meson_options.txt | 4 ++ src/network/bridge_driver_conf.c | 61 ++++++++++++++++++++++++ src/network/bridge_driver_conf.h | 4 ++ src/network/libvirtd_network.aug | 8 +++- src/network/meson.build | 6 +++ src/network/network.conf.in | 11 +++++ src/network/test_libvirtd_network.aug.in | 3 ++ 8 files changed, 109 insertions(+), 2 deletions(-) diff --git a/meson.build b/meson.build index 06d88ad1f3..71da7db741 100644 --- a/meson.build +++ b/meson.build @@ -1646,6 +1646,15 @@ endif if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD') conf.set('WITH_NETWORK', 1) + autoaddr_start = get_option('autoaddr_start') + conf.set_quoted('AUTOADDR_START', autoaddr_start) + + autoaddr_end = get_option('autoaddr_end') + conf.set_quoted('AUTOADDR_END', autoaddr_end) + + autoaddr_prefix = get_option('autoaddr_prefix') + conf.set('AUTOADDR_PREFIX', autoaddr_prefix) + firewall_backend_priority = get_option('firewall_backend_priority') if firewall_backend_priority.length() == 0 if host_machine.system() == 'linux' @@ -2406,7 +2415,10 @@ misc_summary = { } if conf.has('WITH_NETWORK') misc_summary += { - 'firewall backends': firewall_backend_priority, + 'autoaddr_start': autoaddr_start, + 'autoaddr_end': autoaddr_end, + 'autoaddr_prefix': autoaddr_prefix, + 'firewall backends': firewall_backend_priority, } endif summary(misc_summary, section: 'Miscellaneous', bool_yn: true, list_sep: ' ') diff --git a/meson_options.txt b/meson_options.txt index 2d440c63d8..7a1a86211e 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -118,6 +118,10 @@ option('firewalld', type: 'feature', value: 'auto', description: 'firewalld supp # dep:firewalld option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether to install firewalld libvirt zone') option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], value: [], description: 'order in which to try firewall backends') +option('autoaddr_start', type: 'string', value: '192.168.122.0', description: 'Start of range of IPv4 subnets to choose an unused subnet from') +option('autoaddr_end', type: 'string', value: '192.168.255.0', description: 'End of range of IPv4 subnets to choose an unused subnet from') +option('autoaddr_prefix', type: 'integer', value: 24, description: 'prefix of IPv4 subnets to choose an unused subnet from') + option('host_validate', type: 'feature', value: 'auto', description: 'build virt-host-validate') option('init_script', type: 'combo', choices: ['systemd', 'openrc', 'check', 'none'], value: 'check', description: 'Style of init script to install') option('loader_nvram', type: 'string', value: '', description: 'Pass list of pairs of <loader>:<nvram> paths. Both pairs and list items are separated by a colon.') diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c index 9da5e790b7..cb2915550f 100644 --- a/src/network/bridge_driver_conf.c +++ b/src/network/bridge_driver_conf.c @@ -65,6 +65,13 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, const char *filename) { g_autoptr(virConf) conf = NULL; + + const char *autoaddrStart = "192.168.122.0"; + const char *autoaddrEnd = "192.168.255.0"; + unsigned int autoaddrPrefix = 24; + g_autofree char *autoaddrStartFromConf = NULL; + g_autofree char *autoaddrEndFromConf = NULL; + g_autofree char *fwBackendStr = NULL; bool fwBackendSelected = false; size_t i; @@ -88,6 +95,36 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, /* use virConfGetValue*(conf, ...) functions to read any settings into cfg */ + if (virConfGetValueString(conf, "autoaddr_start", &autoaddrStartFromConf) < 0) + return -1; + if (autoaddrStartFromConf) { + VIR_DEBUG("autoaddr_start setting requested from config file %s: '%s'", + filename, autoaddrStartFromConf); + autoaddrStart = autoaddrStartFromConf; + } + + if (virConfGetValueString(conf, "autoaddr_end", &autoaddrEndFromConf) < 0) + return -1; + if (autoaddrEndFromConf) { + VIR_DEBUG("autoaddr_end setting requested from config file %s: '%s'", + filename, autoaddrEndFromConf); + autoaddrEnd = autoaddrEndFromConf; + } + + switch (virConfGetValueUInt(conf, "autoaddr_prefix", &autoaddrPrefix)) { + case 1: + VIR_DEBUG("autoaddr_prefix setting requested from config file %s: '%u'", + filename, autoaddrPrefix); + break; + + case 0: + break; + + case -1: + default: + return -1; + } + if (virConfGetValueString(conf, "firewall_backend", &fwBackendStr) < 0) return -1; @@ -106,6 +143,30 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, } } + if (virSocketAddrParse(&cfg->autoaddrStart, autoaddrStart, AF_INET) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid autoaddr_start '%1$s' in network driver config file %2$s, must be numeric IPv4 network address"), + autoaddrStart, filename); + return -1; + } + + if (virSocketAddrParse(&cfg->autoaddrEnd, autoaddrEnd, AF_INET) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid autoaddr_end '%1$s' in network driver config file %2$s, must be numeric IPv4 network address"), + autoaddrStart, filename); + return -1; + } + + if ((cfg->autoaddrPrefix = autoaddrPrefix) > 32) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid IPv4 prefix '%1$u' in network driver config file %2$s"), + autoaddrPrefix, filename); + return -1; + } + + VIR_INFO("autoaddr_start='%s' autoaddr_end='%s' autoaddr_prefix='%u'", + autoaddrStart, autoaddrEnd, autoaddrPrefix); + for (i = 0; i < nFwBackends && !fwBackendSelected; i++) { switch ((virFirewallBackend)fwBackends[i]) { diff --git a/src/network/bridge_driver_conf.h b/src/network/bridge_driver_conf.h index 8f221f391e..7ebf77037d 100644 --- a/src/network/bridge_driver_conf.h +++ b/src/network/bridge_driver_conf.h @@ -39,6 +39,10 @@ struct _virNetworkDriverConfig { char *pidDir; char *dnsmasqStateDir; + virSocketAddr autoaddrStart; + virSocketAddr autoaddrEnd; + unsigned int autoaddrPrefix; + virFirewallBackend firewallBackend; }; diff --git a/src/network/libvirtd_network.aug b/src/network/libvirtd_network.aug index 5d6d72dd92..5212505e1f 100644 --- a/src/network/libvirtd_network.aug +++ b/src/network/libvirtd_network.aug @@ -22,10 +22,16 @@ module Libvirtd_network = let int_entry (kw:string) = [ key kw . value_sep . int_val ] let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ] + let autoaddr_entry = str_entry "autoaddr_start" + | str_entry "autoaddr_end" + | int_entry "autoaddr_prefix" + let firewall_backend_entry = str_entry "firewall_backend" (* Each entry in the config is one of the following *) - let entry = firewall_backend_entry + let entry = autoaddr_entry + | firewall_backend_entry + let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/network/meson.build b/src/network/meson.build index 07cd5cda55..8faff6eb1c 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -51,6 +51,9 @@ if conf.has('WITH_NETWORK') } network_options_conf = configuration_data({ + 'AUTOADDR_START': autoaddr_start, + 'AUTOADDR_END': autoaddr_end, + 'AUTOADDR_PREFIX': autoaddr_prefix, 'FIREWALL_BACKEND_PRIORITY': ', '.join(firewall_backend_priority), 'FIREWALL_BACKEND': firewall_backend_priority[0], }) @@ -62,6 +65,9 @@ if conf.has('WITH_NETWORK') ) network_options_hack_conf = configuration_data({ + 'AUTOADDR_START': autoaddr_start, + 'AUTOADDR_END': autoaddr_end, + 'AUTOADDR_PREFIX': autoaddr_prefix, 'FIREWALL_BACKEND_PRIORITY': ', '.join(firewall_backend_priority), 'FIREWALL_BACKEND': firewall_backend_priority[0], # This hack is necessary because the output file is going to be diff --git a/src/network/network.conf.in b/src/network/network.conf.in index 5ed64a04a5..83a27df04c 100644 --- a/src/network/network.conf.in +++ b/src/network/network.conf.in @@ -27,3 +27,14 @@ # reloaded using the new backend.) # #firewall_backend = "@FIREWALL_BACKEND@" +# +# autoaddr_start +# autoaddr_end +# autoaddr_prefix +# +# These three setting specify the range of subnets that should be used +# for networks that have "autoaddr='yes'" +# +#autoaddr_start = "@AUTOADDR_START@" +#autoaddr_end = "@AUTOADDR_END@" +#autoaddr_prefix = @AUTOADDR_PREFIX@ diff --git a/src/network/test_libvirtd_network.aug.in b/src/network/test_libvirtd_network.aug.in index 9e29a9192f..6edcbaed62 100644 --- a/src/network/test_libvirtd_network.aug.in +++ b/src/network/test_libvirtd_network.aug.in @@ -3,3 +3,6 @@ module Test_libvirtd_network = test Libvirtd_network.lns get conf = { "firewall_backend" = "@FIREWALL_BACKEND@" } +{ "autoaddr_start" = "@AUTOADDR_START@" } +{ "autoaddr_end" = "@AUTOADDR_END@" } +{ "autoaddr_prefix" = "@AUTOADDR_PREFIX@" } -- 2.45.2

On Wed, Aug 07, 2024 at 01:15:58PM -0400, Laine Stump wrote:
These options are added to network.conf and virNetworkDriverConfig object:
autoaddr_start - start of the range of subnets to search (def: "192.168.122.0") autoaddr_end - end of the range of subnets (def: "192.168.255.0") autoaddr_prefix - prefix of these subnets (def: 24)
So I guess $prefix here is telling us that we need to iterate over the octet at the end of $prefix. ie the 3rd digit in these examples. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 8/7/24 1:41 PM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 01:15:58PM -0400, Laine Stump wrote:
These options are added to network.conf and virNetworkDriverConfig object:
autoaddr_start - start of the range of subnets to search (def: "192.168.122.0") autoaddr_end - end of the range of subnets (def: "192.168.255.0") autoaddr_prefix - prefix of these subnets (def: 24)
So I guess $prefix here is telling us that we need to iterate over the octet at the end of $prefix. ie the 3rd digit in these examples.
The code isn't really thinking of it as octets at all, just 32 bits, and the prefix is telling how many of those bits are the network part of the address; it just increments the network part by 1 each time (by shifting right $prefix-32 bits, incrementing by 1, then shifting left $prefix-32 bits).

On Wed, Aug 07, 2024 at 02:00:24PM -0400, Laine Stump wrote:
On 8/7/24 1:41 PM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 01:15:58PM -0400, Laine Stump wrote:
These options are added to network.conf and virNetworkDriverConfig object:
autoaddr_start - start of the range of subnets to search (def: "192.168.122.0") autoaddr_end - end of the range of subnets (def: "192.168.255.0") autoaddr_prefix - prefix of these subnets (def: 24)
So I guess $prefix here is telling us that we need to iterate over the octet at the end of $prefix. ie the 3rd digit in these examples.
The code isn't really thinking of it as octets at all, just 32 bits, and the prefix is telling how many of those bits are the network part of the address; it just increments the network part by 1 each time (by shifting right $prefix-32 bits, incrementing by 1, then shifting left $prefix-32 bits).
Ok, I guess that makes sense. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

The existing code was checking all ip addresses and routes of the network as each line of /proc/net/route was read. This does not lend itself well to the new autoaddr networks, as each ip address will need to be checked against all routes in /proc/net/route (i.e. the opposite order of nesting the loops) in order to determine if the address must be changed to something unused (and then change it and recheck against all routes until an unused subnet is found). Now there is a separate function (still in bridge_driver_linux.c) that reads all the entries in /proc/net/route and adds them to a hash table, and another that checks an IP/prefix against said hash table. These two functions are called by networkCheckIPAddrCollision() and networkCheckRouteCollision() (split apart and moved to bridge_driver.c) which then iterates through each IP address/route of the virtual network, checking each against all existing routes before moving on to the next. Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 72 ++++++++++++++- src/network/bridge_driver_linux.c | 132 +++++++++++++++------------ src/network/bridge_driver_nop.c | 22 +++-- src/network/bridge_driver_platform.h | 5 +- 4 files changed, 164 insertions(+), 67 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 32572c755f..b8e752f20d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1898,6 +1898,65 @@ networkAddRouteToBridge(virNetworkObj *obj, } +/* XXX: This function can be a lot more exhaustive, there are certainly + * other scenarios where we can ruin host network connectivity. + * XXX: Using a proper library is preferred over parsing /proc + */ +static int +networkCheckIPAddrCollision(virNetworkDef *def, GHashTable *routes) +{ + size_t i; + virNetworkIPDef *ipdef; + + /* check all IPv4 addresses of the network for conflict with + * existing routes + */ + for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_INET, i)); i++) { + unsigned int prefix = virNetworkIPDefPrefix(ipdef); + const char *iface = networkSysRoutesTableFind(routes, &ipdef->address, prefix); + + if (iface) { + g_autofree char *addrStr = virSocketAddrFormat(&ipdef->address); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("requested subnet %1$s/%2$u is already in use by interface %3$s"), + NULLSTR(addrStr), prefix, iface); + return -1; + } + } + + return 0; +} + + +static int +networkCheckRouteCollision(virNetworkDef *def, GHashTable *routes) +{ + size_t i; + virNetDevIPRoute *routedef; + + /* check all IPv4 host routes that will be added for this network + * for conflict with existing host routes + */ + for (i = 0; (routedef = virNetworkDefGetRouteByIndex(def, AF_INET, i)); i++) { + virSocketAddr *addr = virNetDevIPRouteGetAddress(routedef); + int prefix = virNetDevIPRouteGetPrefix(routedef); + const char *iface = networkSysRoutesTableFind(routes, addr, prefix); + + if (iface) { + g_autofree char *addrStr = virSocketAddrFormat(addr); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("requested route for %1$s/%2$u is already in use by interface %3$s"), + NULLSTR(addrStr), prefix, iface); + return -1; + } + } + + return 0; +} + + static int networkStartNetworkVirtual(virNetworkDriverState *driver, virNetworkObj *obj) @@ -1914,10 +1973,17 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, bool firewalRulesAdded = false; virSocketAddr *dnsServer = NULL; virFirewall *fwRemoval = NULL; + g_autoptr(GHashTable) routes = networkSysRoutesTableRead(); - /* Check to see if any network IP collides with an existing route */ - if (networkCheckRouteCollision(def) < 0) - return -1; + if (routes) { + /* Check to see if any network IP collides with an existing route on the host */ + if (networkCheckIPAddrCollision(def, routes) < 0) + return -1; + + /* also check for requested routes that collide with existing routes */ + if (networkCheckRouteCollision(def, routes) < 0) + return -1; + } /* Create and configure the bridge device */ if (!def->bridge) { diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index fe7c6e193c..1d6635743d 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -213,21 +213,53 @@ void networkPostReloadFirewallRules(bool startup G_GNUC_UNUSED) } -/* XXX: This function can be a lot more exhaustive, there are certainly - * other scenarios where we can ruin host network connectivity. - * XXX: Using a proper library is preferred over parsing /proc +static void +networkPrintRouteTableEntry(gpointer key, + gpointer value, + gpointer user_data G_GNUC_UNUSED) +{ + const gint64 *keyval = key; + const char *valueval = value; + + VIR_INFO("key: %016llx value: %s", (unsigned long long)*keyval, valueval); +} +/** + * networkSysRoutesTableRead:: + * + * Returns a GHashTable that contains an entry for each IPv4 route in + * the host system routing table. + * + * The key for each entry is the network address and prefix for the + * route, and the "data" is the name of the interface associated with + * the route (which is only used for error reporting if a conflict is + * found). This will later be used to determine if a given subnet is + * already "in use" (which for our purposes is defined as "there is a + * route in the system routing table with exactly the same subnet + + * prefix/netmask"). + * + * In the case that no routes are found (e.g. due to some unexpected + * changed in the format of /proc/net/route) an empty GHashTable will + * be returned, in order to be as non-disruptive as possible. + * + * The returned GHashTable must be g_hash_table_destroy()ed when the + * caller is finished with it. This can be done automatically by + * defining it as g_autoptr(GHashTable). + * */ -int networkCheckRouteCollision(virNetworkDef *def) +GHashTable * +networkSysRoutesTableRead(void) { int len; char *cur; g_autofree char *buf = NULL; /* allow for up to 100000 routes (each line is 128 bytes) */ enum {MAX_ROUTE_SIZE = 128*100000}; + g_autoptr(GHashTable) routes = g_hash_table_new_full(g_int64_hash, g_int64_equal, g_free, g_free); + /* Read whole routing table into memory */ if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0) - return 0; + return g_steal_pointer(&routes); /* Dropping the last character shouldn't hurt */ if (len > 0) @@ -236,7 +268,7 @@ int networkCheckRouteCollision(virNetworkDef *def) VIR_DEBUG("%s output:\n%s", PROC_NET_ROUTE, buf); if (!STRPREFIX(buf, "Iface")) - return 0; + return g_steal_pointer(&routes); /* First line is just headings, skip it */ cur = strchr(buf, '\n'); @@ -245,11 +277,9 @@ int networkCheckRouteCollision(virNetworkDef *def) while (cur) { char iface[17], dest[128], mask[128]; - unsigned int addr_val, mask_val; - virNetworkIPDef *ipdef; - virNetDevIPRoute *routedef; int num; - size_t i; + unsigned int addr_val, mask_val; + g_autofree gint64 *key = g_new(gint64, 1); /* NUL-terminate the line, so sscanf doesn't go beyond a newline. */ char *nl = strchr(cur, '\n'); @@ -275,60 +305,48 @@ int networkCheckRouteCollision(virNetworkDef *def) continue; } - addr_val &= mask_val; + /* NB: addr_val and mask_val both appear in the file as + * hexadecimal strings that, when converted into integers on + * little-endian hardware, will already be in network byte + * order! (e.g., the IP address 1.2.3.4 will show up as + * "04030201" in the file). When we are later looking up an + * address in the table, we will also be using network byte + * order, so we don't need to do any htonl() here. + */ + *key = ((gint64)(addr_val & mask_val) << 32) + mask_val; + VIR_DEBUG("addr_val: %08u mask_val: %08u key: %016llx iface: %s", + addr_val, mask_val, (unsigned long long)*key, iface); - for (i = 0; - (ipdef = virNetworkDefGetIPByIndex(def, AF_INET, i)); - i++) { + g_hash_table_replace(routes, g_steal_pointer(&key), g_strdup(iface)); + } - unsigned int net_dest; - virSocketAddr netmask; + VIR_INFO("Full Route Hash Table: sizeof(gint64): %d sizeof(long long unsigned): %d", + (int)sizeof(gint64), (int)sizeof(long long unsigned)); + g_hash_table_foreach(routes, networkPrintRouteTableEntry, NULL); - if (virNetworkIPDefNetmask(ipdef, &netmask) < 0) { - VIR_WARN("Failed to get netmask of '%s'", - def->bridge); - continue; - } + return g_steal_pointer(&routes); +} - net_dest = (ipdef->address.data.inet4.sin_addr.s_addr & - netmask.data.inet4.sin_addr.s_addr); - if ((net_dest == addr_val) && - (netmask.data.inet4.sin_addr.s_addr == mask_val)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network is already in use by interface %1$s"), - iface); - return -1; - } - } +const char * +networkSysRoutesTableFind(GHashTable *routesTable, + virSocketAddr *addr, + unsigned int prefix) +{ + virSocketAddr netaddr, netmask; + gint64 compare; - for (i = 0; - (routedef = virNetworkDefGetRouteByIndex(def, AF_INET, i)); - i++) { - - virSocketAddr r_mask, r_addr; - virSocketAddr *tmp_addr = virNetDevIPRouteGetAddress(routedef); - int r_prefix = virNetDevIPRouteGetPrefix(routedef); - - if (!tmp_addr || - virSocketAddrMaskByPrefix(tmp_addr, r_prefix, &r_addr) < 0 || - virSocketAddrPrefixToNetmask(r_prefix, &r_mask, AF_INET) < 0) - continue; - - if ((r_addr.data.inet4.sin_addr.s_addr == addr_val) && - (r_mask.data.inet4.sin_addr.s_addr == mask_val)) { - g_autofree char *addr_str = virSocketAddrFormat(&r_addr); - if (!addr_str) - virResetLastError(); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Route address '%1$s' conflicts with IP address for '%2$s'"), - NULLSTR(addr_str), iface); - return -1; - } - } - } + if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) + return false; - return 0; + if (virSocketAddrMask(addr, &netmask, &netaddr) < 0) + return false; + + compare = ((gint64)netaddr.data.inet4.sin_addr.s_addr << 32) + + netmask.data.inet4.sin_addr.s_addr; + + VIR_DEBUG("Searching for key: %016llx", (unsigned long long)compare); + return g_hash_table_lookup(routesTable, &compare); } diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c index 8bf3367bff..d46aa99a5a 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -32,12 +32,6 @@ void networkPostReloadFirewallRules(bool startup G_GNUC_UNUSED) { } - -int networkCheckRouteCollision(virNetworkDef *def G_GNUC_UNUSED) -{ - return 0; -} - int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, virFirewallBackend firewallBackend, virFirewall **fwRemoval G_GNUC_UNUSED) @@ -59,3 +53,19 @@ int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, void networkRemoveFirewallRules(virNetworkObj *obj G_GNUC_UNUSED) { } + + +GHashTable * +networkSysRoutesTableRead(void) +{ + return NULL; +} + + +const char * +networkSysRoutesTableFind(GHashTable *routesTable G_GNUC_UNUSED, + virSocketAddr *addr G_GNUC_UNUSED, + unsigned int prefix G_GNUC_UNUSED) +{ + return NULL; +} diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index cd2e3fa7b5..67c3db7dca 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -30,7 +30,10 @@ void networkPreReloadFirewallRules(virNetworkDriverState *driver, void networkPostReloadFirewallRules(bool startup); -int networkCheckRouteCollision(virNetworkDef *def); +GHashTable *networkSysRoutesTableRead(void); +const char *networkSysRoutesTableFind(GHashTable *routesTable, + virSocketAddr *addr, + unsigned int prefix); int networkAddFirewallRules(virNetworkDef *def, virFirewallBackend firewallBackend, -- 2.45.2

When a network has an <ip autoaddr='yes'/> element, the network driver will look for an "unused" subnet to assign to the network. The IP address of the network bridge will then be the .1 of that network, and any DHCP range, DHCP static hosts, and tftp boot server will be changed to the new network (while retaining the same host bits). * the range of subnets (and their prefix) have a hostwide configuration in /etc/libvirt/network.conf ("autoaddr_*"). * if the network config has a <dhcp> element with no <range> specified, then a range will automatically be added that starts at ${net}.2 and ends at ${net}.254 (adjusted for prefix - this is example is only if the prefix is 24). * If a network has autoaddr="yes" and also has an IP address defined, then the network driver will check that IP address first, and then start looking for a new address if that initial try fails. * If a new subnet is selected, this change of IP address will be saved to the network's persistent config (along with associated changes to other elements that need to be on the same subnet). This way the next time the network is started, it will attempt to use the same subnet that it had used the previous time. Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 137 ++++++++++++++++++++++++++++++++++-- 1 file changed, 133 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b8e752f20d..cceeb5d941 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1901,19 +1901,78 @@ networkAddRouteToBridge(virNetworkObj *obj, /* XXX: This function can be a lot more exhaustive, there are certainly * other scenarios where we can ruin host network connectivity. * XXX: Using a proper library is preferred over parsing /proc + * + * check for any host route colliding exactly with each requested IPv4 + * address/prefix, If a collision is found and autoaddr isn't set for + * the network, immediately return an error. + + * if autoaddr *is* set, iteratively try subnets using the auto_addr* + * info from the system config (in /etc/libvirt/network.conf) until an + * "unused" subnet is found, then use that subnet, and rewrite the + * persistent network config so that next time we'll start trying + * using the same address we got this time. + * + * If all subnets in the configured autoaddr range are already in use, + * then return an error. */ static int -networkCheckIPAddrCollision(virNetworkDef *def, GHashTable *routes) +networkCheckIPAddrCollision(virNetworkDriverState *driver, + virNetworkObj *obj, + GHashTable *routes) { + g_autoptr(virNetworkDriverConfig) cfg = virNetworkDriverGetConfig(driver); + uint32_t nextNet = ntohl(cfg->autoaddrStart.data.inet4.sin_addr.s_addr); /* next subnet to check */ + uint32_t lastNet = ntohl(cfg->autoaddrEnd.data.inet4.sin_addr.s_addr); + virNetworkDef *def = virNetworkObjGetDef(obj); size_t i; virNetworkIPDef *ipdef; /* check all IPv4 addresses of the network for conflict with * existing routes */ - for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_INET, i)); i++) { + for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); i++) { + virSocketAddr ip = ipdef->address; /* don't disturb the original (yet) */ unsigned int prefix = virNetworkIPDefPrefix(ipdef); - const char *iface = networkSysRoutesTableFind(routes, &ipdef->address, prefix); + const char *iface; + bool done = false; + uint32_t updateNet = 0; /* new subnet in *host* byte order (or 0 if no change) */ + + if (VIR_SOCKET_ADDR_IS_FAMILY(&ip, AF_INET6)) + continue; /* autoaddr isn't used on IPv6 addresses */ + + do { + if (VIR_SOCKET_ADDR_IS_FAMILY(&ip, AF_INET)) + iface = networkSysRoutesTableFind(routes, &ip, prefix); + else + iface = "unset"; /* could be anything !NULL, just need to take upcoming else clause */ + + if (!iface || !ipdef->autoaddr) { + + done = true; + + } else { + g_autofree char *addrStr = NULL; + + if (nextNet > lastNet) { + /* we've tried all of the subnets and they're all used, so give up */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("could not find an unused subnet for autoaddr network '%1$s'"), + def->name); + return -1; + } + + /* this is what we'll try next time through the loop */ + updateNet = nextNet; + virSocketAddrSetIPv4Addr(&ip, updateNet + 1); /* + 1 for host address */ + prefix = cfg->autoaddrPrefix; + + addrStr = virSocketAddrFormat(&ip); + VIR_INFO("autoaddr trying %s/%u", NULLSTR(addrStr), prefix); + + /* get ready for the next "new" request (the loop *after* the next) */ + nextNet = ((updateNet >> (32 - prefix)) + 1) << (32 - prefix); + } + } while (!done); if (iface) { g_autofree char *addrStr = virSocketAddrFormat(&ipdef->address); @@ -1923,6 +1982,76 @@ networkCheckIPAddrCollision(virNetworkDef *def, GHashTable *routes) NULLSTR(addrStr), prefix, iface); return -1; } + + if (updateNet) { + /* address was changed via autoaddr */ + g_autofree char *addrStr = virSocketAddrFormat(&ip); + + VIR_INFO("autoaddr success: %s/%u", NULLSTR(addrStr), prefix); + + /* put the newly found unused IP/prefix in the NetworkDef */ + ipdef->address = ip; + ipdef->prefix = prefix; + memset(&ipdef->netmask, 0, sizeof(ipdef->netmask)); /*in case original had netmask */ + + /* if there is a <dhcp> element also add/update the DHCP + * range, tftp bootserver, and, static hosts (they will + * all have the network part of their address changed, but + * the host bits will remain the same. + */ + + if (ipdef->dhcp) { + size_t j; + uint32_t tmp; + /* in *host* order initially, since we need to use if for some arithmetic */ + uint32_t hostmask = ~(0xffffffff << (32 - prefix)); + + + if (!ipdef->nranges) { + /* if there isn't any address range in the config, + * we need to setup at least one + */ + virNetworkDHCPRangeDef range = { 0 }; + + /* just prime them with the host bits for .2 and + * .254 (adjusted for prefix) + */ + virSocketAddrSetIPv4Addr(&range.addr.start, updateNet + 2); + virSocketAddrSetIPv4Addr(&range.addr.end, updateNet + hostmask - 1); + VIR_APPEND_ELEMENT(ipdef->ranges, ipdef->nranges, range); + } + + updateNet = htonl(updateNet); + hostmask = htonl(hostmask); + + for (j = 0; j < ipdef->nranges; j++) { + tmp = (ipdef->ranges[j].addr.start.data.inet4.sin_addr.s_addr & hostmask) | updateNet; + virSocketAddrSetIPv4AddrNetOrder(&ipdef->ranges[j].addr.start, tmp); + + tmp = (ipdef->ranges[j].addr.end.data.inet4.sin_addr.s_addr & hostmask) | updateNet; + virSocketAddrSetIPv4AddrNetOrder(&ipdef->ranges[j].addr.end, tmp); + } + + for (j = 0; j < ipdef->nhosts; j++) { + tmp = (ipdef->hosts[j].ip.data.inet4.sin_addr.s_addr & hostmask) | updateNet; + virSocketAddrSetIPv4AddrNetOrder(&ipdef->hosts[j].ip, tmp); + } + + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->bootserver, AF_INET)) { + tmp = (ipdef->bootserver.data.inet4.sin_addr.s_addr & hostmask) | updateNet; + virSocketAddrSetIPv4AddrNetOrder(&ipdef->bootserver, tmp); + } + } + + /* if this network is persistent also update on disk (so + * that the discovered address is tried first the next + * time this network is started) + */ + if (virNetworkObjIsPersistent(obj) + && virNetworkSaveConfig(cfg->networkConfigDir, def, driver->xmlopt) < 0) { + VIR_WARN("couldn't update autoaddr in network %1$s", def->name); + } + } } return 0; @@ -1977,7 +2106,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, if (routes) { /* Check to see if any network IP collides with an existing route on the host */ - if (networkCheckIPAddrCollision(def, routes) < 0) + if (networkCheckIPAddrCollision(driver, obj, routes) < 0) return -1; /* also check for requested routes that collide with existing routes */ -- 2.45.2

There has been a problem for several years with libvirt's default virtual network conflicting with the host physical network connection on new installs, particularly when the "host" is actually a virtual machine that is, itself, connected to the libvirt default network on its respective host. If the two default networks use the same subnet, and if the nested host's libvirt happens to start its network before the system networking connects to the L0 host, then network connectivity to the L1 guest is just silently non-working. We've tried several things over the years to eliminate this problem, including: 1) Checking for conflicting routes/interfaces during installation of the libvirt-daemon-config-network package (the one containing the default network config file) which tries different subnets until it finds one that doesn't conflict. This helps in some cases, but fails on 2 points: a) if the installation environment is different from the environment where the system is actually run (e.g. a "live CD" image of a distro, which is built in a container by the distro maintainers, then could later run in any number of places, and b) it modifies the installed files during the rpm installation %post script, which is now discouraged because people building container images don't like dealing with that. 2) When starting a libvirt network, we now check for any route or interface that conflicts with the new network's IP's and routes. This doesn't fix the problem, but does notify the user of the problem *as long as libvirt starts its networks after the host has started its system networks*. 3) New code (in the commits immediately previous to this one) add support for an "autoaddr" attribute in each virtual network <ip> element; when autoaddr is set, the network driver goes one step beyond (2) and actually finds an unused subnet and sets the new virtual network's addresses accordingly. These are all nice in their own ways, but none of them helps in the situation where libvirt's networks are started first (before the host's own network connections are all up). This led to this patch, which does the following: 4) Using a NetworkManager facility (dispatcher.d pscripts, which are run whenever any interface is brought up or down), check for any libvirt networks that conflict with a newly started NetworkManager interface, and if a conflict is found then log a message and destroy the libvirt network. Most usefully, though, if this destroyed network has autoaddr='yes' then the script will immediately restart the network, which will find a new, unused subnet. Once this is in place, the only issues are: 1) It only works with NetworkManager. But of course almost all of the cases where this problem has been an issue, networking is managed by NetworkManager. 2) If there are guests already running and connected to the network, they will be disconnected, and won't be reconnected until libvirtd/virtqemud is restarted (one of the things the QEMU driver does when rereading the status of active guests is to make sure all their interfaces are connected to their respective networks). Signed-off-by: Laine Stump <laine@redhat.com> --- libvirt.spec.in | 2 + src/network/meson.build | 6 + src/network/nm-dispatcher-check-nets.py | 196 ++++++++++++++++++++++++ 3 files changed, 204 insertions(+) create mode 100755 src/network/nm-dispatcher-check-nets.py diff --git a/libvirt.spec.in b/libvirt.spec.in index 29101e74fe..51cecfa598 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -603,6 +603,7 @@ Network filter configuration files for cleaning guest traffic Summary: Network driver plugin for the libvirtd daemon Requires: libvirt-daemon-common = %{version}-%{release} Requires: libvirt-libs = %{version}-%{release} +Requires: python3-libxml2 Requires: dnsmasq >= 2.41 %if %{prefer_nftables} Requires: nftables @@ -2151,6 +2152,7 @@ exit 0 %dir %attr(0755, root, root) %{_localstatedir}/lib/libvirt/dnsmasq/ %attr(0755, root, root) %{_libexecdir}/libvirt_leaseshelper %{_libdir}/libvirt/connection-driver/libvirt_driver_network.so +%{_prefix}/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets %{_mandir}/man8/virtnetworkd.8* %if %{with_firewalld_zone} %{_prefix}/lib/firewalld/zones/libvirt.xml diff --git a/src/network/meson.build b/src/network/meson.build index 8faff6eb1c..f620407759 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -169,4 +169,10 @@ if conf.has('WITH_NETWORK') rename: [ 'libvirt-routed-in.xml' ], ) endif + + install_data( + 'nm-dispatcher-check-nets.py', + install_dir: prefix / 'lib' / 'NetworkManager' / 'dispatcher.d', + rename: [ '50-libvirt-check-nets' ], + ) endif diff --git a/src/network/nm-dispatcher-check-nets.py b/src/network/nm-dispatcher-check-nets.py new file mode 100755 index 0000000000..454c434c88 --- /dev/null +++ b/src/network/nm-dispatcher-check-nets.py @@ -0,0 +1,196 @@ +#!/usr/bin/env python3 +# +# Copyright (C) 2012-2019 Red Hat, Inc. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library. If not, see +# <http://www.gnu.org/licenses/>. + +import libvirt +import sys +import os +import libxml2 +from ipaddress import ip_network + +# This script should be installed in +# /usr/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets. It will be +# called by NetworkManager every time a network interface is taken up +# or down. When a new network comes up, it checks the libvirt virtual +# networks to see if their IP address(es) (including any static +# routes) are in conflict with the IP address(es) (or static routes) +# of the newly added interface. If so, the libvirt network is +# disabled. It is assumed that the user will notice that their guests +# no longer have network connectvity (and/or the message logged by +# this script), see that the network has been disabled, and then +# realize the conflict when they try to restart it. +# +# set checkDefaultOnly=False to check *all* active libvirt networks +# for conflicts with the new interface. Set to True to check only the +# libvirt default network (since most networks other than the default +# network are added post-install at a time when all of the hosts other +# networks are already active, it may be overkill to check all of the +# libvirt networks for conflict here (and instead just add more +# needless overheard to bringing up a new host interface). +# +checkDefaultOnly = False + +# NB: since this file is installed in /usr/lib, it really shouldn't be +# modified by the user, but instead should be copied to +# /etc/NetworkManager/dispatcher.d, where it will override the copy in +# /usr/lib. Even that isn't a proper solution though - if we're going +# to actually have this config knob, perhaps we should check for it in +# the environment, and if someone wants to modify it they can put a +# short script in /etc that exports and environment variable and then +# calls this script? Just thinking out loud here. + + +def checkconflict(conn, netname, hostnets, hostif): + + # ignore if the network has been brought down or removed since we + # got the list + try: + net = conn.networkLookupByName(netname) + except libvirt.libvirtError: + return + + if not net.isActive(): + return + + xml = net.XMLDesc() + doc = libxml2.parseDoc(xml) + ctx = doc.xpathNewContext() + + # see if NetworkManager is informing us that this libvirt network + # itself is coming online + bridge = ctx.xpathEval("/network/bridge/@name") + if bridge and bridge[0].content == hostif: + return + + # check *all* the addresses of this network + addrs = ctx.xpathEval("/network/*[@address]") + for ip in addrs: + ctx.setContextNode(ip) + address = ctx.xpathEval("@address") + prefix = ctx.xpathEval("@prefix") + netmask = ctx.xpathEval("@netmask") + autoaddr = ctx.xpathEval("@autoaddr") + + isAutoaddr = False + if autoaddr and len(autoaddr[0].content): + isAutoaddr = (autoaddr[0].content == "yes") + + if not (address and len(address[0].content)): + continue + + addrstr = address[0].content + if not (prefix and len(prefix[0].content)): + # check for a netmask + if not (netmask and len(netmask[0].content)): + # this element has address, but no prefix or netmask + # probably it is <mac address ...> so we can ignore it + continue + # convert netmask to prefix + prefixstr = str(ip_network("0.0.0.0/%s" % netmask[0].content).prefixlen) + else: + prefixstr = prefix[0].content + + virtnetaddress = ip_network("%s/%s" % (addrstr, prefixstr), strict=False) + + for hostnet in hostnets: + if virtnetaddress == hostnet: + # There is a conflict with this libvirt network and the specified + # net, so we need to disable the libvirt network + print("Stopping libvirt network '%s' because its subnet %s conflicts with newly started interface '%s'')" + % (netname, str(hostnet), hostif)) + try: + net.destroy() + except libvirt.libvirtError: + print("Failed to destroy network %s" % netname) + return + + if isAutoaddr: + print("Restarting autoaddr libvirt network '%s'with new subnet" % (netname)) + try: + net.create() + except libvirt.libvirtError: + print("Failed to restart network '%s'" % netname) + return + return + + +def addHostNets(hostnets, countenv, addrenv): + + count = os.getenv(countenv) + if not count or count == 0: + return + + for num in range(int(count)): + addrstr = os.getenv("%s_%d" % (addrenv, num)) + if not addrstr or addrstr == "": + continue + + net = ip_network(addrstr.split()[0], strict=False) + if net: + hostnets.append(net) + return + + +############################################################ + +if sys.argv[2] != "up": + sys.exit(0) + +hostif = sys.argv[1] + +try: + conn = libvirt.open(None) +except libvirt.libvirtError: + print('Failed to open connection to the hypervisor') + sys.exit(0) + +if checkDefaultOnly: + nets = [] + net = conn.networkLookupByName("default") + if not (net and net.isActive()): + sys.exit(0) + nets.append(net) +else: + nets = conn.listAllNetworks(libvirt.VIR_CONNECT_LIST_NETWORKS_ACTIVE) + if not nets: + sys.exit(0) + +# We have at least one active network. Build a list of all network +# routes added by the new interface, and compare that list to the list +# of all networks used by each active libvirt network. If any are an +# exact match, then we have a conflict and need to shut down the +# libvirt network to avoid killing host networking. + +# When NetworkManager calls scripts in /etc/NetworkManager/dispatcher.d +# it will have all IP addresses and routes associated with the interface +# that is going up or down in the following environment variables: +# +# IP4_NUM_ADDRESSES - number of IPv4 addresses +# IP4_ADDRESS_N - one variable for each address, starting at _0 +# IP4_NUM_ROUTES - number of IPv5 routes +# IP4_ROUTE_N - one for each route, starting at _0 +# (replace "IP4" with "IP6" and repeat) +# +hostnets = [] +addHostNets(hostnets, "IP4_NUM_ADDRESSES", "IP4_ADDRESS") +addHostNets(hostnets, "IP4_NUM_ROUTES", "IP4_ROUTE") +addHostNets(hostnets, "IP6_NUM_ADDRESSES", "IP6_ADDRESS") +addHostNets(hostnets, "IP6_NUM_ROUTES", "IP6_ROUTE") + +for net in nets: + + checkconflict(conn, net.name(), hostnets, hostif) -- 2.45.2

On Wed, Aug 07, 2024 at 01:16:01PM -0400, Laine Stump wrote:
There has been a problem for several years with libvirt's default virtual network conflicting with the host physical network connection on new installs, particularly when the "host" is actually a virtual machine that is, itself, connected to the libvirt default network on its respective host. If the two default networks use the same subnet, and if the nested host's libvirt happens to start its network before the system networking connects to the L0 host, then network connectivity to the L1 guest is just silently non-working.
We've tried several things over the years to eliminate this problem, including:
1) Checking for conflicting routes/interfaces during installation of the libvirt-daemon-config-network package (the one containing the default network config file) which tries different subnets until it finds one that doesn't conflict. This helps in some cases, but fails on 2 points: a) if the installation environment is different from the environment where the system is actually run (e.g. a "live CD" image of a distro, which is built in a container by the distro maintainers, then could later run in any number of places, and b) it modifies the installed files during the rpm installation %post script, which is now discouraged because people building container images don't like dealing with that.
2) When starting a libvirt network, we now check for any route or interface that conflicts with the new network's IP's and routes. This doesn't fix the problem, but does notify the user of the problem *as long as libvirt starts its networks after the host has started its system networks*.
3) New code (in the commits immediately previous to this one) add support for an "autoaddr" attribute in each virtual network <ip> element; when autoaddr is set, the network driver goes one step beyond (2) and actually finds an unused subnet and sets the new virtual network's addresses accordingly.
These are all nice in their own ways, but none of them helps in the situation where libvirt's networks are started first (before the host's own network connections are all up). This led to this patch, which does the following:
4) Using a NetworkManager facility (dispatcher.d pscripts, which are run whenever any interface is brought up or down), check for any libvirt networks that conflict with a newly started NetworkManager interface, and if a conflict is found then log a message and destroy the libvirt network. Most usefully, though, if this destroyed network has autoaddr='yes' then the script will immediately restart the network, which will find a new, unused subnet.
Once this is in place, the only issues are:
1) It only works with NetworkManager. But of course almost all of the cases where this problem has been an issue, networking is managed by NetworkManager.
2) If there are guests already running and connected to the network, they will be disconnected, and won't be reconnected until libvirtd/virtqemud is restarted (one of the things the QEMU driver does when rereading the status of active guests is to make sure all their interfaces are connected to their respective networks).
Signed-off-by: Laine Stump <laine@redhat.com> --- libvirt.spec.in | 2 + src/network/meson.build | 6 + src/network/nm-dispatcher-check-nets.py | 196 ++++++++++++++++++++++++ 3 files changed, 204 insertions(+) create mode 100755 src/network/nm-dispatcher-check-nets.py
diff --git a/libvirt.spec.in b/libvirt.spec.in index 29101e74fe..51cecfa598 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -603,6 +603,7 @@ Network filter configuration files for cleaning guest traffic Summary: Network driver plugin for the libvirtd daemon Requires: libvirt-daemon-common = %{version}-%{release} Requires: libvirt-libs = %{version}-%{release} +Requires: python3-libxml2 Requires: dnsmasq >= 2.41 %if %{prefer_nftables} Requires: nftables @@ -2151,6 +2152,7 @@ exit 0 %dir %attr(0755, root, root) %{_localstatedir}/lib/libvirt/dnsmasq/ %attr(0755, root, root) %{_libexecdir}/libvirt_leaseshelper %{_libdir}/libvirt/connection-driver/libvirt_driver_network.so +%{_prefix}/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets %{_mandir}/man8/virtnetworkd.8* %if %{with_firewalld_zone} %{_prefix}/lib/firewalld/zones/libvirt.xml diff --git a/src/network/meson.build b/src/network/meson.build index 8faff6eb1c..f620407759 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -169,4 +169,10 @@ if conf.has('WITH_NETWORK') rename: [ 'libvirt-routed-in.xml' ], ) endif + + install_data( + 'nm-dispatcher-check-nets.py', + install_dir: prefix / 'lib' / 'NetworkManager' / 'dispatcher.d', + rename: [ '50-libvirt-check-nets' ], + ) endif diff --git a/src/network/nm-dispatcher-check-nets.py b/src/network/nm-dispatcher-check-nets.py new file mode 100755 index 0000000000..454c434c88 --- /dev/null +++ b/src/network/nm-dispatcher-check-nets.py @@ -0,0 +1,196 @@ +#!/usr/bin/env python3 +# +# Copyright (C) 2012-2019 Red Hat, Inc. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library. If not, see +# <http://www.gnu.org/licenses/>. + +import libvirt +import sys +import os +import libxml2 +from ipaddress import ip_network + +# This script should be installed in +# /usr/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets. It will be +# called by NetworkManager every time a network interface is taken up +# or down. When a new network comes up, it checks the libvirt virtual +# networks to see if their IP address(es) (including any static +# routes) are in conflict with the IP address(es) (or static routes) +# of the newly added interface. If so, the libvirt network is +# disabled. It is assumed that the user will notice that their guests +# no longer have network connectvity (and/or the message logged by +# this script), see that the network has been disabled, and then +# realize the conflict when they try to restart it. +# +# set checkDefaultOnly=False to check *all* active libvirt networks +# for conflicts with the new interface. Set to True to check only the +# libvirt default network (since most networks other than the default +# network are added post-install at a time when all of the hosts other +# networks are already active, it may be overkill to check all of the +# libvirt networks for conflict here (and instead just add more +# needless overheard to bringing up a new host interface). +# +checkDefaultOnly = False + +# NB: since this file is installed in /usr/lib, it really shouldn't be +# modified by the user, but instead should be copied to +# /etc/NetworkManager/dispatcher.d, where it will override the copy in +# /usr/lib. Even that isn't a proper solution though - if we're going +# to actually have this config knob, perhaps we should check for it in +# the environment, and if someone wants to modify it they can put a +# short script in /etc that exports and environment variable and then +# calls this script? Just thinking out loud here. + + +def checkconflict(conn, netname, hostnets, hostif):
...snip complex code... It occurrs to me that this python code is entirely duplicating logic that already exists in virtnetworkd that checks for conflicts. This makes me want to figure out a way to just re-use the existing code rather than having to write it again. Could we just send a SIGUSR2 to virtnetworkd as a way to trigger it to re-validate all running networks ? That would be easy to trigger from non-network manager setups too. But then how about ignoring network manager entirely and going right to the source, ie the kernel. Does it send out either udev or netlink events (probably the latter), when NICs have their link status set online/offline ? If so, virtnetworkd could just listen to the kernel events and "do the right thing", regardless of what tool is managing the NICs. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 8/7/24 1:32 PM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 01:16:01PM -0400, Laine Stump wrote:
There has been a problem for several years with libvirt's default virtual network conflicting with the host physical network connection on new installs, particularly when the "host" is actually a virtual machine that is, itself, connected to the libvirt default network on its respective host. If the two default networks use the same subnet, and if the nested host's libvirt happens to start its network before the system networking connects to the L0 host, then network connectivity to the L1 guest is just silently non-working.
We've tried several things over the years to eliminate this problem, including:
1) Checking for conflicting routes/interfaces during installation of the libvirt-daemon-config-network package (the one containing the default network config file) which tries different subnets until it finds one that doesn't conflict. This helps in some cases, but fails on 2 points: a) if the installation environment is different from the environment where the system is actually run (e.g. a "live CD" image of a distro, which is built in a container by the distro maintainers, then could later run in any number of places, and b) it modifies the installed files during the rpm installation %post script, which is now discouraged because people building container images don't like dealing with that.
2) When starting a libvirt network, we now check for any route or interface that conflicts with the new network's IP's and routes. This doesn't fix the problem, but does notify the user of the problem *as long as libvirt starts its networks after the host has started its system networks*.
3) New code (in the commits immediately previous to this one) add support for an "autoaddr" attribute in each virtual network <ip> element; when autoaddr is set, the network driver goes one step beyond (2) and actually finds an unused subnet and sets the new virtual network's addresses accordingly.
These are all nice in their own ways, but none of them helps in the situation where libvirt's networks are started first (before the host's own network connections are all up). This led to this patch, which does the following:
4) Using a NetworkManager facility (dispatcher.d pscripts, which are run whenever any interface is brought up or down), check for any libvirt networks that conflict with a newly started NetworkManager interface, and if a conflict is found then log a message and destroy the libvirt network. Most usefully, though, if this destroyed network has autoaddr='yes' then the script will immediately restart the network, which will find a new, unused subnet.
Once this is in place, the only issues are:
1) It only works with NetworkManager. But of course almost all of the cases where this problem has been an issue, networking is managed by NetworkManager.
2) If there are guests already running and connected to the network, they will be disconnected, and won't be reconnected until libvirtd/virtqemud is restarted (one of the things the QEMU driver does when rereading the status of active guests is to make sure all their interfaces are connected to their respective networks).
Signed-off-by: Laine Stump <laine@redhat.com> --- libvirt.spec.in | 2 + src/network/meson.build | 6 + src/network/nm-dispatcher-check-nets.py | 196 ++++++++++++++++++++++++ 3 files changed, 204 insertions(+) create mode 100755 src/network/nm-dispatcher-check-nets.py
diff --git a/libvirt.spec.in b/libvirt.spec.in index 29101e74fe..51cecfa598 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -603,6 +603,7 @@ Network filter configuration files for cleaning guest traffic Summary: Network driver plugin for the libvirtd daemon Requires: libvirt-daemon-common = %{version}-%{release} Requires: libvirt-libs = %{version}-%{release} +Requires: python3-libxml2 Requires: dnsmasq >= 2.41 %if %{prefer_nftables} Requires: nftables @@ -2151,6 +2152,7 @@ exit 0 %dir %attr(0755, root, root) %{_localstatedir}/lib/libvirt/dnsmasq/ %attr(0755, root, root) %{_libexecdir}/libvirt_leaseshelper %{_libdir}/libvirt/connection-driver/libvirt_driver_network.so +%{_prefix}/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets %{_mandir}/man8/virtnetworkd.8* %if %{with_firewalld_zone} %{_prefix}/lib/firewalld/zones/libvirt.xml diff --git a/src/network/meson.build b/src/network/meson.build index 8faff6eb1c..f620407759 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -169,4 +169,10 @@ if conf.has('WITH_NETWORK') rename: [ 'libvirt-routed-in.xml' ], ) endif + + install_data( + 'nm-dispatcher-check-nets.py', + install_dir: prefix / 'lib' / 'NetworkManager' / 'dispatcher.d', + rename: [ '50-libvirt-check-nets' ], + ) endif diff --git a/src/network/nm-dispatcher-check-nets.py b/src/network/nm-dispatcher-check-nets.py new file mode 100755 index 0000000000..454c434c88 --- /dev/null +++ b/src/network/nm-dispatcher-check-nets.py @@ -0,0 +1,196 @@ +#!/usr/bin/env python3 +# +# Copyright (C) 2012-2019 Red Hat, Inc. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library. If not, see +# <http://www.gnu.org/licenses/>. + +import libvirt +import sys +import os +import libxml2 +from ipaddress import ip_network + +# This script should be installed in +# /usr/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets. It will be +# called by NetworkManager every time a network interface is taken up +# or down. When a new network comes up, it checks the libvirt virtual +# networks to see if their IP address(es) (including any static +# routes) are in conflict with the IP address(es) (or static routes) +# of the newly added interface. If so, the libvirt network is +# disabled. It is assumed that the user will notice that their guests +# no longer have network connectvity (and/or the message logged by +# this script), see that the network has been disabled, and then +# realize the conflict when they try to restart it. +# +# set checkDefaultOnly=False to check *all* active libvirt networks +# for conflicts with the new interface. Set to True to check only the +# libvirt default network (since most networks other than the default +# network are added post-install at a time when all of the hosts other +# networks are already active, it may be overkill to check all of the +# libvirt networks for conflict here (and instead just add more +# needless overheard to bringing up a new host interface). +# +checkDefaultOnly = False + +# NB: since this file is installed in /usr/lib, it really shouldn't be +# modified by the user, but instead should be copied to +# /etc/NetworkManager/dispatcher.d, where it will override the copy in +# /usr/lib. Even that isn't a proper solution though - if we're going +# to actually have this config knob, perhaps we should check for it in +# the environment, and if someone wants to modify it they can put a +# short script in /etc that exports and environment variable and then +# calls this script? Just thinking out loud here. + + +def checkconflict(conn, netname, hostnets, hostif):
...snip complex code...
It occurrs to me that this python code is entirely duplicating logic that already exists in virtnetworkd that checks for conflicts.
Possibly not *exactly* what's in virtnetworkd, since I'm not certain if NM includes all static routes in the list it puts in the environment variables (while they *would* show up in the system routing table that our network driver uses). Of course that sounds like another reason to prefer re-using the existing code!
This makes me want to figure out a way to just re-use the existing code rather than having to write it again.
Could we just send a SIGUSR2 to virtnetworkd as a way to trigger it to re-validate all running networks ? That would be easy to trigger from non-network manager setups too.
That sounds simpler (and probably quicker), and if we could do it that I would prefer it ("only works with NetworkManager" is #1 on my list of 'issues' above after all :-)), but what would we do in the case that virtnetworkd (or libvirtd) wasn't currently running? Run some virsh command to trigger virtnetworkd to start, and then send the SIGUSR2? (this does
But then how about ignoring network manager entirely and going right to the source, ie the kernel. Does it send out either udev or netlink events (probably the latter), when NICs have their link status set online/offline ? If so, virtnetworkd could just listen to the kernel events and "do the right thing", regardless of what tool is managing the NICs.
I imagine *something* is sent out, and it's definitely worth me investigating. But wouldn't virtnetworkd have to be running all of the time in order to use something like that?

On Wed, Aug 07, 2024 at 04:00:09PM -0400, Laine Stump wrote:
On 8/7/24 1:32 PM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 01:16:01PM -0400, Laine Stump wrote:
+ +import libvirt +import sys +import os +import libxml2 +from ipaddress import ip_network + +# This script should be installed in +# /usr/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets. It will be +# called by NetworkManager every time a network interface is taken up +# or down. When a new network comes up, it checks the libvirt virtual +# networks to see if their IP address(es) (including any static +# routes) are in conflict with the IP address(es) (or static routes) +# of the newly added interface. If so, the libvirt network is +# disabled. It is assumed that the user will notice that their guests +# no longer have network connectvity (and/or the message logged by +# this script), see that the network has been disabled, and then +# realize the conflict when they try to restart it. +# +# set checkDefaultOnly=False to check *all* active libvirt networks +# for conflicts with the new interface. Set to True to check only the +# libvirt default network (since most networks other than the default +# network are added post-install at a time when all of the hosts other +# networks are already active, it may be overkill to check all of the +# libvirt networks for conflict here (and instead just add more +# needless overheard to bringing up a new host interface). +# +checkDefaultOnly = False + +# NB: since this file is installed in /usr/lib, it really shouldn't be +# modified by the user, but instead should be copied to +# /etc/NetworkManager/dispatcher.d, where it will override the copy in +# /usr/lib. Even that isn't a proper solution though - if we're going +# to actually have this config knob, perhaps we should check for it in +# the environment, and if someone wants to modify it they can put a +# short script in /etc that exports and environment variable and then +# calls this script? Just thinking out loud here. + + +def checkconflict(conn, netname, hostnets, hostif):
...snip complex code...
It occurrs to me that this python code is entirely duplicating logic that already exists in virtnetworkd that checks for conflicts.
Possibly not *exactly* what's in virtnetworkd, since I'm not certain if NM includes all static routes in the list it puts in the environment variables (while they *would* show up in the system routing table that our network driver uses). Of course that sounds like another reason to prefer re-using the existing code!
Oh definitely a strong reason to reuse the code. We don't want a situation where users file bugs because we don't detect clashes in one scenario, but do in the other.
This makes me want to figure out a way to just re-use the existing code rather than having to write it again.
Could we just send a SIGUSR2 to virtnetworkd as a way to trigger it to re-validate all running networks ? That would be easy to trigger from non-network manager setups too.
That sounds simpler (and probably quicker), and if we could do it that I would prefer it ("only works with NetworkManager" is #1 on my list of 'issues' above after all :-)), but what would we do in the case that virtnetworkd (or libvirtd) wasn't currently running? Run some virsh command to trigger virtnetworkd to start, and then send the SIGUSR2?
As long as there are networks running, libvirtd/virnetworkd should not shut down due to the auto shutdown timer. They might temporarily shutdown due to a restart on package upgrades. If our startup code runs the "check for clashes" logic, then we'll do the right thing. The only problem will be if a user manually stops the daemon and leaves it stopped, while still having a network present. I'm happy to call that user error and not worry about solving clashes in that case, as no normal system should get in that state.
(this does
But then how about ignoring network manager entirely and going right to the source, ie the kernel. Does it send out either udev or netlink events (probably the latter), when NICs have their link status set online/offline ? If so, virtnetworkd could just listen to the kernel events and "do the right thing", regardless of what tool is managing the NICs.
I imagine *something* is sent out, and it's definitely worth me investigating. But wouldn't virtnetworkd have to be running all of the time in order to use something like that?
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 8/16/24 11:23 AM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 04:00:09PM -0400, Laine Stump wrote:
On 8/7/24 1:32 PM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 01:16:01PM -0400, Laine Stump wrote:
+ +import libvirt +import sys +import os +import libxml2 +from ipaddress import ip_network + +# This script should be installed in +# /usr/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets. It will be +# called by NetworkManager every time a network interface is taken up +# or down. When a new network comes up, it checks the libvirt virtual +# networks to see if their IP address(es) (including any static +# routes) are in conflict with the IP address(es) (or static routes) +# of the newly added interface. If so, the libvirt network is +# disabled. It is assumed that the user will notice that their guests +# no longer have network connectvity (and/or the message logged by +# this script), see that the network has been disabled, and then +# realize the conflict when they try to restart it. +# +# set checkDefaultOnly=False to check *all* active libvirt networks +# for conflicts with the new interface. Set to True to check only the +# libvirt default network (since most networks other than the default +# network are added post-install at a time when all of the hosts other +# networks are already active, it may be overkill to check all of the +# libvirt networks for conflict here (and instead just add more +# needless overheard to bringing up a new host interface). +# +checkDefaultOnly = False + +# NB: since this file is installed in /usr/lib, it really shouldn't be +# modified by the user, but instead should be copied to +# /etc/NetworkManager/dispatcher.d, where it will override the copy in +# /usr/lib. Even that isn't a proper solution though - if we're going +# to actually have this config knob, perhaps we should check for it in +# the environment, and if someone wants to modify it they can put a +# short script in /etc that exports and environment variable and then +# calls this script? Just thinking out loud here. + + +def checkconflict(conn, netname, hostnets, hostif):
...snip complex code...
It occurrs to me that this python code is entirely duplicating logic that already exists in virtnetworkd that checks for conflicts.
Possibly not *exactly* what's in virtnetworkd, since I'm not certain if NM includes all static routes in the list it puts in the environment variables (while they *would* show up in the system routing table that our network driver uses). Of course that sounds like another reason to prefer re-using the existing code!
Oh definitely a strong reason to reuse the code. We don't want a situation where users file bugs because we don't detect clashes in one scenario, but do in the other.
This makes me want to figure out a way to just re-use the existing code rather than having to write it again.
Could we just send a SIGUSR2 to virtnetworkd as a way to trigger it to re-validate all running networks ? That would be easy to trigger from non-network manager setups too.
That sounds simpler (and probably quicker), and if we could do it that I would prefer it ("only works with NetworkManager" is #1 on my list of 'issues' above after all :-)), but what would we do in the case that virtnetworkd (or libvirtd) wasn't currently running? Run some virsh command to trigger virtnetworkd to start, and then send the SIGUSR2?
As long as there are networks running, libvirtd/virnetworkd should not shut down due to the auto shutdown timer.
But it *does* auto-shutdown (even if there is an active guest that's using an active virtual network) :-). So I guess that bit was never wired up for virtnetworkd. I'd never before bothered to learn how the idle shutdown timer works, but just spent 5 minutes cscoping back through the code, and didn't get to the magic call made by the qemu driver that's missing from the network driver. If someone can point me at the right bit so I can avoid more digging, that would be appreciated. Anyway, if virtnetworkd doesn't do an idle shutdown, that simplifies things quite a bit (I had just assumed that we didn't want that because it took up extra resources and was (until now) unnecessary).
They might temporarily shutdown due to a restart on package upgrades.
If our startup code runs the "check for clashes" logic, then we'll do the right thing.
The only problem will be if a user manually stops the daemon and leaves it stopped, while still having a network present. I'm happy to call that user error and not worry about solving clashes in that case, as no normal system should get in that state.
Yeah, if you want things to work, play by the rules!
(this does
But then how about ignoring network manager entirely and going right to the source, ie the kernel. Does it send out either udev or netlink events (probably the latter), when NICs have their link status set online/offline ? If so, virtnetworkd could just listen to the kernel events and "do the right thing", regardless of what tool is managing the NICs.
I imagine *something* is sent out, and it's definitely worth me investigating. But wouldn't virtnetworkd have to be running all of the time in order to use something like that?
Right before I left town for a week away from the computer, I found the blog of someone who was wanting to do exactly this. I'm going to go read up on it now, and hopefully it won't lead me down the wrong path! :-) (I have to say though, that needing to look at netlink to try and figure out a new bug report (also just before leaving town) reminded me of how unnecessarily complicated netlink is :-/)

With autoaddr enabled, the subnet to be used for the default network will be verified/changed at the time the network starts. Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/default.xml.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/default.xml.in b/src/network/default.xml.in index 08a3632eb6..a01c6d30ae 100644 --- a/src/network/default.xml.in +++ b/src/network/default.xml.in @@ -2,7 +2,7 @@ <name>default</name> <bridge name='virbr0'/> <forward/> - <ip address='192.168.122.1' netmask='255.255.255.0'> + <ip autoaddr='yes' address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.122.2' end='192.168.122.254'/> </dhcp> -- 2.45.2

On Wed, Aug 07, 2024 at 01:16:02PM -0400, Laine Stump wrote:
With autoaddr enabled, the subnet to be used for the default network will be verified/changed at the time the network starts.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/default.xml.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/network/default.xml.in b/src/network/default.xml.in index 08a3632eb6..a01c6d30ae 100644 --- a/src/network/default.xml.in +++ b/src/network/default.xml.in @@ -2,7 +2,7 @@ <name>default</name> <bridge name='virbr0'/> <forward/> - <ip address='192.168.122.1' netmask='255.255.255.0'> + <ip autoaddr='yes' address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.122.2' end='192.168.122.254'/> </dhcp>
What I find unsettling is that we're providing an address + netmask here, along with a DHCP range, but there's no guarantee any of these are within the start+end addresses in network.conf I'm thinking that perhaps autoaddr='yes' should be mutually exclusive with existence of an explicit address + DHCP range. ie only permit <ip autoaddr='yes'> <dhcp/> </ip> on the basis that if someone wants explicit control over the DHCP range, then they probably shouldn't be relying on auto-addr usage. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 8/7/24 1:45 PM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 01:16:02PM -0400, Laine Stump wrote:
With autoaddr enabled, the subnet to be used for the default network will be verified/changed at the time the network starts.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/default.xml.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/network/default.xml.in b/src/network/default.xml.in index 08a3632eb6..a01c6d30ae 100644 --- a/src/network/default.xml.in +++ b/src/network/default.xml.in @@ -2,7 +2,7 @@ <name>default</name> <bridge name='virbr0'/> <forward/> - <ip address='192.168.122.1' netmask='255.255.255.0'> + <ip autoaddr='yes' address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.122.2' end='192.168.122.254'/> </dhcp>
What I find unsettling is that we're providing an address + netmask here, along with a DHCP range, but there's no guarantee any of these are within the start+end addresses in network.conf
The code removes the network part of any existing dhcp range, static host, or bootpserver, and replaces that with the network part of the newly chosen network, which puts them into the same subnet, so actually it is guaranteed. While I agree that it might be unusual for someone to have static host addresses configured in a network where they wanted to use autoaddr, it's not difficult to support, and makes the handling consistent with the way that the network's IP address, and also the DHCP range and bootp server addresses are handled - basically every IP address associated with the network is moved to the new subnet.
I'm thinking that perhaps autoaddr='yes' should be mutually exclusive with existence of an explicit address + DHCP range. ie only permit
<ip autoaddr='yes'> <dhcp/> </ip>
on the basis that if someone wants explicit control over the DHCP range, then they probably shouldn't be relying on auto-addr usage.
That simplifies it, but would require removing the code that saves the current chosen subnet (so that it can be tried first when the network is next started), making it more likely that addresses would change each time the network is started. I see you've brought up exactly that topic on your response to Patch 7/7 :-)

On Wed, Aug 07, 2024 at 02:15:16PM -0400, Laine Stump wrote:
On 8/7/24 1:45 PM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 01:16:02PM -0400, Laine Stump wrote:
With autoaddr enabled, the subnet to be used for the default network will be verified/changed at the time the network starts.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/default.xml.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/network/default.xml.in b/src/network/default.xml.in index 08a3632eb6..a01c6d30ae 100644 --- a/src/network/default.xml.in +++ b/src/network/default.xml.in @@ -2,7 +2,7 @@ <name>default</name> <bridge name='virbr0'/> <forward/> - <ip address='192.168.122.1' netmask='255.255.255.0'> + <ip autoaddr='yes' address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.122.2' end='192.168.122.254'/> </dhcp>
What I find unsettling is that we're providing an address + netmask here, along with a DHCP range, but there's no guarantee any of these are within the start+end addresses in network.conf
The code removes the network part of any existing dhcp range, static host, or bootpserver, and replaces that with the network part of the newly chosen network, which puts them into the same subnet, so actually it is guaranteed.
While I agree that it might be unusual for someone to have static host addresses configured in a network where they wanted to use autoaddr, it's not difficult to support, and makes the handling consistent with the way that the network's IP address, and also the DHCP range and bootp server addresses are handled - basically every IP address associated with the network is moved to the new subnet.
Hmmm, it would be wierd for users to have the DHCP range / static hosts under a different network from the primary host IP, but users are known todo wierd things. So if someonme has a config like: <ip autoaddr='yes' address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.42.2' end='192.168.42.254'/> </dhcp> And we detect a clash for 192.168.122.0/24, IIUC, you're saying we'll cyhang the DHCP range to 192.168.123.2->192.168.123.254, even though the DHCP range was on a different subnet originally. That'd be quite susprising to me. I think we should enforce that if you have autoaddr=yes, that all DHCP/static host IPs are on the same subnet as the primary network IP.
I'm thinking that perhaps autoaddr='yes' should be mutually exclusive with existence of an explicit address + DHCP range. ie only permit
<ip autoaddr='yes'> <dhcp/> </ip>
on the basis that if someone wants explicit control over the DHCP range, then they probably shouldn't be relying on auto-addr usage.
That simplifies it, but would require removing the code that saves the current chosen subnet (so that it can be tried first when the network is next started), making it more likely that addresses would change each time the network is started. I see you've brought up exactly that topic on your response to Patch 7/7 :-)
Yeah, removing the addrs doesn't actually simplify, since we need to then store the exact same info somewhere else. So what you've done is better, if we can define sane semantics when the DHCP/static hosts are on different subnets by forbidding that config. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 8/16/24 11:30 AM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 02:15:16PM -0400, Laine Stump wrote:
On 8/7/24 1:45 PM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 01:16:02PM -0400, Laine Stump wrote:
With autoaddr enabled, the subnet to be used for the default network will be verified/changed at the time the network starts.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/default.xml.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/network/default.xml.in b/src/network/default.xml.in index 08a3632eb6..a01c6d30ae 100644 --- a/src/network/default.xml.in +++ b/src/network/default.xml.in @@ -2,7 +2,7 @@ <name>default</name> <bridge name='virbr0'/> <forward/> - <ip address='192.168.122.1' netmask='255.255.255.0'> + <ip autoaddr='yes' address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.122.2' end='192.168.122.254'/> </dhcp>
What I find unsettling is that we're providing an address + netmask here, along with a DHCP range, but there's no guarantee any of these are within the start+end addresses in network.conf
The code removes the network part of any existing dhcp range, static host, or bootpserver, and replaces that with the network part of the newly chosen network, which puts them into the same subnet, so actually it is guaranteed.
While I agree that it might be unusual for someone to have static host addresses configured in a network where they wanted to use autoaddr, it's not difficult to support, and makes the handling consistent with the way that the network's IP address, and also the DHCP range and bootp server addresses are handled - basically every IP address associated with the network is moved to the new subnet.
Hmmm, it would be wierd for users to have the DHCP range / static hosts under a different network from the primary host IP, but users are known todo wierd things.
So if someonme has a config like:
<ip autoaddr='yes' address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.42.2' end='192.168.42.254'/> </dhcp>
I'm pretty sure dnsmasq doesn't allow that configuration. And even if it did, libvirt itself would give this error and refuse the config: error: internal error: range 192.168.42.2 - 192.168.42.254 is not entirely within network 192.168.122.1
And we detect a clash for 192.168.122.0/24, IIUC, you're saying we'll cyhang the DHCP range to 192.168.123.2->192.168.123.254, even though the DHCP range was on a different subnet originally. That'd be quite susprising to me. > I think we should enforce that if you have autoaddr=yes, that all DHCP/static host IPs are on the same subnet as the primary network IP.
Fortunately we already enforce that for the DHCP range. But not for static host entries (I tried that and it was accepted by both libvirt and dnsmasq). I'm guessing it was an oversight that this check wasn't done for static host entries, although I can't think of a reasonable use case for it (and maybe there *is* one :-)). And thinking about the tftpserver, I suppose it *is* valid to have a tftpserver on a different subnet, so maybe we should leave that untouched. Or wait! What if we update static host / tftpserver IPs that were on the original (cached/suggested) subnet, but if they already don't match anyway, then we leave them alone? This way someone who had setup, e.g. a guest attached to the network in question as a tftpserver for other guests on the network could do that by a) adding a <host> entry for the tftpserver's interface MAC address that was on the same subnet as the "suggested" <ip> of the network, b) adding a tftpserver with that same IP to the network's config, and c) configuring said tftpserver guest to get its IP by DHCP. If libvirt had to find a different subnet, the tftpserver would get an address on the new subnet, and the other guests would be told to look to that new address for a tftpserver. But on the other hand, if they had a physcial tftpserver somewhere else on the network (by definition another subnet), they could just enter in that tftpserver address (on a different subnet), libvirt wouldn't touch that IP if it had to change the network's subnet, and the other guests would get an IP (and default route) on whatever subnet libvirt found, with the tftpserver IP sent in the DHCP response always the same. I think doing it like this would avoid surprises, while allowing for as much functionality as possible.
I'm thinking that perhaps autoaddr='yes' should be mutually exclusive with existence of an explicit address + DHCP range. ie only permit
<ip autoaddr='yes'> <dhcp/> </ip>
on the basis that if someone wants explicit control over the DHCP range, then they probably shouldn't be relying on auto-addr usage.
That simplifies it, but would require removing the code that saves the current chosen subnet (so that it can be tried first when the network is next started), making it more likely that addresses would change each time the network is started. I see you've brought up exactly that topic on your response to Patch 7/7 :-)
Yeah, removing the addrs doesn't actually simplify, since we need to then store the exact same info somewhere else. So what you've done is better, if we can define sane semantics when the DHCP/static hosts are on different subnets by forbidding that config.

Since the default network now has autoaddr='yes', there is no need to waste time during %post install looking for an unused network and modifying the config of the default virtual network. (It would be even simpler to just install the files directly to /etc rather than installing to the examples and then copying to /etc, but if we did that then someone who had manually net-undefined the default network would keep getting it reinstalled every time there was an update to libvirt-daemon-config-network. Of course these days someone who wants to do that can simply dnf rm daemon-config-network if they didn't want the stock default network config, but nobody will be expecting that they have to do that, leading to hundreds of bug reports about "I deleted the default network and it comes back every time I update my packages!") Signed-off-by: Laine Stump <laine@redhat.com> --- libvirt.spec.in | 36 +++++------------------------------- 1 file changed, 5 insertions(+), 31 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 51cecfa598..cb62e0dcdb 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1949,38 +1949,12 @@ exit 0 %libvirt_systemd_config_pre virtnetworkd %post daemon-config-network +# Installing during %post via cp rather than directly as a part of the package manifest +# prevents us from reinstalling on update in cases where the user has net-undefined +# the default network (and so doesn't *want* it to be reinstalled) if test $1 -eq 1 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml ; then - # see if the network used by default network creates a conflict, - # and try to resolve it - # NB: 192.168.122.0/24 is used in the default.xml template file; - # do not modify any of those values here without also modifying - # them in the template. - orig_sub=122 - sub=${orig_sub} - nl=' -' - routes="${nl}$(ip route show | cut -d' ' -f1)${nl}" - case ${routes} in - *"${nl}192.168.${orig_sub}.0/24${nl}"*) - # there was a match, so we need to look for an unused subnet - for new_sub in $(seq 124 254); do - case ${routes} in - *"${nl}192.168.${new_sub}.0/24${nl}"*) - ;; - *) - sub=$new_sub - break; - ;; - esac - done - ;; - *) - ;; - esac - - sed -e "s/${orig_sub}/${sub}/g" \ - < %{_datadir}/libvirt/networks/default.xml \ - > %{_sysconfdir}/libvirt/qemu/networks/default.xml + cp %{_datadir}/libvirt/networks/default.xml \ + %{_sysconfdir}/libvirt/qemu/networks/default.xml ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml # libvirt saves this file with mode 0600 chmod 0600 %{_sysconfdir}/libvirt/qemu/networks/default.xml -- 2.45.2

On Wed, Aug 07, 2024 at 01:16:03PM -0400, Laine Stump wrote:
Since the default network now has autoaddr='yes', there is no need to waste time during %post install looking for an unused network and modifying the config of the default virtual network.
I think this is possibly still compelling to keep. Consider the user has something network related that they frequently manually start post boot (a VPN perhaps). We have the sequence 1. Machine boots 2. User starts something on subset 192.168.122.0/24 3. User installs libvirt 4. Machine shuts down 5. Machine boots 6. Libvirt starts on boot 7. VM auto-starts 8. User starts something on subset 192.168.122.0/24 with steps 5-8 repeating every day. Currently at step (3), we'll rewrite the default network to 192.168.123.0, and it'll stay on 192.168.123.0 forever which gives stability for the install. With this patch applied, then in step 6 we'll always start with 192.168.122.0, and then in step 8 change to 192.168.123.0, breaking the VM from step 7. That said in the previous patch I suggested that we should make autoaddr=yes be incompatible with specifying address= in the XML, which would *require* this patch as we would no longer have any address in the network XML to modify. This makes me think that the network driver should remember auto-assigned networks across reboots. ie require <ip autoaddr="yes"> <dhcp/> </ip> in the default.xml file, but then have something in /var/lib/libvirt/networks/subnets.db, where we remember the auto-assigned subnets a virtual network previously had. IOW, once an auto-subnet is chosen, it should never change again, unless a new clash arises.
(It would be even simpler to just install the files directly to /etc rather than installing to the examples and then copying to /etc, but if we did that then someone who had manually net-undefined the default network would keep getting it reinstalled every time there was an update to libvirt-daemon-config-network. Of course these days someone who wants to do that can simply dnf rm daemon-config-network if they didn't want the stock default network config, but nobody will be expecting that they have to do that, leading to hundreds of bug reports about "I deleted the default network and it comes back every time I update my packages!")
Signed-off-by: Laine Stump <laine@redhat.com> --- libvirt.spec.in | 36 +++++------------------------------- 1 file changed, 5 insertions(+), 31 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 51cecfa598..cb62e0dcdb 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1949,38 +1949,12 @@ exit 0 %libvirt_systemd_config_pre virtnetworkd
%post daemon-config-network +# Installing during %post via cp rather than directly as a part of the package manifest +# prevents us from reinstalling on update in cases where the user has net-undefined +# the default network (and so doesn't *want* it to be reinstalled) if test $1 -eq 1 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml ; then - # see if the network used by default network creates a conflict, - # and try to resolve it - # NB: 192.168.122.0/24 is used in the default.xml template file; - # do not modify any of those values here without also modifying - # them in the template. - orig_sub=122 - sub=${orig_sub} - nl=' -' - routes="${nl}$(ip route show | cut -d' ' -f1)${nl}" - case ${routes} in - *"${nl}192.168.${orig_sub}.0/24${nl}"*) - # there was a match, so we need to look for an unused subnet - for new_sub in $(seq 124 254); do - case ${routes} in - *"${nl}192.168.${new_sub}.0/24${nl}"*) - ;; - *) - sub=$new_sub - break; - ;; - esac - done - ;; - *) - ;; - esac - - sed -e "s/${orig_sub}/${sub}/g" \ - < %{_datadir}/libvirt/networks/default.xml \ - > %{_sysconfdir}/libvirt/qemu/networks/default.xml + cp %{_datadir}/libvirt/networks/default.xml \ + %{_sysconfdir}/libvirt/qemu/networks/default.xml ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml # libvirt saves this file with mode 0600 chmod 0600 %{_sysconfdir}/libvirt/qemu/networks/default.xml -- 2.45.2
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 8/7/24 1:54 PM, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 01:16:03PM -0400, Laine Stump wrote:
Since the default network now has autoaddr='yes', there is no need to waste time during %post install looking for an unused network and modifying the config of the default virtual network.
I think this is possibly still compelling to keep.
Consider the user has something network related that they frequently manually start post boot (a VPN perhaps). We have the sequence
1. Machine boots 2. User starts something on subset 192.168.122.0/24 3. User installs libvirt 4. Machine shuts down
5. Machine boots 6. Libvirt starts on boot 7. VM auto-starts 8. User starts something on subset 192.168.122.0/24
with steps 5-8 repeating every day.
Currently at step (3), we'll rewrite the default network to 192.168.123.0, and it'll stay on 192.168.123.0 forever which gives stability for the install.
With this patch applied, then in step 6 we'll always start with 192.168.122.0, and then in step 8 change to 192.168.123.0, breaking the VM from step 7.
Not when all of the other patches are also applied - when the network is started the first time and chooses 192.168.123.0, that will be saved in the network's config XML (just like we re-save the config when mac addresses and uuids are changed by libvirt), and so the next time it is started (in your example, the next time the host boots), it would start out by trying 192.168.123.0 again, and only switch to a different network if that was already in use. And with the NM dispatcher script, even if another network starts at 192.168.123.0 after this network starts, this network will switch to yet another network (and, again, save the config with that new address so it will be used again next time). The only remaining issue (which I pointed out in one of the commit logs) is that if guests have already started by the time the NM script forces the network to change, then those guests would have been started with 192.168.123.0, but then the network is changed to 192.168.124.0
That said in the previous patch I suggested that we should make autoaddr=yes be incompatible with specifying address= in the XML, which would *require* this patch as we would no longer have any address in the network XML to modify.
This makes me think that the network driver should remember auto-assigned networks across reboots.
ie require
<ip autoaddr="yes"> <dhcp/> </ip>
in the default.xml file, but then have something in /var/lib/libvirt/networks/subnets.db, where we remember the auto-assigned subnets a virtual network previously had. IOW, once an auto-subnet is chosen, it should never change again, unless a new clash arises.
Yeah, that's the way it works. Except without a new file to keep track of.
(It would be even simpler to just install the files directly to /etc rather than installing to the examples and then copying to /etc, but if we did that then someone who had manually net-undefined the default network would keep getting it reinstalled every time there was an update to libvirt-daemon-config-network. Of course these days someone who wants to do that can simply dnf rm daemon-config-network if they didn't want the stock default network config, but nobody will be expecting that they have to do that, leading to hundreds of bug reports about "I deleted the default network and it comes back every time I update my packages!")
Signed-off-by: Laine Stump <laine@redhat.com> --- libvirt.spec.in | 36 +++++------------------------------- 1 file changed, 5 insertions(+), 31 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 51cecfa598..cb62e0dcdb 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1949,38 +1949,12 @@ exit 0 %libvirt_systemd_config_pre virtnetworkd
%post daemon-config-network +# Installing during %post via cp rather than directly as a part of the package manifest +# prevents us from reinstalling on update in cases where the user has net-undefined +# the default network (and so doesn't *want* it to be reinstalled) if test $1 -eq 1 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml ; then - # see if the network used by default network creates a conflict, - # and try to resolve it - # NB: 192.168.122.0/24 is used in the default.xml template file; - # do not modify any of those values here without also modifying - # them in the template. - orig_sub=122 - sub=${orig_sub} - nl=' -' - routes="${nl}$(ip route show | cut -d' ' -f1)${nl}" - case ${routes} in - *"${nl}192.168.${orig_sub}.0/24${nl}"*) - # there was a match, so we need to look for an unused subnet - for new_sub in $(seq 124 254); do - case ${routes} in - *"${nl}192.168.${new_sub}.0/24${nl}"*) - ;; - *) - sub=$new_sub - break; - ;; - esac - done - ;; - *) - ;; - esac - - sed -e "s/${orig_sub}/${sub}/g" \ - < %{_datadir}/libvirt/networks/default.xml \ - > %{_sysconfdir}/libvirt/qemu/networks/default.xml + cp %{_datadir}/libvirt/networks/default.xml \ + %{_sysconfdir}/libvirt/qemu/networks/default.xml ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml # libvirt saves this file with mode 0600 chmod 0600 %{_sysconfdir}/libvirt/qemu/networks/default.xml -- 2.45.2
With regards, Daniel
participants (2)
-
Daniel P. Berrangé
-
Laine Stump