feat(backingimage): backingimage ha eviction enhancement #2742

ChanYiLin · 2024-04-25T09:59:54Z

ref: longhorn/longhorn#2856

Add minNumberOfCopies
Add nodeSelector, diskSelector
Change the Spec.Disks to Spec.DiskFileSpecMap and Add evictionRequested
Periodically replenish the copies if the number of healthy copies less than the minNumberOfCopies or eviction situation
- replenishBackingImageCopies()
  - if nonFailedCopies >= MinNumberOfCopies, we check if we need to replenish one copy for eviction
    - replenish one if NonEvictingCount < MinNumberOfCopies
  - if nonFailedCopies < MinNumberOfCopies
    - replenish one copy to meet the MinNumberOfCopies requirement.
Periodically delete evicted copies
- cleanupEvictionRequestedBackingImageCopies()
  - If there is no non evicted healthy copy, don't delete the copy.
  - Otherwise, delete the evicted copy.
Add concurrent-backing-image-replenish-per-node-limit to limit the number of BackingImage being synced on the same node at the same time
- if the number = 0, longhorn won't replenish the BackingImage to new node/disk
Adjust replica scheduler, replica won't be scheduled to the node where BackingImage can not be stored.

mergify · 2024-04-25T10:00:34Z

This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏

ChanYiLin · 2024-04-26T07:40:15Z

This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏

fixed.

ChanYiLin · 2024-04-26T09:32:52Z

Hi @shuo-wu
I don't know the context why we always keep the first prepared file when cleanup backing image file on the disk in node controller here
I think maybe there is a case when a user tries to evict the file on that node.
Besides, the first prepared file has nothing special
If all the files are failed, we still cleanup the bids.Spec.NodeID and bids.Spec.DiskUUID to restart the prepare process.

So I changed the logic to

only starts to cleanup backing image file on the disk when the file is transferred
it is okay to clean up the file on the first prepared file

WDYT?
Thansk

mergify · 2024-04-27T02:32:16Z

This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏

ChanYiLin · 2024-05-02T06:45:44Z

This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏

fixed.

mergify · 2024-05-02T15:55:01Z

This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏

ChanYiLin · 2024-05-23T07:07:10Z

This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏

fixed.

ref: longhorn/longhorn 2856 Signed-off-by: Jack Lin <jack.lin@suse.com>

shuo-wu · 2024-06-04T03:38:32Z

client/generated_backing_image.go

@@ -11,12 +11,18 @@ type BackingImage struct {

 	DeletionTimestamp string `json:"deletionTimestamp,omitempty" yaml:"deletion_timestamp,omitempty"`

-	DiskFileStatusMap map[string]BackingImageDiskFileStatus `json:"diskFileStatusMap,omitempty" yaml:"disk_file_status_map,omitempty"`
+	DiskFileStatusMap map[string]interface{} `json:"diskFileStatusMap,omitempty" yaml:"disk_file_status_map,omitempty"`


You need to revert this automatically generated code

shuo-wu · 2024-06-04T03:40:12Z

manager/backingimage.go

+	existingBI := bi.DeepCopy()
+	defer func() {
+		if err == nil {
+			if !reflect.DeepEqual(bi.Spec, existingBI.Spec) {
+				bi, err = m.ds.UpdateBackingImage(bi)
+				return
+			}
+		}
+	}()
+
+	bi.Spec.MinNumberOfCopies = minNumberOfCopies


NIT: There is no need to do deep copy for this field check & update

shuo-wu · 2024-06-04T03:41:33Z

types/setting.go

+		Type:        SettingTypeInt,
+		Required:    true,
+		ReadOnly:    false,
+		Default:     "1",


is it better to use a larger value here? For example, 3

shuo-wu · 2024-06-04T04:54:02Z

controller/replica_controller.go

+		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)
+		}
+		if !res {
+			log.Warnf("The backing image %v can not be put on the disk %v", bi.Name, r.Spec.DiskID)
+			return "", nil
+		}


Probably it's better to return an error when there is no result. Then the call can set the replica condition to false rather than keeping waiting there

shuo-wu · 2024-06-04T06:38:31Z

datastore/longhorn.go

@@ -4975,6 +5003,22 @@ func (s *DataStore) IsV2DataEngineDisabledForNode(nodeName string) (bool, error)
 	return false, nil
 }

