diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index af83e03599..dd153f880c 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -1,7 +1,7 @@ //! This module acts as a switchboard to access different repositories managed by this //! page server. -use std::cell::Cell; +use scopeguard::ScopeGuard; use std::collections::{hash_map, HashMap}; use std::ffi::OsStr; use std::path::Path; @@ -280,29 +280,25 @@ pub async fn create_tenant( ctx: &RequestContext, ) -> Result, TenantMapInsertError> { tenant_map_insert(tenant_id, |vacant_entry| { - let succeed = Cell::new(false); - // We're holding the tenants lock in write mode while doing local IO. // If this section ever becomes contentious, introduce a new `TenantState::Creating` // and do the work in that state. let tenant_directory = super::create_tenant_files(conf, tenant_conf, tenant_id, CreateTenantFilesMode::Create)?; - scopeguard::defer! { - if !succeed.get() { - std::fs::remove_dir_all(&tenant_directory).ok(); // ignore the error + let guard_fs = scopeguard::guard((), |_| { + if let Err(e) = std::fs::remove_dir_all(&tenant_directory) { + // log the error but not throwing it somewhere + warn!("failed to cleanup when tenant {tenant_id} fails to start: {e}"); } - } + }); let created_tenant = - schedule_local_tenant_processing(conf, &tenant_directory, remote_storage, ctx)?; + schedule_local_tenant_processing(conf, &tenant_directory, remote_storage, ctx)?; + let created_tenant = scopeguard::guard(created_tenant, |tenant| { + // As we might have removed the directory, the tenant should directly go into the broken state. + tenant.set_broken("failed to create".into()); + }); - let tenant_cloned = Arc::clone(&created_tenant); - scopeguard::defer! { - if !succeed.get() { - // As we might have removed the directory, the tenant should directly go into the broken state. - tenant_cloned.set_broken("failed to create".into()); - } - } fail::fail_point!("tenant-create-fail", |_| { anyhow::bail!("failpoint: tenant-create-fail"); @@ -316,9 +312,9 @@ pub async fn create_tenant( vacant_entry.insert(Arc::clone(&created_tenant)); - succeed.set(true); + ScopeGuard::into_inner(guard_fs); - Ok(created_tenant) + Ok(ScopeGuard::into_inner(created_tenant)) }).await } diff --git a/test_runner/regress/test_tenants.py b/test_runner/regress/test_tenants.py index 7c056fc262..683b766368 100644 --- a/test_runner/regress/test_tenants.py +++ b/test_runner/regress/test_tenants.py @@ -400,13 +400,11 @@ def test_pageserver_with_empty_tenants( ), f"Tenant {tenant_without_timelines_dir} should have metric as broken" -# Check that empty tenants work with or without the remote storage -@pytest.mark.parametrize( - "remote_storage_kind", available_remote_storages() + [RemoteStorageKind.NOOP] -) -def test_pageserver_create_tenants_fail( +@pytest.mark.parametrize("remote_storage_kind", [RemoteStorageKind.LOCAL_FS]) +def test_failed_tenant_directory_is_removed( neon_env_builder: NeonEnvBuilder, remote_storage_kind: RemoteStorageKind ): + """Tenants which fail to be created are cleaned up from disk and left broken""" neon_env_builder.enable_remote_storage( remote_storage_kind=remote_storage_kind, test_name="test_pageserver_create_tenants_fail", @@ -417,11 +415,14 @@ def test_pageserver_create_tenants_fail( env.pageserver.allowed_errors.append(".*tenant-create-fail.*") env.pageserver.allowed_errors.append(".*Tenant is already in Broken state.*") env.pageserver.allowed_errors.append(".*could not load tenant.*") + env.pageserver.allowed_errors.append(".*InternalServerError.*") client = env.pageserver.http_client() client.configure_failpoints(("tenant-create-fail", "return")) - tenant_id = TenantId("deadbeefdeadbeefdeadbeefdeadbeef") + tenant_id = TenantId.generate() with pytest.raises(PageserverApiException, match="failpoint: tenant-create-fail"): client.tenant_create(tenant_id) assert not (env.repo_dir / "tenants" / str(tenant_id)).exists() + with pytest.raises(PageserverApiException, match="Tenant .* not found"): + client.tenant_status(tenant_id)