storage controller: don't hold detached tenants in memory #10264
Conversation
jcsp
added
t/feature
7238 tests run: 6867 passed, 11 failed, 360 skipped (full report)Failures on Postgres 17
Failures on Postgres 16
Failures on Postgres 15
Failures on Postgres 14
Flaky tests (3)Postgres 17
Postgres 14
Test coverage report is not availableThe comment gets automatically updated with the latest test results
dd47681 at 2025-01-06T19:00:20.682Z :recycle: |
| @@ -330,11 +330,20 @@ impl Persistence { | |||
|
|
|||
| /// At startup, load the high level state for shards, such as their config + policy. This will | |||
| /// be enriched at runtime with state discovered on pageservers. | |||
| /// | |||
| /// We exclude shards configured to be detached. During startup, if we see any attached locations | |||
| /// for such shards, they will automatically be detached as 'orphans'. | |||
| pub(crate) async fn list_tenant_shards(&self) -> DatabaseResult<Vec<TenantShardPersistence>> { | |||
There was a problem hiding this comment.
nit: rename to load_active_tenant_shards?
storage_controller/src/service.rs
Outdated
| if tenant.policy == PlacementPolicy::Detached { | ||
| self.maybe_drop_tenant(tenant.tenant_shard_id.tenant_id, &mut locked); | ||
| } |
There was a problem hiding this comment.
This is a bit scary. Since service state sits behind a sync lock we follow this pattern:
- acquire lock and get some in-mem state
- do something async
- acquire lock again and update something
If step (3) doesn't expect the removal, then we run into trouble. I couldn't find any place with problematic expect or unwrap calls. Generally, this should be pretty safe since we wait on the reconcile spawned by the detach in tenant_location_config and that holds the tenant exclusive lock, but might run into issues if detaches end up taking long.
There was a problem hiding this comment.
This comment isn't really actionable, but I'm curious about your thoughts on this.
There was a problem hiding this comment.
In request handlers, the tenant_op_locks for the tenant should prevent any shenanigans like this.
However, you make an excellent point in this particular context: process_result does not hold that lock. If a request handler took the lock, read the existence of the tenant, and then raced with the processing of a reconciler completion, this could violate the assumption that while holding the lock, a tenant that is in memory should remain in memory.
I think the neatest solution to this is to try and get an exclusive lock around this, and to make our maybe_load and maybe_drop functions take refs to lock handles to prove the callers aren't using them outside the lock, let's try that...
There was a problem hiding this comment.
Tightened up the use of op locks in 61bf182
In the process I had the interesting observation that tenant_location_conf holds the lock much longer than it needs to, really it should drop it before waiting for reconcilers, but I don't want to make that change inline here in case it has any spooky side effects.
|
Let's get this merged and give it a week in staging -- there's a lot of detach/attach churn there, so we should have an excellent chance of spotting any unforseen issues |
Problem
Typical deployments of neon have some tenants that stay in use continuously, and a background churning population of tenants that are created and then fall idle, and are configured to Detached state. Currently, this churn of short lived tenants results in an ever-increasing memory footprint.
Closes: #9712
Summary of changes