[flame] Add HTTP storage backend and flmctl close command#344
Conversation
Use from_default_env() instead of try_from_default_env() to provide a default filter when RUST_LOG environment variable is missing, rather than returning an error.
- Add flame-package-storage service using dufs for HTTP-based package storage - Update ci/flame.yaml and ci/flame-local.yaml to use HTTP storage - Add flmctl close command to close sessions - Add Connection::close_session method to Rust SDK - Fix Python linter warnings with noqa comments for gRPC method names
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 Flame ecosystem by introducing a flexible HTTP-based package storage solution, moving away from local file storage. This change improves scalability and deployment options for package management. Concurrently, it empowers users with greater control over their sessions by adding a new CLI command and a corresponding Rust SDK method to close active sessions. Minor code quality improvements were also included to maintain consistency. Highlights
Changelog
Activity
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
|
![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 introduces an HTTP-based storage backend using dufs and adds a flmctl close command. The changes are mostly well-implemented. My review includes a few suggestions for improvement, primarily concerning the Docker configuration for the new storage service to enhance stability and security, and a recommendation to refactor the logging initialization in the Rust SDK to better align with library best practices.
| - flame-package-storage | ||
|
|
||
| flame-package-storage: | ||
| image: sigoden/dufs:latest |
There was a problem hiding this comment.
Using the :latest tag for Docker images is not recommended as it can lead to unpredictable behavior when the image is updated. It's better to pin to a specific version (e.g., sigoden/dufs:v0.41.0) to ensure build reproducibility and stability.
| logging: | ||
| options: | ||
| max-size: "100m" | ||
| command: ["/data", "-A", "--allow-upload", "--allow-delete"] |
There was a problem hiding this comment.
The command arguments are redundant. The -A (--allow-all) flag is a shortcut for --allow-upload, --allow-delete, and --allow-search. You can simplify this to ["/data", "-A"].
More importantly, using -A without any authentication exposes the package storage to unauthorized uploads and deletions. While this might be acceptable for a local development environment, consider adding some form of authentication for more secure setups, especially if this configuration is used as a base for other environments.
command: ["/data", "-A"]- Pin dufs image to v0.43.0 instead of :latest for reproducibility - Simplify dufs command to just -A (already includes --allow-upload/delete) - Use 127.0.0.1:5050 in flame-local.yaml for host-based development
- Start dufs as package storage server on port 5050 during 'start' - Stop dufs during 'stop' command - Add dufs to status and logs commands - Add check_dufs function with installation instructions - Create packages directory during install
- Install dufs via cargo install - Start dufs as HTTP package storage server on port 5050 - Add dufs logs to failure output - Stop dufs in cleanup step
- Add pytest-timeout dependency with 120s per-test timeout - Add 10-minute job timeout for the test step - Show logs on both failure and cancelled (timeout) - Increase log lines from 100 to 200 - Add active processes and network connections to debug output - Add cluster readiness verification step before tests
1 participant
Summary
flame-package-storageservice usingdufsfor HTTP-based package storage (supports PUT/DELETE operations)ci/flame.yamlandci/flame-local.yamlto use HTTP storage backendflmctl closecommand to close sessions from CLIConnection::close_sessionmethod to Rust SDKChanges
HTTP Storage Backend
flame-package-storagecontainer usingsigoden/dufsimageflmctl close command
Testing
flmctl closecommand successfully closes sessionsNote
The HTTP storage URL resolution between host and container networks needs additional work for seamless local development (host uses
127.0.0.1:5050, containers needflame-package-storage:5000).