[VOL-3628] ATT scenario subscriber add/remove: synchronized flow and tech profile handling + correction clear persistent data

Signed-off-by: mpagenko <michael.pagenkopf@adtran.com>
Change-Id: Id77ba0e8b537b49663439f86b0d84734dba6cf23
diff --git a/VERSION b/VERSION
index 52cd72d..1713aea 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-0.1.13-dev137
+0.1.13-dev138
diff --git a/internal/pkg/onuadaptercore/device_handler.go b/internal/pkg/onuadaptercore/device_handler.go
index 4dce9b5..49bdaaa 100644
--- a/internal/pkg/onuadaptercore/device_handler.go
+++ b/internal/pkg/onuadaptercore/device_handler.go
@@ -652,7 +652,7 @@
 				logger.Errorw("error-updating-device-state", log.Fields{"device-id": dh.deviceID, "error": err})
 			}
 
-			logger.Debugw("DeviceReasonUpdate upon re-enable", log.Fields{
+			logger.Debugw("DeviceReasonUpdate upon disable", log.Fields{
 				"reason": "omci-admin-lock", "device-id": dh.deviceID})
 			// DeviceReason to update acc.to modified py code as per beginning of Sept 2020
 			if err := dh.coreProxy.DeviceReasonUpdate(context.TODO(), dh.deviceID, "omci-admin-lock"); err != nil {
@@ -697,7 +697,11 @@
 		return
 	}
 	if err := pDevEntry.restoreDataFromOnuKvStore(context.TODO()); err != nil {
-		logger.Errorw("reconciling - restoring OnuTp-data failed - abort", log.Fields{"err": err, "device-id": dh.deviceID})
+		if err == fmt.Errorf("no-ONU-data-found") {
+			logger.Debugw("no persistent data found - abort reconciling", log.Fields{"device-id": dh.deviceID})
+		} else {
+			logger.Errorw("reconciling - restoring OnuTp-data failed - abort", log.Fields{"err": err, "device-id": dh.deviceID})
+		}
 		dh.reconciling = false
 		return
 	}
@@ -783,13 +787,14 @@
 	dh.reconciling = false
 }
 
-func (dh *deviceHandler) deleteDevice(device *voltha.Device) error {
-	logger.Debugw("delete-device", log.Fields{"device-id": device.Id, "SerialNumber": device.SerialNumber})
+func (dh *deviceHandler) deleteDevicePersistencyData() error {
+	logger.Debugw("delete device persistency data", log.Fields{"device-id": dh.deviceID})
 
-	pDevEntry := dh.getOnuDeviceEntry(true)
+	pDevEntry := dh.getOnuDeviceEntry(false)
 	if pDevEntry == nil {
-		logger.Errorw("No valid OnuDevice - aborting", log.Fields{"device-id": dh.deviceID})
-		return fmt.Errorf("no valid OnuDevice: %s", dh.deviceID)
+		//IfDevEntry does not exist here, no problem - no persistent data should have been stored
+		logger.Debugw("OnuDevice does not exist - nothing to delete", log.Fields{"device-id": dh.deviceID})
+		return nil
 	}
 	pDevEntry.lockOnuKVStoreMutex()
 	defer pDevEntry.unlockOnuKVStoreMutex()
@@ -1339,6 +1344,7 @@
 	// (but note that the deviceReason may also have changed to e.g. TechProf*Delete_Success in between)
 	if dh.deviceReason != "stopping-openomci" {
 		logger.Debugw("updateInterface-started - stopping-device", log.Fields{"device-id": dh.deviceID})
+
 		//stop all running FSM processing - make use of the DH-state as mirrored in the deviceReason
 		//here no conflict with aborted FSM's should arise as a complete OMCI initialization is assumed on ONU-Up
 		//but that might change with some simple MDS check on ONU-Up treatment -> attention!!!
@@ -1348,6 +1354,8 @@
 			// abort: system behavior is just unstable ...
 			return err
 		}
+		//all stored persitent data are not valid anymore (loosing knowledge abouit the connected ONU)
+		_ = dh.deleteDevicePersistencyData() //ignore possible errors here and continue, hope is that data is synchronized with new ONU-Up
 
 		//deviceEntry stop without omciCC reset here, regarding the OMCI_CC still valid for this ONU
 		// - in contrary to disableDevice - compare with processUniDisableStateDoneEvent
@@ -1441,6 +1449,10 @@
 				//VlanFilterFsm exists and was already started
 				pVlanFilterStatemachine := pVlanFilterFsm.pAdaptFsm.pFsm
 				if pVlanFilterStatemachine != nil {
+					//reset of all Fsm is always accompagnied by global persistency data removal
+					//  no need to remove specific data
+					pVlanFilterFsm.RequestClearPersistency(false)
+					//and reset the UniVlanConfig FSM
 					_ = pVlanFilterStatemachine.Event(vlanEvReset)
 				}
 			}
@@ -2313,7 +2325,7 @@
 		logger.Debugw("reconciling - don't store persistent UniFlowConfig", log.Fields{"device-id": dh.deviceID})
 		return nil
 	}
-	logger.Debugw("Store persistent UniFlowConfig", log.Fields{"device-id": dh.deviceID})
+	logger.Debugw("Store or clear persistent UniFlowConfig", log.Fields{"device-id": dh.deviceID})
 
 	pDevEntry := dh.getOnuDeviceEntry(true)
 	if pDevEntry == nil {
diff --git a/internal/pkg/onuadaptercore/omci_vlan_config.go b/internal/pkg/onuadaptercore/omci_vlan_config.go
index 2fbc5db..36efa13 100644
--- a/internal/pkg/onuadaptercore/omci_vlan_config.go
+++ b/internal/pkg/onuadaptercore/omci_vlan_config.go
@@ -140,6 +140,7 @@
 	omciMIdsResponseReceived    chan bool //seperate channel needed for checking multiInstance OMCI message responses
 	pAdaptFsm                   *AdapterFsm
 	acceptIncrementalEvtoOption bool
+	clearPersistency            bool
 	mutexFlowParams             sync.Mutex
 	uniVlanFlowParamsSlice      []uniVlanFlowParams
 	uniRemoveFlowsSlice         []uniRemoveVlanFlowParams
@@ -173,6 +174,7 @@
 		numUniFlows:                 0,
 		configuredUniFlow:           0,
 		numRemoveFlows:              0,
+		clearPersistency:            true,
 	}
 
 	instFsm.pAdaptFsm = NewAdapterFsm(aName, instFsm.deviceID, aCommChannel)
@@ -299,6 +301,14 @@
 	return nil
 }
 
