Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pageserver: remove attach/detach apis #8134

Merged
merged 6 commits into from
Jun 25, 2024
Merged

pageserver: remove attach/detach apis #8134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c254e0b
clean up mentions of "/attach"
jcsp Jun 21, 2024
2c78f46
tests: call location_conf for attach/detach helpers
jcsp Jun 23, 2024
4ab57f1
pageserver: remove legacy attach/detach APIs
jcsp Jun 23, 2024
384e25a
tests: strip out outdated detach test code
jcsp Jun 23, 2024
c605697
tests: update for using location_conf from tests
jcsp Jun 23, 2024
810bed0
storcon: make attach_hook safer for tests that call concurrently
jcsp Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 0 additions & 37 deletions libs/pageserver_api/src/models.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,31 +607,6 @@ impl TenantConfigRequest {
}
}

#[derive(Debug, Deserialize)]
pub struct TenantAttachRequest {
#[serde(default)]
pub config: TenantAttachConfig,
#[serde(default)]
pub generation: Option<u32>,
}

/// Newtype to enforce deny_unknown_fields on TenantConfig for
/// its usage inside `TenantAttachRequest`.
#[derive(Debug, Serialize, Deserialize, Default)]
#[serde(deny_unknown_fields)]
pub struct TenantAttachConfig {
#[serde(flatten)]
allowing_unknown_fields: TenantConfig,
}

impl std::ops::Deref for TenantAttachConfig {
type Target = TenantConfig;

fn deref(&self) -> &Self::Target {
&self.allowing_unknown_fields
}
}

/// See [`TenantState::attachment_status`] and the OpenAPI docs for context.
#[derive(Serialize, Deserialize, Clone)]
#[serde(tag = "slug", content = "data", rename_all = "snake_case")]
Expand Down Expand Up @@ -1554,18 +1529,6 @@ mod tests {
"expect unknown field `unknown_field` error, got: {}",
err
);

let attach_request = json!({
"config": {
"unknown_field": "unknown_value".to_string(),
},
});
let err = serde_json::from_value::<TenantAttachRequest>(attach_request).unwrap_err();
assert!(
err.to_string().contains("unknown field `unknown_field`"),
"expect unknown field `unknown_field` error, got: {}",
err
);
}

#[test]
Expand Down
16 changes: 4 additions & 12 deletions libs/utils/src/http/json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,22 @@ use super::error::ApiError;
pub async fn json_request<T: for<'de> Deserialize<'de>>(
request: &mut Request<Body>,
) -> Result<T, ApiError> {
json_request_or_empty_body(request)
.await?
.context("missing request body")
.map_err(ApiError::BadRequest)
}

/// Will be removed as part of <https://github.com/neondatabase/neon/issues/4282>
pub async fn json_request_or_empty_body<T: for<'de> Deserialize<'de>>(
request: &mut Request<Body>,
) -> Result<Option<T>, ApiError> {
let body = hyper::body::aggregate(request.body_mut())
.await
.context("Failed to read request body")
.map_err(ApiError::BadRequest)?;

if body.remaining() == 0 {
return Ok(None);
return Err(ApiError::BadRequest(anyhow::anyhow!(
"missing request body"
)));
}

let mut deser = serde_json::de::Deserializer::from_reader(body.reader());

serde_path_to_error::deserialize(&mut deser)
// intentionally stringify because the debug version is not helpful in python logs
.map_err(|e| anyhow::anyhow!("Failed to parse json request: {e}"))
.map(Some)
.map_err(ApiError::BadRequest)
}

Expand Down
13 changes: 1 addition & 12 deletions pageserver/src/http/openapi_spec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,16 +367,7 @@ paths:
$ref: "#/components/schemas/TenantLocationConfigResponse"
"409":
description: |
The tenant is already known to Pageserver in some way,
and hence this `/attach` call has been rejected.

Some examples of how this can happen:
- tenant was created on this pageserver
- tenant attachment was started by an earlier call to `/attach`.

