fix: fix gpu scheduler policy sort bug(#1806)

[kaiiyvwu]

What type of PR is this?
/kind bug

What this PR does / why we need it:
Update the gpu policy Less function. If the gpu scheduler policy is binpack or spread, it will not compare numa as sort key

Which issue(s) this PR fixes:
Fixes #1806

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[hami-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kaiiyvwu
Once this PR has been reviewed and has the lgtm label, please assign wawa0210 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[gemini-code-assist]

Code Review

This pull request refactors the Less method in DeviceUsageList to use a switch statement for GPU scheduler policies and updates corresponding unit tests. However, removing the k8s.io/klog/v2 import will lead to a compilation error as the package is still utilized elsewhere in the file. Additionally, there is a logic inconsistency between the Numa-agnostic Spread policy case and the Numa-aware default case, which should be aligned if the default behavior is intended to be Spread.

I am having trouble creating individual review comments. Click here to see my feedback.

pkg/scheduler/policy/gpu_policy.go (21)

The removal of the k8s.io/klog/v2 import will cause a compilation error because the klog package is still used in the ComputeScore function (lines 74 and 80).

	"github.com/Project-HAMi/HAMi/pkg/util"

	"k8s.io/klog/v2"

pkg/scheduler/policy/gpu_policy.go (44-54)

There is a logic inconsistency between the explicit Spread policy and the default case.

In the new implementation, the Spread policy (line 48) is Numa-agnostic, while the default case (lines 50-53) remains Numa-aware. Since the default behavior is intended to be Spread (as noted in the previous version's comments), these should likely be consistent.

If the goal is to remove Numa comparison for these policies as stated in the PR description, consider updating the default case to match the Spread logic or having the Spread case fall through to default if they share the same implementation.

[codecov]

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/scheduler/policy/gpu_policy.go	62.50%	3 Missing ⚠️

Flag	Coverage Δ
unittests	57.95% <62.50%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
pkg/scheduler/policy/gpu_policy.go	87.09% <62.50%> (-12.91%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mesutoezdil]

1 gap: the default case has no test.
codecov flags 3 lines missing.
the default path is the numa-aware branch and its behavior is unchanged from the original code, but it should still be covered. a test with policy="" or an unknown policy and two devices on different numa nodes would cover both lines in the default case.

[mesutoezdil]

the default branch has no test. add a case with a non-binpack, non-spread policy (e.g. empty string or "topology") and devices on different numa nodes to cover the two return paths here.