fix(BackingImage): use Spec.DiskFileSpecMap as useddisks when getting ready node and disk for BackingImage

ref: longhorn/longhorn 2856

Signed-off-by: Jack Lin <[email protected]>

Author: ChanYiLin

client/generated_backing_image.go

@@ -11,7 +11,7 @@ type BackingImage struct {
 
 	DeletionTimestamp string `json:"deletionTimestamp,omitempty" yaml:"deletion_timestamp,omitempty"`
 
-	DiskFileStatusMap map[string]interface{} `json:"diskFileStatusMap,omitempty" yaml:"disk_file_status_map,omitempty"`
+	DiskFileStatusMap map[string]BackingImageDiskFileStatus `json:"diskFileStatusMap,omitempty" yaml:"disk_file_status_map,omitempty"`
 
 	DiskSelector []string `json:"diskSelector,omitempty" yaml:"disk_selector,omitempty"`
 

controller/backing_image_controller.go

@@ -326,7 +326,7 @@ func (bic *BackingImageController) replenishBackingImageCopies(bi *longhorn.Back
 		return nil
 	}
 
-	concurrentReplenishLimit, err := bic.ds.GetSettingAsInt(types.SettingNameConcurrentBackingImageReplenishPerNodeLimit)
+	concurrentReplenishLimit, err := bic.ds.GetSettingAsInt(types.SettingNameConcurrentBackingImageCopyReplenishPerNodeLimit)
 	if err != nil {
 		return err
 	}
@@ -337,61 +337,40 @@ func (bic *BackingImageController) replenishBackingImageCopies(bi *longhorn.Back
 	}
 
 	nonFailedCopies := 0
-	usedDisks := map[string]bool{}
 	for diskUUID := range bi.Spec.DiskFileSpecMap {
 		fileStatus, exists := bi.Status.DiskFileStatusMap[diskUUID]
 		if !exists || (fileStatus.State != longhorn.BackingImageStateFailed &&
 			fileStatus.State != longhorn.BackingImageStateFailedAndCleanUp &&
 			fileStatus.State != longhorn.BackingImageStateUnknown) {
-
 			// Non-existing file in status could due to not being synced from backing image manager yet.
 			// Consider it as newly created copy and count it as non-failed copies.
 			// So we don't create extra copy when handling copies evictions.
-			usedDisks[diskUUID] = true
 			nonFailedCopies += 1
 		}
 	}
 
+	// Need to count the evicted copies in the nonFailedCopies then handle it in handleBackingImageCopiesEvictions
+	// so we can distinguish the case of "0 healthy copy" and "there is 1 copy but being evicted".
 	if nonFailedCopies == 0 {
 		return nil
-	} else if nonFailedCopies >= bi.Spec.MinNumberOfCopies {
-		if err := bic.handleBackingImageCopiesEvictions(nonFailedCopies, bi, usedDisks); err != nil {
-			return nil
-		}
-	} else { //nonFailedCopies < bi.Spec.MinNumberOfCopies
-		readyNode, readyDiskName, err := bic.ds.GetReadyNodeDiskForBackingImage(bi, usedDisks)
-		logrus.Infof("replicate the copy to node: %v, disk: %v", readyNode, readyDiskName)
-		if err != nil {
-			logrus.WithError(err).Warnf("failed to create the backing image copy")
-			return nil
-		}
-		// BackingImageManager will then sync the BackingImage to the disk
-		bi.Spec.DiskFileSpecMap[readyNode.Status.DiskStatus[readyDiskName].DiskUUID] = &longhorn.BackingImageDiskFileSpec{}
-	}
-
-	return nil
-}
-
-// handleBackingImageCopiesEvictions do creating one more replica for eviction, if requested
-func (bic *BackingImageController) handleBackingImageCopiesEvictions(nonFailedCopies int, bi *longhorn.BackingImage, usedDisks map[string]bool) (err error) {
-	log := getLoggerForBackingImage(bic.logger, bi)
-	NonEvictingCount := nonFailedCopies
-
-	for _, fileSpec := range bi.Spec.DiskFileSpecMap {
-		if fileSpec.EvictionRequested {
-			NonEvictingCount--
+	} else {
+		nonEvictingCount := nonFailedCopies
+		for _, fileSpec := range bi.Spec.DiskFileSpecMap {
+			if fileSpec.EvictionRequested {
+				nonEvictingCount--
+			}
 		}
-	}
 
-	if NonEvictingCount < bi.Spec.MinNumberOfCopies {
-		log.Infof("Creating one more backing image copy for eviction")
-		readyNode, readyDiskName, err := bic.ds.GetReadyNodeDiskForBackingImage(bi, usedDisks)
-		if err != nil {
-			logrus.WithError(err).Warnf("failed to create the backing image copy")
-			return nil
+		if nonEvictingCount < bi.Spec.MinNumberOfCopies {
+			readyNode, readyDiskName, err := bic.ds.GetReadyNodeDiskForBackingImage(bi)
+			logrus.Infof("replicate the copy to node: %v, disk: %v", readyNode, readyDiskName)
+			if err != nil {
+				logrus.WithError(err).Warnf("failed to create the backing image copy")
+				return nil
+			}
+			// BackingImageManager will then sync the BackingImage to the disk
+			bi.Spec.DiskFileSpecMap[readyNode.Status.DiskStatus[readyDiskName].DiskUUID] = &longhorn.BackingImageDiskFileSpec{}
 		}
-		// BackingImageManager will then sync the BackingImage to the disk
-		bi.Spec.DiskFileSpecMap[readyNode.Status.DiskStatus[readyDiskName].DiskUUID] = &longhorn.BackingImageDiskFileSpec{}
 	}
 
 	return nil
@@ -405,8 +384,10 @@ func (bic *BackingImageController) cleanupEvictionRequestedBackingImageCopies(bi
 	hasNonEvictingHealthyBackingImageCopy := false
 	evictingHealthyBackingImageCopyDiskUUID := ""
 	for diskUUID, fileSpec := range bi.Spec.DiskFileSpecMap {
-		fileStatus, exists := bi.Status.DiskFileStatusMap[diskUUID]
-		if exists && fileStatus != nil {
+		fileStatus := bi.Status.DiskFileStatusMap[diskUUID]
+		if fileStatus == nil { // it is newly added, consider it as non healthy
+			continue
+		} else {
 			if fileStatus.State != longhorn.BackingImageStateReady {
 				continue
 			}
@@ -429,6 +410,10 @@ func (bic *BackingImageController) cleanupEvictionRequestedBackingImageCopies(bi
 			bic.eventRecorder.Event(bi, corev1.EventTypeNormal, constant.EventReasonFailedDeleting, msg)
 			continue
 		}
+		// Backing image controller update the spec here because
+		// only this controller can gather all the information of all the copies of this backing image at once.
+		// By deleting the disk from the spec, backing image manager controller will delete the copy on that disk.
+		// TODO: introduce a new CRD for the backing image copy so we can delete the copy like volume controller deletes replicas.
 		delete(bi.Spec.DiskFileSpecMap, diskUUID)
 		log.Infof("Evicted backing image copy on disk %v", diskUUID)
 	}
@@ -537,9 +522,8 @@ func (bic *BackingImageController) handleBackingImageDataSource(bi *longhorn.Bac
 			}
 		}
 
-		// JackLin: BackingIamge Data Source choose node/disk
 		if !foundReadyDisk {
-			readyNode, readyDiskName, err := bic.ds.GetReadyNodeDiskForBackingImage(bi, map[string]bool{})
+			readyNode, readyDiskName, err := bic.ds.GetReadyNodeDiskForBackingImage(bi)
 			if err != nil {
 				return err
 			}
@@ -670,7 +654,7 @@ func (bic *BackingImageController) handleBackingImageDataSource(bi *longhorn.Bac
 		changeNodeDisk := err != nil || node.Name != bids.Spec.NodeID || node.Spec.Disks[diskName].Path != bids.Spec.DiskPath || node.Status.DiskStatus[diskName].DiskUUID != bids.Spec.DiskUUID
 		if changeNodeDisk {
 			log.Warn("Backing image data source current node and disk is not ready, need to switch to another ready node and disk")
-			readyNode, readyDiskName, err := bic.ds.GetReadyNodeDiskForBackingImage(bi, map[string]bool{})
+			readyNode, readyDiskName, err := bic.ds.GetReadyNodeDiskForBackingImage(bi)
 			if err != nil {
 				return err
 			}
@@ -1002,7 +986,7 @@ func (bic *BackingImageController) enqueueBackingImageForNodeUpdate(oldObj, curr
 		}
 	}
 
-	diskBackingImageMap, err := bic.ds.GetDiskBackingImageMap(oldNode)
+	diskBackingImageMap, err := bic.ds.GetDiskBackingImageMap()
 	if err != nil {
 		utilruntime.HandleError(fmt.Errorf("failed to get disk backing image map when handling node update"))
 		return

controller/backing_image_manager_controller.go

@@ -63,7 +63,7 @@ type BackingImageManagerController struct {
 	versionUpdater func(*longhorn.BackingImageManager) error
 
 	replenishLock             *sync.Mutex
-	inProgressReplenishingMap map[string]struct{}
+	inProgressReplenishingMap map[string]string
 }
 
 type BackingImageManagerMonitor struct {
@@ -132,7 +132,7 @@ func NewBackingImageManagerController(
 		versionUpdater: updateBackingImageManagerVersion,
 
 		replenishLock:             &sync.Mutex{},
-		inProgressReplenishingMap: map[string]struct{}{},
+		inProgressReplenishingMap: map[string]string{},
 	}
 
 	var err error
@@ -360,6 +360,16 @@ func (c *BackingImageManagerController) cleanupBackingImageManager(bim *longhorn
 			}
 		}
 	}
+
+	// delete in progress record of this manager from the inProgressReplenishingMap
+	// each manager only controls only one disk
+	currentDiskUUID := bim.Spec.DiskUUID
+	for biNameDiskID, diskUUID := range c.inProgressReplenishingMap {
+		if currentDiskUUID == diskUUID {
+			delete(c.inProgressReplenishingMap, biNameDiskID)
+		}
+	}
+
 	if c.isMonitoring(bim.Name) {
 		c.stopMonitoring(bim.Name)
 	}
@@ -717,7 +727,7 @@ func (c *BackingImageManagerController) prepareBackingImageFiles(currentBIM *lon
 
 		// Before syncing the backing image copy to this backing image manager,
 		// check if there are more than ReplenishPerNodeLimit number of backing image copies are being synced on this node.
-		canSync, err := c.CanSyncCopy(currentBIM, biName, currentBIM.Spec.NodeID, currentBIM.Spec.DiskUUID)
+		canSync, err := c.canSyncCopy(currentBIM, biName, currentBIM.Spec.NodeID, currentBIM.Spec.DiskUUID)
 		if err != nil {
 			return err
 		}
@@ -1255,9 +1265,9 @@ func (c *BackingImageManagerController) isResponsibleFor(bim *longhorn.BackingIm
 	return isControllerResponsibleFor(c.controllerID, c.ds, bim.Name, bim.Spec.NodeID, bim.Status.OwnerID)
 }
 
-func (c *BackingImageManagerController) CanSyncCopy(bim *longhorn.BackingImageManager, biName, nodeID, diskUUID string) (bool, error) {
+func (c *BackingImageManagerController) canSyncCopy(bim *longhorn.BackingImageManager, biName, nodeID, diskUUID string) (bool, error) {
 	log := getLoggerForBackingImageManager(c.logger, bim)
-	concurrentReplenishLimit, err := c.ds.GetSettingAsInt(types.SettingNameConcurrentBackingImageReplenishPerNodeLimit)
+	concurrentReplenishLimit, err := c.ds.GetSettingAsInt(types.SettingNameConcurrentBackingImageCopyReplenishPerNodeLimit)
 	if err != nil {
 		return false, err
 	}
@@ -1288,7 +1298,7 @@ func (c *BackingImageManagerController) CanSyncCopy(bim *longhorn.BackingImageMa
 			biOnTheSameNodeMap[biNameDiskID] = fileInfo
 
 			if backingImageInProgress(fileInfo.State) {
-				c.inProgressReplenishingMap[biNameDiskID] = struct{}{}
+				c.inProgressReplenishingMap[biNameDiskID] = diskUUID
 			}
 		}
 	}
@@ -1316,7 +1326,7 @@ func (c *BackingImageManagerController) CanSyncCopy(bim *longhorn.BackingImageMa
 		return false, nil
 	}
 
-	c.inProgressReplenishingMap[currentBiNameDiskID] = struct{}{}
+	c.inProgressReplenishingMap[currentBiNameDiskID] = diskUUID
 	return true, nil
 }
 

controller/node_controller.go

@@ -1190,6 +1190,9 @@ func (nc *NodeController) cleanUpBackingImagesInDisks(node *longhorn.Node) error
 			log.WithError(err).Warn("Failed to get the backing image data source when cleaning up the images in disks")
 			continue
 		}
+		if bids == nil {
+			continue
+		}
 		existingBackingImage := bi.DeepCopy()
 		BackingImageDiskFileCleanup(node, bi, bids, waitInterval, bi.Spec.MinNumberOfCopies)
 		if !reflect.DeepEqual(existingBackingImage.Spec, bi.Spec) {
@@ -1591,15 +1594,15 @@ func (nc *NodeController) syncBackingImageEvictionRequested(node *longhorn.Node)
 	}
 	log := getLoggerForNode(nc.logger, node)
 
-	diskBackingImageMap, err := nc.ds.GetDiskBackingImageMap(node)
+	diskBackingImageMap, err := nc.ds.GetDiskBackingImageMap()
 	if err != nil {
 		return err
 	}
 
 	type backingImageToSync struct {
 		*longhorn.BackingImage
-		diskUUID string
-		evict    bool
+		diskUUID          string
+		evictionRequested bool
 	}
 	backingImagesToSync := []backingImageToSync{}
 
@@ -1626,23 +1629,22 @@ func (nc *NodeController) syncBackingImageEvictionRequested(node *longhorn.Node)
 
 	for _, backingImageToSync := range backingImagesToSync {
 		backingImageLog := log.WithField("backingimage", backingImageToSync.Name).WithField("disk", backingImageToSync.diskUUID)
-		if backingImageToSync.evict {
-			backingImageLog.Infof("Requesting backing image copy eviction")
-			if _, err := nc.ds.UpdateBackingImageStatus(backingImageToSync.BackingImage); err != nil {
-				backingImageLog.Warn("Failed to request backing image copy eviction, will enqueue then resync the node")
-				nc.enqueueNodeRateLimited(node)
-				continue
-			}
-			nc.eventRecorder.Eventf(backingImageToSync.BackingImage, corev1.EventTypeNormal, constant.EventReasonEvictionUserRequested, "Requesting backing image %v eviction from node %v and disk %v", backingImageToSync.Name, node.Spec.Name, backingImageToSync.diskUUID)
-		} else {
-			backingImageLog.Infof("Cancelling backing image copy eviction")
-			if _, err := nc.ds.UpdateBackingImageStatus(backingImageToSync.BackingImage); err != nil {
-				backingImageLog.Warn("Failed to cancel backing image copy eviction, will enqueue then resync the node")
-				nc.enqueueNodeRateLimited(node)
-				continue
-			}
-			nc.eventRecorder.Eventf(backingImageToSync.BackingImage, corev1.EventTypeNormal, constant.EventReasonEvictionCanceled, "Requesting backing image %v eviction from node %v and disk %v", backingImageToSync.Name, node.Spec.Name, backingImageToSync.diskUUID)
+		eventReason := constant.EventReasonEvictionCanceled    // Default to "EvictionCanceled"
+		logMessage := "Cancelling backing image copy eviction" // Default message
+
+		if backingImageToSync.evictionRequested {
+			eventReason = constant.EventReasonEvictionUserRequested
+			logMessage = "Requesting backing image copy eviction"
 		}
+
+		backingImageLog.Infof(logMessage)
+		if _, err := nc.ds.UpdateBackingImage(backingImageToSync.BackingImage); err != nil {
+			backingImageLog.Warnf("Failed to %s, will enqueue then resync the node", strings.ToLower(logMessage))
+			nc.enqueueNodeRateLimited(node)
+			continue
+		}
+
+		nc.eventRecorder.Eventf(backingImageToSync.BackingImage, corev1.EventTypeNormal, eventReason, "%s from node %v and disk %v", logMessage, node.Spec.Name, backingImageToSync.diskUUID)
 	}
 
 	return nil

controller/replica_controller.go

@@ -419,11 +419,10 @@ func (rc *ReplicaController) GetBackingImagePathForReplicaStarting(r *longhorn.R
 	if _, exists := bi.Spec.DiskFileSpecMap[r.Spec.DiskID]; !exists {
 		res, err := rc.ds.CanPutBackingImageOnDisk(bi, r.Spec.DiskID)
 		if err != nil {
-			log.Warnf("Failed to check if backing image %v can be put on disk %v", bi.Name, r.Spec.DiskID)
+			log.WithError(err).Warnf("Failed to check if backing image %v can be put on disk %v", bi.Name, r.Spec.DiskID)
 		}
 		if !res {
-			log.Warnf("The backing image %v can not be put on the disk %v", bi.Name, r.Spec.DiskID)
-			return "", nil
+			return "", fmt.Errorf("The backing image %v can not be put on the disk %v", bi.Name, r.Spec.DiskID)
 		}
 		bi.Spec.DiskFileSpecMap[r.Spec.DiskID] = &longhorn.BackingImageDiskFileSpec{}
 		log.Infof("Asking backing image %v to download file to node %v disk %v", bi.Name, r.Spec.NodeID, r.Spec.DiskID)

controller/setting_controller.go

@@ -1713,7 +1713,7 @@ func (info *ClusterInfo) collectSettings() error {
 		types.SettingNameConcurrentAutomaticEngineUpgradePerNodeLimit:             true,
 		types.SettingNameConcurrentBackupRestorePerNodeLimit:                      true,
 		types.SettingNameConcurrentReplicaRebuildPerNodeLimit:                     true,
-		types.SettingNameConcurrentBackingImageReplenishPerNodeLimit:              true,
+		types.SettingNameConcurrentBackingImageCopyReplenishPerNodeLimit:          true,
 		types.SettingNameCRDAPIVersion:                                            true,
 		types.SettingNameCreateDefaultDiskLabeledNodes:                            true,
 		types.SettingNameDefaultDataLocality:                                      true,

controller/system_rollout_controller.go

@@ -1669,7 +1669,7 @@ func (c *SystemRolloutController) restoreServiceAccounts() (err error) {
 var systemRolloutIgnoredSettings = [...]string{
 	string(types.SettingNameConcurrentBackupRestorePerNodeLimit),
 	string(types.SettingNameConcurrentReplicaRebuildPerNodeLimit),
-	string(types.SettingNameConcurrentBackingImageReplenishPerNodeLimit),
+	string(types.SettingNameConcurrentBackingImageCopyReplenishPerNodeLimit),
 	string(types.SettingNameBackupTarget),
 	string(types.SettingNameBackupTargetCredentialSecret),
 }

controller/system_rollout_controller_test.go

@@ -814,19 +814,19 @@ func (s *TestSuite) TestSystemRollout(c *C) {
 			expectState:  longhorn.SystemRestoreStateCompleted,
 
 			existSettings: map[SystemRolloutCRName]*longhorn.Setting{
-				SystemRolloutCRName(types.SettingNameConcurrentReplicaRebuildPerNodeLimit):        {Value: "4"},
-				SystemRolloutCRName(types.SettingNameConcurrentBackupRestorePerNodeLimit):         {Value: "5"},
-				SystemRolloutCRName(types.SettingNameConcurrentBackingImageReplenishPerNodeLimit): {Value: "5"},
+				SystemRolloutCRName(types.SettingNameConcurrentReplicaRebuildPerNodeLimit):            {Value: "4"},
+				SystemRolloutCRName(types.SettingNameConcurrentBackupRestorePerNodeLimit):             {Value: "5"},
+				SystemRolloutCRName(types.SettingNameConcurrentBackingImageCopyReplenishPerNodeLimit): {Value: "5"},
 			},
 			backupSettings: map[SystemRolloutCRName]*longhorn.Setting{
-				SystemRolloutCRName(types.SettingNameConcurrentReplicaRebuildPerNodeLimit):        {Value: "6"},
-				SystemRolloutCRName(types.SettingNameConcurrentBackupRestorePerNodeLimit):         {Value: "7"},
-				SystemRolloutCRName(types.SettingNameConcurrentBackingImageReplenishPerNodeLimit): {Value: "8"},
+				SystemRolloutCRName(types.SettingNameConcurrentReplicaRebuildPerNodeLimit):            {Value: "6"},
+				SystemRolloutCRName(types.SettingNameConcurrentBackupRestorePerNodeLimit):             {Value: "7"},
+				SystemRolloutCRName(types.SettingNameConcurrentBackingImageCopyReplenishPerNodeLimit): {Value: "8"},
 			},
 			expectRestoredSettings: map[SystemRolloutCRName]*longhorn.Setting{
-				SystemRolloutCRName(types.SettingNameConcurrentReplicaRebuildPerNodeLimit):        {Value: "4"},
-				SystemRolloutCRName(types.SettingNameConcurrentBackupRestorePerNodeLimit):         {Value: "5"},
-				SystemRolloutCRName(types.SettingNameConcurrentBackingImageReplenishPerNodeLimit): {Value: "5"},
+				SystemRolloutCRName(types.SettingNameConcurrentReplicaRebuildPerNodeLimit):            {Value: "4"},
+				SystemRolloutCRName(types.SettingNameConcurrentBackupRestorePerNodeLimit):             {Value: "5"},
+				SystemRolloutCRName(types.SettingNameConcurrentBackingImageCopyReplenishPerNodeLimit): {Value: "5"},
 			},
 		},
 		"system rollout Volume exist in cluster": {

datastore/longhorn.go

@@ -2666,7 +2666,7 @@ func (s *DataStore) ListReadyNodesContainingEngineImageRO(image string) (map[str
 
 // GetReadyNodeDiskForBackingImage a list of all Node the in the given namespace and
 // returns the first Node && the first Disk of the Node marked with condition ready and allow scheduling
-func (s *DataStore) GetReadyNodeDiskForBackingImage(backingImage *longhorn.BackingImage, usedDisks map[string]bool) (*longhorn.Node, string, error) {
+func (s *DataStore) GetReadyNodeDiskForBackingImage(backingImage *longhorn.BackingImage) (*longhorn.Node, string, error) {
 	logrus.Info("Preparing to find a random ready node disk")
 	nodes, err := s.ListNodesRO()
 	if err != nil {
@@ -2704,7 +2704,7 @@ func (s *DataStore) GetReadyNodeDiskForBackingImage(backingImage *longhorn.Backi
 			if !types.IsSelectorsInTags(diskSpec.Tags, backingImage.Spec.DiskSelector, allowEmptyDiskSelectorVolume) {
 				continue
 			}
-			if _, exists := usedDisks[diskStatus.DiskUUID]; exists {
+			if _, exists := backingImage.Spec.DiskFileSpecMap[diskStatus.DiskUUID]; exists {
 				continue
 			}
 			// TODO: Jack add block type disk for spdk version BackingImage
@@ -5073,16 +5073,16 @@ func (s *DataStore) IsV2DataEngineDisabledForNode(nodeName string) (bool, error)
 	return false, nil
 }
 
-func (s *DataStore) GetDiskBackingImageMap(node *longhorn.Node) (map[string][]*longhorn.BackingImage, error) {
+func (s *DataStore) GetDiskBackingImageMap() (map[string][]*longhorn.BackingImage, error) {
 	diskBackingImageMap := map[string][]*longhorn.BackingImage{}
 	backingImages, err := s.ListBackingImages()
 	if err != nil {
 		return nil, err
 	}
 
 	for _, bi := range backingImages {
-		for diskUIID := range bi.Status.DiskFileStatusMap {
-			diskBackingImageMap[diskUIID] = append(diskBackingImageMap[diskUIID], bi)
+		for diskUUID := range bi.Status.DiskFileStatusMap {
+			diskBackingImageMap[diskUUID] = append(diskBackingImageMap[diskUUID], bi)
 		}
 	}