Skip to content

Commit

Permalink
storage controller: make direct tenant creation more robust (#7247)
Browse files Browse the repository at this point in the history
## Problem

- Creations were not idempotent (unique key violation)
- Creations waited for reconciliation, which control plane blocks while
an operation is in flight

## Summary of changes

- Handle unique key constraint violation as an OK situation: if we're
creating the same tenant ID and shard count, it's reasonable to assume
this is a duplicate creation.
- Make the wait for reconcile during creation tolerate failures: this is
similar to location_conf, where the cloud control plane blocks our
notification calls until it is done with calling into our API (in future
this constraint is expected to relax as the cloud control plane learns
to run multiple operations concurrently for a tenant)
  • Loading branch information
jcsp committed Mar 26, 2024
1 parent 47d2b3a commit b3bb1d1
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 6 deletions.
31 changes: 25 additions & 6 deletions control_plane/attachment_service/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1523,6 +1523,8 @@ impl Service {
&self,
create_req: TenantCreateRequest,
) -> Result<TenantCreateResponse, ApiError> {
let tenant_id = create_req.new_tenant_id.tenant_id;

// Exclude any concurrent attempts to create/access the same tenant ID
let _tenant_lock = self
.tenant_op_locks
Expand All @@ -1531,7 +1533,12 @@ impl Service {

let (response, waiters) = self.do_tenant_create(create_req).await?;

self.await_waiters(waiters, SHORT_RECONCILE_TIMEOUT).await?;
if let Err(e) = self.await_waiters(waiters, SHORT_RECONCILE_TIMEOUT).await {
// Avoid deadlock: reconcile may fail while notifying compute, if the cloud control plane refuses to
// accept compute notifications while it is in the process of creating. Reconciliation will
// be retried in the background.
tracing::warn!(%tenant_id, "Reconcile not done yet while creating tenant ({e})");
}
Ok(response)
}

Expand Down Expand Up @@ -1610,13 +1617,25 @@ impl Service {
splitting: SplitState::default(),
})
.collect();
self.persistence

match self
.persistence
.insert_tenant_shards(persist_tenant_shards)
.await
.map_err(|e| {
// TODO: distinguish primary key constraint (idempotent, OK), from other errors
ApiError::InternalServerError(anyhow::anyhow!(e))
})?;
{
Ok(_) => {}
Err(DatabaseError::Query(diesel::result::Error::DatabaseError(
DatabaseErrorKind::UniqueViolation,
_,
))) => {
// Unique key violation: this is probably a retry. Because the shard count is part of the unique key,
// if we see a unique key violation it means that the creation request's shard count matches the previous
// creation's shard count.
tracing::info!("Tenant shards already present in database, proceeding with idempotent creation...");
}
// Any other database error is unexpected and a bug.
Err(e) => return Err(ApiError::InternalServerError(anyhow::anyhow!(e))),
};

let (waiters, response_shards) = {
let mut locked = self.inner.write().unwrap();
Expand Down
3 changes: 3 additions & 0 deletions test_runner/fixtures/neon_fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -2126,6 +2126,8 @@ def tenant_create(
shard_params = {"count": shard_count}
if shard_stripe_size is not None:
shard_params["stripe_size"] = shard_stripe_size
else:
shard_params["stripe_size"] = 32768

body["shard_parameters"] = shard_params

Expand All @@ -2139,6 +2141,7 @@ def tenant_create(
json=body,
headers=self.headers(TokenScope.PAGE_SERVER_API),
)
response.raise_for_status()
log.info(f"tenant_create success: {response.json()}")

def locate(self, tenant_id: TenantId) -> list[dict[str, Any]]:
Expand Down
5 changes: 5 additions & 0 deletions test_runner/regress/test_sharding_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ def test_sharding_service_smoke(
for tid in tenant_ids:
env.neon_cli.create_tenant(tid, shard_count=shards_per_tenant)

# Repeating a creation should be idempotent (we are just testing it doesn't return an error)
env.storage_controller.tenant_create(
tenant_id=next(iter(tenant_ids)), shard_count=shards_per_tenant
)

for node_id, count in get_node_shard_counts(env, tenant_ids).items():
# we used a multiple of pagservers for the total shard count,
# so expect equal number on all pageservers
Expand Down

1 comment on commit b3bb1d1

@github-actions
Copy link

Choose a reason for hiding this comment

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

2810 tests run: 2655 passed, 2 failed, 153 skipped (full report)


Failures on Postgres 14

  • test_pgbench_intensive_init_workload[neon_off-github-actions-selfhosted-1000]: release
  • test_random_writes[neon-github-actions-selfhosted]: release
# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_pgbench_intensive_init_workload[neon_off-release-pg14-github-actions-selfhosted-1000] or test_random_writes[neon-release-pg14-github-actions-selfhosted]"
Flaky tests (1)

Postgres 16

  • test_vm_bit_clear_on_heap_lock: debug

Code coverage* (full report)

  • functions: 28.2% (6298 of 22349 functions)
  • lines: 47.0% (44244 of 94196 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
b3bb1d1 at 2024-03-26T18:01:23.730Z :recycle:

Please sign in to comment.