feat: Add LXD Container Management REST API Integration #511
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds LXD support: new LXD config and env bindings, ClientService implementing LXD operations (local and remote), HTTP controller and routes for LXD lifecycle, CLI install option to optionally install LXD, docker socket mounts, and Go module dependency bumps; logging switches to internal logger. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as HTTP Client
participant Ctrl as LXD Controller
participant Svc as LXD ClientService
participant LXD as LXD Daemon
Client->>Ctrl: HTTP /lxd/... (Create/List/Get/Start/Stop/...)
activate Ctrl
Ctrl->>Ctrl: parse input & create timeout ctx
Ctrl->>Svc: call operation(ctx, ... )
deactivate Ctrl
activate Svc
alt local (unix socket) or remote (https)
Svc->>LXD: connect (socket or TLS) and send request
end
LXD-->>Svc: operation response / op reference
Svc->>Svc: waitOp(ctx) / poll until done or timeout
deactivate Svc
Svc-->>Ctrl: result or error
Ctrl->>Client: JSON response {status, data/message}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
|
@coderabbitai: Add summary and title please |
coderabbitai
bot
changed the title
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@zhravan, I'll help you add a better title and summary to the PR. 🏁 Script executed: Length of output: 46 @zhravan, I've successfully updated the PR! New Title: Summary Added:
The PR is now ready with a clear, professional title and detailed description that accurately reflects the changes introduced. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
zhravan
left a comment
zhravan
left a comment
There was a problem hiding this comment.
LXD installation setup via API
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/app/commands/install/run.py (1)
519-559: Previous review issues remain unaddressed.The implementation still contains all the critical and major issues identified in earlier reviews:
- Security risk (line 551):
shell=Trueis a security anti-pattern; use list-form args.- Critical bug (line 529):
$USERunder sudo expands to "root", not the invoking user; useSUDO_USERenv var orgetpass.getuser().- Missing validation: No check that
snapexists before attempting installation; useHostInformation.command_exists("snap").- Compatibility (line 534):
zfsdriver requires kernel modules often unavailable; default todirfor portability.- Hang risk: No timeouts on subprocess calls—can block indefinitely if prompts appear.
- Poor error handling (lines 552-553, 557-558): Generic
Exceptionmasks actual error types; catchsubprocess.CalledProcessErrorandsubprocess.TimeoutExpired.- Permission race (line 555): Running
lxd initwithout sudo will fail due to group-membership race; usesudo lxd init --preseed.- No feedback: Return codes and preseed success are not checked or logged.
The comprehensive fixes suggested in the past review comments should be applied.
🧹 Nitpick comments (1)
cli/app/commands/install/run.py (1)
486-489: Minor: Use direct attribute access for consistency.Using
getattr(self, "include_lxd", False)is unnecessarily defensive sinceinclude_lxdis always set in__init__(line 116). Direct access withself.include_lxd(as used on line 520) would be clearer and more consistent.- if getattr(self, "include_lxd", False): + if self.include_lxd: if "LXD_ENABLED" in updated_env: updated_env["LXD_ENABLED"] = "true"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/app/commands/install/command.py(2 hunks)cli/app/commands/install/run.py(5 hunks)docker-compose.yml(1 hunks)helpers/config.dev.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- helpers/config.dev.yaml
- docker-compose.yml
- cli/app/commands/install/command.py
🧰 Additional context used
🧬 Code graph analysis (1)
cli/app/commands/install/run.py (2)
cli/app/utils/lib.py (2)
HostInformation(61-91)get_os_name(63-64)cli/tests/commands/preflight/test_deps.py (2)
warning(40-41)info(37-38)
🔇 Additional comments (2)
cli/app/commands/install/run.py (2)
104-104: LGTM: Clean parameter addition.The
include_lxdparameter follows the existing pattern for optional flags and defaults appropriately toFalsefor opt-in behavior.Also applies to: 116-116
185-185: LGTM: Appropriate placement in installation flow.The LXD installation step is positioned sensibly after SSH setup and before starting services. The "(optional)" label clearly indicates its opt-in nature.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/app/commands/install/run.py (1)
510-550: Critical security and reliability issues remain unaddressedThe
_install_lxd_if_enabledmethod still contains all the security and reliability issues comprehensively identified in the previous reviews above:
- Security:
shell=True(line 542) is a security anti-pattern- User targeting:
$USERunder sudo (line 520) may modify the wrong user- Missing validation: No check for
snapavailability before attempting installation- Error handling: Generic
Exceptioncatching (lines 543-544, 548-549) instead ofsubprocess.CalledProcessError- Portability: Hardcoded
zfsdriver (line 525) unavailable on many systems- Reliability: No timeout on
p.communicate()(line 547), no return code validation, andlxd initruns without sudo despite requiring group membershipPlease implement the comprehensive fixes provided in the previous review comments. The suggested approach includes:
- Validating
snapexists before proceeding- Using list-form subprocess calls instead of
shell=True- Using
getpass.getuser()orSUDO_USERfor correct user targeting- Using the portable
dirstorage driver instead ofzfs- Adding timeouts and proper error handling
- Running
lxd init --preseedwith sudo
🧹 Nitpick comments (1)
cli/app/commands/install/run.py (1)
477-480: Optional: Simplify attribute accessThe
getattr(self, "include_lxd", False)is unnecessary sinceinclude_lxdis always set in__init__(line 90). You can simplify toself.include_lxd.Apply this diff:
- if getattr(self, "include_lxd", False): + if self.include_lxd: if "LXD_ENABLED" in updated_env: updated_env["LXD_ENABLED"] = "true"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/app/commands/install/command.py(3 hunks)cli/app/commands/install/run.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/app/commands/install/command.py
🧰 Additional context used
🧬 Code graph analysis (1)
cli/app/commands/install/run.py (3)
cli/app/utils/config.py (1)
load_user_config(30-36)cli/app/utils/lib.py (2)
HostInformation(61-91)get_os_name(63-64)cli/tests/commands/preflight/test_deps.py (2)
warning(40-41)info(37-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-package-macos (macos-13)
🔇 Additional comments (2)
cli/app/commands/install/run.py (2)
77-77: LGTM: Clean parameter additionThe
include_lxdparameter follows the existing pattern and defaults toFalsefor backward compatibility. Storing_user_config(line 91) is also a reasonable addition.Also applies to: 90-91
175-175: LGTM: Installation step properly positionedThe LXD installation step is correctly placed after SSH setup and before starting services, and is clearly labeled as optional.
2 participants
Issue
Link to related issue(s):
Description
This PR introduces REST API integration for LXD container lifecycle management. The implementation provides HTTP endpoints for comprehensive container operations including creation, listing, retrieval, starting, stopping, restarting, and deletion of LXD container instances.
Key additions:
Scope of Change
Select all applicable areas impacted by this PR:
Screenshot / Video / GIF (if applicable)
Attach or embed screenshots, screen recordings, or GIFs demonstrating the feature or fix.
Related PRs (if any)
Link any related or dependent PRs across repos.
Additional Notes for Reviewers (optional)
Anything reviewers should know before testing or merging (e.g., environment variables, setup steps).
Developer Checklist
To be completed by the developer who raised the PR.
Reviewer Checklist
To be completed by the reviewer before merge.
Summary by CodeRabbit
New Features
Chores