stantheman0128 commented on PR #1106: URL: https://github.com/apache/yunikorn-core/pull/1106#issuecomment-5840676540
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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
