On 9/3/24 10:36 AM, Martin Kletzander wrote: > Once networkUpdateState() identifies a dead network it should clean up > after it as well. > > Resolves: https://issues.redhat.com/browse/RHEL-50968 > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- >   src/network/bridge_driver.c | 6 ++++++ >   1 file changed, 6 insertions(+) > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index e507dcd4c5c9..ebdb39d0743b 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -510,6 +510,12 @@ networkUpdateState(virNetworkObj *obj, >           virNetworkObjSetDnsmasqPid(obj, dnsmasqPid); >       } > +    /* Clean up after networks which were active but we have found > out they are > +     * actually down */ > +    if (!virNetworkObjIsActive(obj)) { > +        networkShutdownNetwork(driver, obj); > +    } > + The good part of this is that it will properly/completely clean up the remnants of any network that has somehow failed (e.g. the bridge device gets deleted by someone else). The bad part is that it means we end up calling all of the cleanup stuff (e.g. birNetDevBandwidthClear(), deleting the bridge device) even for networks that *were* properly shutdown (and so shouldn't have any of that stuff done). Aside from taking extra time (especially if someone has dozen's of inactive networks), this could be problematic if, for example, someone has two virtual networks that they've hand-configured to use the same bridge device (but obey their own policy to never start both at the same time). I *think* calling networkShutdownNetwork on the inactive network then would end up deleting the bridge device that's in use by the active network. (Arguably it's not a good idea to have configs like that when you could just use different bridge names for each, but I can think of at least one esoteric situation where someone might want to do it). Maybe this could be solved by having 3 states instead of 2 - "active" (should be working), "inactive" (definitely confirmed down, all teardown completed), and "zombie" (something has gone wrong with the network, but it hasn't been completely destroyed yet). Then you would only go ahead with the Shutdown if the network was "active" or "zombie", but skip it if the network was "inactive". Or something like that. >       return 0; >   }

Just in case one needs a clean up. Resolves: https://issues.redhat.com/browse/RHEL-50968 Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt;

--- src/network/bridge_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index ebdb39d0743b..3ef3444da241 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -492,8 +492,9 @@ networkUpdateState(virNetworkObj *obj, if (virNetworkObjIsActive(obj)) virNetworkObjPortForEach(obj, networkUpdatePort, obj); - /* Try and read dnsmasq pids of active networks */ - if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { + /* Try and read dnsmasq pids of both active and inactive networks, just in + * case a network became inactive and we need to clean up. */ + if (def->ips && (def->nips > 0)) { const char *binpath = NULL; pid_t dnsmasqPid;

The new function (networkCleanupInactive) can be called from an iterator over the list of networks without the risk of deadlock. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt;

--- src/network/bridge_driver.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3ef3444da241..0e8057e24aed 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -325,10 +325,10 @@ networkDnsmasqConfigFileName(virNetworkDriverConfig *cfg, } -/* do needed cleanup steps and remove the network from the list */ +/* do needed cleanup steps */ static int -networkRemoveInactive(virNetworkDriverState *driver, - virNetworkObj *obj) +networkCleanupInactive(virNetworkDriverState *driver, + virNetworkObj *obj) { g_autoptr(virNetworkDriverConfig) cfg = virNetworkDriverGetConfig(driver); g_autofree char *leasefile = NULL; @@ -372,6 +372,18 @@ networkRemoveInactive(virNetworkDriverState *driver, /* remove status file */ unlink(statusfile); + return 0; +} + + +/* do needed cleanup steps and remove the network from the list */ +static int +networkRemoveInactive(virNetworkDriverState *driver, + virNetworkObj *obj) +{ + if (networkCleanupInactive(driver, obj) < 0) + return -1; + /* remove the network definition */ virNetworkObjRemoveInactive(driver->networks, obj);