Callers should poll the tenant status's `attachment_status` field,
like for status 202. See the longer description for `POST /attach`
for details.
The tenant is already being modified, perhaps by a concurrent call to this API
content:
application/json:
schema:
Expand Down Expand Up @@ -762,8 +753,6 @@ components:
For example this can be caused by s3 being unreachable. The retry may be implemented
with call to detach, though it would be better to not automate it and inspec failed state
manually before proceeding with a retry.

See the tenant `/attach` endpoint for more information.
type: object
required:
- slug
Expand Down
83 changes: 1 addition & 82 deletions pageserver/src/http/routes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,11 @@ use pageserver_api::models::TenantShardLocation;
use pageserver_api::models::TenantShardSplitRequest;
use pageserver_api::models::TenantShardSplitResponse;
use pageserver_api::models::TenantSorting;
use pageserver_api::models::TenantState;
use pageserver_api::models::TopTenantShardItem;
use pageserver_api::models::TopTenantShardsRequest;
use pageserver_api::models::TopTenantShardsResponse;
use pageserver_api::models::{
DownloadRemoteLayersTaskSpawnRequest, LocationConfigMode, TenantAttachRequest,
TenantLocationConfigRequest,
DownloadRemoteLayersTaskSpawnRequest, LocationConfigMode, TenantLocationConfigRequest,
};
use pageserver_api::shard::ShardCount;
use pageserver_api::shard::TenantShardId;
Expand All @@ -51,7 +49,6 @@ use utils::auth::JwtAuth;
use utils::failpoint_support::failpoints_handler;
use utils::http::endpoint::prometheus_metrics_handler;
use utils::http::endpoint::request_span;
use utils::http::json::json_request_or_empty_body;
use utils::http::request::{get_request_param, must_get_query_param, parse_query_param};

use crate::context::{DownloadBehavior, RequestContext};
Expand Down Expand Up @@ -821,58 +818,6 @@ async fn get_timestamp_of_lsn_handler(
}
}

async fn tenant_attach_handler(
mut request: Request<Body>,
_cancel: CancellationToken,
) -> Result<Response<Body>, ApiError> {
let tenant_id: TenantId = parse_request_param(&request, "tenant_id")?;
check_permission(&request, Some(tenant_id))?;

let maybe_body: Option<TenantAttachRequest> = json_request_or_empty_body(&mut request).await?;
let tenant_conf = match &maybe_body {
Some(request) => TenantConfOpt::try_from(&*request.config).map_err(ApiError::BadRequest)?,
None => TenantConfOpt::default(),
};

let ctx = RequestContext::new(TaskKind::MgmtRequest, DownloadBehavior::Warn);

info!("Handling tenant attach {tenant_id}");

let state = get_state(&request);

let generation = get_request_generation(state, maybe_body.as_ref().and_then(|r| r.generation))?;

let tenant_shard_id = TenantShardId::unsharded(tenant_id);
let shard_params = ShardParameters::default();
let location_conf = LocationConf::attached_single(tenant_conf, generation, &shard_params);

let tenant = state
.tenant_manager
.upsert_location(tenant_shard_id, location_conf, None, SpawnMode::Eager, &ctx)
.await?;

let Some(tenant) = tenant else {
// This should never happen: indicates a bug in upsert_location
return Err(ApiError::InternalServerError(anyhow::anyhow!(
"Upsert succeeded but didn't return tenant!"
)));
};

// We might have successfully constructed a Tenant, but it could still
// end up in a broken state:
if let TenantState::Broken {
reason,
backtrace: _,
} = tenant.current_state()
{
return Err(ApiError::InternalServerError(anyhow::anyhow!(
"Tenant state is Broken: {reason}"
)));
}

json_response(StatusCode::ACCEPTED, ())
}

