craigcondit commented on code in PR #983:
URL: https://github.com/apache/yunikorn-core/pull/983#discussion_r1811410796
##########
pkg/scheduler/objects/preemption_queue_test.go:
##########
@@ -53,101 +53,156 @@ type res struct {
assertMessages assertMessage
}
-func TestGetPreemptableResource(t *testing.T) {
- // no guaranteed and no usage. so nothing to preempt
+func setupQueueStructure(t *testing.T) (*Queue, *Queue, *Queue, *Queue) {
rootQ, err := createRootQueue(map[string]string{"first": "20"})
assert.NilError(t, err)
- var parentQ, childQ1, childQ2 *Queue
- parentQ, err = createManagedQueue(rootQ, "parent", true,
map[string]string{"first": "15"})
+ parentQ, err := createManagedQueue(rootQ, "parent", true,
map[string]string{"first": "15"})
assert.NilError(t, err)
- childQ1, err = createManagedQueue(parentQ, "child1", false,
map[string]string{"first": "10"})
+ childQ1, err := createManagedQueue(parentQ, "child1", false,
map[string]string{"first": "10"})
assert.NilError(t, err)
- childQ2, err = createManagedQueue(parentQ, "child2", false,
map[string]string{"first": "5"})
+ childQ2, err := createManagedQueue(parentQ, "child2", false,
map[string]string{"first": "5"})
assert.NilError(t, err)
+ return rootQ, parentQ, childQ1, childQ2
+}
- rootPreemptable, pPreemptable, cPreemptable1, cPreemptable2 :=
getPreemptableResource(rootQ, parentQ, childQ1, childQ2)
- assert.Assert(t, rootPreemptable.IsEmpty(), "nothing to preempt as no
guaranteed set and no usage")
- assert.Assert(t, pPreemptable.IsEmpty(), "nothing to preempt as no
guaranteed set and no usage")
- assert.Assert(t, cPreemptable1.IsEmpty(), "nothing to preempt as no
guaranteed set and no usage")
- assert.Assert(t, cPreemptable2.IsEmpty(), "nothing to preempt as no
guaranteed set and no usage")
+func TestGetPreemptableResource(t *testing.T) {
+ guaranteed1 := resource{resources.Multiply(smallestRes, 2),
smallestRes, childRes, smallestRes}
+ guaranteed2 := resource{nil, smallestRes, nil, nil}
+ allocated1 := resource{nil, nil, nil, nil}
+ allocated2 := resource{resources.Multiply(smallestRes, 2),
resources.Multiply(smallestRes, 2), nil, resources.Multiply(smallestRes, 2)}
+ allocated3 := resource{smallestResDoublePlusChildRes,
smallestResPlusChildRes, childRes, smallestRes}
+ allocated4 := resource{smallestResDoublePlusChildResDouble,
smallestResPlusChildResDouble, childResDouble, nil}
- // guaranteed set but no usage. so nothing to preempt
- // clean start for the snapshot: whole hierarchy with guarantee
- smallestRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ preemptable1 := resource{nil, nil, nil, nil}
+ preemptable2 := resource{nil, smallestRes, nil, smallestRes}
+ preemptable3 := resource{childRes, childRes, nil, nil}
+ preemptable4 := resource{resources.Multiply(childRes, 2),
resources.Multiply(childRes, 2), childRes, nil}
+ preemptable5 := resource{resources.Multiply(smallestRes, 2),
smallestRes, nil, smallestRes}
- // clean start for the snapshot: all set guaranteed - new tests
- // add usage to parent + root: use all guaranteed at parent level
- // add usage to child2: use double than guaranteed
- parentQ.guaranteedResource = smallestRes
- rootQ.allocatedResource = resources.Multiply(smallestRes, 2)
- parentQ.allocatedResource = resources.Multiply(smallestRes, 2)
- childQ2.allocatedResource = resources.Multiply(smallestRes, 2)
- rootPreemptable, pPreemptable, cPreemptable1, cPreemptable2 =
getPreemptableResource(rootQ, parentQ, childQ1, childQ2)
- assert.Assert(t, resources.Equals(rootPreemptable,
resources.Multiply(smallestRes, 2)), "usage is equal to guaranteed in root
queue. so nothing to preempt")
- assert.Assert(t, resources.Equals(pPreemptable, smallestRes), "usage
has exceeded twice than guaranteed in parent queue. preemtable resource should
be equal to guaranteed res")
- assert.Assert(t, resources.IsZero(cPreemptable1), "nothing to preempt
as no usage in child1 queue")
- assert.Assert(t, resources.Equals(cPreemptable2, smallestRes), "usage
has exceeded twice than guaranteed in child2 queue. preemtable resource should
be equal to guaranteed res")
+ assertMessage1 := "nothing to preempt as no guaranteed set and no usage"
+ assertMessage2 := "usage has exceeded guaranteed, preemptable resource
should be as expected"
+ assertMessage3 := "usage has extra resource type not defined as part of
guaranteed, that extra should be preemptable"
+ assertMessage4 := "usage has exceeded guaranteed for some queues,
preemptable resources should be as expected"
- rootQ.guaranteedResource = resources.Multiply(smallestRes, 2)
- parentQ.guaranteedResource = smallestRes
- childQ2.guaranteedResource = smallestRes
- rootQ.allocatedResource = nil
- parentQ.allocatedResource = nil
- childQ2.allocatedResource = nil
- childRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"second": 5})
- childQ1.guaranteedResource = childRes
- rootPreemptable, pPreemptable, cPreemptable1, cPreemptable2 =
getPreemptableResource(rootQ, parentQ, childQ1, childQ2)
- assert.Assert(t, rootPreemptable.IsEmpty(), "nothing to preempt as no
usage")
- assert.Assert(t, pPreemptable.IsEmpty(), "nothing to preempt as no
usage")
- assert.Assert(t, cPreemptable1.IsEmpty(), "nothing to preempt as no
usage")
- assert.Assert(t, cPreemptable2.IsEmpty(), "nothing to preempt as no
usage")
+ assertMessageStruct1 := assertMessage{assertMessage1, assertMessage1,
assertMessage1, assertMessage1}
+ assertMessageStruct2 := assertMessage{assertMessage2, assertMessage2,
assertMessage1, assertMessage2}
+ assertMessageStruct3 := assertMessage{assertMessage3, assertMessage3,
assertMessage1, assertMessage1}
+ assertMessageStruct4 := assertMessage{assertMessage3, assertMessage3,
assertMessage4, assertMessage1}
+
+ var tests = []struct {
+ testName string
+ resources res
+ }{
+ {
+ // no guaranteed and no usage. so nothing to preempt
+ testName: "NoGuaranteedNoUsage",
+ resources: res{
+ guaranteed: resource{nil, nil, nil, nil},
+ allocated: allocated1,
+ remaining: preemptable1,
+ assertMessages: assertMessageStruct1,
Review Comment:
This is ugly. It's difficult to see what is being passed in. Even though
there's a bit of code duplication here, setting the values explicitly here
makes the tests clearer.
##########
pkg/scheduler/objects/preemption_queue_test.go:
##########
@@ -53,101 +53,156 @@ type res struct {
assertMessages assertMessage
}
-func TestGetPreemptableResource(t *testing.T) {
- // no guaranteed and no usage. so nothing to preempt
+func setupQueueStructure(t *testing.T) (*Queue, *Queue, *Queue, *Queue) {
rootQ, err := createRootQueue(map[string]string{"first": "20"})
assert.NilError(t, err)
- var parentQ, childQ1, childQ2 *Queue
- parentQ, err = createManagedQueue(rootQ, "parent", true,
map[string]string{"first": "15"})
+ parentQ, err := createManagedQueue(rootQ, "parent", true,
map[string]string{"first": "15"})
assert.NilError(t, err)
- childQ1, err = createManagedQueue(parentQ, "child1", false,
map[string]string{"first": "10"})
+ childQ1, err := createManagedQueue(parentQ, "child1", false,
map[string]string{"first": "10"})
assert.NilError(t, err)
- childQ2, err = createManagedQueue(parentQ, "child2", false,
map[string]string{"first": "5"})
+ childQ2, err := createManagedQueue(parentQ, "child2", false,
map[string]string{"first": "5"})
assert.NilError(t, err)
+ return rootQ, parentQ, childQ1, childQ2
+}
- rootPreemptable, pPreemptable, cPreemptable1, cPreemptable2 :=
getPreemptableResource(rootQ, parentQ, childQ1, childQ2)
- assert.Assert(t, rootPreemptable.IsEmpty(), "nothing to preempt as no
guaranteed set and no usage")
- assert.Assert(t, pPreemptable.IsEmpty(), "nothing to preempt as no
guaranteed set and no usage")
- assert.Assert(t, cPreemptable1.IsEmpty(), "nothing to preempt as no
guaranteed set and no usage")
- assert.Assert(t, cPreemptable2.IsEmpty(), "nothing to preempt as no
guaranteed set and no usage")
+func TestGetPreemptableResource(t *testing.T) {
+ guaranteed1 := resource{resources.Multiply(smallestRes, 2),
smallestRes, childRes, smallestRes}
+ guaranteed2 := resource{nil, smallestRes, nil, nil}
+ allocated1 := resource{nil, nil, nil, nil}
+ allocated2 := resource{resources.Multiply(smallestRes, 2),
resources.Multiply(smallestRes, 2), nil, resources.Multiply(smallestRes, 2)}
+ allocated3 := resource{smallestResDoublePlusChildRes,
smallestResPlusChildRes, childRes, smallestRes}
+ allocated4 := resource{smallestResDoublePlusChildResDouble,
smallestResPlusChildResDouble, childResDouble, nil}
- // guaranteed set but no usage. so nothing to preempt
- // clean start for the snapshot: whole hierarchy with guarantee
- smallestRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ preemptable1 := resource{nil, nil, nil, nil}
+ preemptable2 := resource{nil, smallestRes, nil, smallestRes}
+ preemptable3 := resource{childRes, childRes, nil, nil}
+ preemptable4 := resource{resources.Multiply(childRes, 2),
resources.Multiply(childRes, 2), childRes, nil}
+ preemptable5 := resource{resources.Multiply(smallestRes, 2),
smallestRes, nil, smallestRes}
- // clean start for the snapshot: all set guaranteed - new tests
- // add usage to parent + root: use all guaranteed at parent level
- // add usage to child2: use double than guaranteed
- parentQ.guaranteedResource = smallestRes
- rootQ.allocatedResource = resources.Multiply(smallestRes, 2)
- parentQ.allocatedResource = resources.Multiply(smallestRes, 2)
- childQ2.allocatedResource = resources.Multiply(smallestRes, 2)
- rootPreemptable, pPreemptable, cPreemptable1, cPreemptable2 =
getPreemptableResource(rootQ, parentQ, childQ1, childQ2)
- assert.Assert(t, resources.Equals(rootPreemptable,
resources.Multiply(smallestRes, 2)), "usage is equal to guaranteed in root
queue. so nothing to preempt")
- assert.Assert(t, resources.Equals(pPreemptable, smallestRes), "usage
has exceeded twice than guaranteed in parent queue. preemtable resource should
be equal to guaranteed res")
- assert.Assert(t, resources.IsZero(cPreemptable1), "nothing to preempt
as no usage in child1 queue")
- assert.Assert(t, resources.Equals(cPreemptable2, smallestRes), "usage
has exceeded twice than guaranteed in child2 queue. preemtable resource should
be equal to guaranteed res")
+ assertMessage1 := "nothing to preempt as no guaranteed set and no usage"
+ assertMessage2 := "usage has exceeded guaranteed, preemptable resource
should be as expected"
+ assertMessage3 := "usage has extra resource type not defined as part of
guaranteed, that extra should be preemptable"
+ assertMessage4 := "usage has exceeded guaranteed for some queues,
preemptable resources should be as expected"
- rootQ.guaranteedResource = resources.Multiply(smallestRes, 2)
- parentQ.guaranteedResource = smallestRes
- childQ2.guaranteedResource = smallestRes
- rootQ.allocatedResource = nil
- parentQ.allocatedResource = nil
- childQ2.allocatedResource = nil
- childRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"second": 5})
- childQ1.guaranteedResource = childRes
- rootPreemptable, pPreemptable, cPreemptable1, cPreemptable2 =
getPreemptableResource(rootQ, parentQ, childQ1, childQ2)
- assert.Assert(t, rootPreemptable.IsEmpty(), "nothing to preempt as no
usage")
- assert.Assert(t, pPreemptable.IsEmpty(), "nothing to preempt as no
usage")
- assert.Assert(t, cPreemptable1.IsEmpty(), "nothing to preempt as no
usage")
- assert.Assert(t, cPreemptable2.IsEmpty(), "nothing to preempt as no
usage")
+ assertMessageStruct1 := assertMessage{assertMessage1, assertMessage1,
assertMessage1, assertMessage1}
+ assertMessageStruct2 := assertMessage{assertMessage2, assertMessage2,
assertMessage1, assertMessage2}
+ assertMessageStruct3 := assertMessage{assertMessage3, assertMessage3,
assertMessage1, assertMessage1}
+ assertMessageStruct4 := assertMessage{assertMessage3, assertMessage3,
assertMessage4, assertMessage1}
+
+ var tests = []struct {
+ testName string
+ resources res
+ }{
+ {
+ // no guaranteed and no usage. so nothing to preempt
+ testName: "NoGuaranteedNoUsage",
+ resources: res{
+ guaranteed: resource{nil, nil, nil, nil},
+ allocated: allocated1,
+ remaining: preemptable1,
+ assertMessages: assertMessageStruct1,
+ },
+ },
+ {
+ // yes guaranteed and no usage. so nothing to preempt
+ testName: "YesGuaranteedNoUsage",
+ resources: res{
+ guaranteed: guaranteed1,
+ allocated: resource{nil, nil, nil, nil},
+ remaining: preemptable1,
+ assertMessages: assertMessageStruct1,
+ },
+ },
+ {
+ // add usage to parent + root: use all guaranteed at
parent level
+ // add usage to child2: use double than guaranteed
+ // usage has exceeded guaranteed for some queues,
preemptable resources should be as expected
+ testName: "GuaranteedSetUsageExceeded",
+ resources: res{
+ guaranteed: guaranteed2,
+ allocated: allocated2,
+ remaining: preemptable5,
+ assertMessages: assertMessageStruct2,
+ },
+ },
+ {
+ // add usage to parent + root: use all guaranteed at
parent level
+ // add usage to child2: use double than guaranteed
+ // usage has exceeded guaranteed for some queues,
preemptable resources should be as expected
+ testName: "GuaranteedSetUsageExceeded",
+ resources: res{
+ guaranteed: guaranteed1,
+ allocated: allocated2,
+ remaining: preemptable2,
+ assertMessages: assertMessageStruct2,
+ },
+ },
+ {
+ // add usage for all: use exactly guaranteed at parent
and child level
+ // parent guarantee used for one type child guarantee
used for second type
+ testName: "GuaranteedSetExactUsage",
+ resources: res{
+ guaranteed: guaranteed1,
+ allocated: allocated3,
+ remaining: preemptable3,
+ assertMessages: assertMessageStruct3,
+ },
+ },
+ {
+ // add usage for root + parent: use exactly guaranteed
at parent and child level
+ // add usage to child1: use double than guaranteed
+ // parent guarantee used for one type child guarantee
used for second type
+ testName: "GuaranteedSetMixedUsage",
+ resources: res{
+ guaranteed: guaranteed1,
+ allocated: allocated4,
+ remaining: preemptable4,
+ assertMessages: assertMessageStruct4,
+ },
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.testName, func(t *testing.T) {
+ rootQ, parentQ, childQ1, childQ2 :=
setupQueueStructure(t)
+ assertPreemptable(t, rootQ, parentQ, childQ1, childQ2,
tt.resources)
Review Comment:
Don't extract assertPreemptable() here, but leave the logic inline. This
makes it *much* easier to see where failures occur, as stack traces will show
this instead of the generic line in assertPreemptable().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
