On 9/3/24 10:36 AM, Martin Kletzander wrote:

The semantic does not change since inside networkUpdatePort() (well, networkNotifyPort, for which the former is a wrapper) exits for inactive networks, but with an error we can easily avoid with this patch.

(which normally wouldn't happen, *except* when a network's bridge device has disappeared causing us to mark the network inactive *during* networkUpdate state. Yep.)

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

--- src/network/bridge_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 915211d1b590..5b510a222cd2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -489,7 +489,8 @@ networkUpdateState(virNetworkObj *obj, return -1; }

- virNetworkObjPortForEach(obj, networkUpdatePort, obj); + if (virNetworkObjIsActive(obj)) + virNetworkObjPortForEach(obj, networkUpdatePort, obj);

/* Try and read dnsmasq pids of active networks */ if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) {

On 9/3/24 10:36 AM, Martin Kletzander wrote:

The function networkShutdownNetwork already does that.

(Harmless, but redundant)(or am I missing something?)

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

--- src/network/bridge_driver.c | 1 - 1 file changed, 1 deletion(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5b510a222cd2..b16d524aa181 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2387,7 +2387,6 @@ networkStartNetwork(virNetworkDriverState *driver, virErrorPtr save_err;

virErrorPreserveLast(&save_err); - virNetworkObjUnsetDefTransient(obj); networkShutdownNetwork(driver, obj); virErrorRestore(&save_err); }

On 9/3/24 10:36 AM, Martin Kletzander wrote:

It will be more useful in there when calling from new places.

(and also the extra call to virNetworkObjDeleteAllPorts() that this creates in the error cleanup of networkStartNetwork() is harmless)

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

--- src/network/bridge_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b16d524aa181..d27a7e9b88c4 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2447,6 +2447,8 @@ networkShutdownNetwork(virNetworkDriverState *driver, return -1; }

+ virNetworkObjDeleteAllPorts(obj, cfg->stateDir); + /* now that we know it's stopped call the hook if present */ networkRunHook(obj, NULL, VIR_HOOK_NETWORK_OP_STOPPED, VIR_HOOK_SUBOP_END); @@ -3458,8 +3460,6 @@ networkDestroy(virNetworkPtr net) if ((ret = networkShutdownNetwork(driver, obj)) < 0) goto cleanup;

- virNetworkObjDeleteAllPorts(obj, cfg->stateDir); - /* @def replaced in virNetworkObjUnsetDefTransient */ def = virNetworkObjGetDef(obj);

On 9/3/24 10:36 AM, Martin Kletzander wrote:

It skips the cleanup from networkStartNetwork and the only other path> already checks if the network is active or not.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

--- src/network/bridge_driver.c | 3 --- 1 file changed, 3 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d27a7e9b88c4..e507dcd4c5c9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2405,9 +2405,6 @@ networkShutdownNetwork(virNetworkDriverState *driver,

VIR_INFO("Shutting down network '%s'", def->name);

- if (!virNetworkObjIsActive(obj)) - return 0; - stateFile = virNetworkConfigFile(cfg->stateDir, def->name); if (!stateFile) return -1;

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@redhat.com> --- 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; }