Add support for try and undo allocate #508
Conversation
pkg/quotaplugins/quota-forest/quota-manager/quota/core/forestcontroller.go
Show resolved
Hide resolved
There was a problem hiding this comment.
When I have added the test case below to the pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager_test.go, I have received this error:
--- FAIL: TestQuotaManagerParallelTryUndoAllocation (0.00s)
--- FAIL: TestQuotaManagerParallelTryUndoAllocation/Gold_consumer_2 (0.01s)
/Users/laurentiu.bradin/work/repos/multi-cluster-app-dispatcher/pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager_test.go:512:
Error Trace: quotamanager_test.go:512
Error: Received unexpected error:
failed undo allocate tree name test-tree
Test: TestQuotaManagerParallelTryUndoAllocation/Gold_consumer_2
Messages: No error expected when calling UndoAllocate consumer
FAIL
FAIL github.com/project-codeflare/multi-cluster-app-dispatcher/pkg/quotaplugins/quota-forest/quota-manager/quota 0.233s
FAIL
This points toward an issue of using the TryAllocate / UndoAllocate combo from multiple threads. I have did not spent a lot of time trying to understand the true cause of the issue, but I suspect that the single snapshot you are using in the tree controller might not be a good thing.
While the current MCAD design only has a single thread (ScheduleNext()) and the implementation you are proposing might be sufficient for this case, the implementation from this PR severely limits any future refactorings that might want to use multiple goroutines to increase the paralism and throughput of MCAD. I am strongly suggesting that you improve the overall design of the qm library to account for this use case.
My objection to merging this PR as is bassed upon the limitations exposed by this testcase.
cc: @astefanutti
func TestQuotaManagerParallelTryUndoAllocation(t *testing.T) {
forestName := "unit-test-1"
qmManagerUnderTest := quota.NewManager()
err := qmManagerUnderTest.AddForest(forestName)
assert.NoError(t, err, "No error expected when adding a forest")
testTreeName, err := qmManagerUnderTest.AddTreeFromString(
`{
"kind": "QuotaTree",
"metadata": {
"name": "test-tree"
},
"spec": {
"resourceNames": [
"cpu",
"memory"
],
"nodes": {
"root": {
"parent": "nil",
"hard": "true",
"quota": {
"cpu": "10",
"memory": "256"
}
},
"gold": {
"parent": "root",
"hard": "true",
"quota": {
"cpu": "10",
"memory": "256"
}
}
}
}
}`)
if err != nil {
assert.Fail(t, "No error expected when adding a tree to forest")
}
err = qmManagerUnderTest.AddTreeToForest(forestName, testTreeName)
assert.NoError(t, err, "No error expected when adding a tree from forest")
modeSet := qmManagerUnderTest.SetMode(quota.Normal)
assert.True(t, modeSet, "Setting the mode should not fail.")
// Define the test table
var tests = []struct {
name string
consumer utils.JConsumer
}{
// the table itself
{"Gold consumer 1",
utils.JConsumer{
Kind: "Consumer",
MetaData: utils.JMetaData{
Name: "gold-consumer-data-1",
},
Spec: utils.JConsumerSpec{
ID: "gold-consumer-1",
Trees: []utils.JConsumerTreeSpec{
{
TreeName: testTreeName,
GroupID: "gold",
Request: map[string]int{
"cpu": 4,
"memory": 4,
"gpu": 0,
},
Priority: 0,
CType: 0,
UnPreemptable: false,
},
},
},
},
},
// the table itself
{"Gold consumer 2",
utils.JConsumer{
Kind: "Consumer",
MetaData: utils.JMetaData{
Name: "gold-consumer-data-2",
},
Spec: utils.JConsumerSpec{
ID: "gold-consumer-2",
Trees: []utils.JConsumerTreeSpec{
{
TreeName: testTreeName,
GroupID: "gold",
Request: map[string]int{
"cpu": 4,
"memory": 4,
"gpu": 0,
},
Priority: 0,
CType: 0,
UnPreemptable: false,
},
},
},
},
},
}
// Execute tests in parallel
for _, tc := range tests {
tc := tc // capture range variable
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
// Get list of quota management tree IDs
qmTreeIDs := qmManagerUnderTest.GetTreeNames()
consumerInfo, err := quota.NewConsumerInfo(tc.consumer)
assert.NoError(t, err, "No error expected when building consumer")
assert.Contains(t, qmTreeIDs, tc.consumer.Spec.Trees[0].TreeName)
added, err := qmManagerUnderTest.AddConsumer(consumerInfo)
assert.NoError(t, err, "No error expected when adding consumer")
assert.True(t, added, "Consumer is expected to be added")
response, err := qmManagerUnderTest.TryAllocate(tc.consumer.Spec.Trees[0].TreeName, consumerInfo.GetID())
if !assert.NoError(t, err, "No error expected when calling TryAllocate consumer") {
assert.Equal(t, 0, len(strings.TrimSpace(response.GetMessage())), "A empty response is expected")
assert.True(t, response.IsAllocated(), "The allocation should succeed")
}
//simulate a long running consumer that has quota allocated
time.Sleep(10 * time.Millisecond)
err = qmManagerUnderTest.UndoAllocate(tc.consumer.Spec.Trees[0].TreeName, consumerInfo.GetID())
assert.NoError(t, err, "No error expected when calling UndoAllocate consumer")
})
}
}
|
Added test described in #508 (review) into |
This makes sense. Enhancing the README to explicitly define the usage in concurrent situations and fixing the failing unit test would allow this PR to be merged. |
|
/approve |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asm582, z103cb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-robot
merged commit 55c21c1
into
project-codeflare:main
Two calls are added to try to allocate a consumer, and if unaccepted, to undo the effect of the allocation trial. If the trial is accepted, no further action is needed. Otherwise, the undo has to be called right after the try allocation, without making any calls to change the trees or allocate/deallocate consumers. These operations are intended only during Normal mode.
TryAllocate(treeName, consumerID)UndoAllocate(treeName, consumerID)TryAllocateForest(forestName, consumerID)UndoAllocateForest(forestName, consumerID)