Skip to content

Commit 4b31306

Browse files
committed
refactor: improve cleanup function naming and semantic clarity
- Rename cleanup_lingering_resources to preflight_cleanup_previous_resources - Rename cleanup_infrastructure to cleanup_test_infrastructure - Add separate stop_test_infrastructure function for containers - Distinguish between stopping containers (immediate) and cleanup (automatic) - Add comprehensive documentation explaining two-phase cleanup strategy - Clarify container vs VM management differences in documentation - Maintain API symmetry while being honest about actual operations This improves code clarity by making function names accurately reflect their actual behavior, especially the distinction between container stopping (explicit) and deletion (automatic via testcontainers).
1 parent b31b504 commit 4b31306

File tree

8 files changed

+131
-55
lines changed

8 files changed

+131
-55
lines changed

src/bin/e2e_config_tests.rs

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,28 @@
2525
//!
2626
//! ## Test Workflow
2727
//!
28-
//! 1. **Container setup** - Build and start Docker container using `docker/provisioned-instance`
29-
//! 2. **SSH verification** - Ensure container is ready for Ansible connectivity
30-
//! 3. **Configuration** - Apply Ansible playbooks to configure services
31-
//! 4. **Validation** - Verify deployments are working correctly
32-
//! 5. **Cleanup** - Stop and remove test containers
28+
//! 1. **Preflight cleanup** - Remove any artifacts from previous test runs that may have failed to clean up
29+
//! 2. **Container setup** - Build and start Docker container using `docker/provisioned-instance`
30+
//! 3. **SSH verification** - Ensure container is ready for Ansible connectivity
31+
//! 4. **Configuration** - Apply Ansible playbooks to configure services
32+
//! 5. **Validation** - Verify deployments are working correctly
33+
//! 6. **Stop container** - Stop the test container (deletion handled automatically by testcontainers)
34+
//! 7. **Test infrastructure cleanup** - No-op for containers (automatic), called for symmetry with VMs
35+
//!
36+
//! ## Two-Phase Cleanup Strategy
37+
//!
38+
//! The cleanup process happens in two distinct phases:
39+
//!
40+
//! - **Phase 1 - Preflight cleanup**: Removes artifacts from previous test runs that may have
41+
//! failed to clean up properly (executed at the start in main function)
42+
//! - **Phase 2 - Test infrastructure cleanup**: For containers, this is automatic via testcontainers
43+
//! (called as no-op in main function for symmetry with VM workflows)
44+
//!
45+
//! ## Container vs VM Management
46+
//!
47+
//! - **Container stopping**: Happens immediately after tests (in test function) for resource management
48+
//! - **Container cleanup**: Automatic via testcontainers library (no explicit action needed)
49+
//! - **Symmetry**: Both workflows have stop+cleanup phases, but container cleanup is automatic
3350
//!
3451
//! This approach addresses network connectivity issues with LXD VMs on GitHub Actions
3552
//! while maintaining comprehensive testing of the configuration and deployment phases.
@@ -51,7 +68,7 @@ use torrust_tracker_deploy::domain::{Environment, EnvironmentName};
5168
use torrust_tracker_deploy::e2e::context::{TestContext, TestContextType};
5269
use torrust_tracker_deploy::e2e::tasks::{
5370
container::{
54-
cleanup_infrastructure::cleanup_infrastructure,
71+
cleanup_infrastructure::{cleanup_test_infrastructure, stop_test_infrastructure},
5572
run_provision_simulation::run_provision_simulation,
5673
},
5774
preflight_cleanup,
@@ -126,10 +143,16 @@ pub async fn main() -> Result<()> {
126143
let test_context =
127144
TestContext::from_environment(false, environment, TestContextType::Container)?.init()?;
128145

129-
preflight_cleanup::cleanup_lingering_resources(&test_context)?;
146+
// Cleanup any artifacts from previous test runs that may have failed to clean up
147+
// This ensures a clean slate before starting new tests
148+
preflight_cleanup::preflight_cleanup_previous_resources(&test_context)?;
130149

131150
let test_result = run_configuration_tests(&test_context).await;
132151

152+
// Cleanup test infrastructure created during this test run
153+
// For containers, this is automatic via testcontainers (no-op), but called for symmetry with VMs
154+
cleanup_test_infrastructure();
155+
133156
let test_duration = test_start.elapsed();
134157

135158
info!(
@@ -178,8 +201,9 @@ async fn run_configuration_tests(test_context: &TestContext) -> Result<()> {
178201
)
179202
.await?;
180203

181-
// Step 4: Cleanup container
182-
cleanup_infrastructure(running_container);
204+
// Stop test infrastructure (container) created during this test run
205+
// This stops the container immediately after tests - deletion is automatic via testcontainers
206+
stop_test_infrastructure(running_container);
183207

184208
info!("Configuration tests completed successfully");
185209

src/bin/e2e_provision_tests.rs

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,19 @@
2828
//!
2929
//! ## Test Workflow
3030
//!
31-
//! 1. **Preflight cleanup** - Remove any lingering test resources
31+
//! 1. **Preflight cleanup** - Remove any artifacts from previous test runs that may have failed to clean up
3232
//! 2. **Infrastructure provisioning** - Create VMs/containers using `OpenTofu`
3333
//! 3. **Basic validation** - Verify VM is created and cloud-init completed
34-
//! 4. **Cleanup** - Remove test resources
34+
//! 4. **Test infrastructure cleanup** - Remove test resources created during this run
35+
//!
36+
//! ## Two-Phase Cleanup Strategy
37+
//!
38+
//! The cleanup process happens in two distinct phases:
39+
//!
40+
//! - **Phase 1 - Preflight cleanup**: Removes artifacts from previous test runs that may have
41+
//! failed to clean up properly (executed at the start in main function)
42+
//! - **Phase 2 - Test infrastructure cleanup**: Destroys resources created specifically during
43+
//! the current test run (executed at the end in main function)
3544
//!
3645
//! This split allows provisioning tests to run reliably on GitHub Actions
3746
//! while configuration tests can be handled separately with different infrastructure.
@@ -46,7 +55,8 @@ use tracing::{error, info};
4655
use torrust_tracker_deploy::domain::{Environment, EnvironmentName};
4756
use torrust_tracker_deploy::e2e::context::{TestContext, TestContextType};
4857
use torrust_tracker_deploy::e2e::tasks::virtual_machine::{
49-
cleanup_infrastructure::cleanup_infrastructure, preflight_cleanup::cleanup_lingering_resources,
58+
cleanup_infrastructure::cleanup_test_infrastructure,
59+
preflight_cleanup::preflight_cleanup_previous_resources,
5060
run_provision_command::run_provision_command,
5161
};
5262
use torrust_tracker_deploy::logging::{self, LogFormat};
@@ -127,14 +137,17 @@ pub async fn main() -> Result<()> {
127137
TestContext::from_environment(cli.keep, environment, TestContextType::VirtualMachine)?
128138
.init()?;
129139

130-
// Perform pre-flight cleanup to remove any lingering resources from interrupted tests
131-
cleanup_lingering_resources(&test_context)?;
140+
// Cleanup any artifacts from previous test runs that may have failed to clean up
141+
// This ensures a clean slate before starting new tests
142+
preflight_cleanup_previous_resources(&test_context)?;
132143

133144
let test_start = Instant::now();
134145

135146
let provision_result = run_provisioning_test(&test_context).await;
136147

137-
cleanup_infrastructure(&test_context);
148+
// Always cleanup test infrastructure created during this test run
149+
// This ensures proper resource cleanup regardless of test success or failure
150+
cleanup_test_infrastructure(&test_context);
138151

139152
let test_duration = test_start.elapsed();
140153

src/bin/e2e_tests_full.rs

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,20 @@
3535
//!
3636
//! ## Test Workflow
3737
//!
38-
//! 1. **Preflight cleanup** - Remove any lingering test resources
38+
//! 1. **Preflight cleanup** - Remove any artifacts from previous test runs that may have failed to clean up
3939
//! 2. **Infrastructure provisioning** - Create LXD VMs using `OpenTofu`
4040
//! 3. **Configuration** - Apply Ansible playbooks for software installation
4141
//! 4. **Validation** - Verify deployments are working correctly
42-
//! 5. **Cleanup** - Remove test resources
42+
//! 5. **Test infrastructure cleanup** - Remove test resources created during this run
43+
//!
44+
//! ## Two-Phase Cleanup Strategy
45+
//!
46+
//! The cleanup process happens in two distinct phases:
47+
//!
48+
//! - **Phase 1 - Preflight cleanup**: Removes artifacts from previous test runs that may have
49+
//! failed to clean up properly (executed at the start in main function)
50+
//! - **Phase 2 - Test infrastructure cleanup**: Destroys resources created specifically during
51+
//! the current test run (executed at the end in main function)
4352
//!
4453
//! The test suite supports different VM providers (LXD, Multipass) and includes
4554
//! comprehensive logging and error reporting.
@@ -57,8 +66,8 @@ use torrust_tracker_deploy::e2e::tasks::{
5766
run_configure_command::run_configure_command,
5867
run_test_command::run_test_command,
5968
virtual_machine::{
60-
cleanup_infrastructure::cleanup_infrastructure,
61-
preflight_cleanup::cleanup_lingering_resources,
69+
cleanup_infrastructure::cleanup_test_infrastructure,
70+
preflight_cleanup::preflight_cleanup_previous_resources,
6271
run_provision_command::run_provision_command,
6372
},
6473
};
@@ -135,8 +144,9 @@ pub async fn main() -> Result<()> {
135144
TestContext::from_environment(cli.keep, environment, TestContextType::VirtualMachine)?
136145
.init()?;
137146

138-
// Perform pre-flight cleanup to remove any lingering resources from interrupted tests
139-
cleanup_lingering_resources(&test_context)?;
147+
// Cleanup any artifacts from previous test runs that may have failed to clean up
148+
// This ensures a clean slate before starting new tests
149+
preflight_cleanup_previous_resources(&test_context)?;
140150

141151
let test_start = Instant::now();
142152

@@ -147,7 +157,9 @@ pub async fn main() -> Result<()> {
147157
Err(_) => Ok(()), // Skip validation if deployment failed
148158
};
149159

150-
cleanup_infrastructure(&test_context);
160+
// Always cleanup test infrastructure created during this test run
161+
// This ensures proper resource cleanup regardless of test success or failure
162+
cleanup_test_infrastructure(&test_context);
151163

152164
let test_duration = test_start.elapsed();
153165

Lines changed: 43 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,43 +1,48 @@
1-
//! Container cleanup task for E2E testing
1+
//! Container management task for E2E testing
22
//!
3-
//! This module provides the E2E testing task for cleaning up Docker containers
4-
//! after test completion. It handles the transition from running to stopped
5-
//! container state and ensures proper resource cleanup.
3+
//! This module provides container management functionality for E2E testing,
4+
//! including stopping containers and handling automatic cleanup through testcontainers.
5+
//! It distinguishes between stopping containers (immediate) and cleanup (automatic).
66
//!
77
//! ## Key Operations
88
//!
9-
//! - Stops running Docker containers
10-
//! - Cleans up container resources
11-
//! - Provides logging for cleanup operations
9+
//! - **Stop**: Immediately stops running Docker containers
10+
//! - **Cleanup**: Automatic deletion handled by testcontainers library
11+
//! - Provides logging for container operations
12+
//!
13+
//! ## Container vs VM Difference
14+
//!
15+
//! Unlike VMs that require explicit infrastructure destruction, Docker containers
16+
//! managed by testcontainers are automatically deleted when they go out of scope.
17+
//! This module provides both stop and cleanup functions for API symmetry.
1218
//!
1319
//! ## Integration
1420
//!
15-
//! This task is specifically designed for container-based E2E testing workflows
16-
//! and should be used as the final step in container test scenarios.
21+
//! This task is specifically designed for container-based E2E testing workflows.
1722
1823
use tracing::info;
1924

2025
use crate::e2e::containers::RunningProvisionedContainer;
2126

22-
/// Cleanup a running Docker container
27+
/// Stop a running Docker container
2328
///
24-
/// This function stops a running Docker container and performs cleanup operations.
25-
/// It transitions the container from running to stopped state and ensures proper
26-
/// resource cleanup.
29+
/// This function stops a running Docker container, transitioning it from running
30+
/// to stopped state. The actual container deletion is handled automatically by
31+
/// the testcontainers library when the container goes out of scope.
2732
///
2833
/// # Arguments
2934
///
30-
/// * `running_container` - The running container to be cleaned up
35+
/// * `running_container` - The running container to be stopped
3136
///
3237
/// # Returns
3338
///
3439
/// This function consumes the running container and returns nothing, ensuring
35-
/// the container cannot be used after cleanup.
40+
/// the container cannot be used after stopping.
3641
///
3742
/// # Example
3843
///
3944
/// ```rust,no_run
40-
/// use torrust_tracker_deploy::e2e::tasks::container::cleanup_infrastructure::cleanup_infrastructure;
45+
/// use torrust_tracker_deploy::e2e::tasks::container::cleanup_infrastructure::stop_test_infrastructure;
4146
/// use torrust_tracker_deploy::e2e::containers::StoppedProvisionedContainer;
4247
///
4348
/// #[tokio::main]
@@ -47,17 +52,17 @@ use crate::e2e::containers::RunningProvisionedContainer;
4752
///
4853
/// // ... perform tests ...
4954
///
50-
/// cleanup_infrastructure(running_container);
51-
/// println!("Container cleanup completed");
55+
/// stop_test_infrastructure(running_container);
56+
/// println!("Container stopped successfully");
5257
/// Ok(())
5358
/// }
5459
/// ```
55-
pub fn cleanup_infrastructure(running_container: RunningProvisionedContainer) {
60+
pub fn stop_test_infrastructure(running_container: RunningProvisionedContainer) {
5661
let container_id = running_container.container_id().to_string();
5762

5863
info!(
5964
container_id = %container_id,
60-
"Starting container cleanup"
65+
"Stopping test container"
6166
);
6267

6368
// Transition container from running to stopped state
@@ -66,6 +71,23 @@ pub fn cleanup_infrastructure(running_container: RunningProvisionedContainer) {
6671
info!(
6772
container_id = %container_id,
6873
status = "success",
69-
"Container cleanup completed successfully"
74+
"Container stopped successfully - deletion handled automatically by testcontainers"
75+
);
76+
}
77+
78+
/// Cleanup test infrastructure for containers (no-op)
79+
///
80+
/// For containers managed by testcontainers, cleanup (deletion) is handled automatically
81+
/// when containers go out of scope. This function is provided for API symmetry with
82+
/// VM-based workflows that require explicit infrastructure destruction.
83+
///
84+
/// # Note
85+
///
86+
/// This function does nothing for containers. The actual cleanup is automatic.
87+
/// Use `stop_test_infrastructure()` to stop running containers.
88+
pub fn cleanup_test_infrastructure() {
89+
info!(
90+
operation = "container_cleanup",
91+
"Container cleanup is automatic via testcontainers - no action needed"
7092
);
7193
}

src/e2e/tasks/container/preflight_cleanup.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ use tracing::{info, warn};
1414

1515
/// Performs pre-flight cleanup for Docker-based E2E tests
1616
///
17-
/// This function is specifically designed for Docker-based E2E tests that use
17+
/// This function cleans up any artifacts remaining from previous test runs that may have
18+
/// failed to clean up properly. It's designed for Docker-based E2E tests that use
1819
/// testcontainers for container lifecycle management. It cleans up directories
1920
/// and any hanging Docker containers from previous interrupted test runs.
2021
///
@@ -29,7 +30,9 @@ use tracing::{info, warn};
2930
/// # Errors
3031
///
3132
/// Returns an error if directory cleanup fails and would prevent new test runs.
32-
pub fn cleanup_lingering_resources(env: &TestContext) -> Result<(), PreflightCleanupError> {
33+
pub fn preflight_cleanup_previous_resources(
34+
env: &TestContext,
35+
) -> Result<(), PreflightCleanupError> {
3336
info!(
3437
operation = "preflight_cleanup_docker",
3538
"Starting pre-flight cleanup for Docker-based E2E tests"
@@ -49,6 +52,7 @@ pub fn cleanup_lingering_resources(env: &TestContext) -> Result<(), PreflightCle
4952
status = "success",
5053
"Pre-flight cleanup for Docker-based E2E tests completed successfully"
5154
);
55+
5256
Ok(())
5357
}
5458

src/e2e/tasks/preflight_cleanup.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use crate::infrastructure::adapters::opentofu::EmergencyDestroyError;
1111
use tracing::{info, warn};
1212

1313
// Re-export functions from the new modular structure for backward compatibility
14-
pub use crate::e2e::tasks::container::preflight_cleanup::cleanup_lingering_resources;
14+
pub use crate::e2e::tasks::container::preflight_cleanup::preflight_cleanup_previous_resources;
1515

1616
/// Errors that can occur during pre-flight cleanup operations
1717
#[derive(Debug)]

src/e2e/tasks/virtual_machine/cleanup_infrastructure.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,12 @@ use tracing::{info, warn};
2323

2424
use crate::e2e::context::TestContext;
2525

26-
/// Clean up test infrastructure
26+
/// Clean up test infrastructure created during VM-based E2E tests
2727
///
28-
/// This function destroys the test infrastructure using `OpenTofu`.
29-
/// If `keep_env` is set in the environment configuration, the cleanup
30-
/// is skipped and the environment is preserved.
28+
/// This function destroys the test infrastructure using `OpenTofu` for resources
29+
/// that were created specifically during this test run. If `keep_env` is set in
30+
/// the environment configuration, the cleanup is skipped and the environment is
31+
/// preserved for debugging purposes.
3132
///
3233
/// # Arguments
3334
///
@@ -39,7 +40,7 @@ use crate::e2e::context::TestContext;
3940
/// - Otherwise, attempts to destroy infrastructure using `OpenTofu`
4041
/// - Logs success or failure appropriately
4142
/// - Does not return errors - failures are logged as warnings
42-
pub fn cleanup_infrastructure(test_context: &TestContext) {
43+
pub fn cleanup_test_infrastructure(test_context: &TestContext) {
4344
if test_context.keep_env {
4445
let instance_name = &test_context.environment.instance_name();
4546
info!(

src/e2e/tasks/virtual_machine/preflight_cleanup.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@ use tracing::{info, warn};
1515

1616
/// Performs comprehensive pre-flight cleanup for VM-based E2E tests
1717
///
18-
/// This function cleans up any lingering resources from previous test runs
19-
/// that may have been interrupted before cleanup, including directories,
20-
/// `OpenTofu` infrastructure, and LXD resources.
18+
/// This function cleans up any artifacts remaining from previous test runs that may have
19+
/// been interrupted before cleanup, including directories, `OpenTofu` infrastructure,
20+
/// and LXD resources. This ensures a clean slate for new test execution.
2121
///
2222
/// # Arguments
2323
///
@@ -30,7 +30,7 @@ use tracing::{info, warn};
3030
/// # Errors
3131
///
3232
/// Returns an error if cleanup fails and would prevent new test runs.
33-
pub fn cleanup_lingering_resources(
33+
pub fn preflight_cleanup_previous_resources(
3434
test_context: &TestContext,
3535
) -> Result<(), PreflightCleanupError> {
3636
info!(

0 commit comments

Comments
 (0)