pageserver: remove attach/detach apis (#8134)

## Problem These APIs have been deprecated for some time, but were still used from test code. Closes: #4282 ## Summary of changes - It is still convenient to do a "tenant_attach" from a test without having to write out a location_conf body, so those test methods have been retained with implementations that call through to their location_conf equivalent.
neondatabase · Jun 25, 2024 · 07f21dd · 07f21dd · github-actions · Jun 25, 2024
1 parent 64a4461
commit 07f21dd
Show file tree

Hide file tree

Showing 12 changed files with 54 additions and 260 deletions.
diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs
@@ -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")]
@@ -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]

diff --git a/libs/utils/src/http/json.rs b/libs/utils/src/http/json.rs
@@ -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)
 }
 

diff --git a/pageserver/src/http/openapi_spec.yml b/pageserver/src/http/openapi_spec.yml
@@ -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:
@@ -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

diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs
@@ -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;
@@ -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};
@@ -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,
@@ -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,
@@ -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)
         })

diff --git a/storage_controller/src/service.rs b/storage_controller/src/service.rs
@@ -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;
+
         // 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.

diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py
@@ -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,
     ):
@@ -2702,7 +2701,6 @@ def tenant_attach(
         return client.tenant_attach(
             tenant_id,
             config,
-            config_null,
             generation=generation,
         )
 

diff --git a/test_runner/fixtures/pageserver/http.py b/test_runner/fixtures/pageserver/http.py
@@ -1,6 +1,5 @@
 from __future__ import annotations
 
-import json
 import time
 from collections import defaultdict
 from dataclasses import dataclass
@@ -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,
+                "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 = {}