Skip to content
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

Jump to bottom

Distinguish between Preemption due to reclamation and fair sharing #2404

Closed
3 tasks
alculquicondor opened this issue Jun 12, 2024 · 5 comments · Fixed by #2411 · May be fixed by #2629
Closed
3 tasks

Distinguish between Preemption due to reclamation and fair sharing #2404

alculquicondor opened this issue Jun 12, 2024 · 5 comments · Fixed by #2411 · May be fixed by #2629
Assignees
@vladikkuzn
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@alculquicondor
Copy link
Contributor

What would you like to be added:

An additional reason in the Preemption condition to indicate that the preemption was due to fair sharing and not regular reclamation.

The current reason for reclamation or fair sharing is InCohort

We could have: InCohortReclamation and InCohortFairSharing.

Why is this needed:

For better visibility

Completion requirements:

This enhancement requires the following artifacts:

  • Design doc
  • API change
  • Docs update

The artifacts should be linked in subsequent comments.
@alculquicondor alculquicondor added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 12, 2024
@vladikkuzn
Copy link
Contributor

/assign

@vladikkuzn vladikkuzn mentioned this issue Jun 14, 2024
Merged
@vladikkuzn
Copy link
Contributor

@alculquicondor

kueue/pkg/scheduler/scheduler.go

Lines 247 to 251 in 6ecaa87

if e.assignment.RepresentativeMode() != flavorassigner.Fit {
if len(e.preemptionTargets) != 0 {
// If preemptions are issued, the next attempt should try all the flavors.
e.LastAssignment = nil
preempted, err := s.preemptor.IssuePreemptions(ctx, &e.Info, e.preemptionTargets, cq)

If Assigment is borrowing then the reason should be InCohortFairSharing? Or should the decision be based on enableFairSharing of Preemptor?

kueue/pkg/scheduler/preemption/preemption.go

Lines 49 to 59 in 45eee46

type Preemptor struct {
client client.Client
recorder record.EventRecorder
workloadOrdering workload.Ordering
enableFairSharing bool
fsStrategies []fsStrategy
// stubs
applyPreemption func(context.Context, *kueue.Workload, string, string) error
}

@alculquicondor
Copy link
Contributor Author

I think the decision is based entirely on the preemptor. If the preemptor fits in the nominal quota of its ClusterQueue, then it's preempting others that are borrowing its quota.
Otherwise, it's past the nominal quota, meaning that it's using resources from the fair share.

Note that whether fair sharing is enabled is global.

/assign @vladikkuzn

@alculquicondor
Copy link
Contributor Author

cc @gabesaba

@trasc
Copy link
Contributor

trasc commented Jun 18, 2024

I think the decision is based entirely on the preemptor. If the preemptor fits in the nominal quota of its ClusterQueue, then it's preempting others that are borrowing its quota.
Otherwise, it's past the nominal quota, meaning that it's using resources from the fair share.

Also, if the preemptor and preemptee are in the same cluster queue we can say it's a simple priority based eviction (no quota is reclaimed from a borrower)

@vladikkuzn vladikkuzn mentioned this issue Jul 16, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vladikkuzn vladikkuzn

Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Milestone
No milestone
3 participants
@alculquicondor @vladikkuzn @trasc