storcon: timeline table, deletion and creation #10440

arpad-m · 2025-01-18T03:01:29Z

This PR adds an optional setting to the storage controller's timeline creation endpoint to also create them on the safekeepers.

In order to support individual safekeepers going offline, we immediately return from the http endpoint if news from a quorum of safekeepers reaches us that they have successfully created the timeline.

We introduce a new safekeeper reconciler task to the storage controller which reconciles outstanding timelines. For durably storing which timelines to reconcile, it uses two specific columns in the newly added timelines table: status_kind and status. Former is for quickly determining which status we want to do reconciliations for (we ask the db to maintain an index of it so that we can query specific states), latter is for status specific metadata in the json format.

TODO until ready for review:

notification API
tests

For follow-ups:

at tenant deletions, issue timeline deletion directly, instead of going via the reconciler: block on all timeline deletions completing enough (at least one SK returns success)
race-condition-free state setting function for timeline
cache safekeepers list instead of reloading them from the db every time
use utilization endpoint instead of COUNT(*) for determining target safekeepers
pruning task to delete timelines with old enough deleted_at

Part of #9011

github-actions · 2025-01-21T15:48:54Z

7403 tests run: 7016 passed, 0 failed, 387 skipped (full report)

Flaky tests (6)

Postgres 17

test_compute_pageserver_connection_stress: debug-x86-64-without-lfc
test_location_conf_churn[1]: release-arm64-without-lfc
test_metrics_normal_work: release-arm64-with-lfc
test_timeline_archival_chaos: release-arm64-without-lfc

Postgres 16

test_metrics_normal_work: release-arm64-with-lfc

Postgres 14

test_pageserver_gc_compaction_idempotent[after_restart]: release-arm64-with-lfc

Code coverage* (full report)

functions: 33.3% (8495 of 25545 functions)
lines: 49.0% (71476 of 145859 lines)

* collected from Rust tests only

_{The comment gets automatically updated with the latest test results
c660d78 at 2025-01-27T20:03:25.839Z :recycle:}

Add APIs for timeline creation and deletion to the safekeeper client crate. Going to be used later in #10440. Split off from #10440. Part of #9011

problame

Just took a cursory look to prepare for tomorrow's meeting.

problame · 2025-01-23T18:02:18Z

storage_controller/src/service.rs

+                // Can't do return Err because of async block, must do ? plus unreachable!()
+                return Err(ApiError::InternalServerError(anyhow!(
+                    "import pgdata doesn't specify the start lsn, aborting creation on safekeepers"
+                )))?;


Don't understand this comment.

ah sorry I will remove it, it was from an earlier state where there was an async block instead of an async fn

problame · 2025-01-23T18:10:28Z

storage_controller/src/service.rs

+        // notify cplane about creation
+        // TODO


This todo indicates that this method is in the wrong place.

Really, this method belongs into the impl Reconciler, where we have self.compute_hook.notify(ShardUpdate{...}).
Which today calls cplane, but, architecturally, cplane doesn't actually need to know which safekeepers a compute should connect to; cplane only needs to funnel through the ShardUpdate to the compute. It could/should be an opaque blob to cplane.

In fact I've been thinking about extending compute_hook for this purpose because that one already more or less has a connection to the control plane.

As for where to put the hook call, I think we can already call the hook the moment a quorum of safekeepers is reached. How to determine that is a good question, I suppose we can run db queries for that. But it needs to be something that is race condition free i.e. we don't want two reconcilers finishing in parallel both issue the reconcile (or the opposite, two reconcilers finishing in parallel not issue reconciles).

problame · 2025-01-23T18:12:24Z

storage_controller/src/service.rs

+        tenant_id: TenantId,
+        timeline_info: &TimelineInfo,
+        create_mode: models::TimelineCreateRequestMode,
+    ) -> Result<(u32, Vec<NodeId>), ApiError> {


This naked u32 should be wrapped in a newtype so it's clear what it means.

Had to jump to caller to learn it's the safekeeper generation.

Generation numbers are super important to get right, correctness of the service depends on them.

problame · 2025-01-23T18:15:35Z

libs/pageserver_api/src/models.rs

+
+    pub safekeepers: Option<Vec<NodeId>>,
+    pub safekeepers_generation: Option<u32>,


I think it would actually be better to use the same data structure here that the reconciler upcall will use (the TODO: notify cplane in tenant_timeline_create_safekeepers_reconcile)

arssher

submitting superficial things on a first glance

arssher · 2025-01-23T14:27:16Z

libs/pageserver_api/src/models.rs

@@ -280,6 +280,18 @@ pub struct TimelineCreateRequest {
    pub new_timeline_id: TimelineId,
    #[serde(flatten)]
    pub mode: TimelineCreateRequestMode,
+    /// Whether to also create timeline on the safekeepers (specific to storcon API)


I was thinking to have this as a global command line argument -- it avoids cplane change and it doesn't seem useful to enable this only for some timelines.

arssher · 2025-01-23T14:30:55Z

libs/pageserver_api/src/models.rs

+    #[serde(flatten)]
+    pub timeline_info: TimelineInfo,
+
+    pub safekeepers: Option<Vec<NodeId>>,


We agreed that it might be useful to also pass hostname here, see
https://github.com/neondatabase/cloud/pull/23181/files#diff-e89538b4960047b678a0a0b78fc0cee4449b07a4d4e9c6e47b54549c8ef3f23b

arssher · 2025-01-23T14:37:37Z

storage_controller/src/persistence.rs

@@ -1135,6 +1198,189 @@ impl Persistence {
        })
        .await
    }
