refactor(page_service): Timeline gate guard holding + cancellation + shutdown #8339

problame · 2024-07-10T13:44:55Z

Since the introduction of sharding, the protocol handling loop in handle_pagerequests cannot know anymore which concrete Tenant/Timeline object any of the incoming PagestreamFeMessage resolves to.
In fact, one message might resolve to one Tenant/Timeline while
the next one may resolve to another one.

To avoid going to tenant manager, we added the shard_timelines which acted as an ever-growing cache that held timeline gate guards open for the lifetime of the connection.
The consequence of holding the gate guards open was that we had to be sensitive to every cached Timeline::cancel on each interaction with the network connection, so that Timeline shutdown would not have to wait for network connection interaction.

We can do better than that, meaning more efficiency & better abstraction.
I proposed a sketch for it in

Code RFC: decouple page_service from Mgr/Tenant/Timeline lifecycle #8286

and this PR implements an evolution of that sketch.

The main idea is is that mod page_service shall be solely concerned with the following:

receiving requests by speaking the protocol / pagestream subprotocol
dispatching the request to a corresponding method on the correct shard/Timeline object
sending response by speaking the protocol / pagestream subprotocol.

The cancellation sensitivity responsibilities are clear cut:

while in page_service code, sensitivity to page_service cancellation is sufficient
while in Timeline code, sensitivity to Timeline::cancel is sufficient

To enforce these responsibilities, we introduce the notion of a timeline::handle::Handle to a Timeline object that is checked out from a timeline::handle::Cache for each request.
The Handle derefs to Timeline and is supposed to be used for a single async method invocation on Timeline.
See the lengthy doc comment in mod handle for details of the design.

