reclaim: add group eviction and victim ordering fixes - #13
Open
hajnalmt wants to merge 21 commits into
Open
Conversation
Add AGENTS.md with comprehensive developer guide for AI coding agents working on the Volcano codebase, covering build commands, test commands, code style, import conventions, and agent/batch scheduler architecture. Update .gitignore with local development tool paths. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Add design document covering motivation, architecture, detailed design, tool comparison (Tilt vs Skaffold, DevSpace, Telepresence), and agentic workflow safety considerations for the Kind + ctlptl + Tilt local development workflow. Update prepare-for-development.md and development.md with Tilt-based quick start and full workflow reference. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Add Tilt-based local development environment using Kind (via ctlptl) with a local container registry. Provides single-command setup (make dev-up) with live-reload for scheduler, controller-manager, and webhook-manager components. Key features: - ctlptl-managed Kind cluster with local Docker registry - Tilt live-update with in-container Go rebuilds via entrypoint.sh - Multi-architecture support (amd64/arm64) for tool downloads, Docker builds, and in-container rebuilds - Integrated monitoring (Prometheus, Grafana) with port-forwards - On-demand unit and e2e test triggers via Tilt resources - All dev tooling self-contained in _output/bin Includes Helm chart fix: use dedicated admission_init_image_name variable for admission init Job image. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
The checking functions in the cache utils had ambiguous logging entries: I0925 07:48:34.403937 5756 util.go:79] schedulerPodName is responsible to Node k3d-volcano-dev-agent-2 Since in single scheduler scenarios the schedulerPodname is not populated. These entries are intimidating, because: - It's not clear that we are in caching territory so no things to be afraid. - Looks weird that there is an empty string after schedulerPodName. Furthermore, if it's empty the consistent hash circle is not needed to be checked. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
…rder Apply plugin job CompareFn first in BuildVictimsPriorityQueue and use TaskOrderFn as the tie-breaker when job ordering is equal, including preemptor-missing paths, so creation timestamp does not dominate same-priority victim selection. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Add BuildVictimsPriorityQueue unit coverage for job-order ties falling back to task-order, including preemptor-found and preemptor-missing paths. Place the test in package framework_test so it can use real plugin wiring without creating an import cycle between framework and plugin packages. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Set deterministic PodGroup creation timestamps and assert JobOrderCompareFn tie conditions so task-order fallback behavior is validated consistently. Update BuildVictimsPriorityQueue comments to match current ordering paths, including orphaned jobs and queue-based handling. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
prevent sibling leaf reclaim loops when parentBasedReclaimEnabled is true and both leaves under the same parent have no relevant deserved signal for the reclaimer request. keep parent-level reclaim semantics unchanged for cross-parent paths. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
add Test_capacityPlugin_ParentBasedReclaimScenarios with case1-case5 coverage for parent-based reclaim behavior under hierarchy. restore Test_capacityPlugin_OnSessionOpenWithHierarchy to baseline scope and keep parent-based scenarios in a dedicated test suite. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
document case1-case5 from Test_capacityPlugin_ParentBasedReclaimScenarios with queue topology diagrams, workload details, and expected reclaim outcomes. reorder key-function section so updateParentQueue and note placement match the design structure. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Use victim queue ordering semantics for cross-queue victim selection in reclaim, so ordering follows queue level and then applies victim task ordering for same-queue comparisons. Document this behavior in reclaim tests and skip the legacy case that asserted queue-priority-driven victim selection. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
…ption The "Preemption between Task within Job" loop overwrites preemptorTasks[job.UID] for every starving job in underRequest, which is shared across all queues. In multi-queue scenarios this mutates the same preemptor state used later by the between-jobs preemption phase. Because queue traversal previously depended on Go map iteration order, the behavior becomes non-deterministic. If queue Q1 (with no relevant preemptors) is visited first, its intra-job pass still iterates shared underRequest entries and can drain/replace Q2's preemptorTasks entry. When Q2 is visited later, the between-jobs loop sees empty preemptor state and skips valid preemption, so starvation can persist. Use a scoped local queue (intraJobPreemptors) for the intra-job pass instead of reusing preemptorTasks[job.UID]. This preserves the original preemptorTasks map populated during job discovery for between-jobs preemption while keeping intra-job behavior isolated. Add and document a multi-queue regression test that models this cross- queue interference path and verifies that intra-job processing in one queue does not invalidate between-jobs preemption in another queue. The test also captures the prior flaky characteristic (majority pass, intermittent fail) caused by non-deterministic queue iteration. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Use util.NewPriorityQueue(ssn.QueueOrderFn) in preempt so queue processing order is deterministic and aligned with allocate/reclaim behavior. While preserving Osykov's original queue-order implementation intent, keep the preemptorTasks overwrite fix behavior from the stacked base commit by using scoped intra-job preemptor queues during intra-job preemption. Also include the topology-aware multi-queue test coverage from the original change to validate queue-ordered preemption behavior. Signed-off-by: Vitalii Osykov <vitaliyosykov@gmail.com> Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Build underRequest entries per queue and process only the current queue's starving jobs in the intra-job preemption loop. This avoids cross-queue iteration in each queue pass and keeps intra-job processing aligned with the active queue context. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Iterate only relevant starving-job queues when building the preempt queue priority structure to avoid unnecessary queue scans and extra no-preemptor iterations. Also fix the topology-aware test case name typo. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Adjust two reclaim unit tests to expect the preemptee selected by the current queue ordering in reclaim victim selection. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Switch normal preemption victim ordering to the Aumovio-specific priority queue so victim selection follows the intended queue-aware ordering in this path. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Reverse capacity victim task ordering so victims sharing more resource dimensions with the preemptor are selected first. This aligns reclaim behavior with the intended victim priority for dimension overlap and avoids evicting lower-overlap tasks ahead of better reclaim candidates. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13213374f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+42
to
+44
| if mySchedulerPodName == "" { | ||
| klog.V(5).Infof("No schedulerPodName specified for %s %s (not a multi-scheduler scenario)", objectType, objectName) | ||
| return true |
There was a problem hiding this comment.
The new isSingleSchedulerScenario short-circuit returns true whenever mySchedulerPodName is empty, before checking the consistent-hash ring (c). In multi-scheduler deployments where c is initialized but SCHEDULER_POD_NAME is missing/empty, responsibleForPod/Node/PodGroup now all fail open and every scheduler instance will claim the same objects, causing duplicate scheduling work and conflicting updates. This should only bypass hash ownership when c == nil (true single-scheduler mode).
Useful? React with 👍 / 👎.
Comment on lines
+122
to
+125
| for _, task := range job.Tasks { | ||
| if task.UID != reclaimee.UID { | ||
| // Evict other tasks in the group | ||
| s.Evict(task, "group-eviction-policy") |
There was a problem hiding this comment.
The group-eviction loop ranges over job.Tasks and recursively calls s.Evict for each member, but s.Evict updates task status via job.UpdateTaskStatus, which deletes and re-adds entries in the same map. Mutating a Go map during iteration produces non-deterministic traversal, so minMember group eviction can unpredictably skip or duplicate members. Collecting tasks into a slice before recursive evictions would make this deterministic.
Useful? React with 👍 / 👎.
Comment on lines
+458
to
+459
| // Clear | ||
| s.lastOps = make(map[api.TaskID]Operation) |
There was a problem hiding this comment.
lastOps is reset in Commit, but not in Discard. Reclaim now has an error path that does stmt.Discard() and then RecoverOperations(savedOriginalStmt): stale lastOps entries cause Evict/Allocate to be treated as duplicates and skipped, so previously saved operations may not be re-applied even though in-memory state was rolled back. Resetting lastOps on discard (or before recovery) is needed to make recovery reliable.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
go test ./pkg/scheduler/plugins/capacity -run Test_capacityPlugin_OnSessionOpenWithoutHierarchymake unit-test(environment-specific failures inpkg/agent/events/handlers/cpuburstdue to/tmppermission issues)make images