Skip to content

[YUNIKORN-2883] Improve sorters & queue_tracker funtion's test coverage #971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[YUNIKORN-2883] Improve sorters & queue_tracker funtion's test coverage #971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions pkg/scheduler/objects/sorters_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,15 +236,15 @@ func TestSortAppsFifo(t *testing.T) {

input["app-1"].askMaxPriority = 3
input["app-3"].askMaxPriority = 5
input["app-2"].SubmissionTime = input["app-3"].SubmissionTime
input["app-2"].SubmissionTime = input["app-3"].SubmissionTime.Add(time.Nanosecond * -5)
input["app-1"].SubmissionTime = input["app-3"].SubmissionTime
list = sortApplications(input, policies.FifoSortPolicy, false, nil)
/*
* apps order: 0, 3, 1, 2
* the resultType of app index is [0, 2, 3, 1]
* app0 with index 0, app1 with index 2, app2 with index 3 and app3 with index 1
* the resultType of app index is [0, 3, 1, 2]
* app0 with index 0, app1 with index 3, app2 with index 1 and app3 with index 2
*/
assertAppList(t, list, []int{0, 2, 3, 1}, "fifo first, priority second")
assertAppList(t, list, []int{0, 3, 1, 2}, "fifo first, priority second")
}

func TestSortAppsPriorityFifo(t *testing.T) {
Expand Down
6 changes: 1 addition & 5 deletions pkg/scheduler/ugm/queue_tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,11 +410,7 @@ func (qt *QueueTracker) canRunApp(hierarchy []string, applicationID string, trac
func (qt *QueueTracker) canBeRemoved() bool {
for _, childQT := range qt.childQueueTrackers {
// quick check to avoid further traversal
if childQT.canBeRemovedInternal() {
if !childQT.canBeRemoved() {
Copy link
Contributor

@ryankert01 ryankert01 Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it meant to do it recursively?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That place seems unreachable.

Copy link
Contributor

@ryankert01 ryankert01 Sep 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you verify it by adding test case with tree that depth >= 3? I think it's for tree traversal

Copy link
Contributor

@ryankert01 ryankert01 Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean len(qt.childQueueTrackers) == 0 on L426 cause this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see how this is going. This function can only remove tree that height=2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already discussion about this code with @manirajv06 please involve him as well.

Copy link
Contributor

@ryankert01 ryankert01 Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gentle ping @manirajv06 , are you available to give us some insight?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check works even for deeper trees. What is not supported is a recursive delete so that is what it prevents from happening.
root -> parent1 .to. parent5 -> child1 .to. child5 -> leaf1 .to. leaf5
The .to. signifies a repeat of the queues. So we have 5 parents under the root. Each with 5 child queues etc.
When we check for a removal we traverse the tree and would do a depth first check. To remove parent1 it has to be empty and have no quota set, that is the original call. Before we go all the way to the leaf at any point we check if the child queues at the current level can be removed.
If any child has a quota set, even without any usage, we cannot remove parent and would return false.
I think the check len(qt.childQueueTrackers) == 0 in canBeRemovedInternal() should not be there as well as it stops going down the tree. I think @SP12893678 and @ryankert01 made the correct remarks to start of the conversation. I think we need to fix that vunction and make it work recursively as it should.

return false
}
} else {
if !childQT.canBeRemovedInternal() {
return false
}
}
Expand Down
26 changes: 26 additions & 0 deletions pkg/scheduler/ugm/queue_tracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,26 @@ func TestCanBeRemoved(t *testing.T) {
removeQT := root.decreaseTrackedResource(strings.Split(queuePath1, configs.DOT), TestApp1, usage1, true)
assert.Assert(t, removeQT)
assert.Assert(t, root.canBeRemoved())

// case: child can not be removed
usage2, err := resources.NewResourceFromConf(map[string]string{"mem": "0", "vcore": "0"})
assert.NilError(t, err)

root.increaseTrackedResource(strings.Split(queuePath1, configs.DOT), TestApp1, user, usage2)
parentQ = root.childQueueTrackers["parent"]
childQ = parentQ.childQueueTrackers["child1"]
assert.Assert(t, !root.canBeRemoved())
assert.Assert(t, !parentQ.canBeRemoved())
assert.Assert(t, !childQ.canBeRemoved())

removeQT = root.decreaseTrackedResource(strings.Split(path5, configs.DOT), TestApp1, usage2, true)
parentQ = root.childQueueTrackers["parent"]
childQ = parentQ.childQueueTrackers["child1"]

assert.Assert(t, !removeQT)
assert.Assert(t, !root.canBeRemoved())
assert.Assert(t, !parentQ.canBeRemoved())
assert.Assert(t, !childQ.canBeRemoved())
}

func TestGetResourceUsageDAOInfo(t *testing.T) {
Expand Down Expand Up @@ -451,3 +471,9 @@ func getQTResource(qt *QueueTracker) map[string]*resources.Resource {
usage := qt.getResourceUsageDAOInfo("")
return internalGetResource(usage, resources)
}

func TestString(t *testing.T) {
assert.Equal(t, none.String(), "none")
assert.Equal(t, user.String(), "user")
assert.Equal(t, group.String(), "group")
}
Loading