Just for code review #1
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR represents a significant refactoring that replaces a manual container implementation using low-level syscalls (clone, execvp) with a more robust systemd-based containerization approach. The changes eliminate custom cgroup management in favor of systemd's resource control capabilities, and introduce a configuration-based system for managing container resources.
Key changes:
- Replaced manual process isolation with systemd-run based container management
- Introduced configuration file parsing for runtime parameters (memory, CPU, time limits)
- Added comprehensive container lifecycle management (setup, execution, cleanup)
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tester | Removed test bash script that was used for manual process execution |
| src/worker.rs | Stripped down to minimal stub structure, removing all cgroup and process management code |
| src/main.rs | Complete rewrite to use new container system with config-based resource limits and systemd integration |
| src/container.rs | New file implementing ContainerJail and ContainerJailBuilder for systemd-based container management |
| src/common.rs | New utility module with config parsing and memory size conversion macro |
| container.conf | New configuration file template for container runtime parameters |
| README.md | Added project documentation describing the container module |
| Cargo.toml | Updated project name and dependencies (nix version bump, added shellexpand) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut container_builder = Box::new(ContainerJailBuilder::new()); | ||
| let mut container = match container_builder.new_container() { | ||
| Ok(x) => x, | ||
| Err(x) => panic!("Error creating container") |
There was a problem hiding this comment.
The error message doesn't include the actual error details. Change to panic!(\"Error creating container: {x}\") to provide useful debugging information.
| Err(x) => panic!("Error creating container") | |
| Err(x) => panic!("Error creating container: {:?}", x) |
| let mut hostname_file = OpenOptions::new() | ||
| .append(true) | ||
| .open(format!("{}/etc/hostname", &self.root_dir))?; | ||
| writeln!(hostname_file, "{}", &self.container_id); |
There was a problem hiding this comment.
The result of writeln! is not checked. This can silently fail to write the hostname. Add error handling: writeln!(hostname_file, \"{}\", &self.container_id)?;
| writeln!(hostname_file, "{}", &self.container_id); | |
| writeln!(hostname_file, "{}", &self.container_id)?; |
|
|
||
| pub fn clean(&self) -> () { | ||
| println!("Cleaning container {}", self.container_id); | ||
| std::fs::remove_dir_all(Path::new(&self.root_dir)); |
There was a problem hiding this comment.
The Result from remove_dir_all is ignored. If cleanup fails, it should be logged or handled. Consider adding .ok() explicitly or logging the error.
| std::fs::remove_dir_all(Path::new(&self.root_dir)); | |
| if let Err(e) = std::fs::remove_dir_all(Path::new(&self.root_dir)) { | |
| eprintln!("Failed to remove directory {}: {}", self.root_dir, e); | |
| } |
| .arg(format!("{}.service", &self.container_id)) | ||
| .spawn() | ||
| .expect("Cleaning systemctl unit"); | ||
| ctl_clean.wait(); |
There was a problem hiding this comment.
The Result from wait() is ignored. This should be handled with .ok() or proper error handling to avoid potential issues.
| ctl_clean.wait(); | |
| ctl_clean.wait().ok(); |
| let mut cgroups_slice = Command::new("systemctl") | ||
| .args(["set-property", "jails.slice", "MemoryAccounting=yes"]) | ||
| .spawn()?; | ||
| cgroups_slice.wait(); |
There was a problem hiding this comment.
The Result from wait() is ignored. This should be handled with .ok() or proper error handling.
| cgroups_slice.wait(); | |
| cgroups_slice.wait()?; |
| EasyContest - one day this will be a selfmade programming contest platform focused on ease of use and extendability, but now it's just beginning) | ||
|
|
||
| ## Container module | ||
| Incapsulates running compiled executables in separate "container jail" with limited resources and permissions. Right on only statically linked binaries supported. |
There was a problem hiding this comment.
Corrected spelling of 'Incapsulates' to 'Encapsulates'.
| Incapsulates running compiled executables in separate "container jail" with limited resources and permissions. Right on only statically linked binaries supported. | |
| Encapsulates running compiled executables in separate "container jail" with limited resources and permissions. Right on only statically linked binaries supported. |
| EasyContest - one day this will be a selfmade programming contest platform focused on ease of use and extendability, but now it's just beginning) | ||
|
|
||
| ## Container module | ||
| Incapsulates running compiled executables in separate "container jail" with limited resources and permissions. Right on only statically linked binaries supported. |
There was a problem hiding this comment.
Grammatical error: 'Right on only' should be 'Right now only' or 'Currently only'.
| Incapsulates running compiled executables in separate "container jail" with limited resources and permissions. Right on only statically linked binaries supported. | |
| Incapsulates running compiled executables in separate "container jail" with limited resources and permissions. Right now, only statically linked binaries are supported. |
PR just for convenient code review UI