diff --git a/pageserver/src/http/openapi_spec.yml b/pageserver/src/http/openapi_spec.yml index 2098f848d5..b8c3bffcd5 100644 --- a/pageserver/src/http/openapi_spec.yml +++ b/pageserver/src/http/openapi_spec.yml @@ -183,6 +183,12 @@ paths: application/json: schema: $ref: "#/components/schemas/ForbiddenError" + "404": + description: Timeline not found + content: + application/json: + schema: + $ref: "#/components/schemas/NotFoundError" "500": description: Generic operation error content: @@ -383,6 +389,12 @@ paths: application/json: schema: $ref: "#/components/schemas/ForbiddenError" + "404": + description: Tenant not found + content: + application/json: + schema: + $ref: "#/components/schemas/NotFoundError" "500": description: Generic operation error content: diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 04b7928d31..ba53729ea9 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -131,6 +131,29 @@ impl From for ApiError { } } +impl From for ApiError { + fn from(value: crate::tenant::DeleteTimelineError) -> Self { + use crate::tenant::DeleteTimelineError::*; + match value { + NotFound => ApiError::NotFound(anyhow::anyhow!("timeline not found")), + HasChildren => ApiError::BadRequest(anyhow::anyhow!( + "Cannot delete timeline which has child timelines" + )), + Other(e) => ApiError::InternalServerError(e), + } + } +} + +impl From for ApiError { + fn from(value: crate::tenant::mgr::DeleteTimelineError) -> Self { + use crate::tenant::mgr::DeleteTimelineError::*; + match value { + Tenant(t) => ApiError::from(t), + Timeline(t) => ApiError::from(t), + } + } +} + // Helper function to construct a TimelineInfo struct for a timeline async fn build_timeline_info( timeline: &Arc, diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index b462c93b2d..0a167fd787 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -431,6 +431,16 @@ remote: } } +#[derive(Debug, thiserror::Error)] +pub enum DeleteTimelineError { + #[error("NotFound")] + NotFound, + #[error("HasChildren")] + HasChildren, + #[error(transparent)] + Other(#[from] anyhow::Error), +} + struct RemoteStartupData { index_part: IndexPart, remote_metadata: TimelineMetadata, @@ -1307,7 +1317,7 @@ impl Tenant { &self, timeline_id: TimelineId, _ctx: &RequestContext, - ) -> anyhow::Result<()> { + ) -> Result<(), DeleteTimelineError> { // Transition the timeline into TimelineState::Stopping. // This should prevent new operations from starting. let timeline = { @@ -1319,13 +1329,13 @@ impl Tenant { .iter() .any(|(_, entry)| entry.get_ancestor_timeline_id() == Some(timeline_id)); - anyhow::ensure!( - !children_exist, - "Cannot delete timeline which has child timelines" - ); + if children_exist { + return Err(DeleteTimelineError::HasChildren); + } + let timeline_entry = match timelines.entry(timeline_id) { Entry::Occupied(e) => e, - Entry::Vacant(_) => bail!("timeline not found"), + Entry::Vacant(_) => return Err(DeleteTimelineError::NotFound), }; let timeline = Arc::clone(timeline_entry.get()); diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index 26a2bb972c..4971186206 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -321,11 +321,20 @@ pub async fn get_tenant( } } +#[derive(Debug, thiserror::Error)] +pub enum DeleteTimelineError { + #[error("Tenant {0}")] + Tenant(#[from] TenantStateError), + + #[error("Timeline {0}")] + Timeline(#[from] crate::tenant::DeleteTimelineError), +} + pub async fn delete_timeline( tenant_id: TenantId, timeline_id: TimelineId, ctx: &RequestContext, -) -> Result<(), TenantStateError> { +) -> Result<(), DeleteTimelineError> { let tenant = get_tenant(tenant_id, true).await?; tenant.delete_timeline(timeline_id, ctx).await?; Ok(()) diff --git a/test_runner/regress/test_timeline_delete.py b/test_runner/regress/test_timeline_delete.py index b9c4f5b83f..30d894e04c 100644 --- a/test_runner/regress/test_timeline_delete.py +++ b/test_runner/regress/test_timeline_delete.py @@ -25,9 +25,11 @@ def test_timeline_delete(neon_simple_env: NeonEnv): with pytest.raises( PageserverApiException, match=f"NotFound: tenant {invalid_tenant_id}", - ): + ) as exc: ps_http.timeline_delete(tenant_id=invalid_tenant_id, timeline_id=invalid_timeline_id) + assert exc.value.status_code == 404 + # construct pair of branches to validate that pageserver prohibits # deletion of ancestor timelines when they have child branches parent_timeline_id = env.neon_cli.create_branch("test_ancestor_branch_delete_parent", "empty") @@ -39,7 +41,7 @@ def test_timeline_delete(neon_simple_env: NeonEnv): ps_http = env.pageserver.http_client() with pytest.raises( PageserverApiException, match="Cannot delete timeline which has child timelines" - ): + ) as exc: timeline_path = ( env.repo_dir / "tenants" @@ -53,6 +55,8 @@ def test_timeline_delete(neon_simple_env: NeonEnv): assert not timeline_path.exists() + assert exc.value.status_code == 400 + timeline_path = ( env.repo_dir / "tenants" / str(env.initial_tenant) / "timelines" / str(leaf_timeline_id) ) @@ -71,7 +75,7 @@ def test_timeline_delete(neon_simple_env: NeonEnv): with pytest.raises( PageserverApiException, match=f"Timeline {env.initial_tenant}/{leaf_timeline_id} was not found", - ): + ) as exc: ps_http.timeline_detail(env.initial_tenant, leaf_timeline_id) # FIXME leaves tenant without timelines, should we prevent deletion of root timeline? @@ -80,3 +84,5 @@ def test_timeline_delete(neon_simple_env: NeonEnv): interval=0.2, func=lambda: ps_http.timeline_delete(env.initial_tenant, parent_timeline_id), ) + + assert exc.value.status_code == 404