Split higher level cell when allocated bad cells #27

abuccts · 2020-07-08T07:37:09Z

When buddy allocation failed due to bad cells, try to split a higher level cell to get current level cells.

pkg/algorithm/cell.go

zhypku

Add UT?

Fix deletion errors.

codecov-commenter · 2020-07-08T10:38:06Z

Codecov Report

Merging #27 into master will increase coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
+ Coverage   88.92%   89.06%   +0.13%     
==========================================
  Files           8        8              
  Lines        2177     2231      +54     
==========================================
+ Hits         1936     1987      +51     
- Misses        190      192       +2     
- Partials       51       52       +1

Flag	Coverage Δ
#unittests	`89.06% <ø> (+0.13%)`	⬆️

Impacted Files	Coverage Δ
...ft/hivedscheduler/pkg/algorithm/hived_algorithm.go	`85.25% <0.00%> (+0.06%)`	⬆️
...cheduler/pkg/algorithm/topology_aware_scheduler.go	`91.51% <0.00%> (+0.07%)`	⬆️
...ft/hivedscheduler/pkg/algorithm/cell_allocation.go	`93.27% <0.00%> (+0.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4372c8c...fa1912d. Read the comment docs.

hzhua

The variable "l" might be confusing. I suggest to use "checkingLevel".

hzhua · 2020-07-08T08:55:17Z

pkg/algorithm/cell_allocation.go

+// check whether it is safe to split a higher level cell to get current level cells.
+func checkSplitSafety(freeList ChainCellList, freeCellNum map[CellLevel]int32, l CellLevel) bool {


Suggested change

// check whether it is safe to split a higher level cell to get current level cells.

func checkSplitSafety(freeList ChainCellList, freeCellNum map[CellLevel]int32, l CellLevel) bool {

// check whether it is safe to split a higher level cell to get a cell at the checking level.

func checkSplitSafety(freeList ChainCellList, freeCellNum map[CellLevel]int32, checkingLevel CellLevel) bool {

change to currentLevel as the same in buddyAlloc

hzhua · 2020-07-08T08:56:33Z

pkg/algorithm/cell_allocation.go

+		}
+	}
+	// if there exists a higher level cell with splitable num > 0
+	for l++; l <= CellLevel(len(freeList)); l++ {


Suggested change

for l++; l <= CellLevel(len(freeList)); l++ {

for l := CellLevel(checkingLevel)+1 ; l <= CellLevel(len(freeList)); l++ {

yqwang-ms · 2020-07-08T13:53:49Z

pkg/algorithm/cell_allocation.go

+		// check safety
+		if splitableNum[i] < 0 {
+			return false
+		}


When will splitableNum[i] < 0?
Seems we already safety break, and the whole scheduler should crash. #Closed

in a test case, will change to panic later

print more debug info when VC Safety Broken

In reply to: 451561941 [](ancestors = 451561941)

yqwang-ms · 2020-07-08T13:54:32Z

pkg/algorithm/cell_allocation.go

+	var splitableCell Cell
+	splitableNum := map[CellLevel]int32{}
+	for i := CellLevel(len(freeList)); i >= CellLevel(1); i-- {
+		// calculate splitable number


calculate splitable number [](start = 5, length = 26)

Seems we can early stop the "calculate splitable number" to l+1 #Closed

check safety to panic

(Background: The panic will only fail current pod schedule, instead of exit the whole process)
Lower level safety problems seems does not necessary to fail current allocation, do we need to panic here?
@zhypku any suggestion? #Closed

Seems current behaiour is: even if there are safety broken, but if the broken does not impact current pod scheduling, the pod can still be scheduled.
If so, we can insist this behaiour. #Closed

early stop at current level + 1

yqwang-ms · 2020-07-08T13:54:52Z

pkg/algorithm/cell_allocation.go

+// when buddyAlloc allocates bad cells,
+// check whether it is safe to split a higher level cell to get current level cells.
+func checkSplitSafety(freeList ChainCellList, freeCellNum map[CellLevel]int32, l CellLevel) bool {
+	var splitableCell Cell


splitable [](start = 5, length = 9)

typo: splitable -> splittable #Closed

yqwang-ms · 2020-07-08T13:58:36Z

pkg/algorithm/cell_allocation.go

 			freeList, c.cell.GetLevel()), suggestedNodes, ignoreSuggestedNodes, bindings) {
-			return false
+			l := getLowestFreeCellLevel(freeList, c.cell.GetLevel())


getLowestFreeCellLevel(freeList, c.cell.GetLevel()) [](start = 8, length = 51)

As buddyAlloc may modify the freeList too.
Should we reuse the getLowestFreeCellLevel instead of calling here again?
Such as:

l := getLowestFreeCellLevel(freeList, c.cell.GetLevel()) buddyAlloc(..., l, ...) Infof(...l) freeList[l] = CellList{} ``` #Closed

