feat: enhance FlameCluster.policy to Vec for dynamic plugin control#432
Conversation
- Rename policy field to policies (Vec<String>) for multi-policy support - PluginManager filters plugins based on enabled policies config - shim plugin always enabled (required for executor-session matching) - Remove deprecated installer/ dir and skaffold.yaml - Update all config files to use new policies list format Available configurable policies: priority, fairshare, gang
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the system from a single scheduling policy to a list of multiple enabled policies, updating configuration schemas, core data structures, and the plugin loading logic in the session manager. It also removes obsolete Kubernetes and Skaffold manifests. Feedback suggests making the default policy constant public for better reuse across modules, optimizing the plugin instantiation process to avoid unnecessary allocations, and adding validation to ensure only supported policies are configured.
| /// Default policies to enable when none specified in config. | ||
| /// Available configurable policies: "priority", "fairshare", "gang" | ||
| /// Note: "shim" plugin is always enabled (required for executor matching) | ||
| const DEFAULT_POLICIES: &[&str] = &["priority", "fairshare", "gang"]; |
There was a problem hiding this comment.
The DEFAULT_POLICIES constant should be public so it can be reused in tests and other modules (like session_manager), avoiding the need to hardcode the policy list in multiple places and ensuring consistency across the codebase.
| const DEFAULT_POLICIES: &[&str] = &["priority", "fairshare", "gang"]; | |
| pub const DEFAULT_POLICIES: &[&str] = &["priority", "fairshare", "gang"]; |
| let configurable_plugins: Vec<(&str, PluginPtr)> = vec![ | ||
| ("priority", PriorityPlugin::new_ptr()), | ||
| ("fairshare", FairShare::new_ptr()), | ||
| ("gang", GangPlugin::new_ptr()), | ||
| ]; | ||
|
|
||
| let mut plugins: Vec<(String, PluginPtr)> = configurable_plugins | ||
| .into_iter() | ||
| .filter(|(name, _)| enabled_policies.iter().any(|p| p == *name)) | ||
| .map(|(name, plugin)| (name.to_string(), plugin)) | ||
| .collect(); |
There was a problem hiding this comment.
Instantiating all configurable plugins before filtering them is inefficient, as it creates objects that may be immediately dropped. Additionally, this is a good place to validate that the policies specified in the configuration are actually supported, returning an error if an unknown policy is found.
for p in enabled_policies {
match p.as_str() {
"priority" | "fairshare" | "gang" => {}
_ => return Err(FlameError::InvalidConfig(format!("Unknown policy: {}", p))),
}
}
let mut plugins: Vec<(String, PluginPtr)> = Vec::with_capacity(enabled_policies.len() + 1);
if enabled_policies.iter().any(|p| p == "priority") {
plugins.push(("priority".to_string(), PriorityPlugin::new_ptr()));
}
if enabled_policies.iter().any(|p| p == "fairshare") {
plugins.push(("fairshare".to_string(), FairShare::new_ptr()));
}
if enabled_policies.iter().any(|p| p == "gang") {
plugins.push(("gang".to_string(), GangPlugin::new_ptr()));
}| let default_policies: Vec<String> = vec![ | ||
| "priority".to_string(), | ||
| "fairshare".to_string(), | ||
| "gang".to_string(), | ||
| ]; |
There was a problem hiding this comment.
Avoid hardcoding the list of default policies in tests. Instead, use the DEFAULT_POLICIES constant from the common::ctx module (after making it public) to ensure consistency and simplify maintenance.
let default_policies = common::ctx::DEFAULT_POLICIES
.iter()
.map(|s| s.to_string())
.collect::<Vec<_>>();- Make DEFAULT_POLICIES public for reuse in tests - Validate unknown policies before instantiating plugins - Use DEFAULT_POLICIES constant in tests instead of hardcoding
- Add name() method to Plugin trait for canonical plugin names - Create PLUGIN_REGISTRY as single source of truth for plugins - Add configurable_policy_names() to derive valid policies from registry - PluginManager::setup() now uses registry for validation and instantiation - Shim plugin marked as non-configurable (always enabled)
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Summary
policyfield topolicies(Vec<String>) for multi-policy supportPluginManagernow filters plugins based on enabled policies from configshimplugin is always enabled (required for executor-session matching)installer/directory andskaffold.yamlChanges
Config Format (Breaking Change)
Available Configurable Policies
priority- Priority-based session orderingfairshare- Proportional allocationgang- Batch scheduling (min_instances)Note:
shimplugin is always enabled automatically (required for executor-to-session matching).Testing