+//RequestClearPersistency sets the internal flag to not clear persistency data (false=NoClear)
+func (oFsm *UniVlanConfigFsm) RequestClearPersistency(aClear bool) {
+	//mutex protection is required for possible concurrent access to FSM members
+	oFsm.mutexFlowParams.Lock()
+	defer oFsm.mutexFlowParams.Unlock()
+	oFsm.clearPersistency = aClear
+}
+
 //SetUniFlowParams verifies on existence of flow parameters to be configured,
 // optionally udates the cookie list or appends a new flow if there is space
 // if possible the FSM is trigggerd to start with the processing
@@ -470,6 +480,9 @@
 						oFsm.numUniFlows = 0              //no more flows
 						oFsm.configuredUniFlow = 0        //no more flows configured
 						oFsm.uniVlanFlowParamsSlice = nil //reset the slice
+						//at this point it is evident that no flow anymore refers to a still possibly active Techprofile
+						//request that this profile gets deleted before a new flow add is allowed
+						oFsm.pUniTechProf.setProfileToDelete(oFsm.pOnuUniPort.uniID, true)
 						logger.Debugw("UniVlanConfigFsm flow removal - no more flows", log.Fields{
 							"device-id": oFsm.deviceID})
 					} else {
@@ -479,9 +492,28 @@
 							//TODO!! might be needed to consider still outstanding configure requests ..
 							//  so a flow at removal might still not be configured !?!
 						}
+						usedTpID := storedUniFlowParams.VlanRuleParams.TpID
 						//cut off the requested flow by slicing out this element
 						oFsm.uniVlanFlowParamsSlice = append(
 							oFsm.uniVlanFlowParamsSlice[:flow], oFsm.uniVlanFlowParamsSlice[flow+1:]...)
