lease max free memory #48
Conversation
## Walkthrough
The changes clarify the meaning of non-positive `cores` values in the `LeaseRequest` struct comment and add `cores` and `memory` fields to `LeaseResponse`. The `Executor::lease` method was updated to reject non-positive resource requests and available resources, while `ExecutorDB::open_lease` now supports `memory == -1` to lease all free memory and sets allocated cores and memory in the lease response. The client and manager code were adjusted to use these updated fields, improving internal resource handling without changing public interfaces.
## Changes
| File(s) | Change Summary |
|--------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------|
| rfaas/include/rfaas/allocation.hpp | Clarified comment on `LeaseRequest::cores` to include zero; added `cores` and `memory` fields to `LeaseResponse` struct. |
| server/resource_manager/executor.cpp | Added guard clause in `Executor::lease` to reject non-positive cores/memory or insufficient free resources before leasing. |
| server/resource_manager/db.cpp | Modified `ExecutorDB::open_lease` to handle `memory == -1` by leasing all free memory; set allocated cores and memory in lease.|
| rfaas/include/rfaas/client.hpp | Refactored `client::lease` to cache lease response in a local reference, reducing repeated calls to `_resource_mgr.response()`.|
| server/resource_manager/manager.cpp | Changed `_handle_client_message` to assign allocated cores and memory from client response buffer instead of request variables.|
## Poem
> In the warren of code, a tweak to explain,
> Now zero and negative cores mean the same—
> Lease all the memory, if you wish, just say minus one,
> Resource manager rabbits keep hopping till the work is done.
> 🐇✨Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/resource_manager/executor.cpp (1)
57-60: Consider validating all negative memory valuesWhile you've implemented the special case for memory = -1, consider adding validation for other negative memory values to prevent unintended behavior. For example:
// Lease entire free memory when receiving reserved value -1 if (memory == -1) { memory = _free_memory; } +// Reject other negative memory values +else if (memory < 0) { + return false; +} // Not enough memory? skip if(_free_memory < memory) { return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
rfaas/include/rfaas/allocation.hpp(1 hunks)server/resource_manager/executor.cpp(1 hunks)
🔇 Additional comments (2)
server/resource_manager/executor.cpp (2)
52-55: Good implementation of the "lease max free memory" featureThe implementation correctly handles the special case where memory = -1 by setting it to the entire available free memory before the resource validation checks.
62-62: Helpful comment about reserved core valuesThis comment aligns well with the updated comment in allocation.hpp and improves code maintainability.
| struct LeaseRequest { | ||
| // > 0: Number of cores to be allocated | ||
| // < 0: client_id with negative sign, deallocation & disconnect request | ||
| // <= 0: client_id with negative sign, deallocation & disconnect request |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Comment update is good, but document memory=-1 special value too
The update that clarifies cores <= 0 values represent deallocation requests is helpful. However, since this PR introduces the special memory=-1 value to request maximum free memory, this feature should also be documented in the LeaseRequest struct to make the API contract clear to users.
// <= 0: client_id with negative sign, deallocation & disconnect request
int16_t cores;
+ // Regular positive value: Amount of memory to be allocated
+ // -1: Request to allocate all available free memory
int32_t memory;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // <= 0: client_id with negative sign, deallocation & disconnect request | |
| // <= 0: client_id with negative sign, deallocation & disconnect request | |
| int16_t cores; | |
| // Regular positive value: Amount of memory to be allocated | |
| // -1: Request to allocate all available free memory | |
| int32_t memory; |
🤖 Prompt for AI Agents
In rfaas/include/rfaas/allocation.hpp at line 11, the comment currently explains
that cores <= 0 indicate deallocation and disconnect requests, but it lacks
documentation for the special memory=-1 value introduced to request maximum free
memory. Update the comment in the LeaseRequest struct to explicitly document
that memory=-1 is a special value used to request the maximum available free
memory, ensuring the API contract is clear to users.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rfaas/include/rfaas/allocation.hpp(2 hunks)rfaas/include/rfaas/client.hpp(1 hunks)server/resource_manager/db.cpp(2 hunks)server/resource_manager/executor.cpp(1 hunks)server/resource_manager/manager.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- rfaas/include/rfaas/client.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- rfaas/include/rfaas/allocation.hpp
- server/resource_manager/executor.cpp
🔇 Additional comments (2)
server/resource_manager/db.cpp (1)
92-93: LGTM! Populating lease response with actual allocated resources.Correctly setting the lease response with the actual allocated cores and memory ensures consistency between what was requested and what was granted, especially important for the new -1 memory feature.
server/resource_manager/manager.cpp (1)
325-326: LGTM! Using actual allocated resources from response.Correctly using the response buffer values ensures that the executor receives the actual allocated resources rather than the original request values. This is essential for the new -1 memory feature where the actual memory allocation differs from the requested value.
| // Lease entire free memory when receiving reserved value -1 | ||
| if (memory == -1) { | ||
| memory = shared_ptr->_free_memory; | ||
| } |
There was a problem hiding this comment.
Fix parameter modification to prevent incorrect behavior with multiple executors.
Modifying the memory parameter directly can cause issues when multiple executors are evaluated. If the first executor can't satisfy the request, subsequent executors will see the modified memory value instead of the original -1.
Apply this diff to use a local variable instead:
- // Lease entire free memory when receiving reserved value -1
- if (memory == -1) {
- memory = shared_ptr->_free_memory;
- }
+ // Lease entire free memory when receiving reserved value -1
+ int actual_memory = memory;
+ if (actual_memory == -1) {
+ actual_memory = shared_ptr->_free_memory;
+ }Then update the lease call to use the local variable:
- if(!shared_ptr->lease(numcores, memory)) {
+ if(!shared_ptr->lease(numcores, actual_memory)) {And update the lease response assignment:
- lease.memory = memory;
+ lease.memory = actual_memory;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Lease entire free memory when receiving reserved value -1 | |
| if (memory == -1) { | |
| memory = shared_ptr->_free_memory; | |
| } | |
| // Lease entire free memory when receiving reserved value -1 | |
| int actual_memory = memory; | |
| if (actual_memory == -1) { | |
| actual_memory = shared_ptr->_free_memory; | |
| } | |
| if (!shared_ptr->lease(numcores, actual_memory)) { | |
| continue; | |
| } | |
| lease.memory = actual_memory; |
🤖 Prompt for AI Agents
In server/resource_manager/db.cpp around lines 78 to 81, avoid modifying the
input parameter 'memory' directly as it causes incorrect behavior when multiple
executors are evaluated. Instead, create a local variable initialized to the
original 'memory' parameter value; if 'memory' is -1, assign the free memory to
this local variable. Use this local variable in the lease call and update the
lease response assignment accordingly to preserve the original parameter value
for subsequent executors.
1 participant
Reserve the value -1 for memory in LeaseRequest to lease an executor with the maximum amount of free memory available.
Summary by CodeRabbit