[YUNIKORN-3330] Run predicate checks in Required node preemption

[manirajv06]

Description

Run preemption predicate checks for required node preemption.

• PreFilter checks
• Filter Predicate Checks.
  Refactored existing predicate checks functions used for Intra queue preemption to make use of the same even for required node preemption as well
  Added new unit tests
  Modified existing test cases as well

Type of change

Please delete options that are not relevant.

• Bug Fix
• Improvement
• Feature
• Refactoring
• Documentation

Jira issue

Jira ID : https://issues.apache.org/jira/browse/YUNIKORN-3330

• I have created a Jira issue for this pull request.
• The Jira ID is part of the title of this pull request.

AI Tooling

If an AI tool was used:

• The PR includes the phrase "Generated by <tool>", where <tool> is the name of the AI tool used.
• My use of AI contributions follows the ASF legal policy.

Check https://www.apache.org/legal/generative-tooling.html for details.

How has this been tested?

• New unit tests were added to cover new or changed code paths.
• make test_all was run, and no failures reported.
• A pull request will be opened for new e2e tests (apache/yunikorn-k8shim repository).

Questions:

• The change needs documentation, a pull request for apache/yunikorn-site repository will be created.
• There is breaking changes for older versions: jira is tagged with release-notes label.
• The licenses files needs to be updated.

Screenshots or other details

[stantheman0128]

Thanks for extending the predicate checks to the required node path. I read through the change and have a few comments.

1. Released allocations do not match the preempted ones (pkg/scheduler/objects/required_node_preemptor.go:96)

After this change the loop at line 77 marks, counts and emits events only for finalVictims, but line 96 still calls notifyRMAllocationReleased(victims, ...) with the full list from GetVictims(). finalVictims comes from populateVictims, which keeps only victimList[0..index] (predicates.go:108-111). So whenever the shim returns an index smaller than len(victims)-1, the extra allocations are sent to the RM for release without MarkPreempted(), without IncPreemptingResource() on their queue, and without a preempted event. The intra-queue path passes finalVictims here (preemption.go:723). I think line 96 should pass finalVictims as well. Minor, related: the "Found victims for required node preemption" log at line 227 reports len(victims) before trimming, so it can overstate the count.

2. Branch needs a rebase on master, and the head does not build as pushed

At b367fcc, runPredicates calls p.requiredAsk.preAllocateConditions(true) (required_node_preemptor.go:197), but Allocation has no such method in that tree. The new test also calls mockCommon.NewPreemptionPredicatePlugin(preemptions, nil, true, false) (application_test.go:3546), while the mock in that tree still has the old 3-arg signature (reservations, allocs map[string]string, preempt []Preemption) (pkg/mock/preemption_predicate_plugin.go:129). Both pieces landed on master with YUNIKORN-3300, so I assume the branch lost its base during the conflicts cleanup. When rebasing, note that master's preemption_test.go already defines resetNode and resetQ. The versions added here (preemption_test.go:124-148) also clear node.reservations and the app reservations, which the new test needs because it reserves the node, so those bits probably need to be merged into the existing helpers instead of added again.

3. StartIndex is always 0 (required_node_preemptor.go:218)

The intra-queue path sends the index at which the core already expects the ask to fit, counting the node's available resources (StartIndex: int32(idx), preemption.go:594). Here it is always 0. Is that intentional? Starting at 0 lets the shim return the smallest prefix that fits, so it may well be the right choice. I only wanted to check that the difference from the intra-queue path was considered.

4. Test does not cover a trimmed victim list (application_test.go:3533)

All cases in TestRequiredNodePreemptionWithPredicates use a single victim (ask-1) with index 0, so victims and finalVictims are always identical and the issue in point 1 is not exercised. A case with two victims on the node and the mock returning index 0 would cover it: the second allocation should stay unpreempted and should not be released.

5. Nit: PredicateChecks does not need to be exported (predicates.go:115)

Both callers (preemption.go:449 and required_node_preemptor.go:220) are in package objects, and the function returns the unexported *predicateCheckResult. Keeping it unexported (predicateChecks) would avoid widening the package API.

[manirajv06]

@stantheman0128 @wilfred-s

Addressed all review comments. Please take a look.

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.32%. Comparing base (0ef9fd7) to head (77695f3).

Files with missing lines	Patch %	Lines
pkg/scheduler/objects/required_node_preemptor.go	93.33%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1106   +/-   ##
=======================================
  Coverage   83.31%   83.32%           
=======================================
  Files         104      104           
  Lines       14652    14696   +44     
=======================================
+ Hits        12208    12246   +38     
- Misses       2146     2151    +5     
- Partials      298      299    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

[wilfred-s]

Two minor point and then we're good.