Further refactoring zedrouter to make it much more robust

pkg/pillar/cmd/zedagent/handlenetworkinstance.go

@@ -90,10 +90,12 @@ func prepareAndPublishNetworkInstanceInfoMsg(ctx *zedagentContext,
 			errInfo.Timestamp = errTime
 			info.NetworkErr = append(info.NetworkErr, errInfo)
 			info.State = zinfo.ZNetworkInstanceState_ZNETINST_STATE_ERROR
+		} else if status.ChangeInProgress != types.ChangeInProgressTypeNone {
+			info.State = zinfo.ZNetworkInstanceState_ZNETINST_STATE_INIT
 		} else if status.Activated {
 			info.State = zinfo.ZNetworkInstanceState_ZNETINST_STATE_ONLINE
 		} else {
-			info.State = zinfo.ZNetworkInstanceState_ZNETINST_STATE_INIT
+			info.State = zinfo.ZNetworkInstanceState_ZNETINST_STATE_UNSPECIFIED
 		}
 		info.Activated = status.Activated
 

pkg/pillar/cmd/zedrouter/appnetwork.go

@@ -140,6 +140,8 @@ func (z *zedrouter) doActivateAppNetwork(config types.AppNetworkConfig,
 
 	// Update AppNetwork and NetworkInstance status.
 	status.Activated = true
+	status.PendingAdd = false
+	status.PendingModify = false
 	z.publishAppNetworkStatus(status)
 	z.updateNIStatusAfterAppNetworkActivate(status)
 
@@ -305,6 +307,7 @@ func (z *zedrouter) doUpdateActivatedAppNetwork(oldConfig, newConfig types.AppNe
 	// Update app network status as well as status of connected network instances.
 	z.processAppConnReconcileStatus(appConnRecStatus, status)
 	z.reloadStatusOfAssignedIPs(status)
+	status.PendingModify = false
 	z.publishAppNetworkStatus(status)
 	z.updateNIStatusAfterAppNetworkActivate(status)
 }
@@ -332,6 +335,8 @@ func (z *zedrouter) doInactivateAppNetwork(config types.AppNetworkConfig,
 
 	// Update AppNetwork and NetworkInstance status.
 	status.Activated = false
+	status.PendingModify = false
+	status.PendingDelete = false
 	z.updateNIStatusAfterAppNetworkInactivate(status)
 	z.removeAssignedIPsFromAppNetStatus(status)
 	z.publishAppNetworkStatus(status)

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(&status); 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(status); 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,56 +13,56 @@ import (
 )
 
 func (z *zedrouter) doNetworkInstanceSanityCheck(
-	status *types.NetworkInstanceStatus) error {
+	config *types.NetworkInstanceConfig) (niConflict bool, err error) {
 	z.log.Functionf("Sanity Checking NetworkInstance(%s-%s): type:%d, IpType:%d",
-		status.DisplayName, status.UUID, status.Type, status.IpType)
+		config.DisplayName, config.UUID, config.Type, config.IpType)
 
 	//  Check NetworkInstanceType
-	switch status.Type {
+	switch config.Type {
 	case types.NetworkInstanceTypeLocal:
 		// Do nothing
 	case types.NetworkInstanceTypeSwitch:
 		// Do nothing
 	default:
-		return fmt.Errorf("network instance type %d is not supported", status.Type)
+		return false, fmt.Errorf("network instance type %d is not supported", config.Type)
 	}
 
 	// IpType - Check for valid types
-	switch status.IpType {
+	switch config.IpType {
 	case types.AddressTypeNone:
 		// Do nothing
 	case types.AddressTypeIPV4, types.AddressTypeIPV6,
 		types.AddressTypeCryptoIPV4, types.AddressTypeCryptoIPV6:
-		err := z.doNetworkInstanceSubnetSanityCheck(status)
+		niConflict, err = z.doNetworkInstanceSubnetSanityCheck(config)
 		if err != nil {
-			return err
+			return niConflict, err
 		}
-		err = z.doNetworkInstanceDhcpRangeSanityCheck(status)
+		err = z.doNetworkInstanceDhcpRangeSanityCheck(config)
 		if err != nil {
-			return err
+			return false, err
 		}
-		err = z.doNetworkInstanceGatewaySanityCheck(status)
+		err = z.doNetworkInstanceGatewaySanityCheck(config)
 		if err != nil {
-			return err
+			return false, err
 		}
 
 	default:
-		return fmt.Errorf("IpType %d not supported", status.IpType)
+		return false, fmt.Errorf("IpType %d not supported", config.IpType)
 	}
-	return nil
+	return false, nil
 }
 
 func (z *zedrouter) doNetworkInstanceSubnetSanityCheck(
-	status *types.NetworkInstanceStatus) error {
-	if status.Subnet.IP == nil || status.Subnet.IP.IsUnspecified() {
-		return fmt.Errorf("subnet unspecified for %s-%s: %+v",
-			status.Key(), status.DisplayName, status.Subnet)
+	config *types.NetworkInstanceConfig) (niConflict bool, err error) {
+	if config.Subnet.IP == nil || config.Subnet.IP.IsUnspecified() {
+		return false, fmt.Errorf("subnet unspecified for %s-%s: %+v",
+			config.Key(), config.DisplayName, config.Subnet)
 	}
 
-	items := z.pubNetworkInstanceStatus.GetAll()
-	for key2, status2 := range items {
-		niStatus2 := status2.(types.NetworkInstanceStatus)
-		if status.Key() == key2 {
+	items := z.subNetworkInstanceConfig.GetAll()
+	for key2, config2 := range items {
+		niConfig2 := config2.(types.NetworkInstanceConfig)
+		if config.Key() == key2 {
 			continue
 		}
 
@@ -71,62 +71,62 @@ func (z *zedrouter) doNetworkInstanceSubnetSanityCheck(
 		// any other NI and vice-versa ( Other NI Subnet addrs are not
 		// contained in the current NI subnet)
 
-		// Check if status.Subnet is contained in iterStatusEntry.Subnet
-		if niStatus2.Subnet.Contains(status.Subnet.IP) {
-			return fmt.Errorf("subnet(%s, IP:%s) overlaps with another "+
+		// Check if config.Subnet is contained in iterStatusEntry.Subnet
+		if niConfig2.Subnet.Contains(config.Subnet.IP) {
+			return true, fmt.Errorf("subnet(%s, IP:%s) overlaps with another "+
 				"network instance(%s-%s) Subnet(%s)",
-				status.Subnet.String(), status.Subnet.IP.String(),
-				niStatus2.DisplayName, niStatus2.UUID,
-				niStatus2.Subnet.String())
+				config.Subnet.String(), config.Subnet.IP.String(),
+				niConfig2.DisplayName, niConfig2.UUID,
+				niConfig2.Subnet.String())
 		}
 
-		// Reverse check: check if iterStatusEntry.Subnet is contained in status.subnet
-		if status.Subnet.Contains(niStatus2.Subnet.IP) {
-			return fmt.Errorf("another network instance(%s-%s) Subnet(%s) "+
+		// Reverse check: check if iterStatusEntry.Subnet is contained in config.subnet
+		if config.Subnet.Contains(niConfig2.Subnet.IP) {
+			return true, fmt.Errorf("another network instance(%s-%s) Subnet(%s) "+
 				"overlaps with Subnet(%s)",
-				niStatus2.DisplayName, niStatus2.UUID,
-				niStatus2.Subnet.String(),
-				status.Subnet.String())
+				niConfig2.DisplayName, niConfig2.UUID,
+				niConfig2.Subnet.String(),
+				config.Subnet.String())
 		}
 	}
-	return nil
+	return false, nil
 }
 
 func (z *zedrouter) doNetworkInstanceDhcpRangeSanityCheck(
-	status *types.NetworkInstanceStatus) error {
-	if status.DhcpRange.Start == nil || status.DhcpRange.Start.IsUnspecified() {
+	config *types.NetworkInstanceConfig) error {
+	if config.DhcpRange.Start == nil || config.DhcpRange.Start.IsUnspecified() {
 		return fmt.Errorf("DhcpRange Start Unspecified: %+v",
-			status.DhcpRange.Start)
+			config.DhcpRange.Start)
 	}
-	if !status.Subnet.Contains(status.DhcpRange.Start) {
+	if !config.Subnet.Contains(config.DhcpRange.Start) {
 		return fmt.Errorf("DhcpRange Start(%s) not within Subnet(%s)",
-			status.DhcpRange.Start.String(), status.Subnet.String())
+			config.DhcpRange.Start.String(), config.Subnet.String())
 	}
-	if status.DhcpRange.End == nil || status.DhcpRange.End.IsUnspecified() {
+	if config.DhcpRange.End == nil || config.DhcpRange.End.IsUnspecified() {
 		return fmt.Errorf("DhcpRange End Unspecified: %+v",
-			status.DhcpRange.Start)
+			config.DhcpRange.Start)
 	}
-	if !status.Subnet.Contains(status.DhcpRange.End) {
+	if !config.Subnet.Contains(config.DhcpRange.End) {
 		return fmt.Errorf("DhcpRange End(%s) not within Subnet(%s)",
-			status.DhcpRange.End.String(), status.Subnet.String())
+			config.DhcpRange.End.String(), config.Subnet.String())
 	}
 	return nil
 }
 
 func (z *zedrouter) doNetworkInstanceGatewaySanityCheck(
-	status *types.NetworkInstanceStatus) error {
-	if status.Gateway == nil || status.Gateway.IsUnspecified() {
+	config *types.NetworkInstanceConfig) error {
+	if config.Gateway == nil || config.Gateway.IsUnspecified() {
 		return fmt.Errorf("gateway is not specified: %+v",
-			status.Gateway)
+			config.Gateway)
 	}
-	if !status.Subnet.Contains(status.Gateway) {
+	if !config.Subnet.Contains(config.Gateway) {
 		return fmt.Errorf("gateway(%s) not within Subnet(%s)",
-			status.Gateway.String(), status.Subnet.String())
+			config.Gateway.String(), config.Subnet.String())
 	}
-	if status.DhcpRange.Contains(status.Gateway) {
+	if config.DhcpRange.Contains(config.Gateway) {
 		return fmt.Errorf("gateway(%s) is in DHCP Range(%v,%v)",
-			status.Gateway, status.DhcpRange.Start,
-			status.DhcpRange.End)
+			config.Gateway, config.DhcpRange.Start,
+			config.DhcpRange.End)
 	}
 	return nil
 }

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)
 				}
 			}
@@ -798,7 +798,7 @@ func (z *zedrouter) processNIReconcileStatus(recStatus nireconciler.NIReconcileS
 		niStatus.BridgeName = recStatus.BrIfName
 		changed = true
 	}
-	if !recStatus.AsyncInProgress {
+	if !recStatus.InProgress {
 		if niStatus.ChangeInProgress != types.ChangeInProgressTypeNone {
 			niStatus.ChangeInProgress = types.ChangeInProgressTypeNone
 			changed = true
@@ -835,10 +835,10 @@ func (z *zedrouter) processAppConnReconcileStatus(
 		}
 		return false
 	}
-	var asyncInProgress bool
+	var inProgress bool
 	var failedItems []string
 	for _, vif := range recStatus.VIFs {
-		asyncInProgress = asyncInProgress || vif.AsyncInProgress
+		inProgress = inProgress || vif.InProgress
 		for itemRef, itemErr := range vif.FailedItems {
 			failedItems = append(failedItems, fmt.Sprintf("%v (%v)", itemRef, itemErr))
 		}
@@ -853,13 +853,9 @@ func (z *zedrouter) processAppConnReconcileStatus(
 			}
 		}
 	}
-	if !asyncInProgress {
-		if appNetStatus.Pending() {
-			changed = true
-		}
-		appNetStatus.PendingAdd = false
-		appNetStatus.PendingModify = false
-		appNetStatus.PendingDelete = false
+	if appNetStatus.ConfigInSync != !inProgress {
+		changed = true
+		appNetStatus.ConfigInSync = !inProgress
 	}
 	if len(failedItems) > 0 {
 		err := fmt.Errorf("failed items: %s", strings.Join(failedItems, ";"))
@@ -1069,6 +1065,11 @@ func (z *zedrouter) publishNetworkInstanceMetricsAll(nms *types.NetworkMetrics)
 	}
 	for _, ni := range niList {
 		status := ni.(types.NetworkInstanceStatus)
+		config := z.lookupNetworkInstanceConfig(status.Key())
+		if config == nil || !config.Activate {
+			// NI was deleted or is inactive - skip metrics publishing.
+			continue
+		}
 		netMetrics := z.createNetworkInstanceMetrics(&status, nms)
 		err := z.pubNetworkInstanceMetrics.Publish(netMetrics.Key(), *netMetrics)
 		if err != nil {