You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2022/12/07 10:01:40 UTC
[GitHub] [yunikorn-core] manirajv06 opened a new pull request, #470: [YUNIKORN-1398] Remove non daemonset reservation from node before allocating daemonset pod
manirajv06 opened a new pull request, #470:
URL: https://github.com/apache/yunikorn-core/pull/470
### What is this PR for?
Cancel non daemon set reservations (if any) on specific node to give preference to daemon sets allocation during upscaling
### What type of PR is it?
* [ ] - Improvement
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1398
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] codecov[bot] commented on pull request #470: [YUNIKORN-1398] Remove non daemonset reservation from node before allocating daemonset pod
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #470:
URL: https://github.com/apache/yunikorn-core/pull/470#issuecomment-1343918181
# [Codecov](https://codecov.io/gh/apache/yunikorn-core/pull/470?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#470](https://codecov.io/gh/apache/yunikorn-core/pull/470?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d53cdd0) into [master](https://codecov.io/gh/apache/yunikorn-core/commit/4ed33b8d7f0784271a1ad85ab600e1110b732df5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4ed33b8) will **decrease** coverage by `0.35%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #470 +/- ##
==========================================
- Coverage 72.88% 72.52% -0.36%
==========================================
Files 67 67
Lines 10057 10098 +41
==========================================
- Hits 7330 7324 -6
- Misses 2482 2528 +46
- Partials 245 246 +1
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-core/pull/470?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/yunikorn-core/pull/470/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `56.33% <0.00%> (-1.85%)` | :arrow_down: |
| [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/yunikorn-core/pull/470/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.32% <0.00%> (ø)` | |
| [pkg/scheduler/partition.go](https://codecov.io/gh/apache/yunikorn-core/pull/470/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `77.15% <0.00%> (-0.59%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #470: [YUNIKORN-1398] Remove non daemonset reservation from node before allocating daemonset pod
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #470:
URL: https://github.com/apache/yunikorn-core/pull/470#discussion_r1044263508
##########
pkg/scheduler/partition_test.go:
##########
@@ -1116,6 +1116,155 @@ func TestRequiredNodeReservation(t *testing.T) {
assertUserGroupResource(t, getTestUserGroup(), res)
}
+// allocate ask request with required node having non daemon set reservations
+func TestRequiredNodeCancelNonDSReservations(t *testing.T) {
+ partition := createQueuesNodes(t)
+ if partition == nil {
+ t.Fatal("partition create failed")
+ }
+ if alloc := partition.tryAllocate(); alloc != nil {
+ t.Fatalf("empty cluster allocate returned allocation: %s", alloc)
+ }
+
+ // override the reservation delay, and cleanup when done
+ objects.SetReservationDelay(10 * time.Nanosecond)
+ defer objects.SetReservationDelay(2 * time.Second)
+
+ // turn off the second node
+ node2 := partition.GetNode(nodeID2)
+ node2.SetSchedulable(false)
+
+ res, err := resources.NewResourceFromConf(map[string]string{"vcore": "7"})
+ assert.NilError(t, err, "failed to create resource")
+
+ // only one resource for alloc fits on a node
+ app := newApplication(appID1, "default", "root.parent.sub-leaf")
+ err = partition.AddApplication(app)
+ assert.NilError(t, err, "failed to add app-1 to partition")
+
+ ask := newAllocationAskRepeat("alloc-1", appID1, res, 2)
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "failed to add ask to app")
+ // calculate the resource size using the repeat request (reuse is possible using proto conversions in ask)
+ res.MultiplyTo(2)
+ assert.Assert(t, resources.Equals(res, app.GetPendingResource()), "pending resource not set as expected")
+ assert.Assert(t, resources.Equals(res, partition.root.GetPendingResource()), "pending resource not set as expected on root queue")
+
+ // the first one should be allocated
+ alloc := partition.tryAllocate()
+ if alloc == nil {
+ t.Fatal("1st allocation did not return the correct allocation")
+ }
+ assert.Equal(t, objects.Allocated, alloc.GetResult(), "allocation result should have been allocated")
+
+ // the second one should be reserved as the 2nd node is not scheduling
+ alloc = partition.tryAllocate()
+ if alloc != nil {
+ t.Fatal("2nd allocation did not return the correct allocation")
+ }
+ // check if updated (must be after allocate call)
+ assert.Equal(t, 1, len(app.GetReservations()), "ask should have been reserved")
+ assert.Equal(t, 1, len(app.GetQueue().GetReservedApps()), "queue reserved apps should be 1")
+
+ res1, err := resources.NewResourceFromConf(map[string]string{"vcore": "1"})
+ assert.NilError(t, err, "failed to create resource")
+
+ // only one resource for alloc fits on a node
+ app1 := newApplication(appID2, "default", "root.parent.sub-leaf")
+ err = partition.AddApplication(app1)
+ assert.NilError(t, err, "failed to add app-2 to partition")
+
+ // required node set on ask
+ ask2 := newAllocationAsk("alloc-2", appID2, res1)
+ ask2.SetRequiredNode(nodeID1)
+ err = app1.AddAllocationAsk(ask2)
+ assert.NilError(t, err, "failed to add ask alloc-2 to app-1")
+
+ alloc = partition.tryAllocate()
+ if alloc == nil {
+ t.Fatal("1st allocation did not return the correct allocation")
+ }
+ assert.Equal(t, objects.Allocated, alloc.GetResult(), "allocation result should have been allocated")
+
+ // earlier app (app1) reservation count should be zero
+ assert.Equal(t, 0, len(app.GetReservations()), "ask should have been reserved")
+ assert.Equal(t, 0, len(app.GetQueue().GetReservedApps()), "queue reserved apps should be 0")
+}
+
+// allocate ask request with required node having daemon set reservations
+func TestRequiredNodeCancelDSReservations(t *testing.T) {
+ partition := createQueuesNodes(t)
+ if partition == nil {
+ t.Fatal("partition create failed")
+ }
+ if alloc := partition.tryAllocate(); alloc != nil {
+ t.Fatalf("empty cluster allocate returned allocation: %s", alloc)
+ }
+
+ // override the reservation delay, and cleanup when done
+ objects.SetReservationDelay(10 * time.Nanosecond)
+ defer objects.SetReservationDelay(2 * time.Second)
+
+ // turn off the second node
+ node2 := partition.GetNode(nodeID2)
+ node2.SetSchedulable(false)
+
+ res, err := resources.NewResourceFromConf(map[string]string{"vcore": "7"})
+ assert.NilError(t, err, "failed to create resource")
+
+ // only one resource for alloc fits on a node
+ app := newApplication(appID1, "default", "root.parent.sub-leaf")
+ err = partition.AddApplication(app)
+ assert.NilError(t, err, "failed to add app-1 to partition")
+
+ ask := newAllocationAskRepeat("alloc-1", appID1, res, 2)
+ ask.SetRequiredNode(nodeID1)
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "failed to add ask to app")
+ // calculate the resource size using the repeat request (reuse is possible using proto conversions in ask)
+ res.MultiplyTo(2)
+ assert.Assert(t, resources.Equals(res, app.GetPendingResource()), "pending resource not set as expected")
+ assert.Assert(t, resources.Equals(res, partition.root.GetPendingResource()), "pending resource not set as expected on root queue")
+
+ // the first one should be allocated
+ alloc := partition.tryAllocate()
+ if alloc == nil {
+ t.Fatal("1st allocation did not return the correct allocation")
+ }
+ assert.Equal(t, objects.Allocated, alloc.GetResult(), "allocation result should have been allocated")
+
+ // the second one should be reserved as the 2nd node is not scheduling
+ alloc = partition.tryAllocate()
+ if alloc != nil {
+ t.Fatal("2nd allocation did not return the correct allocation")
+ }
+ // check if updated (must be after allocate call)
+ assert.Equal(t, 1, len(app.GetReservations()), "ask should have been reserved")
+ assert.Equal(t, 1, len(app.GetQueue().GetReservedApps()), "queue reserved apps should be 1")
+
+ res1, err := resources.NewResourceFromConf(map[string]string{"vcore": "1"})
+ assert.NilError(t, err, "failed to create resource")
+
+ // only one resource for alloc fits on a node
+ app1 := newApplication(appID2, "default", "root.parent.sub-leaf")
+ err = partition.AddApplication(app1)
+ assert.NilError(t, err, "failed to add app-2 to partition")
+
+ // required node set on ask
+ ask2 := newAllocationAsk("alloc-2", appID2, res1)
+ ask2.SetRequiredNode(nodeID1)
+ err = app1.AddAllocationAsk(ask2)
+ assert.NilError(t, err, "failed to add ask alloc-2 to app-1")
+
+ alloc = partition.tryAllocate()
+ if alloc != nil {
+ t.Fatal("3rd allocation did not return the correct allocation")
Review Comment:
Same as for the 2nd allocation message:
"3rd allocation should not return allocation"
##########
pkg/scheduler/partition_test.go:
##########
@@ -1116,6 +1116,155 @@ func TestRequiredNodeReservation(t *testing.T) {
assertUserGroupResource(t, getTestUserGroup(), res)
}
+// allocate ask request with required node having non daemon set reservations
+func TestRequiredNodeCancelNonDSReservations(t *testing.T) {
+ partition := createQueuesNodes(t)
+ if partition == nil {
+ t.Fatal("partition create failed")
+ }
+ if alloc := partition.tryAllocate(); alloc != nil {
+ t.Fatalf("empty cluster allocate returned allocation: %s", alloc)
+ }
+
+ // override the reservation delay, and cleanup when done
+ objects.SetReservationDelay(10 * time.Nanosecond)
+ defer objects.SetReservationDelay(2 * time.Second)
+
+ // turn off the second node
+ node2 := partition.GetNode(nodeID2)
+ node2.SetSchedulable(false)
+
+ res, err := resources.NewResourceFromConf(map[string]string{"vcore": "7"})
+ assert.NilError(t, err, "failed to create resource")
+
+ // only one resource for alloc fits on a node
+ app := newApplication(appID1, "default", "root.parent.sub-leaf")
+ err = partition.AddApplication(app)
+ assert.NilError(t, err, "failed to add app-1 to partition")
+
+ ask := newAllocationAskRepeat("alloc-1", appID1, res, 2)
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "failed to add ask to app")
+ // calculate the resource size using the repeat request (reuse is possible using proto conversions in ask)
+ res.MultiplyTo(2)
+ assert.Assert(t, resources.Equals(res, app.GetPendingResource()), "pending resource not set as expected")
+ assert.Assert(t, resources.Equals(res, partition.root.GetPendingResource()), "pending resource not set as expected on root queue")
+
+ // the first one should be allocated
+ alloc := partition.tryAllocate()
+ if alloc == nil {
+ t.Fatal("1st allocation did not return the correct allocation")
+ }
+ assert.Equal(t, objects.Allocated, alloc.GetResult(), "allocation result should have been allocated")
+
+ // the second one should be reserved as the 2nd node is not scheduling
+ alloc = partition.tryAllocate()
+ if alloc != nil {
+ t.Fatal("2nd allocation did not return the correct allocation")
+ }
+ // check if updated (must be after allocate call)
+ assert.Equal(t, 1, len(app.GetReservations()), "ask should have been reserved")
+ assert.Equal(t, 1, len(app.GetQueue().GetReservedApps()), "queue reserved apps should be 1")
+
+ res1, err := resources.NewResourceFromConf(map[string]string{"vcore": "1"})
+ assert.NilError(t, err, "failed to create resource")
+
+ // only one resource for alloc fits on a node
+ app1 := newApplication(appID2, "default", "root.parent.sub-leaf")
+ err = partition.AddApplication(app1)
+ assert.NilError(t, err, "failed to add app-2 to partition")
+
+ // required node set on ask
+ ask2 := newAllocationAsk("alloc-2", appID2, res1)
+ ask2.SetRequiredNode(nodeID1)
+ err = app1.AddAllocationAsk(ask2)
+ assert.NilError(t, err, "failed to add ask alloc-2 to app-1")
+
+ alloc = partition.tryAllocate()
+ if alloc == nil {
+ t.Fatal("1st allocation did not return the correct allocation")
+ }
+ assert.Equal(t, objects.Allocated, alloc.GetResult(), "allocation result should have been allocated")
+
+ // earlier app (app1) reservation count should be zero
+ assert.Equal(t, 0, len(app.GetReservations()), "ask should have been reserved")
+ assert.Equal(t, 0, len(app.GetQueue().GetReservedApps()), "queue reserved apps should be 0")
+}
+
+// allocate ask request with required node having daemon set reservations
+func TestRequiredNodeCancelDSReservations(t *testing.T) {
+ partition := createQueuesNodes(t)
+ if partition == nil {
+ t.Fatal("partition create failed")
+ }
+ if alloc := partition.tryAllocate(); alloc != nil {
+ t.Fatalf("empty cluster allocate returned allocation: %s", alloc)
+ }
+
+ // override the reservation delay, and cleanup when done
+ objects.SetReservationDelay(10 * time.Nanosecond)
+ defer objects.SetReservationDelay(2 * time.Second)
+
+ // turn off the second node
+ node2 := partition.GetNode(nodeID2)
+ node2.SetSchedulable(false)
+
+ res, err := resources.NewResourceFromConf(map[string]string{"vcore": "7"})
+ assert.NilError(t, err, "failed to create resource")
+
+ // only one resource for alloc fits on a node
+ app := newApplication(appID1, "default", "root.parent.sub-leaf")
+ err = partition.AddApplication(app)
+ assert.NilError(t, err, "failed to add app-1 to partition")
+
+ ask := newAllocationAskRepeat("alloc-1", appID1, res, 2)
+ ask.SetRequiredNode(nodeID1)
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "failed to add ask to app")
+ // calculate the resource size using the repeat request (reuse is possible using proto conversions in ask)
+ res.MultiplyTo(2)
+ assert.Assert(t, resources.Equals(res, app.GetPendingResource()), "pending resource not set as expected")
+ assert.Assert(t, resources.Equals(res, partition.root.GetPendingResource()), "pending resource not set as expected on root queue")
+
+ // the first one should be allocated
+ alloc := partition.tryAllocate()
+ if alloc == nil {
+ t.Fatal("1st allocation did not return the correct allocation")
+ }
+ assert.Equal(t, objects.Allocated, alloc.GetResult(), "allocation result should have been allocated")
+
+ // the second one should be reserved as the 2nd node is not scheduling
+ alloc = partition.tryAllocate()
+ if alloc != nil {
+ t.Fatal("2nd allocation did not return the correct allocation")
Review Comment:
I was thrown off by this message.
Really should be: "2nd allocation should not return allocation"
##########
pkg/scheduler/objects/application.go:
##########
@@ -920,6 +928,47 @@ func (sa *Application) tryAllocate(headRoom *resources.Resource, nodeIterator fu
return nil
}
+func (sa *Application) cancelReservations(reservations []*reservation) bool {
+ for _, res := range reservations {
+ // skip the node
+ if res.ask.GetRequiredNode() != "" {
+ log.Logger().Warn("reservation for ask with required node already exists on the node",
+ zap.String("required node", res.node.NodeID),
+ zap.String("existing ask reservation key", res.getKey()))
+ return false
+ }
+ }
+ var err error
+ var num int
+ // un reserve all the apps that were reserved on the node
+ for _, res := range reservations {
+ if res.app.ApplicationID == sa.ApplicationID {
+ num, err = res.app.unReserveInternal(res.node, res.ask)
Review Comment:
nit: this should be `sa.unReserveInternal()` for clarity
##########
pkg/scheduler/partition_test.go:
##########
@@ -1212,15 +1361,11 @@ func TestPreemptionForRequiredNodeNormalAlloc(t *testing.T) {
partition, app := setupPreemptionForRequiredNode(t)
// now try the allocation again: the normal path
alloc := partition.tryAllocate()
- if alloc == nil {
+ if alloc != nil {
t.Fatal("allocation attempt should have returned an allocation")
}
// check if updated (must be after allocate call)
- assert.Equal(t, 0, len(app.GetReservations()), "ask should have no longer be reserved")
- assert.Equal(t, alloc.GetResult(), objects.AllocatedReserved, "result is not the expected AllocatedReserved")
- assert.Equal(t, alloc.GetReleaseCount(), 0, "released allocations should have been 0")
- assert.Equal(t, alloc.GetAllocationKey(), allocID2, "expected ask alloc-2 to be allocated")
- assertUserGroupResource(t, getTestUserGroup(), resources.NewResourceFromMap(map[string]resources.Quantity{"vcore": 8000}))
+ assert.Equal(t, 1, len(app.GetReservations()), "ask should have no longer be reserved")
Review Comment:
Messages are not correct:
fatal message: "allocations should not have returned an allocation"
assert message: "ask should have been reserved"
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s closed pull request #470: [YUNIKORN-1398] Remove non daemonset reservation from node before allocating daemonset pod
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #470: [YUNIKORN-1398] Remove non daemonset reservation from node before allocating daemonset pod
URL: https://github.com/apache/yunikorn-core/pull/470
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #470: [YUNIKORN-1398] Remove non daemonset reservation from node before allocating daemonset pod
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #470:
URL: https://github.com/apache/yunikorn-core/pull/470#discussion_r1042963395
##########
pkg/scheduler/objects/application.go:
##########
@@ -920,6 +926,49 @@ func (sa *Application) tryAllocate(headRoom *resources.Resource, nodeIterator fu
return nil
}
+func (sa *Application) removeReservations(reservations map[string]*reservation) {
Review Comment:
We need to return a value for the outcome of our cancel action: if another reservation exists with a required node you leave that reservation intact. That fact needs to be logged.
Leaving the reservation will directly cause a nil to be returned by tryNode() that follows as the node is reserved but not for that ask. We then try to reserve the node for this app/ask. Currently we have a limit of 1 reservation per node (node.go @ line 436) so that will fail...
If we leave a reservation we should skip or we need to check if multiple reservations work.
##########
pkg/scheduler/objects/application.go:
##########
@@ -920,6 +926,49 @@ func (sa *Application) tryAllocate(headRoom *resources.Resource, nodeIterator fu
return nil
}
+func (sa *Application) removeReservations(reservations map[string]*reservation) {
Review Comment:
I also think `cancelNodeReservations()` is a better name for this
##########
pkg/scheduler/objects/node.go:
##########
@@ -117,6 +117,16 @@ func (sn *Node) GetAttribute(key string) string {
return sn.attributes[key]
}
+func (sn *Node) getReservations() map[string]*reservation {
Review Comment:
We already have a GetReservations in the node from YUNIKORN-1448. We should re-use that one.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org