+						//here we have to check, if there are still other flows referencing to the actual ProfileId
+						//  before we can request that this profile gets deleted before a new flow add is allowed
+						tpIDInOtherFlows := false
+						for _, tpUniFlowParams := range oFsm.uniVlanFlowParamsSlice {
+							if tpUniFlowParams.VlanRuleParams.TpID == usedTpID {
+								tpIDInOtherFlows = true
+								break // search loop can be left
+							}
+						}
+						if tpIDInOtherFlows {
+							logger.Debugw("UniVlanConfigFsm tp-id used in deleted flow is still used in other flows", log.Fields{
+								"device-id": oFsm.deviceID, "tp-id": usedTpID})
+						} else {
+							logger.Debugw("UniVlanConfigFsm tp-id used in deleted flow is not used anymore", log.Fields{
+								"device-id": oFsm.deviceID, "tp-id": usedTpID})
+							//request that this profile gets deleted before a new flow add is allowed
+							oFsm.pUniTechProf.setProfileToDelete(oFsm.pOnuUniPort.uniID, true)
+						}
 						logger.Debugw("UniVlanConfigFsm flow removal - specific flow removed from data", log.Fields{
 							"device-id": oFsm.deviceID})
 					}
@@ -943,7 +975,20 @@
 	logger.Debugw("UniVlanConfigFsm enters disabled state", log.Fields{"device-id": oFsm.deviceID})
 	oFsm.pLastTxMeInstance = nil
 	if oFsm.pDeviceHandler != nil {
-		//request removal of 'reference' in the Handler (completely clear the FSM)
+		//TODO: to clarify with improved error treatment for VlanConfigFsm (timeout,reception) errors
+		//  current code removes the complete FSM including all flow/rule configuration done so far
+		//  this might be a bit to much, it would require fully new flow config from rwCore (at least on OnuDown/up)
+		//  maybe a more sophisticated approach is possible without clearing the data
+		if oFsm.clearPersistency {
+			//permanently remove possibly stored persistent data
+			if len(oFsm.uniVlanFlowParamsSlice) > 0 {
+				var emptySlice = make([]uniVlanFlowParams, 0)
+				_ = oFsm.pDeviceHandler.storePersUniFlowConfig(oFsm.pOnuUniPort.uniID, &emptySlice) //ignore errors
+			}
+		} else {
+			logger.Debugw("UniVlanConfigFsm persistency data not cleared", log.Fields{"device-id": oFsm.deviceID})
+		}
+		//request removal of 'reference' in the Handler (completely clear the FSM and its data)
 		go oFsm.pDeviceHandler.RemoveVlanFilterFsm(oFsm.pOnuUniPort)
 	}
 }