async fn timeline_delete_handler(
request: Request<Body>,
_cancel: CancellationToken,
Expand Down Expand Up @@ -903,26 +848,6 @@ async fn timeline_delete_handler(
json_response(StatusCode::ACCEPTED, ())
}

async fn tenant_detach_handler(
request: Request<Body>,
_cancel: CancellationToken,
) -> Result<Response<Body>, ApiError> {
let tenant_id: TenantId = parse_request_param(&request, "tenant_id")?;
check_permission(&request, Some(tenant_id))?;
// This is a legacy API (`/location_conf` is the replacement). It only supports unsharded tenants
let tenant_shard_id = TenantShardId::unsharded(tenant_id);

let state = get_state(&request);
let conf = state.conf;
state
.tenant_manager
.detach_tenant(conf, tenant_shard_id, &state.deletion_queue_client)
.instrument(info_span!("tenant_detach", %tenant_id, shard_id=%tenant_shard_id.shard_slug()))
.await?;

json_response(StatusCode::OK, ())
}

async fn tenant_reset_handler(
request: Request<Body>,
_cancel: CancellationToken,
Expand Down Expand Up @@ -2711,12 +2636,6 @@ pub fn make_router(
.post("/v1/tenant/:tenant_shard_id/timeline", |r| {
api_handler(r, timeline_create_handler)
})
.post("/v1/tenant/:tenant_id/attach", |r| {
api_handler(r, tenant_attach_handler)
})
.post("/v1/tenant/:tenant_id/detach", |r| {
api_handler(r, tenant_detach_handler)
})
.post("/v1/tenant/:tenant_shard_id/reset", |r| {
api_handler(r, tenant_reset_handler)
})
Expand Down
7 changes: 7 additions & 0 deletions storage_controller/src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,13 @@ impl Service {
&self,
attach_req: AttachHookRequest,
) -> anyhow::Result<AttachHookResponse> {
let _tenant_lock = trace_exclusive_lock(
&self.tenant_op_locks,
attach_req.tenant_shard_id.tenant_id,
TenantOperations::ShardSplit,
)
.await;
jcsp marked this conversation as resolved.
Show resolved Hide resolved

// This is a test hook. To enable using it on tenants that were created directly with
// the pageserver API (not via this service), we will auto-create any missing tenant
// shards with default state.
Expand Down
2 changes: 0 additions & 2 deletions test_runner/fixtures/neon_fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2684,7 +2684,6 @@ def tenant_attach(
self,
tenant_id: TenantId,
config: None | Dict[str, Any] = None,
config_null: bool = False,
generation: Optional[int] = None,
override_storage_controller_generation: bool = False,
):
Expand All @@ -2702,7 +2701,6 @@ def tenant_attach(
return client.tenant_attach(
tenant_id,
config,
config_null,
generation=generation,
)

Expand Down
46 changes: 18 additions & 28 deletions test_runner/fixtures/pageserver/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from __future__ import annotations

import json
import time
from collections import defaultdict
from dataclasses import dataclass
Expand Down Expand Up @@ -253,39 +252,30 @@ def tenant_attach(
self,
tenant_id: Union[TenantId, TenantShardId],
config: None | Dict[str, Any] = None,
config_null: bool = False,
generation: Optional[int] = None,
):
if config_null:
assert config is None
body: Any = None
else:
# null-config is prohibited by the API
config = config or {}
body = {"config": config}
if generation is not None:
body.update({"generation": generation})
config = config or {}

res = self.post(
f"http://localhost:{self.port}/v1/tenant/{tenant_id}/attach",
data=json.dumps(body),
headers={"Content-Type": "application/json"},
return self.tenant_location_conf(
tenant_id,
location_conf={
"mode": "AttachedSingle",
"secondary_conf": None,
jcsp marked this conversation as resolved.
Show resolved Hide resolved
"tenant_conf": config,
"generation": generation,
},
)
self.verbose_error(res)

def tenant_detach(self, tenant_id: TenantId, detach_ignored=False, timeout_secs=None):
params = {}
if detach_ignored:
params["detach_ignored"] = "true"

kwargs = {}
if timeout_secs is not None:
kwargs["timeout"] = timeout_secs

res = self.post(
f"http://localhost:{self.port}/v1/tenant/{tenant_id}/detach", params=params, **kwargs
def tenant_detach(self, tenant_id: TenantId):
return self.tenant_location_conf(
tenant_id,
location_conf={
"mode": "Detached",
"secondary_conf": None,
"tenant_conf": {},
"generation": None,
},
)
self.verbose_error(res)

def tenant_reset(self, tenant_id: Union[TenantId, TenantShardId], drop_cache: bool):
params = {}
Expand Down
Loading
Loading