diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 13d5994a..68359ca8 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -6,6 +6,7 @@ This directory contains architectural decision records for the Torrust Tracker D | Status | Date | Decision | Summary | | ------------- | ---------- | --------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------ | +| ✅ Accepted | 2026-01-24 | [Bind Mount Standardization](./bind-mount-standardization.md) | Use bind mounts exclusively for all Docker Compose volumes for observability and backup | | ✅ Accepted | 2026-01-21 | [TryFrom for DTO to Domain Conversion](./tryfrom-for-dto-to-domain-conversion.md) | Use standard TryFrom trait for self-documenting, discoverable DTO→Domain conversions | | ✅ Accepted | 2026-01-21 | [Validated Deserialization for Domain Types](./validated-deserialization-for-domain-types.md) | Use custom Deserialize impl with Raw struct to enforce domain invariants during parsing | | ✅ Accepted | 2026-01-20 | [Grafana Default Port 3000](./grafana-default-port-3000.md) | Use Grafana's default port 3000 instead of 3100 for dedicated VM deployments | diff --git a/docs/decisions/bind-mount-standardization.md b/docs/decisions/bind-mount-standardization.md new file mode 100644 index 00000000..86c8da5c --- /dev/null +++ b/docs/decisions/bind-mount-standardization.md @@ -0,0 +1,176 @@ +# Decision: Bind Mount Standardization for Docker Compose + +## Status + +Accepted + +## Date + +2026-01-24 + +## Context + +The Docker Compose template currently uses a mix of named volumes and bind mounts for persistent data: + +```yaml +# Bind mounts (host path → container path) - CURRENT PATTERN +- ./storage/tracker/lib:/var/lib/torrust/tracker:Z + +# Named volumes (volume name → container path) - PROBLEMATIC +- caddy_data:/data +- grafana_data:/var/lib/grafana +- mysql_data:/var/lib/mysql +``` + +This inconsistency creates several problems: + +### 1. Observability + +- Named volumes hide data in `/var/lib/docker/volumes/` - not obvious to users +- Users cannot easily see where persistent data is stored +- File system tools (ls, du, find) don't work directly on named volume data + +### 2. Backup Complexity + +- Named volumes require `docker volume` commands or finding the internal path +- No single command can back up all data +- Docker-specific tooling is required +- Standard backup scripts don't work without modification + +### 3. Restore Complexity + +- Restoring named volumes requires Docker volume recreation +- Cannot simply copy files to restore data +- Migration between hosts requires Docker volume export/import + +### 4. Inconsistency + +- Some services use bind mounts, others use named volumes +- Different patterns for different services create cognitive overhead +- No predictable directory structure + +### 5. Portability Limitations + +- Named volumes cannot be moved between hosts by copying files +- Docker volume export/import dance is required +- Tied to Docker's internal storage format + +### 6. Debugging & Troubleshooting Difficulties + +- Cannot directly inspect files without entering containers +- Checking file permissions, ownership, disk usage is difficult +- Cannot modify config files directly for debugging +- Log files not accessible without `docker logs` + +### 7. Development Experience + +- Cannot easily reset state by deleting directories +- Cannot pre-populate data for testing scenarios +- IDE file watchers cannot observe changes in named volumes + +### 8. Deployment Architecture Complexity + +- Named volumes require a top-level `volumes:` section in docker-compose.yml +- Must derive which volumes are required based on enabled services +- Ansible must manage both directories and Docker volumes + +### 9. Security Visibility + +- File permissions are hidden inside Docker volume directories +- SELinux labels cannot be applied consistently +- Data locations are not transparent to users + +## Decision + +Standardize on **bind mounts exclusively** for all persistent data in Docker Compose deployments. + +All persistent data will be stored under `./storage/{service}/`: + +| Service | Bind Mount | Data Location | +| ---------- | ------------------------------------------ | -------------------------- | +| Tracker | `./storage/tracker/lib:/var/lib/torrust` | `./storage/tracker/lib` | +| Tracker | `./storage/tracker/log:/var/log/torrust` | `./storage/tracker/log` | +| Tracker | `./storage/tracker/etc:/etc/torrust` | `./storage/tracker/etc` | +| Caddy | `./storage/caddy/data:/data` | `./storage/caddy/data` | +| Caddy | `./storage/caddy/config:/config` | `./storage/caddy/config` | +| Caddy | `./storage/caddy/etc/Caddyfile:/etc/...` | `./storage/caddy/etc` | +| Grafana | `./storage/grafana/data:/var/lib/grafana` | `./storage/grafana/data` | +| Prometheus | `./storage/prometheus/etc:/etc/prometheus` | `./storage/prometheus/etc` | +| MySQL | `./storage/mysql/data:/var/lib/mysql` | `./storage/mysql/data` | + +Mount options: + +- `:ro` - Read-only for config files that shouldn't be modified +- `:Z` - SELinux private relabeling for writable data directories + +## Consequences + +### Positive + +- **Simplified backup**: Single command `cp -r ./storage/ backup/` backs up everything +- **Easy restore**: Copy files back to `./storage/` to restore +- **Full observability**: All persistent data is visible at predictable paths +- **Consistent pattern**: Same approach for all services +- **Portable**: Data directory can be moved between hosts by copying +- **Easy debugging**: Direct file inspection without entering containers +- **Better development experience**: Reset state by deleting directories +- **Simpler deployment**: No top-level `volumes:` section needed in docker-compose.yml +- **Security visibility**: File permissions are visible and controllable + +### Negative + +- **Explicit directory creation**: Directories must be created with correct permissions before container start +- **Permission management**: Must ensure correct ownership for non-root containers (Grafana: 472:472, MySQL: 999:999) +- **SELinux handling**: Must apply `:Z` suffix for writable directories on SELinux systems +- **Additional Ansible playbooks**: Need playbooks to create directories with correct ownership + +### Risks + +- **Breaking change**: Existing deployments using named volumes will need migration +- **Permission errors**: Incorrect directory ownership will prevent containers from starting + +### Mitigation + +- Create new Ansible playbooks for Grafana and MySQL directory creation with correct ownership +- Document migration path for existing deployments +- E2E tests will verify correct permission handling + +## Alternatives Considered + +### 1. Named Volumes Only + +**Rejected** because: + +- Data is hidden in `/var/lib/docker/volumes/` +- Backup requires Docker-specific commands +- Inconsistent with our observability principles +- Users cannot easily access or inspect persistent data + +### 2. Mixed Approach (Current State) + +**Rejected** because: + +- Inconsistency creates confusion and maintenance burden +- Different services have different storage patterns +- No single backup strategy works for all services +- Cognitive overhead for developers and operators + +### 3. Docker Volume Plugins + +**Rejected** because: + +- Overkill for single-VM deployments +- Adds complexity and external dependencies +- Our deployment model is single-VM, not distributed +- Standard bind mounts meet all our requirements + +## Related Decisions + +- [Grafana Integration Pattern](./grafana-integration-pattern.md) - This ADR supersedes the volume recommendations in that decision +- [Configuration Directories as Secrets](./configuration-directories-as-secrets.md) - Related security considerations for data directories + +## References + +- [Docker Compose bind mounts documentation](https://docs.docker.com/compose/compose-file/07-volumes/) +- [SELinux and Docker](https://docs.docker.com/storage/bind-mounts/#configure-the-selinux-label) +- [Refactoring Plan: Docker Compose Topology Domain Model](../refactors/plans/docker-compose-topology-domain-model.md) diff --git a/packages/dependency-installer/tests/containers/image_builder.rs b/packages/dependency-installer/tests/containers/image_builder.rs index 7348d443..11b45396 100644 --- a/packages/dependency-installer/tests/containers/image_builder.rs +++ b/packages/dependency-installer/tests/containers/image_builder.rs @@ -98,8 +98,7 @@ impl ImageBuilder { .arg("inspect") .arg(full_image_name) .output() - .map(|output| output.status.success()) - .unwrap_or(false) + .is_ok_and(|output| output.status.success()) } } diff --git a/packages/linting/src/utils.rs b/packages/linting/src/utils.rs index d1fdd381..eb42a32b 100644 --- a/packages/linting/src/utils.rs +++ b/packages/linting/src/utils.rs @@ -12,8 +12,7 @@ pub fn is_command_available(command: &str) -> bool { Command::new("which") .arg(command) .output() - .map(|output| output.status.success()) - .unwrap_or(false) + .is_ok_and(|output| output.status.success()) } /// Install a tool using npm globally diff --git a/src/application/command_handlers/configure/handler.rs b/src/application/command_handlers/configure/handler.rs index 7b32902c..c8099279 100644 --- a/src/application/command_handlers/configure/handler.rs +++ b/src/application/command_handlers/configure/handler.rs @@ -144,9 +144,8 @@ impl ConfigureCommandHandler { // Allow tests or CI to skip Docker installation // (useful for container-based tests where Docker is already installed via Dockerfile) - let skip_docker = std::env::var("TORRUST_TD_SKIP_DOCKER_INSTALL_IN_CONTAINER") - .map(|v| v == "true") - .unwrap_or(false); + let skip_docker = + std::env::var("TORRUST_TD_SKIP_DOCKER_INSTALL_IN_CONTAINER").is_ok_and(|v| v == "true"); let current_step = ConfigureStep::InstallDocker; if skip_docker { @@ -185,9 +184,8 @@ impl ConfigureCommandHandler { // Allow tests or CI to explicitly skip the firewall configuration step // (useful for container-based test runs where iptables/ufw require // elevated kernel capabilities not available in unprivileged containers). - let skip_firewall = std::env::var("TORRUST_TD_SKIP_FIREWALL_IN_CONTAINER") - .map(|v| v == "true") - .unwrap_or(false); + let skip_firewall = + std::env::var("TORRUST_TD_SKIP_FIREWALL_IN_CONTAINER").is_ok_and(|v| v == "true"); if skip_firewall { info!( diff --git a/src/domain/environment/state/common.rs b/src/domain/environment/state/common.rs index 8d012b9a..86d26240 100644 --- a/src/domain/environment/state/common.rs +++ b/src/domain/environment/state/common.rs @@ -104,7 +104,7 @@ mod tests { let context: BaseFailureContext = serde_json::from_str(&json).unwrap(); assert_eq!(context.error_summary, "Deserialized error"); - assert_eq!(context.execution_duration, Duration::from_secs(60)); + assert_eq!(context.execution_duration, Duration::from_mins(1)); } #[test] diff --git a/src/presentation/views/progress/mod.rs b/src/presentation/views/progress/mod.rs index de76fca2..db3b79c7 100644 --- a/src/presentation/views/progress/mod.rs +++ b/src/presentation/views/progress/mod.rs @@ -685,7 +685,7 @@ mod tests { #[test] fn it_should_format_seconds_correctly() { - let duration = Duration::from_millis(1000); + let duration = Duration::from_secs(1); assert_eq!(format_duration(duration), "1.0s"); let duration = Duration::from_millis(2345); diff --git a/src/testing/e2e/containers/image_builder.rs b/src/testing/e2e/containers/image_builder.rs index b31a5874..27a44d00 100644 --- a/src/testing/e2e/containers/image_builder.rs +++ b/src/testing/e2e/containers/image_builder.rs @@ -155,7 +155,7 @@ impl ContainerImageBuilder { tag: "latest".to_string(), dockerfile_path: None, context_path: PathBuf::from("."), - build_timeout: Duration::from_secs(300), + build_timeout: Duration::from_mins(5), } } @@ -433,7 +433,7 @@ mod tests { assert_eq!(builder.tag(), "latest"); assert_eq!(builder.dockerfile_path(), None); assert_eq!(builder.context_path(), &PathBuf::from(".")); - assert_eq!(builder.build_timeout(), Duration::from_secs(300)); + assert_eq!(builder.build_timeout(), Duration::from_mins(5)); } #[test] @@ -480,7 +480,7 @@ mod tests { #[test] fn it_should_configure_build_timeout() { - let timeout = Duration::from_secs(600); + let timeout = Duration::from_mins(10); let builder = ContainerImageBuilder::new().with_build_timeout(timeout); assert_eq!(builder.build_timeout(), timeout); @@ -493,7 +493,7 @@ mod tests { .with_tag("v2.0") .with_dockerfile(PathBuf::from("custom/Dockerfile")) .with_context(PathBuf::from("./src")) - .with_build_timeout(Duration::from_secs(900)); + .with_build_timeout(Duration::from_mins(15)); assert_eq!(builder.image_name(), Some("my-app")); assert_eq!(builder.tag(), "v2.0"); @@ -503,7 +503,7 @@ mod tests { Some(&PathBuf::from("custom/Dockerfile")) ); assert_eq!(builder.context_path(), &PathBuf::from("./src")); - assert_eq!(builder.build_timeout(), Duration::from_secs(900)); + assert_eq!(builder.build_timeout(), Duration::from_mins(15)); } #[test] diff --git a/src/testing/e2e/containers/timeout.rs b/src/testing/e2e/containers/timeout.rs index 3014a0e0..fe5b959a 100644 --- a/src/testing/e2e/containers/timeout.rs +++ b/src/testing/e2e/containers/timeout.rs @@ -24,10 +24,10 @@ pub struct ContainerTimeouts { impl Default for ContainerTimeouts { fn default() -> Self { Self { - docker_build: Duration::from_secs(300), // 5 minutes - container_start: Duration::from_secs(60), // 1 minute - ssh_ready: Duration::from_secs(30), // 30 seconds - ssh_setup: Duration::from_secs(15), // 15 seconds + docker_build: Duration::from_mins(5), // 5 minutes + container_start: Duration::from_mins(1), // 1 minute + ssh_ready: Duration::from_secs(30), // 30 seconds + ssh_setup: Duration::from_secs(15), // 15 seconds } } } diff --git a/src/testing/e2e/tasks/black_box/test_runner.rs b/src/testing/e2e/tasks/black_box/test_runner.rs index b8a14e33..3a319299 100644 --- a/src/testing/e2e/tasks/black_box/test_runner.rs +++ b/src/testing/e2e/tasks/black_box/test_runner.rs @@ -341,8 +341,7 @@ impl E2eTestRunner { pub fn run_services(&self) -> Result<()> { // Check if run should be skipped (Docker not available in test container) let skip_run = std::env::var("TORRUST_TD_SKIP_RUN_IN_CONTAINER") - .map(|v| v.to_lowercase() == "true") - .unwrap_or(false); + .is_ok_and(|v| v.to_lowercase() == "true"); if skip_run { info!(