Skip to content

Commit

Permalink
Don't unarchive a timeline if its ancestor is archived (#8853)
Browse files Browse the repository at this point in the history
If a timeline unarchival request comes in, give an error if the parent
timeline is archived. This prevents us from the situation of having an
archived timeline with children that are not archived.

Follow up of #8824

Part of #8088

---------

Co-authored-by: Joonas Koivunen <joonas@neon.tech>
  • Loading branch information
arpad-m and koivunej authored Aug 29, 2024
1 parent c748140 commit 96b5c4d
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 3 deletions.
3 changes: 3 additions & 0 deletions pageserver/src/http/routes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,9 @@ impl From<crate::tenant::TimelineArchivalError> for ApiError {
match value {
NotFound => ApiError::NotFound(anyhow::anyhow!("timeline not found").into()),
Timeout => ApiError::Timeout("hit pageserver internal timeout".into()),
e @ HasArchivedParent(_) => {
ApiError::PreconditionFailed(e.to_string().into_boxed_str())
}
HasUnarchivedChildren(children) => ApiError::PreconditionFailed(
format!(
"Cannot archive timeline which has non-archived child timelines: {children:?}"
Expand Down
19 changes: 16 additions & 3 deletions pageserver/src/tenant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,9 @@ pub enum TimelineArchivalError {
#[error("Timeout")]
Timeout,

#[error("ancestor is archived: {}", .0)]
HasArchivedParent(TimelineId),

#[error("HasUnarchivedChildren")]
HasUnarchivedChildren(Vec<TimelineId>),

Expand All @@ -524,6 +527,7 @@ impl Debug for TimelineArchivalError {
match self {
Self::NotFound => write!(f, "NotFound"),
Self::Timeout => write!(f, "Timeout"),
Self::HasArchivedParent(p) => f.debug_tuple("HasArchivedParent").field(p).finish(),
Self::HasUnarchivedChildren(c) => {
f.debug_tuple("HasUnarchivedChildren").field(c).finish()
}
Expand Down Expand Up @@ -1369,11 +1373,20 @@ impl Tenant {
let timeline = {
let timelines = self.timelines.lock().unwrap();

let timeline = match timelines.get(&timeline_id) {
Some(t) => t,
None => return Err(TimelineArchivalError::NotFound),
let Some(timeline) = timelines.get(&timeline_id) else {
return Err(TimelineArchivalError::NotFound);
};

if state == TimelineArchivalState::Unarchived {
if let Some(ancestor_timeline) = timeline.ancestor_timeline() {
if ancestor_timeline.is_archived() == Some(true) {
return Err(TimelineArchivalError::HasArchivedParent(
ancestor_timeline.timeline_id,
));
}
}
}

// Ensure that there are no non-archived child timelines
let children: Vec<TimelineId> = timelines
.iter()
Expand Down
5 changes: 5 additions & 0 deletions pageserver/src/tenant/timeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,11 @@ impl Timeline {
.map(|ancestor| ancestor.timeline_id)
}

/// Get the ancestor timeline
pub(crate) fn ancestor_timeline(&self) -> Option<&Arc<Timeline>> {
self.ancestor_timeline.as_ref()
}

/// Get the bytes written since the PITR cutoff on this branch, and
/// whether this branch's ancestor_lsn is within its parent's PITR.
pub(crate) fn get_pitr_history_stats(&self) -> (u64, bool) {
Expand Down
26 changes: 26 additions & 0 deletions test_runner/regress/test_timeline_archive.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,29 @@ def test_timeline_archive(neon_simple_env: NeonEnv):
timeline_id=parent_timeline_id,
state=TimelineArchivalState.ARCHIVED,
)

# Test that the leaf can't be unarchived
with pytest.raises(
PageserverApiException,
match="ancestor is archived",
) as exc:
assert timeline_path.exists()

ps_http.timeline_archival_config(
tenant_id=env.initial_tenant,
timeline_id=leaf_timeline_id,
state=TimelineArchivalState.UNARCHIVED,
)

# Unarchive works for the leaf if the parent gets unarchived first
ps_http.timeline_archival_config(
tenant_id=env.initial_tenant,
timeline_id=parent_timeline_id,
state=TimelineArchivalState.UNARCHIVED,
)

ps_http.timeline_archival_config(
tenant_id=env.initial_tenant,
timeline_id=leaf_timeline_id,
state=TimelineArchivalState.UNARCHIVED,
)

1 comment on commit 96b5c4d

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3801 tests run: 3694 passed, 1 failed, 106 skipped (full report)


Failures on Postgres 16

# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_timeline_copy[debug-pg16-500000]"
Flaky tests (1)

Postgres 15

Test coverage report is not available

The comment gets automatically updated with the latest test results
96b5c4d at 2024-08-29T14:08:45.695Z :recycle:

Please sign in to comment.