pbacsko commented on code in PR #1047:
URL: https://github.com/apache/yunikorn-core/pull/1047#discussion_r2514740477
##########
pkg/scheduler/objects/quota_change_preemptor.go:
##########
@@ -149,24 +164,39 @@ func (qcp *QuotaChangePreemptionContext) preemptVictims()
{
zap.Int("total victims", len(qcp.allocations)))
apps := make(map[*Application][]*Allocation)
victimsTotalResource := resources.NewResource()
- selectedVictimsTotalResource := resources.NewResource()
+ isGuaranteedAndMaxEquals := qcp.maxResource != nil &&
qcp.guaranteedResource != nil && resources.Equals(qcp.maxResource,
qcp.guaranteedResource)
+ log.Log(log.ShedQuotaChangePreemption).Info("Found victims for quota
change preemption",
+ zap.String("queue", qcp.queue.GetQueuePath()),
+ zap.Int("total victims", len(qcp.allocations)),
+ zap.String("max resources", qcp.maxResource.String()),
+ zap.String("guaranteed resources",
qcp.guaranteedResource.String()),
+ zap.String("allocated resources",
qcp.allocatedResource.String()),
+ zap.String("preemptable resources",
qcp.preemptableResource.String()),
+ zap.Bool("isGuaranteedSet", isGuaranteedAndMaxEquals),
Review Comment:
Nit: "isGuaranteedSet" -> "guaranteed == max"
##########
pkg/scheduler/objects/quota_change_preemptor.go:
##########
@@ -149,24 +164,39 @@ func (qcp *QuotaChangePreemptionContext) preemptVictims()
{
zap.Int("total victims", len(qcp.allocations)))
apps := make(map[*Application][]*Allocation)
victimsTotalResource := resources.NewResource()
- selectedVictimsTotalResource := resources.NewResource()
+ isGuaranteedAndMaxEquals := qcp.maxResource != nil &&
qcp.guaranteedResource != nil && resources.Equals(qcp.maxResource,
qcp.guaranteedResource)
+ log.Log(log.ShedQuotaChangePreemption).Info("Found victims for quota
change preemption",
+ zap.String("queue", qcp.queue.GetQueuePath()),
+ zap.Int("total victims", len(qcp.allocations)),
+ zap.String("max resources", qcp.maxResource.String()),
+ zap.String("guaranteed resources",
qcp.guaranteedResource.String()),
+ zap.String("allocated resources",
qcp.allocatedResource.String()),
+ zap.String("preemptable resources",
qcp.preemptableResource.String()),
+ zap.Bool("isGuaranteedSet", isGuaranteedAndMaxEquals),
+ )
for _, victim := range qcp.allocations {
- // stop collecting the victims once ask resource requirement met
+ if
!qcp.preemptableResource.FitInMaxUndef(victim.GetAllocatedResource()) {
+ continue
+ }
+ application := qcp.queue.GetApplication(victim.applicationID)
+
+ // Keep collecting the victims until preemptable resource
reaches and subtract the usage
if
qcp.preemptableResource.StrictlyGreaterThanOnlyExisting(victimsTotalResource) {
- application :=
qcp.queue.GetApplication(victim.applicationID)
- if _, ok := apps[application]; !ok {
- apps[application] = []*Allocation{}
- }
apps[application] = append(apps[application], victim)
-
selectedVictimsTotalResource.AddTo(victim.GetAllocatedResource())
+
qcp.allocatedResource.SubFrom(victim.GetAllocatedResource())
}
- victimsTotalResource.AddTo(victim.GetAllocatedResource())
- }
- if
qcp.preemptableResource.StrictlyGreaterThanOnlyExisting(victimsTotalResource) ||
-
selectedVictimsTotalResource.StrictlyGreaterThanOnlyExisting(qcp.preemptableResource)
{
- // either there is a shortfall or exceeding little above than
required, so try "best effort" approach later
- return
+ // Has usage gone below the guaranteed resources?
+ // If yes, revert the recently added victim steps completely
and try next victim.
+ if isGuaranteedAndMaxEquals &&
!qcp.allocatedResource.StrictlyGreaterThanOnlyExisting(qcp.guaranteedResource) {
Review Comment:
Why do we check this only if max == guaranteed? An artifical example:
Before undo:
```
max cpu: 11, memory: 11
guaranteed cpu: 10, memory: 10
allocated cpu: 12, memory: 12
last victim cpu: 3, memory: 3
```
After undoing the last victim, we'll end up with `cpu:9, memory:9`
allocated, below the guaranteed. We might not even able to find victims which
preserve the allocated >= guaranteed law. Not sure how we handle this in the
regular preemption though.
##########
pkg/scheduler/objects/quota_change_preemptor.go:
##########
@@ -149,24 +164,39 @@ func (qcp *QuotaChangePreemptionContext) preemptVictims()
{
zap.Int("total victims", len(qcp.allocations)))
apps := make(map[*Application][]*Allocation)
victimsTotalResource := resources.NewResource()
- selectedVictimsTotalResource := resources.NewResource()
+ isGuaranteedAndMaxEquals := qcp.maxResource != nil &&
qcp.guaranteedResource != nil && resources.Equals(qcp.maxResource,
qcp.guaranteedResource)
+ log.Log(log.ShedQuotaChangePreemption).Info("Found victims for quota
change preemption",
+ zap.String("queue", qcp.queue.GetQueuePath()),
+ zap.Int("total victims", len(qcp.allocations)),
+ zap.String("max resources", qcp.maxResource.String()),
+ zap.String("guaranteed resources",
qcp.guaranteedResource.String()),
+ zap.String("allocated resources",
qcp.allocatedResource.String()),
+ zap.String("preemptable resources",
qcp.preemptableResource.String()),
+ zap.Bool("isGuaranteedSet", isGuaranteedAndMaxEquals),
+ )
for _, victim := range qcp.allocations {
- // stop collecting the victims once ask resource requirement met
+ if
!qcp.preemptableResource.FitInMaxUndef(victim.GetAllocatedResource()) {
+ continue
+ }
+ application := qcp.queue.GetApplication(victim.applicationID)
Review Comment:
Unlikely potential race: application gets removed. The time window is very
small, but it can happen.
##########
pkg/scheduler/objects/quota_change_preemptor.go:
##########
@@ -75,7 +85,7 @@ func (qcp *QuotaChangePreemptionContext) tryPreemption() {
// It could contain both positive and negative values. Only negative values
are preemptable.
func (qcp *QuotaChangePreemptionContext) getPreemptableResources()
*resources.Resource {
maxRes := qcp.queue.CloneMaxResource()
Review Comment:
Do we need this clone here? We get maxResource inside
`NewQuotaChangePreemptor()`.
--
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]