yqwang-ms · 2020-07-08T14:00:00Z

pkg/algorithm/cell_allocation.go

 			freeList, c.cell.GetLevel()), suggestedNodes, ignoreSuggestedNodes, bindings) {
-			return false
+			l := getLowestFreeCellLevel(freeList, c.cell.GetLevel())
+			klog.Infof("Buddy allocation failed due to bad cells, removing level %v free list: %v", l, freeList[l])


Buddy allocation failed due to bad cells, removing level %v free list: %v [](start = 15, length = 73)

Buddy allocation failed due to bad cells for cell level %v with free list %v, skip the level and try to split higher level cells #Closed

yqwang-ms · 2020-07-08T14:02:40Z

pkg/algorithm/cell_allocation.go

@@ -89,21 +90,59 @@ func getLowestFreeCellLevel(freeList ChainCellList, l CellLevel) CellLevel {
 		"even split to the highest level %v", l-1))
 }

+// when buddyAlloc allocates bad cells,


when buddyAlloc allocates bad cells, [](start = 3, length = 36)

after buddyAlloc failed to allocate cells, ... #Closed

Add allocate function to split higher level cells.

Fix test.

Add test case.

zhypku · 2020-07-12T02:23:27Z

pkg/algorithm/cell_allocation.go

+	var splittableCell Cell
+	splittableNum := map[CellLevel]int32{}
+	for i := CellLevel(len(freeList)); i >= CellLevel(1); i-- {
+		// calculate splitable number


splitable -> splittable

pkg/algorithm/cell_allocation.go

zhypku · 2020-07-12T02:31:44Z

pkg/algorithm/cell_allocation.go

@@ -78,6 +79,70 @@ func buddyAlloc(
 	return false
 }

+// after buddyAlloc failed to allocate cells,
+// try to split a higher level cell to get current level cells.


try to split a higher level cell to get current level cells when it won't break safety guarantee?

used Fan's comment

zhypku · 2020-07-12T02:33:22Z

pkg/algorithm/cell_allocation.go

+		}
+		// check safety
+		if splittableNum[i] < 0 {
+			panic(fmt.Sprintf("VC Safety Broken: level %v cell is unsplittable", i))


maybe we should print cell address in the panic log as well, as we cannot see cell chain in this function

zhypku · 2020-07-12T02:42:10Z

pkg/algorithm/cell_allocation.go

+			freeList[l] = freeList[l][cellNum:]
+			splittableNum[l] -= cellNum
+			for sl := l; sl > currentLevel; sl-- {
+				for _, sc := range splitList {


length of splitList is changed during the iteration. Will this impact the correctness of the iteration?

no, range uses copy of the array, here's an explanation

your example will not change the slice itself.
current operation is dangerous, pls use index instead #Closed

zhypku · 2020-07-12T02:45:18Z

pkg/algorithm/hived_algorithm.go

@@ -913,10 +914,15 @@ func (h *HivedAlgorithm) scheduleGuaranteedAffinityGroup(
 	common.SortInt32(gpuNums)
 	lazyPreemptedGroups := h.tryLazyPreempt(virtualPlacement, gpuNums, sr.affinityGroupName)
 	preassignedCells, nonPreassignedCells := virtualPlacement.toBindingPaths(gpuNums, bindings)
+	freeCellNum := map[CellLevel]int32{}


freeCellNumCopy?

and maybe comment why we need a copy (we will make temporary changes to the data structure in the below function call)

zhypku · 2020-07-12T02:49:29Z

pkg/algorithm/cell_allocation.go

@@ -78,6 +79,70 @@ func buddyAlloc(
 	return false
 }

+// after buddyAlloc failed to allocate cells,
+// try to split a higher level cell to get current level cells.
+func splitAlloc(


maybe we need a clearer and easier-to-understand name for this function...

I can't come up with a good name, any suggestion?

@hzhua @yqwang-ms any suggestion? 😂

How about "badCellAvoidanceAlloc"?

But we also aware bad nodes in buddyAlloc. The only difference is that splitAlloc can try more. How about relaxedBuddyAlloc? #Closed

From the perspective of the rule of the algorithm, it indeed is a relaxed version of BuddyAlloc, as it allows to split a free cell higher than the lowest level. Maybe safeRelaxedBuddyAlloc?

@zhypku, sounds good to me.

updated to safeRelaxedBuddyAlloc

Is it possible that a safe split will trigger the merge operation of buddyAlloc? e.g., suppose a L2 cell's L1 cells are all bad cells. the safeRelaxedBuddyAlloc will split the L2 cell (named A), and go ahead to split another L2 cell (B). Since all the L1 cells of A are free cells (albeit bad), the BuddyAlloc will merge them into a L2 cell (i.e., A).
My question is, when will buddyAlloc do the merging?

The free list for cell allocation is not persistent. It is just a copy of the ground truth free list. So even safeRelaxedBuddyAlloc splits a cell, it is not effective immediately. It is effective when we confirm this allocation. In your example, what we truly allocate is an L1 cell from B. So we only split B in the ground truth free list, and won't split A.

zhypku · 2020-07-12T02:50:49Z

pkg/algorithm/hived_algorithm_test.go

+	} else {
+		compareSchedulingResult(t, pod, psr)
+	}
+}


how about adding a case where it could not split but it does?

yqwang-ms · 2020-07-13T02:36:14Z

pkg/algorithm/cell_allocation.go

+		}
+		if cellNum > 0 {
+			splitList := freeList[l][:cellNum]
+			freeList[l] = freeList[l][cellNum:]


freeList[l] = freeList[l][cellNum:] [](start = 3, length = 35)

when expand splitList, freeList may be overwritten #Closed

yqwang-ms · 2020-07-13T07:09:33Z

pkg/algorithm/cell_allocation.go

+			splittableNum[l] -= cellNum
+			for sl := l; sl > currentLevel; sl-- {
+				for _, sc := range splitList {
+					splitList = append(splitList[1:], sc.GetChildren()...)


splitList[1:] [](start = 24, length = 13)

Be aware subslice mem leak #Closed

Resolve comments.

Add free list in panic log when safety is broken.

yqwang-ms · 2020-07-13T10:01:18Z

pkg/algorithm/cell_allocation.go

+					copy(splitList[:], splitList[1:])
+					splitList[len(splitList)-1] = nil
+					splitList = splitList[:len(splitList)-1]
+				}


Too complex, why not use a new slice to store children? Then do 1 swap. #Closed

yqwang-ms · 2020-07-13T10:03:52Z

pkg/algorithm/cell_allocation.go

+			cellNum = splittableNum[l]
+		}
+		if cellNum > 0 {
+			splitList := CellList{}


splitList [](start = 3, length = 9)

splittableList? #Closed

the list is used to really split, not splittable only...

zhypku · 2020-07-13T10:32:46Z

pkg/algorithm/cell_allocation.go

+				for i := len(splitList); i > 0; i-- {
+					splitList = append(splitList, splitList[0].GetChildren()...)
+					copy(splitList[:], splitList[1:])
+					splitList[len(splitList)-1] = nil


Why copy splitList[1:]? This is equivalent:
splitList[0] = nil
splitList = splitList[1:]

zhypku · 2020-07-13T10:33:22Z

pkg/algorithm/hived_algorithm_test.go

@@ -589,6 +614,7 @@ func TestHivedAlgorithm(t *testing.T) {
 	testSuggestedNodes(t, configFilePath)
 	testStatefulPreemption(t, configFilePath)
 	testBadNodes(t, configFilePath)
+	testSplitAlloc(t, configFilePath)


change name accordingly?

Add test case when unable to split due to safety guarantee.

Update.

yqwang-ms · 2020-07-13T13:34:59Z

@zhypku will below leak?:

func removePickedGpus(gpus CellList, indices []int32) CellList {
	for i, index := range indices {
		offset := int32(i)
		if i < len(indices)-1 {
			nextIndex := indices[i+1]
			copy(gpus[index-offset:nextIndex-offset-1], gpus[index+1:nextIndex])
		} else {
			copy(gpus[index-offset:], gpus[index+1:])
		}
	}
	return gpus[:len(gpus)-len(indices)]
}
``` #Closed

zhypku · 2020-07-13T14:13:45Z

@zhypku will below leak?:

func removePickedGpus(gpus CellList, indices []int32) CellList {
	for i, index := range indices {
		offset := int32(i)
		if i < len(indices)-1 {
			nextIndex := indices[i+1]
			copy(gpus[index-offset:nextIndex-offset-1], gpus[index+1:nextIndex])
		} else {
			copy(gpus[index-offset:], gpus[index+1:])
		}
	}
	return gpus[:len(gpus)-len(indices)]
}

Ah, seems it will. Can @abuccts help fix it? Just insert the code before the return statement:

for i := len(gpus) - len(indices); i < len(gpus); i++ {
	gpus[i] = nil
}

Fix memory leak in `removePickedGpus`.

Early stop safety check at current level.

yqwang-ms

yqwang-ms

Split higher level cell when allocated bad cells

f688217

When buddy allocation failed due to bad cells, try to split a higher level cell to get current level cells.

abuccts requested review from zhypku and yqwang-ms July 8, 2020 07:37

fanyangCS mentioned this pull request Jul 8, 2020

Handling cells with hardware failure #23

Closed

4 tasks

yqwang-ms reviewed Jul 8, 2020

View reviewed changes

pkg/algorithm/cell.go Outdated Show resolved Hide resolved

zhypku reviewed Jul 8, 2020

View reviewed changes

fanyangCS requested a review from hzhua July 8, 2020 08:37

Fix deletion errors

e07af20

Fix deletion errors.

hzhua reviewed Jul 8, 2020

View reviewed changes

yqwang-ms reviewed Jul 8, 2020

View reviewed changes

abuccts added 3 commits July 9, 2020 17:03

Add allocate function to split higher level cells

6336665

Add allocate function to split higher level cells.

Fix test

5b25c3c

Fix test.

Add test case

105d544

Add test case.

zhypku reviewed Jul 12, 2020

View reviewed changes

yqwang-ms reviewed Jul 13, 2020

View reviewed changes

abuccts added 2 commits July 13, 2020 16:59

Resolve comments

fc858a0

Resolve comments.

Add free list in panic log when safety is broken

02cc5b0

Add free list in panic log when safety is broken.

yqwang-ms reviewed Jul 13, 2020

View reviewed changes

zhypku reviewed Jul 13, 2020

View reviewed changes

abuccts added 2 commits July 13, 2020 19:03

Add test case when unable to split

342a3c8

Add test case when unable to split due to safety guarantee.

Update

a957de8

Update.

abuccts added 2 commits July 14, 2020 10:33

Fix memory leak in removePickedGpus

6f2dcd9

Fix memory leak in `removePickedGpus`.

Early stop safety check at current level

fa1912d

Early stop safety check at current level.

yqwang-ms approved these changes Jul 14, 2020

View reviewed changes

zhypku approved these changes Jul 14, 2020

View reviewed changes

hzhua approved these changes Jul 15, 2020

View reviewed changes

abuccts merged commit 76ed604 into master Jul 15, 2020

abuccts deleted the xiongyf/handle-bad-cell branch July 15, 2020 05:05

fanyangCS mentioned this pull request Aug 10, 2020

Infinitely retry in intra-VC scheduler due to inconsistent view of bad cells. #25

Closed

		// check whether it is safe to split a higher level cell to get current level cells.
		func checkSplitSafety(freeList ChainCellList, freeCellNum map[CellLevel]int32, l CellLevel) bool {

	for l++; l <= CellLevel(len(freeList)); l++ {
	for l := CellLevel(checkingLevel)+1 ; l <= CellLevel(len(freeList)); l++ {

Split higher level cell when allocated bad cells #27

Split higher level cell when allocated bad cells #27

Conversation

abuccts commented Jul 8, 2020

zhypku left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 8, 2020 • edited Loading

Codecov Report

hzhua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzhua Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

yqwang-ms Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yqwang-ms Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yqwang-ms Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

yqwang-ms Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yqwang-ms Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yqwang-ms Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

yqwang-ms Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

yqwang-ms Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhypku Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yqwang-ms Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhypku Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yqwang-ms Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yqwang-ms Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yqwang-ms Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yqwang-ms Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

yqwang-ms Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 8, 2020 •

edited

Loading

hzhua Jul 8, 2020 •

edited

Loading

yqwang-ms Jul 8, 2020 •

edited

Loading

yqwang-ms Jul 8, 2020 •

edited

Loading

yqwang-ms Jul 14, 2020 •

edited

Loading

yqwang-ms Jul 14, 2020 •

edited

Loading

yqwang-ms Jul 8, 2020 •

edited

Loading

yqwang-ms Jul 8, 2020 •

edited

Loading

yqwang-ms Jul 8, 2020 •

edited

Loading

yqwang-ms Jul 8, 2020 •

edited

Loading

zhypku Jul 12, 2020 •

edited

Loading

yqwang-ms Jul 12, 2020 •

edited

Loading

zhypku Jul 12, 2020 •

edited

Loading

yqwang-ms Jul 13, 2020 •

edited

Loading

yqwang-ms Jul 13, 2020 •

edited

Loading

yqwang-ms Jul 13, 2020 •

edited

Loading

yqwang-ms Jul 13, 2020 •

edited

Loading

yqwang-ms Jul 13, 2020 •

edited

Loading

yqwang-ms commented Jul 13, 2020 •

edited

Loading

zhypku commented Jul 13, 2020 •

edited

Loading