Prepare code for two-level scheduling

[lchrzaszcz]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PRs introduces some preparation changes that do not change the logic of the code, but makes it easier to review https://github.com/kubernetes-sigs/kueue/pull/5353/files which is TAS two-level scheduling

Relates to: #5439
Preparation PR for: https://github.com/kubernetes-sigs/kueue/pull/5353/files

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	35f49cc
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/683ee76ae05031000861ec54

[k8s-ci-robot]

Hi @lchrzaszcz. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[lchrzaszcz]

Previous code relied on a fact that domains are in descending order, so "!=" works perfectly in finding the tightest fit. I'm just changing it to "<" to make it more explicit.

[mimowo]

I'm not convinced I like the proposed abtraction - I would prefer avoid duplicating this.

[lchrzaszcz]

From the perspective of this PR it seems redundant, but looking at introduction or chunks, there'll be more custom logic to both modes.

What we could do is something like that:

if a.state == b.state {
	return slices.Compare(a.levelValues, b.levelValues)
}
if useLeastFreeCapacityAlgorithm(unconstrained) {
	// ascending order
	return cmp.Compare(a.state, b.state)
} else {
	// descending order
	return cmp.Compare(b.state, a.state)
}

What do you think?

[mimowo]

lgtm

[mimowo]

ok, I think the reason we are going to update this logic while working on two-level scheduling is motivation to avoid duplication, and thus diverging the code more in the future

[lchrzaszcz]

Got it, well I've just realized that with suggested solution I'm reinventing the wheel and the code look like that before. So I'm reverting my proposed change and I'll revert similar change in two-level scheduling PR.

[lchrzaszcz]

The code should subtract the chosen domain state from remainingCount. Old code subtracted not the chosen one, but next in line. It is the same for all domains, apart from the last one in BestFit. The old code worked ok, because remainingCount is a local variable, and we only care it is "not greater than 0" at the end of the function, so even if it was lower than 0 it was fine.

I'm fixing it to account for the optimized last domain correctly.

[gabesaba]

/lgtm

will leave approval for @mimowo

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: ffdf58608c9950c875679e783bfb37fd3c9f0c05

[mimowo]

I would like to wait for #5469 (comment)

[mimowo]

/lgtm
/approve
Thanks 👍

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 0d6e3978a71601639d29505149d1321cdefc15f2

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lchrzaszcz, 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:

• pkg/cache/OWNERS [mimowo]

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

[lchrzaszcz]

I would like to wait for #5469 (comment)

Done. I've reverted the code as it looked like that before my changes.

[mimowo]

/ok-to-test
/hold
to make sure tests pass, feel free to unhold

[lchrzaszcz]

/unhold