The remaining use of the `Tenant` object is to check `tenant.cancel`. That check is incorrect [if the pageserver hosts multiple shards](#7427 (comment)). I'll fix that in a future PR where I completely eliminate the holding of `Tenant/Timeline` objects across requests. See [my code RFC](#8286) for the high level idea.

…sponse code

github-actions · 2024-07-11T16:04:35Z

3150 tests run: 3029 passed, 0 failed, 121 skipped (full report)

Code coverage* (full report)

functions: 32.7% (7069 of 21609 functions)
lines: 50.1% (56430 of 112548 lines)

* collected from Rust tests only

_{The comment gets automatically updated with the latest test results
38b0f3c at 2024-07-31T14:17:39.538Z :recycle:}

…cess_query

…ler::process_query" This reverts commit fef082b. Actually, we can just store the cancellation token in PageServiceHandler

… worker

This operation isn't used in practice, so let's remove it. Context: in #8339

Trouble with the sync mutex now :( Async mutex doesn't work because we need to lock from destructors.

This reverts commit dfe1120.

This reverts commit 17a0f64.

pageserver/src/page_service.rs

jcsp

This approach makes sense.

It's quite verbose, but that's justified by unit testing & the general encapsulation of the cache/handle concept in handle.rs

Just one request for change: let's not decrease the active tenant timeout in this PR.

…ment)

…shutdown (#8339) Since the introduction of sharding, the protocol handling loop in `handle_pagerequests` cannot know anymore which concrete `Tenant`/`Timeline` object any of the incoming `PagestreamFeMessage` resolves to. In fact, one message might resolve to one `Tenant`/`Timeline` while the next one may resolve to another one. To avoid going to tenant manager, we added the `shard_timelines` which acted as an ever-growing cache that held timeline gate guards open for the lifetime of the connection. The consequence of holding the gate guards open was that we had to be sensitive to every cached `Timeline::cancel` on each interaction with the network connection, so that Timeline shutdown would not have to wait for network connection interaction. We can do better than that, meaning more efficiency & better abstraction. I proposed a sketch for it in * #8286 and this PR implements an evolution of that sketch. The main idea is is that `mod page_service` shall be solely concerned with the following: 1. receiving requests by speaking the protocol / pagestream subprotocol 2. dispatching the request to a corresponding method on the correct shard/`Timeline` object 3. sending response by speaking the protocol / pagestream subprotocol. The cancellation sensitivity responsibilities are clear cut: * while in `page_service` code, sensitivity to page_service cancellation is sufficient * while in `Timeline` code, sensitivity to `Timeline::cancel` is sufficient To enforce these responsibilities, we introduce the notion of a `timeline::handle::Handle` to a `Timeline` object that is checked out from a `timeline::handle::Cache` for **each request**. The `Handle` derefs to `Timeline` and is supposed to be used for a single async method invocation on `Timeline`. See the lengthy doc comment in `mod handle` for details of the design.

…ation + shutdown (#8339)" This reverts commit 4e3b70e.

pageserver/src/page_service.rs

We've noticed increased memory usage with the latest release. Drain the joinset of `page_service` connection handlers to avoid leaking them until shutdown. An alternative would be to use a TaskTracker. TaskTracker was not discussed in original PR #8339 review, so not hot fixing it in here either.

… to dis-incentivize global tasks via task_mgr in the future (As of #8339 all remaining task_mgr usage is tenant or timeline scoped.)

problame mentioned this pull request Jul 10, 2024

pageserver: stuck detach operation #7427

Closed

problame added 4 commits July 11, 2024 14:36

remove sensitivity to handler_timeline while in read-request/write-re…

c1da923

…sponse code

PageStreamError no longer has an Other variant

a45d714

comments and better structure

5f78c8f

problame force-pushed the problame/slow-detach-fix branch from 8bdea5c to 5f78c8f Compare July 11, 2024 14:46

problame force-pushed the problame/slow-detach-fix branch from ddfb450 to e157ea8 Compare July 12, 2024 18:56

problame added 7 commits July 12, 2024 18:59

remove page_service show <tenant_id>

8efdfce

postgres_backend: pass the .run() CancellationToken to Handler::pro…

b227697

…cess_query

Revert "postgres_backend: pass the .run() CancellationToken to Hand…

3406f0e

…ler::process_query" This reverts commit fef082b. Actually, we can just store the cancellation token in PageServiceHandler

refactor: don't use task_mgr for libpq, mgmt API, consumption metrics…

f3172f0

… worker

clean up consumption metrics launch & shutdown

c40cd3e

no more task_mgr for disk-usage-based eviction

ba552e4

no more task_mgr for page_service

1cfe9d8

problame force-pushed the problame/slow-detach-fix branch from e157ea8 to 1cfe9d8 Compare July 12, 2024 19:01

problame mentioned this pull request Jul 12, 2024

remove page_service show <tenant_id> #8372

Merged

problame added a commit that referenced this pull request Jul 15, 2024

remove page_service show <tenant_id> (#8372)

b49b450

This operation isn't used in practice, so let's remove it. Context: in #8339

problame added 12 commits July 16, 2024 17:29

track background purges without task_mgr

77ea76a

WIP: implement cache

4f25f0e

polish names & add docstrings

ec68633

address the TODO

f46903b

WIP: shard routing in cache

95c5094

compile fix

94726f9

WIP: integrate ("wait for active") is missing

c62ec52

inline remaining get_active_... methods

3469436

ShardSelector == GetArg

6388f15

WIP impl the tennat manager trait & bring back wait-for-active

8aafc0d

Trouble with the sync mutex now :( Async mutex doesn't work because we need to lock from destructors.

WIP

76f1b58

finish (no more smart drops, shut_down flag instead)

f523457

problame and others added 10 commits July 22, 2024 20:50

add test for behavior on connection drop (and fix a bug uncovered by it)

46295e8

improve efficiency of the fix from previous commit

5a0b941

fix docstrings

dfe1120

Revert "fix docstrings"

4fcbb39

This reverts commit dfe1120.

replace trait Types with individual generic params

17a0f64

Revert "replace trait Types with individual generic params"

0506b83

This reverts commit 17a0f64.

fix docstrings

544cc48

centralize docs & document design in module-level comment

2df61e4

Merge remote-tracking branch 'origin/main' into problame/slow-detach-fix

46744ea

clippy

f9961a0

problame changed the title ~~refactor(page_service): decouple from Mgr/Tenant/Timeline lifecycle~~ refactor(page_service): Timeline gate guard holding + cancellation + shutdown Jul 29, 2024

problame marked this pull request as ready for review July 29, 2024 13:39

problame requested a review from a team as a code owner July 29, 2024 13:39

problame requested a review from jcsp July 29, 2024 13:39

jcsp reviewed Jul 31, 2024

View reviewed changes

pageserver/src/page_service.rs Show resolved Hide resolved

jcsp approved these changes Jul 31, 2024

View reviewed changes

ACTIVE_TENANT_TIMEOUT: fix accidental use of http timeout; #8339 (com…

38b0f3c

…ment)

problame merged commit 4e3b70e into main Jul 31, 2024
65 checks passed

problame deleted the problame/slow-detach-fix branch July 31, 2024 15:05

skyzh added a commit that referenced this pull request Aug 7, 2024

Revert "refactor(page_service): Timeline gate guard holding + cancell…

933bb88

…ation + shutdown (#8339)" This reverts commit 4e3b70e.

koivunej reviewed Aug 7, 2024

View reviewed changes

pageserver/src/page_service.rs Show resolved Hide resolved

koivunej reviewed Aug 7, 2024

View reviewed changes

pageserver/src/page_service.rs Show resolved Hide resolved

koivunej mentioned this pull request Aug 7, 2024

fix: drain completed page_service connections #8632

Merged

koivunej reviewed Aug 7, 2024

View reviewed changes

pageserver/src/page_service.rs Show resolved Hide resolved

arpad-m mentioned this pull request Aug 7, 2024

Storage release 2024-08-07 #8642

Merged

problame added a commit that referenced this pull request Aug 20, 2024

task_mgr::spawn: require a TenantId (#8462)

ef57e73

… to dis-incentivize global tasks via task_mgr in the future (As of #8339 all remaining task_mgr usage is tenant or timeline scoped.)

problame mentioned this pull request Aug 21, 2024

Code RFC: decouple page_service from Mgr/Tenant/Timeline lifecycle #8286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(page_service): Timeline gate guard holding + cancellation + shutdown #8339

refactor(page_service): Timeline gate guard holding + cancellation + shutdown #8339

problame commented Jul 10, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading

jcsp left a comment

refactor(page_service): Timeline gate guard holding + cancellation + shutdown #8339

refactor(page_service): Timeline gate guard holding + cancellation + shutdown #8339

Conversation

problame commented Jul 10, 2024 • edited Loading

github-actions bot commented Jul 11, 2024 • edited Loading

3150 tests run: 3029 passed, 0 failed, 121 skipped (full report)

Code coverage* (full report)

jcsp left a comment

Choose a reason for hiding this comment

problame commented Jul 10, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading