reclaim: add group eviction and victim ordering fixes - #12
Closed
hajnalmt wants to merge 14 commits into
Closed
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>
![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: 85c3c1e409
ℹ️ 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
+343
to
344
| mergedStmt := framework.SaveOperations(savedOriginalStmt, nodeStmt) | ||
| nodeStmt.Discard() |
There was a problem hiding this comment.
This path now serializes successful per-node reclaim work through SaveOperations/RecoverOperations, but RecoverOperations replays Pipeline with evictionOccurred=false, so a task that was pipelined after evictions loses its eviction marker. That changes downstream behavior that depends on EvictionOccurred (for example nomination signaling in task scheduling reason), so reclaimed tasks can be treated as non-preemptive placements even when evictions happened.
Useful? React with 👍 / 👎.
Comment on lines
+84
to
+86
| if lastOp, exists := s.lastOps[reclaimee.UID]; exists && lastOp == Evict { | ||
| // Skip this eviction | ||
| return nil |
There was a problem hiding this comment.
The new lastOps short-circuit returns early for tasks already marked as Evict, but discard/rollback paths do not clear that marker for evictions. In the reclaim fallback (stmt.Discard() then stmt.RecoverOperations(savedOriginalStmt)), recovered evictions for previously-evicted tasks can be skipped silently, leaving the restored statement missing expected eviction side effects and operations.
Useful? React with 👍 / 👎.
Owner
Author
|
Superseded by #13 |
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
1 participant
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
make imagessuccessfully.