-
Notifications
You must be signed in to change notification settings - Fork 11
chore: add three model for flmadm install #338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Klaus Ma <klausm@nvidia.com>
Summary of ChangesHello @k82cn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the deployment flexibility and robustness of the Flame system by introducing installation profiles for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant and valuable refactoring of the installation process by adding installation profiles (--control-plane, --worker, --client) to flmadm. This greatly improves deployment flexibility. The use of ${FLAME_HOME} for path expansion is also a good change for making the installation more relocatable.
However, there is a critical security regression: the systemd services are now configured to run as root instead of a dedicated flame user. This violates the principle of least privilege and should be addressed.
I've also included a few medium-severity suggestions regarding code duplication and opportunities for refactoring to improve maintainability.
I am having trouble creating individual review comments. Click here to see my feedback.
flmadm/src/managers/systemd.rs (248-249)
Removing the User=flame and Group=flame directives from the systemd service template is a significant security concern. This change will cause the flame-session-manager service to run as the root user by default in system-wide installations. Running services as root violates the principle of least privilege and exposes the system to greater risk if a vulnerability is found in the service. It is highly recommended to restore the behavior of running as a dedicated, unprivileged user.
flmadm/src/managers/systemd.rs (279-280)
Similar to the session manager, removing the User and Group directives here will cause the flame-executor-manager to run as root. This is an even greater security risk for a worker node service that may execute user-provided code. Any vulnerability could lead to a full system compromise. Please consider running this service as a dedicated, unprivileged user.
executor_manager/src/shims/host_shim.rs (203-205)
Using unwrap_or here means that if an environment variable like FLAME_HOME is not set, the command will be executed with the unexpanded string (e.g., ${FLAME_HOME}/bin/uv), which will fail. While this works, it might be better to fail earlier with a more explicit error message if a required environment variable is missing. Consider using shellexpand::env_with_context and providing a custom context that errors on missing variables.
flmadm/src/commands/install.rs (154-190)
This find_uv_executable function is also defined in flmadm/src/managers/installation.rs. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, this function should be defined in a single, shared location and reused. Consider creating a utility module within the flmadm crate (e.g., flmadm/src/utils.rs) for common functions like this.
flmadm/src/managers/user.rs (1-143)
Since all user creation and management logic has been removed from this file, the UserManager struct now only contains the is_root method. This method is a simple utility function. To simplify the project structure, consider moving is_root to a new shared utility module (e.g., flmadm/src/utils.rs) and removing the UserManager struct and this file entirely.
flmexec/src/script/lang/python.rs (33-44)
This function contains a hardcoded default for FLAME_HOME and a fallback to /usr/bin/uv. This is inconsistent with the new flmadm installation logic, which makes the installation self-contained by copying uv into ${FLAME_HOME}/bin.
Relying on a hardcoded fallback path can lead to using an unintended version of uv. It would be more robust to rely solely on the FLAME_HOME environment variable and fail fast if it's not set or if uv is not found at the expected path.
Note: The calling code at line 91 will need to be adjusted to handle the Result, for example let uv_cmd = get_uv_cmd()?;.
/// Get the uv command path from FLAME_HOME.
fn get_uv_cmd() -> Result<String, FlameError> {
let flame_home = std::env::var("FLAME_HOME")
.map_err(|_| FlameError::Internal("FLAME_HOME environment variable is not set".to_string()))?;
let uv_path = format!("{}/bin/uv", flame_home);
if !std::path::Path::new(&uv_path).exists() {
return Err(FlameError::Internal(format!("uv executable not found at {}", uv_path)));
}
Ok(uv_path)
}Signed-off-by: Klaus Ma <klausm@nvidia.com>
No description provided.