Recreate NI after modify/delete resolved inter-NI conflict

Inter-NI conflict (e.g. subnet overlap) maye happen even when going from
one valid intended state to another. This is because zedrouter receives
create/modify/delete NI notifications separately for each NI from
zedagent and a single change on its own may not be valid.

Signed-off-by: Milan Lenco <milan@zededa.com>

pkg/pillar/cmd/zedrouter/networkinstance.go

@@ -252,26 +252,32 @@ func (z *zedrouter) maybeDelOrInactivateNetworkInstance(
 		return false
 	}
 
-	if status.Activated {
-		z.doInactivateNetworkInstance(status)
-		// If NI config was deleted, status will be unpublished when async operations
-		// of NI inactivation complete.
-	} else if config == nil {
-		z.unpublishNetworkInstanceStatus(status)
-	}
-
 	if config != nil {
 		// Should be only inactivated, not yet deleted.
+		if status.Activated {
+			z.doInactivateNetworkInstance(status)
+		}
 		return true
 	}
 
+	z.delNetworkInstance(status)
+	z.log.Noticef("maybeDelOrInactivateNetworkInstance(%s) done", status.Key())
+	return true
+}
+
+func (z *zedrouter) delNetworkInstance(status *types.NetworkInstanceStatus) {
+	if status.Activated {
+		z.doInactivateNetworkInstance(status)
+		// Status will be unpublished when async operations of NI inactivation complete.
+	} else {
+		z.unpublishNetworkInstanceStatus(status)
+	}
 	if status.RunningUplinkProbing {
 		err := z.uplinkProber.StopNIProbing(status.UUID)
 		if err != nil {
 			z.log.Error(err)
 		}
 	}
-
 	if status.BridgeNum != 0 {
 		bridgeNumKey := types.UuidToNumKey{UUID: status.UUID}
 		err := z.bridgeNumAllocator.Free(bridgeNumKey, false)
@@ -288,6 +294,28 @@ func (z *zedrouter) maybeDelOrInactivateNetworkInstance(
 	}
 
 	z.deleteNetworkInstanceMetrics(status.Key())
-	z.log.Noticef("maybeDelOrInactivateNetworkInstance(%s) done", status.Key())
-	return true
+}
+
+// Called when a NetworkInstance is deleted or modified to check if there are other
+// NIs that previously could not be configured due to inter-NI config conflicts.
+func (z *zedrouter) checkConflictingNetworkInstances() {
+	for _, item := range z.pubNetworkInstanceStatus.GetAll() {
+		niStatus := item.(types.NetworkInstanceStatus)
+		if !niStatus.NIConflict {
+			continue
+		}
+		niConfig := z.lookupNetworkInstanceConfig(niStatus.Key())
+		if niConfig == nil {
+			continue
+		}
+		niConflict, err := z.doNetworkInstanceSanityCheck(niConfig)
+		if err == nil && niConflict == false {
+			// Try to re-create the network instance now that the conflict is gone.
+			z.log.Noticef("Recreating NI %s (%s) now that inter-NI conflict "+
+				"is not present anymore", niConfig.UUID, niConfig.DisplayName)
+			// First release whatever has been already allocated for this NI.
+			z.delNetworkInstance(&niStatus)
+			z.handleNetworkInstanceCreate(nil, niConfig.Key(), *niConfig)
+		}
+	}
 }

pkg/pillar/cmd/zedrouter/pubsubhandlers.go

@@ -176,10 +176,11 @@ func (z *zedrouter) handleNetworkInstanceCreate(ctxArg interface{}, key string,
 		return
 	}
 
-	if err := z.doNetworkInstanceSanityCheck(&config); err != nil {
+	if niConflict, err := z.doNetworkInstanceSanityCheck(&config); err != nil {
 		z.log.Error(err)
 		status.SetErrorNow(err.Error())
 		status.ChangeInProgress = types.ChangeInProgressTypeNone
+		status.NIConflict = niConflict
 		z.publishNetworkInstanceStatus(&status)
 		return
 	}
@@ -295,11 +296,12 @@ func (z *zedrouter) handleNetworkInstanceModify(ctxArg interface{}, key string,
 
 	prevPortLL := status.PortLogicalLabel
 	status.NetworkInstanceConfig = config
-	if err := z.doNetworkInstanceSanityCheck(&config); err != nil {
+	if niConflict, err := z.doNetworkInstanceSanityCheck(&config); err != nil {
 		z.log.Error(err)
 		status.SetErrorNow(err.Error())
 		status.WaitingForUplink = false
 		status.ChangeInProgress = types.ChangeInProgressTypeNone
+		status.NIConflict = niConflict
 		z.publishNetworkInstanceStatus(status)
 		return
 	}
@@ -391,6 +393,9 @@ func (z *zedrouter) handleNetworkInstanceModify(ctxArg interface{}, key string,
 	} else if status.Activated {
 		z.doUpdateActivatedNetworkInstance(config, status)
 	}
+
+	// Check if some inter-NI conflicts were resolved by this modification.
+	z.checkConflictingNetworkInstances()
 	z.log.Functionf("handleNetworkInstanceModify(%s) done", key)
 }
 
@@ -407,6 +412,8 @@ func (z *zedrouter) handleNetworkInstanceDelete(ctxArg interface{}, key string,
 	z.publishNetworkInstanceStatus(status)
 
 	done := z.maybeDelOrInactivateNetworkInstance(status)
+	// Check if some inter-NI conflicts were resolved by this delete.
+	z.checkConflictingNetworkInstances()
 	z.log.Functionf("handleNetworkInstanceDelete(%s) done %t", key, done)
 }
 

pkg/pillar/cmd/zedrouter/validation.go

@@ -13,7 +13,7 @@ import (
 )
 
 func (z *zedrouter) doNetworkInstanceSanityCheck(
-	config *types.NetworkInstanceConfig) error {
+	config *types.NetworkInstanceConfig) (niConflict bool, err error) {
 	z.log.Functionf("Sanity Checking NetworkInstance(%s-%s): type:%d, IpType:%d",
 		config.DisplayName, config.UUID, config.Type, config.IpType)
 
@@ -24,7 +24,7 @@ func (z *zedrouter) doNetworkInstanceSanityCheck(
 	case types.NetworkInstanceTypeSwitch:
 		// Do nothing
 	default:
-		return fmt.Errorf("network instance type %d is not supported", config.Type)
+		return false, fmt.Errorf("network instance type %d is not supported", config.Type)
 	}
 
 	// IpType - Check for valid types
@@ -33,29 +33,29 @@ func (z *zedrouter) doNetworkInstanceSanityCheck(
 		// Do nothing
 	case types.AddressTypeIPV4, types.AddressTypeIPV6,
 		types.AddressTypeCryptoIPV4, types.AddressTypeCryptoIPV6:
-		err := z.doNetworkInstanceSubnetSanityCheck(config)
+		niConflict, err = z.doNetworkInstanceSubnetSanityCheck(config)
 		if err != nil {
-			return err
+			return niConflict, err
 		}
 		err = z.doNetworkInstanceDhcpRangeSanityCheck(config)
 		if err != nil {
-			return err
+			return false, err
 		}
 		err = z.doNetworkInstanceGatewaySanityCheck(config)
 		if err != nil {
-			return err
+			return false, err
 		}
 
 	default:
-		return fmt.Errorf("IpType %d not supported", config.IpType)
+		return false, fmt.Errorf("IpType %d not supported", config.IpType)
 	}
-	return nil
+	return false, nil
 }
 
 func (z *zedrouter) doNetworkInstanceSubnetSanityCheck(
-	config *types.NetworkInstanceConfig) error {
+	config *types.NetworkInstanceConfig) (niConflict bool, err error) {
 	if config.Subnet.IP == nil || config.Subnet.IP.IsUnspecified() {
-		return fmt.Errorf("subnet unspecified for %s-%s: %+v",
+		return false, fmt.Errorf("subnet unspecified for %s-%s: %+v",
 			config.Key(), config.DisplayName, config.Subnet)
 	}
 
@@ -73,7 +73,7 @@ func (z *zedrouter) doNetworkInstanceSubnetSanityCheck(
 
 		// Check if config.Subnet is contained in iterStatusEntry.Subnet
 		if niConfig2.Subnet.Contains(config.Subnet.IP) {
-			return fmt.Errorf("subnet(%s, IP:%s) overlaps with another "+
+			return true, fmt.Errorf("subnet(%s, IP:%s) overlaps with another "+
 				"network instance(%s-%s) Subnet(%s)",
 				config.Subnet.String(), config.Subnet.IP.String(),
 				niConfig2.DisplayName, niConfig2.UUID,
@@ -82,14 +82,14 @@ func (z *zedrouter) doNetworkInstanceSubnetSanityCheck(
 
 		// Reverse check: check if iterStatusEntry.Subnet is contained in config.subnet
 		if config.Subnet.Contains(niConfig2.Subnet.IP) {
-			return fmt.Errorf("another network instance(%s-%s) Subnet(%s) "+
+			return true, fmt.Errorf("another network instance(%s-%s) Subnet(%s) "+
 				"overlaps with Subnet(%s)",
 				niConfig2.DisplayName, niConfig2.UUID,
 				niConfig2.Subnet.String(),
 				config.Subnet.String())
 		}
 	}
-	return nil
+	return false, nil
 }
 
 func (z *zedrouter) doNetworkInstanceDhcpRangeSanityCheck(

pkg/pillar/cmd/zedrouter/zedrouter.go

@@ -416,7 +416,7 @@ func (z *zedrouter) run(ctx context.Context) (err error) {
 				}
 				if appNetConfig == nil && recUpdate.AppConnStatus.Deleted &&
 					appNetStatus != nil && !appNetStatus.HasError() &&
-					!appNetStatus.Pending() {
+					!appNetStatus.Pending() && appNetStatus.ConfigInSync {
 					z.unpublishAppNetworkStatus(appNetStatus)
 				}
 			}

pkg/pillar/types/zedroutertypes.go

@@ -2610,6 +2610,7 @@ type NetworkInstanceStatus struct {
 	Activate uint64
 
 	ChangeInProgress ChangeInProgressType
+	NIConflict       bool // True if config conflicts with another NI
 
 	// Activated is true if the network instance has been created in the network stack.
 	Activated bool