+
+    /// Timelines must be persisted before we schedule them for the first time.
+    pub(crate) async fn insert_timeline(&self, entry: TimelinePersistence) -> DatabaseResult<()> {


Let's call it upsert.

It's not really an upsert call though, it only inserts. If the timeline already exists, it returns an error.

I think an upsert like functionality is dangerous, especially for something that happens regularly. upsert APIs are prone to race conditions.

That being said, we should return something more nice than an error if the entry already exists, so that we can use this information to make higher level calls idempotent.

arssher · 2025-01-23T14:38:25Z

storage_controller/src/persistence.rs

+                    .do_nothing()
+                    .execute(conn)?;
+
+                if inserted_updated != 1 {


What is inserted_updated if DO NOTHING happened?

probably it's 0. yeah, this case should probably be improved.

arssher · 2025-01-23T14:43:39Z

storage_controller/src/safekeeper_client.rs

+
+macro_rules! measured_request {
+    ($name:literal, $method:expr, $node_id: expr, $invoke:expr) => {{
+        let labels = crate::metrics::PageserverRequestLabelGroup {


PageserverRequestLabelGroup

…hema

arpad-m force-pushed the arpad/sk_timelines_schema branch from 7fe0060 to 361f491 Compare January 21, 2025 14:55

arpad-m force-pushed the arpad/sk_timelines_schema branch from 46ddaaa to 0ec5046 Compare January 21, 2025 18:48

arpad-m mentioned this pull request Jan 22, 2025

Expose safekeeper APIs for creation and deletion #10478

Merged

arpad-m force-pushed the arpad/sk_timelines_schema branch from 9ddb0dc to 5bdda19 Compare January 22, 2025 17:04

arpad-m added 13 commits January 22, 2025 20:07

Add schema for timelines table

7d296b3

wip

3d81af8

wip

e35a726

wip

78c4a82

Move to different function, clears up things a little

e805058

persist

f0fe5fa

Draft for a reconciler

b5c2980

remove line

1ffe95c

Add status_kind

ebe9ba0

Implement the creation part of the reconciler

bf0d53a

Add support for timeline deletion

00380ce

Optional get_timeline result

a63153f

Mark timelines for deletion during tenant deletion

f0777cf

arpad-m force-pushed the arpad/sk_timelines_schema branch from 5bdda19 to a7bd98b Compare January 22, 2025 19:08

arpad-m mentioned this pull request Jan 22, 2025

Store timeline locations on storage controller (schema, insert, timeline creation and passthrough to the majority) #9011

Open

arpad-m added 2 commits January 22, 2025 20:35

clippy

e56aa82

deleted_at column instead of new_sk_set

7ec08ee

arpad-m force-pushed the arpad/sk_timelines_schema branch from a7bd98b to 7ec08ee Compare January 22, 2025 23:28

problame reviewed Jan 23, 2025

View reviewed changes

arssher approved these changes Jan 24, 2025

View reviewed changes

arpad-m added 4 commits January 25, 2025 03:33

Address simple review comments

986db00

Add SafekeeperGeneration

1745fe5

Merge remote-tracking branch 'origin/main' into arpad/sk_timelines_sc…

ad56b9f

…hema

Fixes due to merge

c660d78

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storcon: timeline table, deletion and creation #10440

storcon: timeline table, deletion and creation #10440

arpad-m commented Jan 18, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025 •

edited

Loading

Postgres 17

Postgres 16

Postgres 14

problame left a comment

problame Jan 23, 2025

arpad-m Jan 24, 2025

problame Jan 23, 2025

arpad-m Jan 25, 2025

problame Jan 23, 2025

problame Jan 23, 2025

arssher left a comment

arssher Jan 23, 2025

arssher Jan 23, 2025

arssher Jan 23, 2025

arpad-m Jan 24, 2025

arssher Jan 23, 2025

arpad-m Jan 24, 2025

arssher Jan 23, 2025


		pub safekeepers: Option<Vec<NodeId>>,
		pub safekeepers_generation: Option<u32>,

storcon: timeline table, deletion and creation #10440

Are you sure you want to change the base?

storcon: timeline table, deletion and creation #10440

Conversation

arpad-m commented Jan 18, 2025 • edited Loading

github-actions bot commented Jan 21, 2025 • edited Loading

7403 tests run: 7016 passed, 0 failed, 387 skipped (full report)

Postgres 17

Postgres 16

Postgres 14

Code coverage* (full report)

problame left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arssher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arpad-m commented Jan 18, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025 •

edited

Loading