Define FairShare Score for Cohorts; Generalize to Hierarchical Case

[gabesaba]

What type of PR is this?

/kind feature

What this PR does / why we need it:

We define FairShare (DominantResourceShare) score for Cohorts.

We additionally support the case where a CQ or Cohort has some lendable resource available to it, but not in its direct parent. Rather than counting only lendable resources in the parent Cohort, we use the potentiallyAvailable function to see how much capacity is available to that Cohort, when it borrows from its parent. While this doesn't differentiate capacity available in the parent Cohort's subtree, versus capacity that the parent Cohort borrows, the FairShare score should only be used for local comparisons: comparing two children of the same node. Therefore, this will serve as a valid denominator.

Which issue(s) this PR fixes:

Part of #3759

Special notes for your reviewer:

No release note for this change, as we will do a single release note for the entire feature.

Does this PR introduce a user-facing change?

NONE

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	ca44b8a
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67b5b47728e7b80008893169
😎 Deploy Preview	https://deploy-preview-4313--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mimowo]

ack

[mimowo]

The code itself lgtm, but the impact of the change is non-obvious to me. Let me ask some clarifying questions.

Rather than counting only lendable resources in the parent Cohort, we use the potentiallyAvailable function

Is this change transparent to the flat cohorts? If so, why? I see all tests passing , but wondering if there might be some corner cases we change relative to the current logic, impacting users of FairSharing with flat cohorts.

No release note for this change, as we will do a single release note for the entire feature.

What will remain missing to complete fair sharing with hierarchical cohorts?

Is this PR adding some functionality which could already be tested in scheduler_test.go? If so then I believe it would nicely demonstrate the impact of the change.

[gabesaba]

The code itself lgtm, but the impact of the change is non-obvious to me. Let me ask some clarifying questions.

Rather than counting only lendable resources in the parent Cohort, we use the potentiallyAvailable function

Is this change transparent to the flat cohorts? If so, why? I see all tests passing , but wondering if there might be some corner cases we change relative to the current logic, impacting users of FairSharing with flat cohorts.

Yes, it does not change behavior in flat Cohorts. This is because we're solving for the case where there may be some lendable resource available, but not in the direct parent. In the flat case - it has to be available in the direct parent, or the CQ should not be above its nominal quota anyway

No release note for this change, as we will do a single release note for the entire feature.

What will remain missing to complete fair sharing with hierarchical cohorts?

We need to use this function to help with admission, and during preemption. The algorithm is more involved than described in the KEP - it is not as simple as finding the lowest DominantResource score, and admitting that CQ - as in that case the Cohort that the CQ is part of could already be out of balance, and admission to that CQ may make the situation worse.

Is this PR adding some functionality which could already be tested in scheduler_test.go? If so then I believe it would nicely demonstrate the impact of the change.

Not yet. The scheduling and preemption changes are rather involved, and I plan to send them out as separate PRs

[mimowo]

but not in the direct parent. In the flat case - it has to be available in the direct parent, or the CQ should not be above its nominal quota anyway

This is my focus for this PR: to make sure we don't break what is already working, or if we fix, then we have a release not. I think this is subtle. Looking at the old code:

func (r ResourceNode) calculateLendable() map[corev1.ResourceName]int64 {
        lendable := make(map[corev1.ResourceName]int64, len(r.SubtreeQuota))
        for fr, q := range r.SubtreeQuota {
                lendable[fr.Resource] += q
        }
        return lendable
}

So, IIUC for a CQ this is equal to Nominal based on

kueue/pkg/cache/resource_node.go

Line 163 in 260070f

cq.resourceNode.SubtreeQuota[fr] = quota.Nominal

In the new code we have we add potentialAvailable, so :

func potentialAvailable(node hierarchicalResourceNode, fr resources.FlavorResource) int64 {
	r := node.getResourceNode()
	if !node.HasParent() {
		return r.SubtreeQuota[fr]    // returned for cohort
	}
	available := r.guaranteedQuota(fr) + potentialAvailable(node.parentHRN(), fr)
	if borrowingLimit := r.Quotas[fr].BorrowingLimit; borrowingLimit != nil {
		maxWithBorrowing := r.SubtreeQuota[fr] + *borrowingLimit
		available = min(maxWithBorrowing, available)
	}
	return available
}

now assuming no borrowing limit this equals guaranteedQuota + potentialAvailable in the whole subtree, so it may be way above nominal. So, IIUC calculateLendable changes its semantic for CQ (leaf nodes), but this is not that relevant because for DRF we always use calculateLendable on the parent: https://github.com/kubernetes-sigs/kueue/pull/4313/files#diff-e822642151fb9037f5ada06309828ea5c2ecb4a3cb9619ae445ce138622511a4L64

So, for the "cohort" the values was essentially SubtreeQuota which is by definition the "lendable" quota in the subtree. Similarly, in that case the potentialAvailable returns SubtreeQuota for the cohort in this line with "returned for cohort" comment. So, I think this is ok.

The code-wise the PR looks ok, seeing no problem for the current semantics:
/lgtm
/approve

@gabesaba please double check if my reasoning about making sure the results are the same is correct. If I missed something please flag and follow up.

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 30c980145862c47284fe1bbcd8ffec7124ca5f68

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabesaba, mimowo

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:

• OWNERS [mimowo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mimowo]

nit: this could be simplified with getRootUnsafe, but feel free for follow up.

[mimowo]

ah no this is first level parent, forget