+func (s *DataStore) GetDiskBackingImageMap(node *longhorn.Node) (map[string][]*longhorn.BackingImage, error) {


The input is not used.

shuo-wu · 2024-06-04T07:16:14Z

datastore/longhorn.go

 // returns the first Node && the first Disk of the Node marked with condition ready and allow scheduling
-func (s *DataStore) GetRandomReadyNodeDisk() (*longhorn.Node, string, error) {
+func (s *DataStore) GetReadyNodeDiskForBackingImage(backingImage *longhorn.BackingImage, usedDisks map[string]bool) (*longhorn.Node, string, error) {


NIT: No need to input usedDisks. The info is already in backingImage

shuo-wu · 2024-06-04T07:22:27Z

controller/node_controller.go

+		if diskSpec.EvictionRequested || node.Spec.EvictionRequested {
+			for _, backingImage := range diskBackingImageMap[diskUUID] {
+				// trigger eviction request
+				backingImage.Status.DiskFileStatusMap[diskUUID].EvictionRequested = true


~~Wait... Should we add field EvictionRequested in backingImage.Status.DiskFileStatusMap?~~
~~I prefer to:~~
~~1. Not update Backing Image Status in a controller other than BackingImageController~~
~~2. If possible, evict a backing image disk file by adding this field to backingImage.Spec.Disks. Can the implementation be similar to that of replicas?~~

You already add backingImage.Spec.DiskFileSpecMap[diskUUID].EvictionRequested in the 2nd commit. Then you should call API UpdateBackingImage instead.

ChanYiLin requested review from derekbit and shuo-wu April 25, 2024 09:59

ChanYiLin self-assigned this Apr 25, 2024

ChanYiLin force-pushed the LH2856_BackingImage_HA branch 3 times, most recently from f6f2738 to cb486bd Compare April 26, 2024 07:21

ChanYiLin force-pushed the LH2856_BackingImage_HA branch 2 times, most recently from 93b1320 to d1e592d Compare April 26, 2024 09:27

ChanYiLin marked this pull request as ready for review April 26, 2024 09:33

ChanYiLin force-pushed the LH2856_BackingImage_HA branch 2 times, most recently from a598b5f to 260a658 Compare April 26, 2024 15:46

ChanYiLin force-pushed the LH2856_BackingImage_HA branch 5 times, most recently from d17ffbf to b10b965 Compare April 29, 2024 09:55

ChanYiLin mentioned this pull request Apr 30, 2024

feat(backingimage): add backing image ha, eviction and selector testing longhorn/longhorn-tests#1877

Draft

ChanYiLin force-pushed the LH2856_BackingImage_HA branch from b10b965 to bd8a313 Compare May 2, 2024 06:45

ChanYiLin mentioned this pull request May 2, 2024

[FEATURE] HA backing image longhorn/longhorn#2856

Open

ChanYiLin force-pushed the LH2856_BackingImage_HA branch 3 times, most recently from 5bcae3b to 72fc205 Compare May 2, 2024 07:52

ChanYiLin force-pushed the LH2856_BackingImage_HA branch 2 times, most recently from 2226465 to 5d5160f Compare May 23, 2024 07:06

innobead self-requested a review May 23, 2024 07:11

ChanYiLin force-pushed the LH2856_BackingImage_HA branch from 5d5160f to f019cea Compare May 24, 2024 08:54

ChanYiLin marked this pull request as draft May 28, 2024 13:13

feat(backingimage): backingimage ha eviction enhancement

4dbc627

ref: longhorn/longhorn 2856 Signed-off-by: Jack Lin <jack.lin@suse.com>

ChanYiLin force-pushed the LH2856_BackingImage_HA branch 2 times, most recently from 3b208ac to 0799c06 Compare May 30, 2024 04:26

ChanYiLin marked this pull request as ready for review May 30, 2024 06:49

feat(backingimage): add new diskFileSpecMap to backingimage

6566ee6

ref: longhorn/longhorn 2856 Signed-off-by: Jack Lin <jack.lin@suse.com>

ChanYiLin force-pushed the LH2856_BackingImage_HA branch from 0799c06 to 6566ee6 Compare May 30, 2024 07:13

shuo-wu reviewed Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backingimage): backingimage ha eviction enhancement #2742

feat(backingimage): backingimage ha eviction enhancement #2742

ChanYiLin commented Apr 25, 2024 •

edited

mergify bot commented Apr 25, 2024

ChanYiLin commented Apr 26, 2024

ChanYiLin commented Apr 26, 2024 •

edited

mergify bot commented Apr 27, 2024

ChanYiLin commented May 2, 2024

mergify bot commented May 2, 2024

ChanYiLin commented May 23, 2024

shuo-wu Jun 4, 2024

shuo-wu Jun 4, 2024

shuo-wu Jun 4, 2024

shuo-wu Jun 4, 2024

shuo-wu Jun 4, 2024

shuo-wu Jun 4, 2024

shuo-wu Jun 4, 2024

feat(backingimage): backingimage ha eviction enhancement #2742

Are you sure you want to change the base?

feat(backingimage): backingimage ha eviction enhancement #2742

Conversation

ChanYiLin commented Apr 25, 2024 • edited

mergify bot commented Apr 25, 2024

ChanYiLin commented Apr 26, 2024

ChanYiLin commented Apr 26, 2024 • edited

mergify bot commented Apr 27, 2024

ChanYiLin commented May 2, 2024

mergify bot commented May 2, 2024

ChanYiLin commented May 23, 2024

shuo-wu Jun 4, 2024

Choose a reason for hiding this comment

shuo-wu Jun 4, 2024

Choose a reason for hiding this comment

shuo-wu Jun 4, 2024

Choose a reason for hiding this comment

shuo-wu Jun 4, 2024

Choose a reason for hiding this comment

shuo-wu Jun 4, 2024

Choose a reason for hiding this comment

shuo-wu Jun 4, 2024

Choose a reason for hiding this comment

shuo-wu Jun 4, 2024

Choose a reason for hiding this comment

ChanYiLin commented Apr 25, 2024 •

edited

ChanYiLin commented Apr 26, 2024 •

edited