diff --git a/internal/pkg/onuadaptercore/onu_device_entry.go b/internal/pkg/onuadaptercore/onu_device_entry.go
index ad683b0..66dfabd 100644
--- a/internal/pkg/onuadaptercore/onu_device_entry.go
+++ b/internal/pkg/onuadaptercore/onu_device_entry.go
@@ -542,8 +542,8 @@
 			logger.Debugw("ONU-data", log.Fields{"sOnuPersistentData": oo.sOnuPersistentData,
 				"device-id": oo.deviceID})
 		} else {
-			logger.Errorw("no ONU-data found", log.Fields{"path": oo.onuKVStorePath, "device-id": oo.deviceID})
-			return fmt.Errorf(fmt.Sprintf("no-ONU-data-found-%s", oo.deviceID))
+			logger.Debugw("no ONU-data found", log.Fields{"path": oo.onuKVStorePath, "device-id": oo.deviceID})
+			return fmt.Errorf("no-ONU-data-found")
 		}
 	} else {
 		logger.Errorw("unable to read from KVstore", log.Fields{"device-id": oo.deviceID})
@@ -572,6 +572,10 @@
 
 func (oo *OnuDeviceEntry) deletePersistentData(ctx context.Context, aProcessingStep uint8) {
 
+	logger.Debugw("delete and clear internal persistency data", log.Fields{"device-id": oo.deviceID})
+	oo.sOnuPersistentData.PersUniConfig = nil                                             //releasing all UniConfig entries to garbage collector
+	oo.sOnuPersistentData = onuPersistentData{0, 0, "", "", "", make([]uniPersConfig, 0)} //default entry
+
 	logger.Debugw("delete ONU-data from KVStore", log.Fields{"device-id": oo.deviceID})
 	err := oo.onuKVStore.Delete(ctx, oo.onuKVStorePath)
 	if err != nil {
@@ -669,6 +673,11 @@
 				//  (during which the flows are removed/re-assigned but the techProf is left active)
 				//and as the TechProfile is regarded as active we have to verify, if some flow configuration still waits on it
 				//  (should not be the case, but should not harm or be more robust ...)
+				// and to be sure, that for some reason the corresponding TpDelete was lost somewhere in history
+				//  we also reset a possibly outstanding delete request - repeated TpConfig is regarded as valid for waiting flow config
+				if oo.baseDeviceHandler.pOnuTP != nil {
+					oo.baseDeviceHandler.pOnuTP.setProfileToDelete(aUniID, false)
+				}
 				go oo.baseDeviceHandler.VerifyVlanConfigRequest(aUniID)
 			}
 			return false //indicate 'no change' - nothing more to do, TechProf inter-adapter message is return with success anyway here
diff --git a/internal/pkg/onuadaptercore/onu_uni_tp.go b/internal/pkg/onuadaptercore/onu_uni_tp.go
index 97b4dbd..2e19274 100644
--- a/internal/pkg/onuadaptercore/onu_uni_tp.go
+++ b/internal/pkg/onuadaptercore/onu_uni_tp.go
@@ -54,6 +54,7 @@
 	techProfileType       string
 	techProfileID         uint16
 	techProfileConfigDone bool
+	techProfileToDelete   bool
 }
 
 type tcontParamStruct struct {
@@ -297,6 +298,7 @@
 	//note the limitation on ID range (probably even more limited) - based on usage within OMCI EntityID
 	onuTP.mapUniTpIndication[aUniID].techProfileID = uint16(profID)
 	onuTP.mapUniTpIndication[aUniID].techProfileConfigDone = false
+	onuTP.mapUniTpIndication[aUniID].techProfileToDelete = false
 	logger.Debugw("tech-profile path indications",
 		log.Fields{"device-id": onuTP.deviceID, "uni-id": aUniID,
 			"profType": onuTP.mapUniTpIndication[aUniID].techProfileType,
@@ -569,9 +571,23 @@
 		if onuTP.mapUniTpIndication[aUniID].techProfileID == aTpID {
 			onuTP.mutexTPState.Lock()
 			defer onuTP.mutexTPState.Unlock()
+			if onuTP.mapUniTpIndication[aUniID].techProfileToDelete {
+				logger.Debugw("TechProfile not relevant for requested flow config - waiting on delete",
+					log.Fields{"device-id": onuTP.deviceID, "uni-id": aUniID})
+				return false //still waiting for removal of this techProfile first
+			}
 			return onuTP.mapUniTpIndication[aUniID].techProfileConfigDone
 		}
 	}
 	//for all other constellations indicate false = Config not done
 	return false
 }
+
+// setProfileToDelete sets the requested techProfile toDelete state (if possible)
+func (onuTP *onuUniTechProf) setProfileToDelete(aUniID uint8, aState bool) {
+	if _, existTP := onuTP.mapUniTpIndication[aUniID]; existTP {
+		onuTP.mutexTPState.Lock()
+		onuTP.mapUniTpIndication[aUniID].techProfileToDelete = aState
+		onuTP.mutexTPState.Unlock()
+	} //else: the state is just ignored (does not exist)
+}
diff --git a/internal/pkg/onuadaptercore/openonu.go b/internal/pkg/onuadaptercore/openonu.go
index 5fbaa31..cdce427 100644
--- a/internal/pkg/onuadaptercore/openonu.go
+++ b/internal/pkg/onuadaptercore/openonu.go
@@ -326,19 +326,15 @@
 
 // Delete_device deletes the given device
 func (oo *OpenONUAC) Delete_device(device *voltha.Device) error {
-	logger.Debugw("Delete_device", log.Fields{"device-id": device.Id})
+	logger.Debugw("Delete_device", log.Fields{"device-id": device.Id, "SerialNumber": device.SerialNumber})
 	if handler := oo.getDeviceHandler(device.Id, false); handler != nil {
-		err := handler.deleteDevice(device)
+		err := handler.deleteDevicePersistencyData()
 		//don't leave any garbage - even in error case
 		oo.deleteDeviceHandlerToMap(handler)
-		if err != nil {
-			return err
-		}
-	} else {
-		logger.Warnw("no handler found for device-deletion", log.Fields{"device-id": device.Id})
-		return fmt.Errorf(fmt.Sprintf("handler-not-found-%s", device.Id))
+		return err
 	}
-	return nil
+	logger.Warnw("no handler found for device-deletion", log.Fields{"device-id": device.Id})
+	return fmt.Errorf(fmt.Sprintf("handler-not-found-%s", device.Id))
 }
 
 //Get_device_details unimplemented