Merge pull request #6973 from neondatabase/rc/2024-02-29-manual

Release 2024-02-29

.github/ISSUE_TEMPLATE/epic-template.md

@@ -16,9 +16,9 @@ assignees: ''
 
 ## Implementation ideas
 
-## Tasks
+
 ```[tasklist]
-- [ ] Example Task
+### Tasks
 ```
 
 

.github/workflows/build_and_test.yml

@@ -1132,9 +1132,11 @@ jobs:
               -f branch=main \
               -f dockerTag=${{needs.tag.outputs.build-tag}}
           elif [[ "$GITHUB_REF_NAME" == "release-proxy" ]]; then
-            gh workflow --repo neondatabase/aws run deploy-proxy-prod.yml --ref main \
+            gh workflow --repo neondatabase/aws run deploy-prod.yml --ref main \
               -f deployPgSniRouter=true \
               -f deployProxy=true \
+              -f deployStorage=false \
+              -f deployStorageBroker=false \
               -f branch=main \
               -f dockerTag=${{needs.tag.outputs.build-tag}}
           else

.github/workflows/release.yml

@@ -97,7 +97,7 @@ jobs:
           **Please merge this Pull Request using 'Create a merge commit' button**
         EOF
 
-        gh pr create --title "Proxy release ${RELEASE_DATE}" \
+        gh pr create --title "Proxy release ${RELEASE_DATE}}" \
                      --body-file "body.md" \
                      --head "${RELEASE_BRANCH}" \
                      --base "release-proxy"

Cargo.toml

@@ -92,7 +92,7 @@ http-types = { version = "2", default-features = false }
 humantime = "2.1"
 humantime-serde = "1.1.1"
 hyper = "0.14"
-hyper-tungstenite = "0.13.0"
+hyper-tungstenite = "0.11"
 inotify = "0.10.2"
 ipnet = "2.9.0"
 itertools = "0.10"
@@ -129,8 +129,8 @@ reqwest-retry = "0.2.2"
 routerify = "3"
 rpds = "0.13"
 rustc-hash = "1.1.0"
-rustls = "0.22"
-rustls-pemfile = "2"
+rustls = "0.21"
+rustls-pemfile = "1"
 rustls-split = "0.3"
 scopeguard = "1.1"
 sysinfo = "0.29.2"
@@ -156,11 +156,12 @@ test-context = "0.1"
 thiserror = "1.0"
 tikv-jemallocator = "0.5"
 tikv-jemalloc-ctl = "0.5"
+tls-listener = { version = "0.7", features = ["rustls", "hyper-h1"] }
 tokio = { version = "1.17", features = ["macros"] }
 tokio-epoll-uring = { git = "https://github.com/neondatabase/tokio-epoll-uring.git" , branch = "main" }
 tokio-io-timeout = "1.2.0"
-tokio-postgres-rustls = "0.11.0"
-tokio-rustls = "0.25"
+tokio-postgres-rustls = "0.10.0"
+tokio-rustls = "0.24"
 tokio-stream = "0.1"
 tokio-tar = "0.3"
 tokio-util = { version = "0.7.10", features = ["io", "rt"] }
@@ -219,7 +220,7 @@ workspace_hack = { version = "0.1", path = "./workspace_hack/" }
 
 ## Build dependencies
 criterion = "0.5.1"
-rcgen = "0.12"
+rcgen = "0.11"
 rstest = "0.18"
 camino-tempfile = "1.0.2"
 tonic-build = "0.9"

Dockerfile

@@ -53,7 +53,7 @@ RUN set -e \
       --bin pagectl  \
       --bin safekeeper  \
       --bin storage_broker  \
-      --bin storage_controller  \
+      --bin attachment_service  \
       --bin proxy  \
       --bin neon_local \
       --locked --release \
@@ -81,7 +81,7 @@ COPY --from=build --chown=neon:neon /home/nonroot/target/release/pageserver
 COPY --from=build --chown=neon:neon /home/nonroot/target/release/pagectl             /usr/local/bin
 COPY --from=build --chown=neon:neon /home/nonroot/target/release/safekeeper          /usr/local/bin
 COPY --from=build --chown=neon:neon /home/nonroot/target/release/storage_broker      /usr/local/bin
-COPY --from=build --chown=neon:neon /home/nonroot/target/release/storage_controller  /usr/local/bin
+COPY --from=build --chown=neon:neon /home/nonroot/target/release/attachment_service  /usr/local/bin
 COPY --from=build --chown=neon:neon /home/nonroot/target/release/proxy               /usr/local/bin
 COPY --from=build --chown=neon:neon /home/nonroot/target/release/neon_local          /usr/local/bin
 

README.md

@@ -230,8 +230,6 @@ postgres=# select * from t;
 > cargo neon stop
 ```
 
-More advanced usages can be found at [Control Plane and Neon Local](./control_plane/README.md).
-
 #### Handling build failures
 
 If you encounter errors during setting up the initial tenant, it's best to stop everything (`cargo neon stop`) and remove the `.neon` directory. Then fix the problems, and start the setup again.

clippy.toml

@@ -3,10 +3,3 @@ disallowed-methods = [
     # Allow this for now, to deny it later once we stop using Handle::block_on completely
     # "tokio::runtime::Handle::block_on",
 ]
-
-disallowed-macros = [
-    # use std::pin::pin
-    "futures::pin_mut",
-    # cannot disallow this, because clippy finds used from tokio macros
-    #"tokio::pin",
-]

compute_tools/src/compute.rs

@@ -17,8 +17,9 @@ use chrono::{DateTime, Utc};
 use futures::future::join_all;
 use futures::stream::FuturesUnordered;
 use futures::StreamExt;
-use postgres::error::SqlState;
 use postgres::{Client, NoTls};
+use tokio;
+use tokio_postgres;
 use tracing::{debug, error, info, instrument, warn};
 use utils::id::{TenantId, TimelineId};
 use utils::lsn::Lsn;
@@ -396,9 +397,9 @@ impl ComputeNode {
     // Gets the basebackup in a retry loop
     #[instrument(skip_all, fields(%lsn))]
     pub fn get_basebackup(&self, compute_state: &ComputeState, lsn: Lsn) -> Result<()> {
-        let mut retry_period_ms = 500.0;
+        let mut retry_period_ms = 500;
         let mut attempts = 0;
-        let max_attempts = 10;
+        let max_attempts = 5;
         loop {
             let result = self.try_get_basebackup(compute_state, lsn);
             match result {
@@ -410,8 +411,8 @@ impl ComputeNode {
                         "Failed to get basebackup: {} (attempt {}/{})",
                         e, attempts, max_attempts
                     );
-                    std::thread::sleep(std::time::Duration::from_millis(retry_period_ms as u64));
-                    retry_period_ms *= 1.5;
+                    std::thread::sleep(std::time::Duration::from_millis(retry_period_ms));
+                    retry_period_ms *= 2;
                 }
                 Err(_) => {
                     return result;
@@ -764,26 +765,6 @@ impl ComputeNode {
         Ok((pg, logs_handle))
     }
 
-    /// Do post configuration of the already started Postgres. This function spawns a background thread to
-    /// configure the database after applying the compute spec. Currently, it upgrades the neon extension
-    /// version. In the future, it may upgrade all 3rd-party extensions.
-    #[instrument(skip_all)]
-    pub fn post_apply_config(&self) -> Result<()> {
-        let connstr = self.connstr.clone();
-        thread::spawn(move || {
-            let func = || {
-                let mut client = Client::connect(connstr.as_str(), NoTls)?;
-                handle_neon_extension_upgrade(&mut client)
-                    .context("handle_neon_extension_upgrade")?;
-                Ok::<_, anyhow::Error>(())
-            };
-            if let Err(err) = func() {
-                error!("error while post_apply_config: {err:#}");
-            }
-        });
-        Ok(())
-    }
-
     /// Do initial configuration of the already started Postgres.
     #[instrument(skip_all)]
     pub fn apply_config(&self, compute_state: &ComputeState) -> Result<()> {
@@ -795,34 +776,27 @@ impl ComputeNode {
         // but we can create a new one and grant it all privileges.
         let connstr = self.connstr.clone();
         let mut client = match Client::connect(connstr.as_str(), NoTls) {
-            Err(e) => match e.code() {
-                Some(&SqlState::INVALID_PASSWORD)
-                | Some(&SqlState::INVALID_AUTHORIZATION_SPECIFICATION) => {
-                    // connect with zenith_admin if cloud_admin could not authenticate
-                    info!(
-                        "cannot connect to postgres: {}, retrying with `zenith_admin` username",
-                        e
-                    );
-                    let mut zenith_admin_connstr = connstr.clone();
+            Err(e) => {
+                info!(
+                    "cannot connect to postgres: {}, retrying with `zenith_admin` username",
+                    e
+                );
+                let mut zenith_admin_connstr = connstr.clone();
 
-                    zenith_admin_connstr
-                        .set_username("zenith_admin")
-                        .map_err(|_| anyhow::anyhow!("invalid connstr"))?;
+                zenith_admin_connstr
+                    .set_username("zenith_admin")
+                    .map_err(|_| anyhow::anyhow!("invalid connstr"))?;
 
-                    let mut client =
-                        Client::connect(zenith_admin_connstr.as_str(), NoTls)
-                            .context("broken cloud_admin credential: tried connecting with cloud_admin but could not authenticate, and zenith_admin does not work either")?;
-                    // Disable forwarding so that users don't get a cloud_admin role
-                    client.simple_query("SET neon.forward_ddl = false")?;
-                    client.simple_query("CREATE USER cloud_admin WITH SUPERUSER")?;
-                    client.simple_query("GRANT zenith_admin TO cloud_admin")?;
-                    drop(client);
+                let mut client = Client::connect(zenith_admin_connstr.as_str(), NoTls)?;
+                // Disable forwarding so that users don't get a cloud_admin role
+                client.simple_query("SET neon.forward_ddl = false")?;
+                client.simple_query("CREATE USER cloud_admin WITH SUPERUSER")?;
+                client.simple_query("GRANT zenith_admin TO cloud_admin")?;
+                drop(client);
 
-                    // reconnect with connstring with expected name
-                    Client::connect(connstr.as_str(), NoTls)?
-                }
-                _ => return Err(e.into()),
-            },
+                // reconnect with connstring with expected name
+                Client::connect(connstr.as_str(), NoTls)?
+            }
             Ok(client) => client,
         };
 
@@ -1018,21 +992,18 @@ impl ComputeNode {
         let pg_process = self.start_postgres(pspec.storage_auth_token.clone())?;
 
         let config_time = Utc::now();
-        if pspec.spec.mode == ComputeMode::Primary {
-            if !pspec.spec.skip_pg_catalog_updates {
-                let pgdata_path = Path::new(&self.pgdata);
-                // temporarily reset max_cluster_size in config
-                // to avoid the possibility of hitting the limit, while we are applying config:
-                // creating new extensions, roles, etc...
-                config::compute_ctl_temp_override_create(pgdata_path, "neon.max_cluster_size=-1")?;
-                self.pg_reload_conf()?;
+        if pspec.spec.mode == ComputeMode::Primary && !pspec.spec.skip_pg_catalog_updates {
+            let pgdata_path = Path::new(&self.pgdata);
+            // temporarily reset max_cluster_size in config
+            // to avoid the possibility of hitting the limit, while we are applying config:
+            // creating new extensions, roles, etc...
+            config::compute_ctl_temp_override_create(pgdata_path, "neon.max_cluster_size=-1")?;
+            self.pg_reload_conf()?;
 
-                self.apply_config(&compute_state)?;
+            self.apply_config(&compute_state)?;
 
-                config::compute_ctl_temp_override_remove(pgdata_path)?;
-                self.pg_reload_conf()?;
-            }
-            self.post_apply_config()?;
+            config::compute_ctl_temp_override_remove(pgdata_path)?;
+            self.pg_reload_conf()?;
         }
 
         let startup_end_time = Utc::now();

compute_tools/src/extension_server.rs

@@ -71,7 +71,7 @@ More specifically, here is an example ext_index.json
     }
 }
 */
-use anyhow::Result;
+use anyhow::{self, Result};
 use anyhow::{bail, Context};
 use bytes::Bytes;
 use compute_api::spec::RemoteExtSpec;

compute_tools/src/http/api.rs

@@ -13,6 +13,8 @@ use compute_api::responses::{ComputeStatus, ComputeStatusResponse, GenericAPIErr
 use anyhow::Result;
 use hyper::service::{make_service_fn, service_fn};
 use hyper::{Body, Method, Request, Response, Server, StatusCode};
+use num_cpus;
+use serde_json;
 use tokio::task;
 use tracing::{error, info, warn};
 use tracing_utils::http::OtelName;

compute_tools/src/spec.rs

@@ -744,17 +744,7 @@ pub fn handle_extension_neon(client: &mut Client) -> Result<()> {
     // - extension was just installed
     // - extension was already installed and is up to date
     let query = "ALTER EXTENSION neon UPDATE";
-    info!("update neon extension version with query: {}", query);
-    client.simple_query(query)?;
-
-    Ok(())
-}
-
-#[instrument(skip_all)]
-pub fn handle_neon_extension_upgrade(client: &mut Client) -> Result<()> {
-    info!("handle neon extension upgrade");
-    let query = "ALTER EXTENSION neon UPDATE";
-    info!("update neon extension version with query: {}", query);
+    info!("update neon extension schema with query: {}", query);
     client.simple_query(query)?;
 
     Ok(())

control_plane/README.md

@@ -1,26 +0,0 @@
-# Control Plane and Neon Local
-
-This crate contains tools to start a Neon development environment locally. This utility can be used with the `cargo neon` command.
-
-## Example: Start with Postgres 16
-
-To create and start a local development environment with Postgres 16, you will need to provide `--pg-version` flag to 3 of the start-up commands.
-
-```shell
-cargo neon init --pg-version 16
-cargo neon start
-cargo neon tenant create --set-default --pg-version 16
-cargo neon endpoint create main --pg-version 16
-cargo neon endpoint start main
-```
-
-## Example: Create Test User and Database
-
-By default, `cargo neon` starts an endpoint with `cloud_admin` and `postgres` database. If you want to have a role and a database similar to what we have on the cloud service, you can do it with the following commands when starting an endpoint.
-
-```shell
-cargo neon endpoint create main --pg-version 16 --update-catalog true
-cargo neon endpoint start main --create-test-user true
-```
-
-The first command creates `neon_superuser` and necessary roles. The second command creates `test` user and `neondb` database. You will see a connection string that connects you to the test user after running the second command.

control_plane/attachment_service/Cargo.toml

@@ -4,10 +4,6 @@ version = "0.1.0"
 edition.workspace = true
 license.workspace = true
 
-[[bin]]
-name = "storage_controller"
-path = "src/main.rs"
-
 [features]
 default = []
 # Enables test-only APIs and behaviors

control_plane/attachment_service/migrations/2024-02-29-094122_generations_null/down.sql

@@ -1,2 +0,0 @@
-ALTER TABLE tenant_shards ALTER generation SET NOT NULL;
-ALTER TABLE tenant_shards ALTER generation_pageserver SET NOT NULL;

control_plane/attachment_service/migrations/2024-02-29-094122_generations_null/up.sql

@@ -1,4 +0,0 @@
-
-
-ALTER TABLE tenant_shards ALTER generation DROP NOT NULL;
-ALTER TABLE tenant_shards ALTER generation_pageserver DROP NOT NULL;

control_plane/attachment_service/src/compute_hook.rs

@@ -3,7 +3,7 @@ use std::{collections::HashMap, time::Duration};
 use control_plane::endpoint::{ComputeControlPlane, EndpointStatus};
 use control_plane::local_env::LocalEnv;
 use hyper::{Method, StatusCode};
-use pageserver_api::shard::{ShardCount, ShardNumber, ShardStripeSize, TenantShardId};
+use pageserver_api::shard::{ShardIndex, ShardNumber, TenantShardId};
 use postgres_connection::parse_host_port;
 use serde::{Deserialize, Serialize};
 use tokio_util::sync::CancellationToken;
@@ -19,66 +19,8 @@ const SLOWDOWN_DELAY: Duration = Duration::from_secs(5);
 
 pub(crate) const API_CONCURRENCY: usize = 32;
 
-struct ShardedComputeHookTenant {
-    stripe_size: ShardStripeSize,
-    shard_count: ShardCount,
-    shards: Vec<(ShardNumber, NodeId)>,
-}
-
-enum ComputeHookTenant {
-    Unsharded(NodeId),
-    Sharded(ShardedComputeHookTenant),
-}
-
-impl ComputeHookTenant {
-    /// Construct with at least one shard's information
-    fn new(tenant_shard_id: TenantShardId, stripe_size: ShardStripeSize, node_id: NodeId) -> Self {
-        if tenant_shard_id.shard_count.count() > 1 {
-            Self::Sharded(ShardedComputeHookTenant {
-                shards: vec![(tenant_shard_id.shard_number, node_id)],
-                stripe_size,
-                shard_count: tenant_shard_id.shard_count,
-            })
-        } else {
-            Self::Unsharded(node_id)
-        }
-    }
-
-    /// Set one shard's location.  If stripe size or shard count have changed, Self is reset
-    /// and drops existing content.
-    fn update(
-        &mut self,
-        tenant_shard_id: TenantShardId,
-        stripe_size: ShardStripeSize,
-        node_id: NodeId,
-    ) {
-        match self {
-            Self::Unsharded(existing_node_id) if tenant_shard_id.shard_count.count() == 1 => {
-                *existing_node_id = node_id
-            }
-            Self::Sharded(sharded_tenant)
-                if sharded_tenant.stripe_size == stripe_size
-                    && sharded_tenant.shard_count == tenant_shard_id.shard_count =>
-            {
-                if let Some(existing) = sharded_tenant
-                    .shards
-                    .iter()
-                    .position(|s| s.0 == tenant_shard_id.shard_number)
-                {
-                    sharded_tenant.shards.get_mut(existing).unwrap().1 = node_id;
-                } else {
-                    sharded_tenant
-                        .shards
-                        .push((tenant_shard_id.shard_number, node_id));
-                    sharded_tenant.shards.sort_by_key(|s| s.0)
-                }
-            }
-            _ => {
-                // Shard count changed: reset struct.
-                *self = Self::new(tenant_shard_id, stripe_size, node_id);
-            }
-        }
-    }
+pub(super) struct ComputeHookTenant {
+    shards: Vec<(ShardIndex, NodeId)>,
 }
 
 #[derive(Serialize, Deserialize, Debug)]
@@ -91,7 +33,6 @@ struct ComputeHookNotifyRequestShard {
 #[derive(Serialize, Deserialize, Debug)]
 struct ComputeHookNotifyRequest {
     tenant_id: TenantId,
-    stripe_size: Option<ShardStripeSize>,
     shards: Vec<ComputeHookNotifyRequestShard>,
 }
 
@@ -122,43 +63,42 @@ pub(crate) enum NotifyError {
 }
 
 impl ComputeHookTenant {
-    fn maybe_reconfigure(&self, tenant_id: TenantId) -> Option<ComputeHookNotifyRequest> {
-        match self {
-            Self::Unsharded(node_id) => Some(ComputeHookNotifyRequest {
-                tenant_id,
-                shards: vec![ComputeHookNotifyRequestShard {
-                    shard_number: ShardNumber(0),
-                    node_id: *node_id,
-                }],
-                stripe_size: None,
-            }),
-            Self::Sharded(sharded_tenant)
-                if sharded_tenant.shards.len() == sharded_tenant.shard_count.count() as usize =>
-            {
-                Some(ComputeHookNotifyRequest {
-                    tenant_id,
-                    shards: sharded_tenant
-                        .shards
-                        .iter()
-                        .map(|(shard_number, node_id)| ComputeHookNotifyRequestShard {
-                            shard_number: *shard_number,
-                            node_id: *node_id,
-                        })
-                        .collect(),
-                    stripe_size: Some(sharded_tenant.stripe_size),
-                })
-            }
-            Self::Sharded(sharded_tenant) => {
-                // Sharded tenant doesn't yet have information for all its shards
+    async fn maybe_reconfigure(&mut self, tenant_id: TenantId) -> Option<ComputeHookNotifyRequest> {
+        // Find the highest shard count and drop any shards that aren't
+        // for that shard count.
+        let shard_count = self.shards.iter().map(|(k, _v)| k.shard_count).max();
+        let Some(shard_count) = shard_count else {
+            // No shards, nothing to do.
+            tracing::info!("ComputeHookTenant::maybe_reconfigure: no shards");
+            return None;
+        };
 
-                tracing::info!(
-                    "ComputeHookTenant::maybe_reconfigure: not enough shards ({}/{})",
-                    sharded_tenant.shards.len(),
-                    sharded_tenant.shard_count.count()
-                );
-                None
-            }
+        self.shards.retain(|(k, _v)| k.shard_count == shard_count);
+        self.shards
+            .sort_by_key(|(shard, _node_id)| shard.shard_number);
+
+        if self.shards.len() == shard_count.count() as usize || shard_count.is_unsharded() {
+            // We have pageservers for all the shards: emit a configuration update
+            return Some(ComputeHookNotifyRequest {
+                tenant_id,
+                shards: self
+                    .shards
+                    .iter()
+                    .map(|(shard, node_id)| ComputeHookNotifyRequestShard {
+                        shard_number: shard.shard_number,
+                        node_id: *node_id,
+                    })
+                    .collect(),
+            });
+        } else {
+            tracing::info!(
+                "ComputeHookTenant::maybe_reconfigure: not enough shards ({}/{})",
+                self.shards.len(),
+                shard_count.count()
+            );
         }
+
+        None
     }
 }
 
@@ -199,11 +139,7 @@ impl ComputeHook {
         };
         let cplane =
             ComputeControlPlane::load(env.clone()).expect("Error loading compute control plane");
-        let ComputeHookNotifyRequest {
-            tenant_id,
-            shards,
-            stripe_size,
-        } = reconfigure_request;
+        let ComputeHookNotifyRequest { tenant_id, shards } = reconfigure_request;
 
         let compute_pageservers = shards
             .into_iter()
@@ -220,9 +156,7 @@ impl ComputeHook {
         for (endpoint_name, endpoint) in &cplane.endpoints {
             if endpoint.tenant_id == tenant_id && endpoint.status() == EndpointStatus::Running {
                 tracing::info!("Reconfiguring endpoint {}", endpoint_name,);
-                endpoint
-                    .reconfigure(compute_pageservers.clone(), stripe_size)
-                    .await?;
+                endpoint.reconfigure(compute_pageservers.clone()).await?;
             }
         }
 
@@ -337,26 +271,30 @@ impl ComputeHook {
         &self,
         tenant_shard_id: TenantShardId,
         node_id: NodeId,
-        stripe_size: ShardStripeSize,
         cancel: &CancellationToken,
     ) -> Result<(), NotifyError> {
         let mut locked = self.state.lock().await;
+        let entry = locked
+            .entry(tenant_shard_id.tenant_id)
+            .or_insert_with(|| ComputeHookTenant { shards: Vec::new() });
 
-        use std::collections::hash_map::Entry;
-        let tenant = match locked.entry(tenant_shard_id.tenant_id) {
-            Entry::Vacant(e) => e.insert(ComputeHookTenant::new(
-                tenant_shard_id,
-                stripe_size,
-                node_id,
-            )),
-            Entry::Occupied(e) => {
-                let tenant = e.into_mut();
-                tenant.update(tenant_shard_id, stripe_size, node_id);
-                tenant
-            }
+        let shard_index = ShardIndex {
+            shard_count: tenant_shard_id.shard_count,
+            shard_number: tenant_shard_id.shard_number,
         };
 
-        let reconfigure_request = tenant.maybe_reconfigure(tenant_shard_id.tenant_id);
+        let mut set = false;
+        for (existing_shard, existing_node) in &mut entry.shards {
+            if *existing_shard == shard_index {
+                *existing_node = node_id;
+                set = true;
+            }
+        }
+        if !set {
+            entry.shards.push((shard_index, node_id));
+        }
+
+        let reconfigure_request = entry.maybe_reconfigure(tenant_shard_id.tenant_id).await;
         let Some(reconfigure_request) = reconfigure_request else {
             // The tenant doesn't yet have pageservers for all its shards: we won't notify anything
             // until it does.
@@ -378,85 +316,3 @@ impl ComputeHook {
         }
     }
 }
-
-#[cfg(test)]
-pub(crate) mod tests {
-    use pageserver_api::shard::{ShardCount, ShardNumber};
-    use utils::id::TenantId;
-
-    use super::*;
-
-    #[test]
-    fn tenant_updates() -> anyhow::Result<()> {
-        let tenant_id = TenantId::generate();
-        let mut tenant_state = ComputeHookTenant::new(
-            TenantShardId {
-                tenant_id,
-                shard_count: ShardCount::new(0),
-                shard_number: ShardNumber(0),
-            },
-            ShardStripeSize(12345),
-            NodeId(1),
-        );
-
-        // An unsharded tenant is always ready to emit a notification
-        assert!(tenant_state.maybe_reconfigure(tenant_id).is_some());
-        assert_eq!(
-            tenant_state
-                .maybe_reconfigure(tenant_id)
-                .unwrap()
-                .shards
-                .len(),
-            1
-        );
-        assert!(tenant_state
-            .maybe_reconfigure(tenant_id)
-            .unwrap()
-            .stripe_size
-            .is_none());
-
-        // Writing the first shard of a multi-sharded situation (i.e. in a split)
-        // resets the tenant state and puts it in an non-notifying state (need to
-        // see all shards)
-        tenant_state.update(
-            TenantShardId {
-                tenant_id,
-                shard_count: ShardCount::new(2),
-                shard_number: ShardNumber(1),
-            },
-            ShardStripeSize(32768),
-            NodeId(1),
-        );
-        assert!(tenant_state.maybe_reconfigure(tenant_id).is_none());
-
-        // Writing the second shard makes it ready to notify
-        tenant_state.update(
-            TenantShardId {
-                tenant_id,
-                shard_count: ShardCount::new(2),
-                shard_number: ShardNumber(0),
-            },
-            ShardStripeSize(32768),
-            NodeId(1),
-        );
-
-        assert!(tenant_state.maybe_reconfigure(tenant_id).is_some());
-        assert_eq!(
-            tenant_state
-                .maybe_reconfigure(tenant_id)
-                .unwrap()
-                .shards
-                .len(),
-            2
-        );
-        assert_eq!(
-            tenant_state
-                .maybe_reconfigure(tenant_id)
-                .unwrap()
-                .stripe_size,
-            Some(ShardStripeSize(32768))
-        );
-
-        Ok(())
-    }
-}

control_plane/attachment_service/src/http.rs

@@ -3,7 +3,7 @@ use crate::service::{Service, STARTUP_RECONCILE_TIMEOUT};
 use hyper::{Body, Request, Response};
 use hyper::{StatusCode, Uri};
 use pageserver_api::models::{
-    TenantConfigRequest, TenantCreateRequest, TenantLocationConfigRequest, TenantShardSplitRequest,
+    TenantCreateRequest, TenantLocationConfigRequest, TenantShardSplitRequest,
     TenantTimeTravelRequest, TimelineCreateRequest,
 };
 use pageserver_api::shard::TenantShardId;
@@ -117,7 +117,6 @@ async fn handle_tenant_create(
     check_permissions(&req, Scope::PageServerApi)?;
 
     let create_req = json_request::<TenantCreateRequest>(&mut req).await?;
-
     json_response(
         StatusCode::CREATED,
         service.tenant_create(create_req).await?,
@@ -186,27 +185,6 @@ async fn handle_tenant_location_config(
     )
 }
 
-async fn handle_tenant_config_set(
-    service: Arc<Service>,
-    mut req: Request<Body>,
-) -> Result<Response<Body>, ApiError> {
-    check_permissions(&req, Scope::PageServerApi)?;
-
-    let config_req = json_request::<TenantConfigRequest>(&mut req).await?;
-
-    json_response(StatusCode::OK, service.tenant_config_set(config_req).await?)
-}
-
-async fn handle_tenant_config_get(
-    service: Arc<Service>,
-    req: Request<Body>,
-) -> Result<Response<Body>, ApiError> {
-    let tenant_id: TenantId = parse_request_param(&req, "tenant_id")?;
-    check_permissions(&req, Scope::PageServerApi)?;
-
-    json_response(StatusCode::OK, service.tenant_config_get(tenant_id)?)
-}
-
 async fn handle_tenant_time_travel_remote_storage(
     service: Arc<Service>,
     mut req: Request<Body>,
@@ -238,15 +216,7 @@ async fn handle_tenant_time_travel_remote_storage(
             done_if_after_raw,
         )
         .await?;
-    json_response(StatusCode::OK, ())
-}
 
-async fn handle_tenant_secondary_download(
-    service: Arc<Service>,
-    req: Request<Body>,
-) -> Result<Response<Body>, ApiError> {
-    let tenant_id: TenantId = parse_request_param(&req, "tenant_id")?;
-    service.tenant_secondary_download(tenant_id).await?;
     json_response(StatusCode::OK, ())
 }
 
@@ -581,21 +551,12 @@ pub fn make_router(
         .delete("/v1/tenant/:tenant_id", |r| {
             tenant_service_handler(r, handle_tenant_delete)
         })
-        .put("/v1/tenant/config", |r| {
-            tenant_service_handler(r, handle_tenant_config_set)
-        })
-        .get("/v1/tenant/:tenant_id/config", |r| {
-            tenant_service_handler(r, handle_tenant_config_get)
-        })
         .put("/v1/tenant/:tenant_id/location_config", |r| {
             tenant_service_handler(r, handle_tenant_location_config)
         })
         .put("/v1/tenant/:tenant_id/time_travel_remote_storage", |r| {
             tenant_service_handler(r, handle_tenant_time_travel_remote_storage)
         })
-        .post("/v1/tenant/:tenant_id/secondary/download", |r| {
-            tenant_service_handler(r, handle_tenant_secondary_download)
-        })
         // Timeline operations
         .delete("/v1/tenant/:tenant_id/timeline/:timeline_id", |r| {
             tenant_service_handler(r, handle_tenant_timeline_delete)

control_plane/attachment_service/src/lib.rs

@@ -1,4 +1,4 @@
-use serde::Serialize;
+use serde::{Deserialize, Serialize};
 use utils::seqwait::MonotonicCounter;
 
 mod auth;
@@ -13,6 +13,17 @@ mod schema;
 pub mod service;
 mod tenant_state;
 
+#[derive(Clone, Serialize, Deserialize, Debug)]
+enum PlacementPolicy {
+    /// Cheapest way to attach a tenant: just one pageserver, no secondary
+    Single,
+    /// Production-ready way to attach a tenant: one attached pageserver and
+    /// some number of secondaries.
+    Double(usize),
+    /// Do not attach to any pageservers
+    Detached,
+}
+
 #[derive(Ord, PartialOrd, Eq, PartialEq, Copy, Clone, Serialize)]
 struct Sequence(u64);
 
@@ -49,3 +60,9 @@ impl Sequence {
         Sequence(self.0 + 1)
     }
 }
+
+impl Default for PlacementPolicy {
+    fn default() -> Self {
+        PlacementPolicy::Double(1)
+    }
+}

control_plane/attachment_service/src/main.rs

@@ -9,7 +9,7 @@ use attachment_service::http::make_router;
 use attachment_service::metrics::preinitialize_metrics;
 use attachment_service::persistence::Persistence;
 use attachment_service::service::{Config, Service};
-use aws_config::{BehaviorVersion, Region};
+use aws_config::{self, BehaviorVersion, Region};
 use camino::Utf8PathBuf;
 use clap::Parser;
 use diesel::Connection;
@@ -79,38 +79,13 @@ impl Secrets {
         "neon-storage-controller-control-plane-jwt-token";
     const PUBLIC_KEY_SECRET: &'static str = "neon-storage-controller-public-key";
 
-    const DATABASE_URL_ENV: &'static str = "DATABASE_URL";
-    const PAGESERVER_JWT_TOKEN_ENV: &'static str = "PAGESERVER_JWT_TOKEN";
-    const CONTROL_PLANE_JWT_TOKEN_ENV: &'static str = "CONTROL_PLANE_JWT_TOKEN";
-    const PUBLIC_KEY_ENV: &'static str = "PUBLIC_KEY";
-
-    /// Load secrets from, in order of preference:
-    /// - CLI args if database URL is provided on the CLI
-    /// - Environment variables if DATABASE_URL is set.
-    /// - AWS Secrets Manager secrets
     async fn load(args: &Cli) -> anyhow::Result<Self> {
         match &args.database_url {
             Some(url) => Self::load_cli(url, args),
-            None => match std::env::var(Self::DATABASE_URL_ENV) {
-                Ok(database_url) => Self::load_env(database_url),
-                Err(_) => Self::load_aws_sm().await,
-            },
+            None => Self::load_aws_sm().await,
         }
     }
 
-    fn load_env(database_url: String) -> anyhow::Result<Self> {
-        let public_key = match std::env::var(Self::PUBLIC_KEY_ENV) {
-            Ok(public_key) => Some(JwtAuth::from_key(public_key).context("Loading public key")?),
-            Err(_) => None,
-        };
-        Ok(Self {
-            database_url,
-            public_key,
-            jwt_token: std::env::var(Self::PAGESERVER_JWT_TOKEN_ENV).ok(),
-            control_plane_jwt_token: std::env::var(Self::CONTROL_PLANE_JWT_TOKEN_ENV).ok(),
-        })
-    }
-
     async fn load_aws_sm() -> anyhow::Result<Self> {
         let Ok(region) = std::env::var("AWS_REGION") else {
             anyhow::bail!("AWS_REGION is not set, cannot load secrets automatically: either set this, or use CLI args to supply secrets");

control_plane/attachment_service/src/node.rs

@@ -1,16 +1,6 @@
-use std::{str::FromStr, time::Duration};
-
-use hyper::StatusCode;
-use pageserver_api::{
-    controller_api::{
-        NodeAvailability, NodeRegisterRequest, NodeSchedulingPolicy, TenantLocateResponseShard,
-    },
-    shard::TenantShardId,
-};
-use pageserver_client::mgmt_api;
+use pageserver_api::controller_api::{NodeAvailability, NodeSchedulingPolicy};
 use serde::Serialize;
-use tokio_util::sync::CancellationToken;
-use utils::{backoff, id::NodeId};
+use utils::id::NodeId;
 
 use crate::persistence::NodePersistence;
 
@@ -22,29 +12,16 @@ use crate::persistence::NodePersistence;
 /// implementation of serialization on this type is only for debug dumps.
 #[derive(Clone, Serialize)]
 pub(crate) struct Node {
-    id: NodeId,
+    pub(crate) id: NodeId,
 
-    availability: NodeAvailability,
-    scheduling: NodeSchedulingPolicy,
+    pub(crate) availability: NodeAvailability,
+    pub(crate) scheduling: NodeSchedulingPolicy,
 
-    listen_http_addr: String,
-    listen_http_port: u16,
+    pub(crate) listen_http_addr: String,
+    pub(crate) listen_http_port: u16,
 
-    listen_pg_addr: String,
-    listen_pg_port: u16,
-
-    // This cancellation token means "stop any RPCs in flight to this node, and don't start
-    // any more". It is not related to process shutdown.
-    #[serde(skip)]
-    cancel: CancellationToken,
-}
-
-/// When updating [`Node::availability`] we use this type to indicate to the caller
-/// whether/how they changed it.
-pub(crate) enum AvailabilityTransition {
-    ToActive,
-    ToOffline,
-    Unchanged,
+    pub(crate) listen_pg_addr: String,
+    pub(crate) listen_pg_port: u16,
 }
 
 impl Node {
@@ -52,71 +29,6 @@ impl Node {
         format!("http://{}:{}", self.listen_http_addr, self.listen_http_port)
     }
 
-    pub(crate) fn get_id(&self) -> NodeId {
-        self.id
-    }
-
-    pub(crate) fn set_scheduling(&mut self, scheduling: NodeSchedulingPolicy) {
-        self.scheduling = scheduling
-    }
-
-    /// Does this registration request match `self`?  This is used when deciding whether a registration
-    /// request should be allowed to update an existing record with the same node ID.
-    pub(crate) fn registration_match(&self, register_req: &NodeRegisterRequest) -> bool {
-        self.id == register_req.node_id
-            && self.listen_http_addr == register_req.listen_http_addr
-            && self.listen_http_port == register_req.listen_http_port
-            && self.listen_pg_addr == register_req.listen_pg_addr
-            && self.listen_pg_port == register_req.listen_pg_port
-    }
-
-    /// For a shard located on this node, populate a response object
-    /// with this node's address information.
-    pub(crate) fn shard_location(&self, shard_id: TenantShardId) -> TenantLocateResponseShard {
-        TenantLocateResponseShard {
-            shard_id,
-            node_id: self.id,
-            listen_http_addr: self.listen_http_addr.clone(),
-            listen_http_port: self.listen_http_port,
-            listen_pg_addr: self.listen_pg_addr.clone(),
-            listen_pg_port: self.listen_pg_port,
-        }
-    }
-
-    pub(crate) fn set_availability(
-        &mut self,
-        availability: NodeAvailability,
-    ) -> AvailabilityTransition {
-        use NodeAvailability::*;
-        let transition = match (self.availability, availability) {
-            (Offline, Active) => {
-                // Give the node a new cancellation token, effectively resetting it to un-cancelled.  Any
-                // users of previously-cloned copies of the node will still see the old cancellation
-                // state.  For example, Reconcilers in flight will have to complete and be spawned
-                // again to realize that the node has become available.
-                self.cancel = CancellationToken::new();
-                AvailabilityTransition::ToActive
-            }
-            (Active, Offline) => {
-                // Fire the node's cancellation token to cancel any in-flight API requests to it
-                self.cancel.cancel();
-                AvailabilityTransition::ToOffline
-            }
-            _ => AvailabilityTransition::Unchanged,
-        };
-        self.availability = availability;
-        transition
-    }
-
-    /// Whether we may send API requests to this node.
-    pub(crate) fn is_available(&self) -> bool {
-        // When we clone a node, [`Self::availability`] is a snapshot, but [`Self::cancel`] holds
-        // a reference to the original Node's cancellation status.  Checking both of these results
-        // in a "pessimistic" check where we will consider a Node instance unavailable if it was unavailable
-        // when we cloned it, or if the original Node instance's cancellation token was fired.
-        matches!(self.availability, NodeAvailability::Active) && !self.cancel.is_cancelled()
-    }
-
     /// Is this node elegible to have work scheduled onto it?
     pub(crate) fn may_schedule(&self) -> bool {
         match self.availability {
@@ -132,26 +44,6 @@ impl Node {
         }
     }
 
-    pub(crate) fn new(
-        id: NodeId,
-        listen_http_addr: String,
-        listen_http_port: u16,
-        listen_pg_addr: String,
-        listen_pg_port: u16,
-    ) -> Self {
-        Self {
-            id,
-            listen_http_addr,
-            listen_http_port,
-            listen_pg_addr,
-            listen_pg_port,
-            scheduling: NodeSchedulingPolicy::Filling,
-            // TODO: we shouldn't really call this Active until we've heartbeated it.
-            availability: NodeAvailability::Active,
-            cancel: CancellationToken::new(),
-        }
-    }
-
     pub(crate) fn to_persistent(&self) -> NodePersistence {
         NodePersistence {
             node_id: self.id.0 as i64,
@@ -162,96 +54,4 @@ impl Node {
             listen_pg_port: self.listen_pg_port as i32,
         }
     }
-
-    pub(crate) fn from_persistent(np: NodePersistence) -> Self {
-        Self {
-            id: NodeId(np.node_id as u64),
-            // At startup we consider a node offline until proven otherwise.
-            availability: NodeAvailability::Offline,
-            scheduling: NodeSchedulingPolicy::from_str(&np.scheduling_policy)
-                .expect("Bad scheduling policy in DB"),
-            listen_http_addr: np.listen_http_addr,
-            listen_http_port: np.listen_http_port as u16,
-            listen_pg_addr: np.listen_pg_addr,
-            listen_pg_port: np.listen_pg_port as u16,
-            cancel: CancellationToken::new(),
-        }
-    }
-
-    /// Wrapper for issuing requests to pageserver management API: takes care of generic
-    /// retry/backoff for retryable HTTP status codes.
-    ///
-    /// This will return None to indicate cancellation.  Cancellation may happen from
-    /// the cancellation token passed in, or from Self's cancellation token (i.e. node
-    /// going offline).
-    pub(crate) async fn with_client_retries<T, O, F>(
-        &self,
-        mut op: O,
-        jwt: &Option<String>,
-        warn_threshold: u32,
-        max_retries: u32,
-        timeout: Duration,
-        cancel: &CancellationToken,
-    ) -> Option<mgmt_api::Result<T>>
-    where
-        O: FnMut(mgmt_api::Client) -> F,
-        F: std::future::Future<Output = mgmt_api::Result<T>>,
-    {
-        fn is_fatal(e: &mgmt_api::Error) -> bool {
-            use mgmt_api::Error::*;
-            match e {
-                ReceiveBody(_) | ReceiveErrorBody(_) => false,
-                ApiError(StatusCode::SERVICE_UNAVAILABLE, _)
-                | ApiError(StatusCode::GATEWAY_TIMEOUT, _)
-                | ApiError(StatusCode::REQUEST_TIMEOUT, _) => false,
-                ApiError(_, _) => true,
-                Cancelled => true,
-            }
-        }
-
-        backoff::retry(
-            || {
-                let http_client = reqwest::ClientBuilder::new()
-                    .timeout(timeout)
-                    .build()
-                    .expect("Failed to construct HTTP client");
-
-                let client =
-                    mgmt_api::Client::from_client(http_client, self.base_url(), jwt.as_deref());
-
-                let node_cancel_fut = self.cancel.cancelled();
-
-                let op_fut = op(client);
-
-                async {
-                    tokio::select! {
-                        r = op_fut=> {r},
-                        _ = node_cancel_fut => {
-                        Err(mgmt_api::Error::Cancelled)
-                    }}
-                }
-            },
-            is_fatal,
-            warn_threshold,
-            max_retries,
-            &format!(
-                "Call to node {} ({}:{}) management API",
-                self.id, self.listen_http_addr, self.listen_http_port
-            ),
-            cancel,
-        )
-        .await
-    }
-}
-
-impl std::fmt::Display for Node {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{} ({})", self.id, self.listen_http_addr)
-    }
-}
-
-impl std::fmt::Debug for Node {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{} ({})", self.id, self.listen_http_addr)
-    }
 }

control_plane/attachment_service/src/persistence.rs

@@ -9,7 +9,7 @@ use camino::Utf8PathBuf;
 use diesel::pg::PgConnection;
 use diesel::prelude::*;
 use diesel::Connection;
-use pageserver_api::controller_api::{NodeSchedulingPolicy, PlacementPolicy};
+use pageserver_api::controller_api::NodeSchedulingPolicy;
 use pageserver_api::models::TenantConfig;
 use pageserver_api::shard::{ShardCount, ShardNumber, TenantShardId};
 use serde::{Deserialize, Serialize};
@@ -17,6 +17,7 @@ use utils::generation::Generation;
 use utils::id::{NodeId, TenantId};
 
 use crate::node::Node;
+use crate::PlacementPolicy;
 
 /// ## What do we store?
 ///
@@ -207,7 +208,7 @@ impl Persistence {
                 tenant.tenant_id = tenant_id.to_string();
                 tenant.config = serde_json::to_string(&TenantConfig::default())
                     .map_err(|e| DatabaseError::Logical(format!("Serialization error: {e}")))?;
-                tenant.placement_policy = serde_json::to_string(&PlacementPolicy::Single)
+                tenant.placement_policy = serde_json::to_string(&PlacementPolicy::default())
                     .map_err(|e| DatabaseError::Logical(format!("Serialization error: {e}")))?;
             }
         }
@@ -330,15 +331,7 @@ impl Persistence {
                 shard_number: ShardNumber(tsp.shard_number as u8),
                 shard_count: ShardCount::new(tsp.shard_count as u8),
             };
-
-            let Some(g) = tsp.generation else {
-                // If the generation_pageserver column was non-NULL, then the generation column should also be non-NULL:
-                // we only set generation_pageserver when setting generation.
-                return Err(DatabaseError::Logical(
-                    "Generation should always be set after incrementing".to_string(),
-                ));
-            };
-            result.insert(tenant_shard_id, Generation::new(g as u32));
+            result.insert(tenant_shard_id, Generation::new(tsp.generation as u32));
         }
 
         Ok(result)
@@ -371,85 +364,7 @@ impl Persistence {
             })
             .await?;
 
-        // Generation is always non-null in the rseult: if the generation column had been NULL, then we
-        // should have experienced an SQL Confilict error while executing a query that tries to increment it.
-        debug_assert!(updated.generation.is_some());
-        let Some(g) = updated.generation else {
-            return Err(DatabaseError::Logical(
-                "Generation should always be set after incrementing".to_string(),
-            )
-            .into());
-        };
-
-        Ok(Generation::new(g as u32))
-    }
-
-    /// For use when updating a persistent property of a tenant, such as its config or placement_policy.
-    ///
-    /// Do not use this for settting generation, unless in the special onboarding code path (/location_config)
-    /// API: use [`Self::increment_generation`] instead.  Setting the generation via this route is a one-time thing
-    /// that we only do the first time a tenant is set to an attached policy via /location_config.
-    pub(crate) async fn update_tenant_shard(
-        &self,
-        tenant_shard_id: TenantShardId,
-        input_placement_policy: PlacementPolicy,
-        input_config: TenantConfig,
-        input_generation: Option<Generation>,
-    ) -> DatabaseResult<()> {
-        use crate::schema::tenant_shards::dsl::*;
-
-        self.with_conn(move |conn| {
-            let query = diesel::update(tenant_shards)
-                .filter(tenant_id.eq(tenant_shard_id.tenant_id.to_string()))
-                .filter(shard_number.eq(tenant_shard_id.shard_number.0 as i32))
-                .filter(shard_count.eq(tenant_shard_id.shard_count.literal() as i32));
-
-            if let Some(input_generation) = input_generation {
-                // Update includes generation column
-                query
-                    .set((
-                        generation.eq(Some(input_generation.into().unwrap() as i32)),
-                        placement_policy
-                            .eq(serde_json::to_string(&input_placement_policy).unwrap()),
-                        config.eq(serde_json::to_string(&input_config).unwrap()),
-                    ))
-                    .execute(conn)?;
-            } else {
-                // Update does not include generation column
-                query
-                    .set((
-                        placement_policy
-                            .eq(serde_json::to_string(&input_placement_policy).unwrap()),
-                        config.eq(serde_json::to_string(&input_config).unwrap()),
-                    ))
-                    .execute(conn)?;
-            }
-
-            Ok(())
-        })
-        .await?;
-
-        Ok(())
-    }
-
-    pub(crate) async fn update_tenant_config(
-        &self,
-        input_tenant_id: TenantId,
-        input_config: TenantConfig,
-    ) -> DatabaseResult<()> {
-        use crate::schema::tenant_shards::dsl::*;
-
-        self.with_conn(move |conn| {
-            diesel::update(tenant_shards)
-                .filter(tenant_id.eq(input_tenant_id.to_string()))
-                .set((config.eq(serde_json::to_string(&input_config).unwrap()),))
-                .execute(conn)?;
-
-            Ok(())
-        })
-        .await?;
-
-        Ok(())
+        Ok(Generation::new(updated.generation as u32))
     }
 
     pub(crate) async fn detach(&self, tenant_shard_id: TenantShardId) -> anyhow::Result<()> {
@@ -460,7 +375,7 @@ impl Persistence {
                 .filter(shard_number.eq(tenant_shard_id.shard_number.0 as i32))
                 .filter(shard_count.eq(tenant_shard_id.shard_count.literal() as i32))
                 .set((
-                    generation_pageserver.eq(Option::<i64>::None),
+                    generation_pageserver.eq(i64::MAX),
                     placement_policy.eq(serde_json::to_string(&PlacementPolicy::Detached).unwrap()),
                 ))
                 .execute(conn)?;
@@ -586,15 +501,12 @@ pub(crate) struct TenantShardPersistence {
     pub(crate) shard_stripe_size: i32,
 
     // Latest generation number: next time we attach, increment this
-    // and use the incremented number when attaching.
-    //
-    // Generation is only None when first onboarding a tenant, where it may
-    // be in PlacementPolicy::Secondary and therefore have no valid generation state.
-    pub(crate) generation: Option<i32>,
+    // and use the incremented number when attaching
+    pub(crate) generation: i32,
 
     // Currently attached pageserver
     #[serde(rename = "pageserver")]
-    pub(crate) generation_pageserver: Option<i64>,
+    pub(crate) generation_pageserver: i64,
 
     #[serde(default)]
     pub(crate) placement_policy: String,

control_plane/attachment_service/src/reconciler.rs

@@ -1,5 +1,6 @@
 use crate::persistence::Persistence;
 use crate::service;
+use pageserver_api::controller_api::NodeAvailability;
 use pageserver_api::models::{
     LocationConfig, LocationConfigMode, LocationConfigSecondary, TenantConfig,
 };
@@ -25,18 +26,17 @@ pub(super) struct Reconciler {
     /// of a tenant's state from when we spawned a reconcile task.
     pub(super) tenant_shard_id: TenantShardId,
     pub(crate) shard: ShardIdentity,
-    pub(crate) generation: Option<Generation>,
+    pub(crate) generation: Generation,
     pub(crate) intent: TargetState,
-
-    /// Nodes not referenced by [`Self::intent`], from which we should try
-    /// to detach this tenant shard.
-    pub(crate) detach: Vec<Node>,
-
     pub(crate) config: TenantConfig,
     pub(crate) observed: ObservedState,
 
     pub(crate) service_config: service::Config,
 
+    /// A snapshot of the pageservers as they were when we were asked
+    /// to reconcile.
+    pub(crate) pageservers: Arc<HashMap<NodeId, Node>>,
+
     /// A hook to notify the running postgres instances when we change the location
     /// of a tenant.  Use this via [`Self::compute_notify`] to update our failure flag
     /// and guarantee eventual retries.
@@ -67,37 +67,29 @@ pub(super) struct Reconciler {
 /// and the TargetState is just the instruction for a particular Reconciler run.
 #[derive(Debug)]
 pub(crate) struct TargetState {
-    pub(crate) attached: Option<Node>,
-    pub(crate) secondary: Vec<Node>,
+    pub(crate) attached: Option<NodeId>,
+    pub(crate) secondary: Vec<NodeId>,
 }
 
 impl TargetState {
-    pub(crate) fn from_intent(nodes: &HashMap<NodeId, Node>, intent: &IntentState) -> Self {
+    pub(crate) fn from_intent(intent: &IntentState) -> Self {
         Self {
-            attached: intent.get_attached().map(|n| {
-                nodes
-                    .get(&n)
-                    .expect("Intent attached referenced non-existent node")
-                    .clone()
-            }),
-            secondary: intent
-                .get_secondary()
-                .iter()
-                .map(|n| {
-                    nodes
-                        .get(n)
-                        .expect("Intent secondary referenced non-existent node")
-                        .clone()
-                })
-                .collect(),
+            attached: *intent.get_attached(),
+            secondary: intent.get_secondary().clone(),
         }
     }
+
+    fn all_pageservers(&self) -> Vec<NodeId> {
+        let mut result = self.secondary.clone();
+        if let Some(node_id) = &self.attached {
+            result.push(*node_id);
+        }
+        result
+    }
 }
 
 #[derive(thiserror::Error, Debug)]
 pub(crate) enum ReconcileError {
-    #[error(transparent)]
-    Remote(#[from] mgmt_api::Error),
     #[error(transparent)]
     Notify(#[from] NotifyError),
     #[error("Cancelled")]
@@ -109,83 +101,44 @@ pub(crate) enum ReconcileError {
 impl Reconciler {
     async fn location_config(
         &mut self,
-        node: &Node,
+        node_id: NodeId,
         config: LocationConfig,
         flush_ms: Option<Duration>,
-        lazy: bool,
-    ) -> Result<(), ReconcileError> {
-        self.observed
-            .locations
-            .insert(node.get_id(), ObservedStateLocation { conf: None });
-
-        // TODO: amend locations that use long-polling: they will hit this timeout.
-        let timeout = Duration::from_secs(25);
-
-        tracing::info!("location_config({node}) calling: {:?}", config);
-        let tenant_shard_id = self.tenant_shard_id;
-        let config_ref = &config;
-        match node
-            .with_client_retries(
-                |client| async move {
-                    let config = config_ref.clone();
-                    client
-                        .location_config(tenant_shard_id, config.clone(), flush_ms, lazy)
-                        .await
-                },
-                &self.service_config.jwt_token,
-                1,
-                3,
-                timeout,
-                &self.cancel,
-            )
-            .await
-        {
-            Some(Ok(_)) => {}
-            Some(Err(e)) => return Err(e.into()),
-            None => return Err(ReconcileError::Cancel),
-        };
-        tracing::info!("location_config({node}) complete: {:?}", config);
+    ) -> anyhow::Result<()> {
+        let node = self
+            .pageservers
+            .get(&node_id)
+            .expect("Pageserver may not be removed while referenced");
 
         self.observed
             .locations
-            .insert(node.get_id(), ObservedStateLocation { conf: Some(config) });
+            .insert(node.id, ObservedStateLocation { conf: None });
+
+        tracing::info!("location_config({}) calling: {:?}", node_id, config);
+        let client =
+            mgmt_api::Client::new(node.base_url(), self.service_config.jwt_token.as_deref());
+        client
+            .location_config(self.tenant_shard_id, config.clone(), flush_ms)
+            .await?;
+        tracing::info!("location_config({}) complete: {:?}", node_id, config);
+
+        self.observed
+            .locations
+            .insert(node.id, ObservedStateLocation { conf: Some(config) });
 
         Ok(())
     }
 
-    fn get_node(&self, node_id: &NodeId) -> Option<&Node> {
-        if let Some(node) = self.intent.attached.as_ref() {
-            if node.get_id() == *node_id {
-                return Some(node);
-            }
-        }
-
-        if let Some(node) = self
-            .intent
-            .secondary
-            .iter()
-            .find(|n| n.get_id() == *node_id)
-        {
-            return Some(node);
-        }
-
-        if let Some(node) = self.detach.iter().find(|n| n.get_id() == *node_id) {
-            return Some(node);
-        }
-
-        None
-    }
-
     async fn maybe_live_migrate(&mut self) -> Result<(), ReconcileError> {
-        let destination = if let Some(node) = &self.intent.attached {
-            match self.observed.locations.get(&node.get_id()) {
+        let destination = if let Some(node_id) = self.intent.attached {
+            match self.observed.locations.get(&node_id) {
                 Some(conf) => {
                     // We will do a live migration only if the intended destination is not
                     // currently in an attached state.
                     match &conf.conf {
                         Some(conf) if conf.mode == LocationConfigMode::Secondary => {
                             // Fall through to do a live migration
-                            node
+                            node_id
                         }
                         None | Some(_) => {
                             // Attached or uncertain: don't do a live migration, proceed
@@ -198,7 +151,7 @@ impl Reconciler {
                 None => {
                     // Our destination is not attached: maybe live migrate if some other
                     // node is currently attached.  Fall through.
-                    node
+                    node_id
                 }
             }
         } else {
@@ -211,13 +164,15 @@ impl Reconciler {
         for (node_id, state) in &self.observed.locations {
             if let Some(observed_conf) = &state.conf {
                 if observed_conf.mode == LocationConfigMode::AttachedSingle {
+                    let node = self
+                        .pageservers
+                        .get(node_id)
+                        .expect("Nodes may not be removed while referenced");
                     // We will only attempt live migration if the origin is not offline: this
                     // avoids trying to do it while reconciling after responding to an HA failover.
-                    if let Some(node) = self.get_node(node_id) {
-                        if node.is_available() {
-                            origin = Some(node.clone());
-                            break;
-                        }
+                    if !matches!(node.availability, NodeAvailability::Offline) {
+                        origin = Some(*node_id);
+                        break;
                     }
                 }
             }
@@ -230,7 +185,7 @@ impl Reconciler {
 
         // We have an origin and a destination: proceed to do the live migration
         tracing::info!("Live migrating {}->{}", origin, destination);
-        self.live_migrate(origin, destination.clone()).await?;
+        self.live_migrate(origin, destination).await?;
 
         Ok(())
     }
@@ -238,8 +193,13 @@ impl Reconciler {
     async fn get_lsns(
         &self,
         tenant_shard_id: TenantShardId,
-        node: &Node,
+        node_id: &NodeId,
     ) -> anyhow::Result<HashMap<TimelineId, Lsn>> {
+        let node = self
+            .pageservers
+            .get(node_id)
+            .expect("Pageserver may not be removed while referenced");
+
         let client =
             mgmt_api::Client::new(node.base_url(), self.service_config.jwt_token.as_deref());
 
@@ -250,27 +210,19 @@ impl Reconciler {
             .collect())
     }
 
-    async fn secondary_download(
-        &self,
-        tenant_shard_id: TenantShardId,
-        node: &Node,
-    ) -> Result<(), ReconcileError> {
-        match node
-            .with_client_retries(
-                |client| async move { client.tenant_secondary_download(tenant_shard_id).await },
-                &self.service_config.jwt_token,
-                1,
-                1,
-                Duration::from_secs(60),
-                &self.cancel,
-            )
-            .await
-        {
-            None => Err(ReconcileError::Cancel),
-            Some(Ok(_)) => Ok(()),
-            Some(Err(e)) => {
-                tracing::info!("  (skipping destination download: {})", e);
-                Ok(())
+    async fn secondary_download(&self, tenant_shard_id: TenantShardId, node_id: &NodeId) {
+        let node = self
+            .pageservers
+            .get(node_id)
+            .expect("Pageserver may not be removed while referenced");
+
+        let client =
+            mgmt_api::Client::new(node.base_url(), self.service_config.jwt_token.as_deref());
+
+        match client.tenant_secondary_download(tenant_shard_id).await {
+            Ok(()) => {}
+            Err(_) => {
+                tracing::info!("  (skipping, destination wasn't in secondary mode)")
             }
         }
     }
@@ -278,14 +230,17 @@ impl Reconciler {
     async fn await_lsn(
         &self,
         tenant_shard_id: TenantShardId,
-        node: &Node,
+        pageserver_id: &NodeId,
         baseline: HashMap<TimelineId, Lsn>,
     ) -> anyhow::Result<()> {
         loop {
-            let latest = match self.get_lsns(tenant_shard_id, node).await {
+            let latest = match self.get_lsns(tenant_shard_id, pageserver_id).await {
                 Ok(l) => l,
                 Err(e) => {
-                    tracing::info!("🕑 Can't get LSNs on node {node} yet, waiting ({e})",);
+                    println!(
+                        "🕑 Can't get LSNs on pageserver {} yet, waiting ({e})",
+                        pageserver_id
+                    );
                     std::thread::sleep(Duration::from_millis(500));
                     continue;
                 }
@@ -295,7 +250,7 @@ impl Reconciler {
             for (timeline_id, baseline_lsn) in &baseline {
                 match latest.get(timeline_id) {
                     Some(latest_lsn) => {
-                        tracing::info!("🕑 LSN origin {baseline_lsn} vs destination {latest_lsn}");
+                        println!("🕑 LSN origin {baseline_lsn} vs destination {latest_lsn}");
                         if latest_lsn < baseline_lsn {
                             any_behind = true;
                         }
@@ -310,7 +265,7 @@ impl Reconciler {
             }
 
             if !any_behind {
-                tracing::info!("✅ LSN caught up.  Proceeding...");
+                println!("✅ LSN caught up.  Proceeding...");
                 break;
             } else {
                 std::thread::sleep(Duration::from_millis(500));
@@ -322,11 +277,11 @@ impl Reconciler {
 
     pub async fn live_migrate(
         &mut self,
-        origin_ps: Node,
-        dest_ps: Node,
-    ) -> Result<(), ReconcileError> {
+        origin_ps_id: NodeId,
+        dest_ps_id: NodeId,
+    ) -> anyhow::Result<()> {
         // `maybe_live_migrate` is responsibble for sanity of inputs
-        assert!(origin_ps.get_id() != dest_ps.get_id());
+        assert!(origin_ps_id != dest_ps_id);
 
         fn build_location_config(
             shard: &ShardIdentity,
@@ -346,7 +301,10 @@ impl Reconciler {
             }
         }
 
-        tracing::info!("🔁 Switching origin node {origin_ps} to stale mode",);
+        tracing::info!(
+            "🔁 Switching origin pageserver {} to stale mode",
+            origin_ps_id
+        );
 
         // FIXME: it is incorrect to use self.generation here, we should use the generation
         // from the ObservedState of the origin pageserver (it might be older than self.generation)
@@ -354,57 +312,58 @@ impl Reconciler {
             &self.shard,
             &self.config,
             LocationConfigMode::AttachedStale,
-            self.generation,
+            Some(self.generation),
             None,
         );
-        self.location_config(&origin_ps, stale_conf, Some(Duration::from_secs(10)), false)
+        self.location_config(origin_ps_id, stale_conf, Some(Duration::from_secs(10)))
             .await?;
 
-        let baseline_lsns = Some(self.get_lsns(self.tenant_shard_id, &origin_ps).await?);
+        let baseline_lsns = Some(self.get_lsns(self.tenant_shard_id, &origin_ps_id).await?);
 
         // If we are migrating to a destination that has a secondary location, warm it up first
-        if let Some(destination_conf) = self.observed.locations.get(&dest_ps.get_id()) {
+        if let Some(destination_conf) = self.observed.locations.get(&dest_ps_id) {
             if let Some(destination_conf) = &destination_conf.conf {
                 if destination_conf.mode == LocationConfigMode::Secondary {
-                    tracing::info!("🔁 Downloading latest layers to destination node {dest_ps}",);
-                    self.secondary_download(self.tenant_shard_id, &dest_ps)
-                        .await?;
+                    tracing::info!(
+                        "🔁 Downloading latest layers to destination pageserver {}",
+                        dest_ps_id,
+                    );
+                    self.secondary_download(self.tenant_shard_id, &dest_ps_id)
+                        .await;
                 }
             }
         }
 
         // Increment generation before attaching to new pageserver
-        self.generation = Some(
-            self.persistence
-                .increment_generation(self.tenant_shard_id, dest_ps.get_id())
-                .await?,
-        );
+        self.generation = self
+            .persistence
+            .increment_generation(self.tenant_shard_id, dest_ps_id)
+            .await?;
 
         let dest_conf = build_location_config(
             &self.shard,
             &self.config,
             LocationConfigMode::AttachedMulti,
-            self.generation,
+            Some(self.generation),
             None,
         );
 
-        tracing::info!("🔁 Attaching to pageserver {dest_ps}");
-        self.location_config(&dest_ps, dest_conf, None, false)
-            .await?;
+        tracing::info!("🔁 Attaching to pageserver {}", dest_ps_id);
+        self.location_config(dest_ps_id, dest_conf, None).await?;
 
         if let Some(baseline) = baseline_lsns {
             tracing::info!("🕑 Waiting for LSN to catch up...");
-            self.await_lsn(self.tenant_shard_id, &dest_ps, baseline)
+            self.await_lsn(self.tenant_shard_id, &dest_ps_id, baseline)
                 .await?;
         }
 
-        tracing::info!("🔁 Notifying compute to use pageserver {dest_ps}");
+        tracing::info!("🔁 Notifying compute to use pageserver {}", dest_ps_id);
 
         // During a live migration it is unhelpful to proceed if we couldn't notify compute: if we detach
         // the origin without notifying compute, we will render the tenant unavailable.
         while let Err(e) = self.compute_notify().await {
             match e {
-                NotifyError::Fatal(_) => return Err(ReconcileError::Notify(e)),
+                NotifyError::Fatal(_) => return Err(anyhow::anyhow!(e)),
                 _ => {
                     tracing::warn!(
                         "Live migration blocked by compute notification error, retrying: {e}"
@@ -422,81 +381,39 @@ impl Reconciler {
             None,
             Some(LocationConfigSecondary { warm: true }),
         );
-        self.location_config(&origin_ps, origin_secondary_conf.clone(), None, false)
+        self.location_config(origin_ps_id, origin_secondary_conf.clone(), None)
             .await?;
         // TODO: we should also be setting the ObservedState on earlier API calls, in case we fail
         // partway through.  In fact, all location conf API calls should be in a wrapper that sets
         // the observed state to None, then runs, then sets it to what we wrote.
         self.observed.locations.insert(
-            origin_ps.get_id(),
+            origin_ps_id,
             ObservedStateLocation {
                 conf: Some(origin_secondary_conf),
             },
         );
 
-        tracing::info!("🔁 Switching to AttachedSingle mode on node {dest_ps}",);
+        println!(
+            "🔁 Switching to AttachedSingle mode on pageserver {}",
+            dest_ps_id
+        );
         let dest_final_conf = build_location_config(
             &self.shard,
             &self.config,
             LocationConfigMode::AttachedSingle,
-            self.generation,
+            Some(self.generation),
             None,
         );
-        self.location_config(&dest_ps, dest_final_conf.clone(), None, false)
+        self.location_config(dest_ps_id, dest_final_conf.clone(), None)
             .await?;
         self.observed.locations.insert(
-            dest_ps.get_id(),
+            dest_ps_id,
             ObservedStateLocation {
                 conf: Some(dest_final_conf),
             },
         );
 
-        tracing::info!("✅ Migration complete");
-
-        Ok(())
-    }
-
-    async fn maybe_refresh_observed(&mut self) -> Result<(), ReconcileError> {
-        // If the attached node has uncertain state, read it from the pageserver before proceeding: this
-        // is important to avoid spurious generation increments.
-        //
-        // We don't need to do this for secondary/detach locations because it's harmless to just PUT their
-        // location conf, whereas for attached locations it can interrupt clients if we spuriously destroy/recreate
-        // the `Timeline` object in the pageserver.
-
-        let Some(attached_node) = self.intent.attached.as_ref() else {
-            // Nothing to do
-            return Ok(());
-        };
-
-        if matches!(
-            self.observed.locations.get(&attached_node.get_id()),
-            Some(ObservedStateLocation { conf: None })
-        ) {
-            let tenant_shard_id = self.tenant_shard_id;
-            let observed_conf = match attached_node
-                .with_client_retries(
-                    |client| async move { client.get_location_config(tenant_shard_id).await },
-                    &self.service_config.jwt_token,
-                    1,
-                    1,
-                    Duration::from_secs(5),
-                    &self.cancel,
-                )
-                .await
-            {
-                Some(Ok(observed)) => observed,
-                Some(Err(e)) => return Err(e.into()),
-                None => return Err(ReconcileError::Cancel),
-            };
-            tracing::info!("Scanned location configuration on {attached_node}: {observed_conf:?}");
-            self.observed.locations.insert(
-                attached_node.get_id(),
-                ObservedStateLocation {
-                    conf: observed_conf,
-                },
-            );
-        }
+        println!("✅ Migration complete");
 
         Ok(())
     }
@@ -508,80 +425,32 @@ impl Reconciler {
     /// general case reconciliation where we walk through the intent by pageserver
     /// and call out to the pageserver to apply the desired state.
     pub(crate) async fn reconcile(&mut self) -> Result<(), ReconcileError> {
-        // Prepare: if we have uncertain `observed` state for our would-be attachement location, then refresh it
-        self.maybe_refresh_observed().await?;
+        // TODO: if any of self.observed is None, call to remote pageservers
+        // to learn correct state.
 
         // Special case: live migration
         self.maybe_live_migrate().await?;
 
         // If the attached pageserver is not attached, do so now.
-        if let Some(node) = self.intent.attached.as_ref() {
-            // If we are in an attached policy, then generation must have been set (null generations
-            // are only present when a tenant is initially loaded with a secondary policy)
-            debug_assert!(self.generation.is_some());
-            let Some(generation) = self.generation else {
-                return Err(ReconcileError::Other(anyhow::anyhow!(
-                    "Attempted to attach with NULL generation"
-                )));
-            };
-
-            let mut wanted_conf = attached_location_conf(generation, &self.shard, &self.config);
-            match self.observed.locations.get(&node.get_id()) {
+        if let Some(node_id) = self.intent.attached {
+            let mut wanted_conf =
+                attached_location_conf(self.generation, &self.shard, &self.config);
+            match self.observed.locations.get(&node_id) {
                 Some(conf) if conf.conf.as_ref() == Some(&wanted_conf) => {
                     // Nothing to do
-                    tracing::info!(node_id=%node.get_id(), "Observed configuration already correct.")
+                    tracing::info!(%node_id, "Observed configuration already correct.")
                 }
-                observed => {
+                _ => {
                     // In all cases other than a matching observed configuration, we will
                     // reconcile this location.  This includes locations with different configurations, as well
                     // as locations with unknown (None) observed state.
-
-                    // The general case is to increment the generation.  However, there are cases
-                    // where this is not necessary:
-                    // - if we are only updating the TenantConf part of the location
-                    // - if we are only changing the attachment mode (e.g. going to attachedmulti or attachedstale)
-                    //   and the location was already in the correct generation
-                    let increment_generation = match observed {
-                        None => true,
-                        Some(ObservedStateLocation { conf: None }) => true,
-                        Some(ObservedStateLocation {
-                            conf: Some(observed),
-                        }) => {
-                            let generations_match = observed.generation == wanted_conf.generation;
-
-                            use LocationConfigMode::*;
-                            let mode_transition_requires_gen_inc =
-                                match (observed.mode, wanted_conf.mode) {
-                                    // Usually the short-lived attachment modes (multi and stale) are only used
-                                    // in the case of [`Self::live_migrate`], but it is simple to handle them correctly
-                                    // here too.  Locations are allowed to go Single->Stale and Multi->Single within the same generation.
-                                    (AttachedSingle, AttachedStale) => false,
-                                    (AttachedMulti, AttachedSingle) => false,
-                                    (lhs, rhs) => lhs != rhs,
-                                };
-
-                            !generations_match || mode_transition_requires_gen_inc
-                        }
-                    };
-
-                    if increment_generation {
-                        let generation = self
-                            .persistence
-                            .increment_generation(self.tenant_shard_id, node.get_id())
-                            .await?;
-                        self.generation = Some(generation);
-                        wanted_conf.generation = generation.into();
-                    }
-                    tracing::info!(node_id=%node.get_id(), "Observed configuration requires update.");
-
-                    // Because `node` comes from a ref to &self, clone it before calling into a &mut self
-                    // function: this could be avoided by refactoring the state mutated by location_config into
-                    // a separate type to Self.
-                    let node = node.clone();
-
-                    // Use lazy=true, because we may run many of Self concurrently, and do not want to
-                    // overload the pageserver with logical size calculations.
-                    self.location_config(&node, wanted_conf, None, true).await?;
+                    self.generation = self
+                        .persistence
+                        .increment_generation(self.tenant_shard_id, node_id)
+                        .await?;
+                    wanted_conf.generation = self.generation.into();
+                    tracing::info!(%node_id, "Observed configuration requires update.");
+                    self.location_config(node_id, wanted_conf, None).await?;
                     self.compute_notify().await?;
                 }
             }
@@ -590,27 +459,33 @@ impl Reconciler {
         // Configure secondary locations: if these were previously attached this
         // implicitly downgrades them from attached to secondary.
         let mut changes = Vec::new();
-        for node in &self.intent.secondary {
+        for node_id in &self.intent.secondary {
             let wanted_conf = secondary_location_conf(&self.shard, &self.config);
-            match self.observed.locations.get(&node.get_id()) {
+            match self.observed.locations.get(node_id) {
                 Some(conf) if conf.conf.as_ref() == Some(&wanted_conf) => {
                     // Nothing to do
-                    tracing::info!(node_id=%node.get_id(), "Observed configuration already correct.")
+                    tracing::info!(%node_id, "Observed configuration already correct.")
                 }
                 _ => {
                     // In all cases other than a matching observed configuration, we will
                     // reconcile this location.
-                    tracing::info!(node_id=%node.get_id(), "Observed configuration requires update.");
-                    changes.push((node.clone(), wanted_conf))
+                    tracing::info!(%node_id, "Observed configuration requires update.");
+                    changes.push((*node_id, wanted_conf))
                 }
             }
         }
 
         // Detach any extraneous pageservers that are no longer referenced
         // by our intent.
-        for node in &self.detach {
+        let all_pageservers = self.intent.all_pageservers();
+        for node_id in self.observed.locations.keys() {
+            if all_pageservers.contains(node_id) {
+                // We are only detaching pageservers that aren't used at all.
+                continue;
+            }
+
             changes.push((
-                node.clone(),
+                *node_id,
                 LocationConfig {
                     mode: LocationConfigMode::Detached,
                     generation: None,
@@ -623,11 +498,11 @@ impl Reconciler {
             ));
         }
 
-        for (node, conf) in changes {
+        for (node_id, conf) in changes {
             if self.cancel.is_cancelled() {
                 return Err(ReconcileError::Cancel);
             }
-            self.location_config(&node, conf, None, false).await?;
+            self.location_config(node_id, conf, None).await?;
         }
 
         Ok(())
@@ -636,21 +511,16 @@ impl Reconciler {
     pub(crate) async fn compute_notify(&mut self) -> Result<(), NotifyError> {
         // Whenever a particular Reconciler emits a notification, it is always notifying for the intended
         // destination.
-        if let Some(node) = &self.intent.attached {
+        if let Some(node_id) = self.intent.attached {
             let result = self
                 .compute_hook
-                .notify(
-                    self.tenant_shard_id,
-                    node.get_id(),
-                    self.shard.stripe_size,
-                    &self.cancel,
-                )
+                .notify(self.tenant_shard_id, node_id, &self.cancel)
                 .await;
             if let Err(e) = &result {
                 // It is up to the caller whether they want to drop out on this error, but they don't have to:
                 // in general we should avoid letting unavailability of the cloud control plane stop us from
                 // making progress.
-                tracing::warn!("Failed to notify compute of attached pageserver {node}: {e}");
+                tracing::warn!("Failed to notify compute of attached pageserver {node_id}: {e}");
                 // Set this flag so that in our ReconcileResult we will set the flag on the shard that it
                 // needs to retry at some point.
                 self.compute_notify_failure = true;

control_plane/attachment_service/src/scheduler.rs

@@ -43,7 +43,7 @@ impl Scheduler {
         let mut scheduler_nodes = HashMap::new();
         for node in nodes {
             scheduler_nodes.insert(
-                node.get_id(),
+                node.id,
                 SchedulerNode {
                     shard_count: 0,
                     may_schedule: node.may_schedule(),
@@ -68,7 +68,7 @@ impl Scheduler {
         let mut expect_nodes: HashMap<NodeId, SchedulerNode> = HashMap::new();
         for node in nodes {
             expect_nodes.insert(
-                node.get_id(),
+                node.id,
                 SchedulerNode {
                     shard_count: 0,
                     may_schedule: node.may_schedule(),
@@ -156,7 +156,7 @@ impl Scheduler {
 
     pub(crate) fn node_upsert(&mut self, node: &Node) {
         use std::collections::hash_map::Entry::*;
-        match self.nodes.entry(node.get_id()) {
+        match self.nodes.entry(node.id) {
             Occupied(mut entry) => {
                 entry.get_mut().may_schedule = node.may_schedule();
             }
@@ -255,6 +255,7 @@ impl Scheduler {
 pub(crate) mod test_utils {
 
     use crate::node::Node;
+    use pageserver_api::controller_api::{NodeAvailability, NodeSchedulingPolicy};
     use std::collections::HashMap;
     use utils::id::NodeId;
     /// Test helper: synthesize the requested number of nodes, all in active state.
@@ -263,17 +264,18 @@ pub(crate) mod test_utils {
     pub(crate) fn make_test_nodes(n: u64) -> HashMap<NodeId, Node> {
         (1..n + 1)
             .map(|i| {
-                (NodeId(i), {
-                    let node = Node::new(
-                        NodeId(i),
-                        format!("httphost-{i}"),
-                        80 + i as u16,
-                        format!("pghost-{i}"),
-                        5432 + i as u16,
-                    );
-                    assert!(node.is_available());
-                    node
-                })
+                (
+                    NodeId(i),
+                    Node {
+                        id: NodeId(i),
+                        availability: NodeAvailability::Active,
+                        scheduling: NodeSchedulingPolicy::Active,
+                        listen_http_addr: format!("httphost-{i}"),
+                        listen_http_port: 80 + i as u16,
+                        listen_pg_addr: format!("pghost-{i}"),
+                        listen_pg_port: 5432 + i as u16,
+                    },
+                )
             })
             .collect()
     }
@@ -282,6 +284,7 @@ pub(crate) mod test_utils {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use utils::id::NodeId;
 
     use crate::tenant_state::IntentState;
     #[test]

control_plane/attachment_service/src/schema.rs

@@ -17,8 +17,8 @@ diesel::table! {
         shard_number -> Int4,
         shard_count -> Int4,
         shard_stripe_size -> Int4,
-        generation -> Nullable<Int4>,
-        generation_pageserver -> Nullable<Int8>,
+        generation -> Int4,
+        generation_pageserver -> Int8,
         placement_policy -> Varchar,
         splitting -> Int2,
         config -> Text,

control_plane/attachment_service/src/tenant_state.rs

@@ -1,11 +1,7 @@
-use std::{
-    collections::{HashMap, HashSet},
-    sync::Arc,
-    time::Duration,
-};
+use std::{collections::HashMap, sync::Arc, time::Duration};
 
 use crate::{metrics, persistence::TenantShardPersistence};
-use pageserver_api::controller_api::PlacementPolicy;
+use pageserver_api::controller_api::NodeAvailability;
 use pageserver_api::{
     models::{LocationConfig, LocationConfigMode, TenantConfig},
     shard::{ShardIdentity, TenantShardId},
@@ -29,7 +25,7 @@ use crate::{
         attached_location_conf, secondary_location_conf, ReconcileError, Reconciler, TargetState,
     },
     scheduler::{ScheduleError, Scheduler},
-    service, Sequence,
+    service, PlacementPolicy, Sequence,
 };
 
 /// Serialization helper
@@ -57,11 +53,8 @@ pub(crate) struct TenantState {
     pub(crate) sequence: Sequence,
 
     // Latest generation number: next time we attach, increment this
-    // and use the incremented number when attaching.
-    //
-    // None represents an incompletely onboarded tenant via the [`Service::location_config`]
-    // API, where this tenant may only run in PlacementPolicy::Secondary.
-    pub(crate) generation: Option<Generation>,
+    // and use the incremented number when attaching
+    pub(crate) generation: Generation,
 
     // High level description of how the tenant should be set up.  Provided
     // externally.
@@ -188,13 +181,6 @@ impl IntentState {
         }
     }
 
-    /// Remove the last secondary node from the list of secondaries
-    pub(crate) fn pop_secondary(&mut self, scheduler: &mut Scheduler) {
-        if let Some(node_id) = self.secondary.pop() {
-            scheduler.node_dec_ref(node_id);
-        }
-    }
-
     pub(crate) fn clear(&mut self, scheduler: &mut Scheduler) {
         if let Some(old_attached) = self.attached.take() {
             scheduler.node_dec_ref(old_attached);
@@ -222,13 +208,11 @@ impl IntentState {
         &self.secondary
     }
 
-    /// If the node is in use as the attached location, demote it into
-    /// the list of secondary locations.  This is used when a node goes offline,
-    /// and we want to use a different node for attachment, but not permanently
-    /// forget the location on the offline node.
+    /// When a node goes offline, we update intents to avoid using it
+    /// as their attached pageserver.
     ///
     /// Returns true if a change was made
-    pub(crate) fn demote_attached(&mut self, node_id: NodeId) -> bool {
+    pub(crate) fn notify_offline(&mut self, node_id: NodeId) -> bool {
         if self.attached == Some(node_id) {
             // TODO: when scheduler starts tracking attached + secondary counts separately, we will
             // need to call into it here.
@@ -331,7 +315,7 @@ pub(crate) struct ReconcileResult {
     pub(crate) result: Result<(), ReconcileError>,
 
     pub(crate) tenant_shard_id: TenantShardId,
-    pub(crate) generation: Option<Generation>,
+    pub(crate) generation: Generation,
     pub(crate) observed: ObservedState,
 
     /// Set [`TenantState::pending_compute_notification`] from this flag
@@ -356,7 +340,7 @@ impl TenantState {
             tenant_shard_id,
             policy,
             intent: IntentState::default(),
-            generation: Some(Generation::new(0)),
+            generation: Generation::new(0),
             shard,
             observed: ObservedState::default(),
             config: TenantConfig::default(),
@@ -374,7 +358,7 @@ impl TenantState {
     /// [`ObservedState`], even if it violates my [`PlacementPolicy`].  Call [`Self::schedule`] next,
     /// to get an intent state that complies with placement policy.  The overall goal is to do scheduling
     /// in a way that makes use of any configured locations that already exist in the outside world.
-    pub(crate) fn intent_from_observed(&mut self, scheduler: &mut Scheduler) {
+    pub(crate) fn intent_from_observed(&mut self) {
         // Choose an attached location by filtering observed locations, and then sorting to get the highest
         // generation
         let mut attached_locs = self
@@ -399,7 +383,7 @@ impl TenantState {
 
         attached_locs.sort_by_key(|i| i.1);
         if let Some((node_id, _gen)) = attached_locs.into_iter().last() {
-            self.intent.set_attached(scheduler, Some(*node_id));
+            self.intent.attached = Some(*node_id);
         }
 
         // All remaining observed locations generate secondary intents.  This includes None
@@ -410,7 +394,7 @@ impl TenantState {
         // will take care of promoting one of these secondaries to be attached.
         self.observed.locations.keys().for_each(|node_id| {
             if Some(*node_id) != self.intent.attached {
-                self.intent.push_secondary(scheduler, *node_id);
+                self.intent.secondary.push(*node_id);
             }
         });
     }
@@ -454,16 +438,10 @@ impl TenantState {
         // more work on the same pageservers we're already using.
         let mut modified = false;
 
-        // Add/remove nodes to fulfil policy
         use PlacementPolicy::*;
         match self.policy {
             Single => {
                 // Should have exactly one attached, and zero secondaries
-                if !self.intent.secondary.is_empty() {
-                    self.intent.clear_secondary(scheduler);
-                    modified = true;
-                }
-
                 let (modified_attached, _attached_node_id) = self.schedule_attached(scheduler)?;
                 modified |= modified_attached;
 
@@ -473,23 +451,6 @@ impl TenantState {
                 }
             }
             Double(secondary_count) => {
-                let retain_secondaries = if self.intent.attached.is_none()
-                    && scheduler.node_preferred(&self.intent.secondary).is_some()
-                {
-                    // If we have no attached, and one of the secondaries is elegible to be promoted, retain
-                    // one more secondary than we usually would, as one of them will become attached futher down this function.
-                    secondary_count + 1
-                } else {
-                    secondary_count
-                };
-
-                while self.intent.secondary.len() > retain_secondaries {
-                    // We have no particular preference for one secondary location over another: just
-                    // arbitrarily drop from the end
-                    self.intent.pop_secondary(scheduler);
-                    modified = true;
-                }
-
                 // Should have exactly one attached, and N secondaries
                 let (modified_attached, attached_node_id) = self.schedule_attached(scheduler)?;
                 modified |= modified_attached;
@@ -502,28 +463,15 @@ impl TenantState {
                     modified = true;
                 }
             }
-            Secondary => {
-                if let Some(node_id) = self.intent.get_attached() {
-                    // Populate secondary by demoting the attached node
-                    self.intent.demote_attached(*node_id);
-                    modified = true;
-                } else if self.intent.secondary.is_empty() {
-                    // Populate secondary by scheduling a fresh node
-                    let node_id = scheduler.schedule_shard(&[])?;
-                    self.intent.push_secondary(scheduler, node_id);
-                    modified = true;
-                }
-                while self.intent.secondary.len() > 1 {
-                    // We have no particular preference for one secondary location over another: just
-                    // arbitrarily drop from the end
-                    self.intent.pop_secondary(scheduler);
-                    modified = true;
-                }
-            }
             Detached => {
-                // Never add locations in this mode
-                if self.intent.get_attached().is_some() || !self.intent.get_secondary().is_empty() {
-                    self.intent.clear(scheduler);
+                // Should have no attached or secondary pageservers
+                if self.intent.attached.is_some() {
+                    self.intent.set_attached(scheduler, None);
+                    modified = true;
+                }
+
+                if !self.intent.secondary.is_empty() {
+                    self.intent.clear_secondary(scheduler);
                     modified = true;
                 }
             }
@@ -568,20 +516,13 @@ impl TenantState {
         }
     }
 
-    fn dirty(&self, nodes: &Arc<HashMap<NodeId, Node>>) -> bool {
-        let mut dirty_nodes = HashSet::new();
-
+    fn dirty(&self) -> bool {
         if let Some(node_id) = self.intent.attached {
-            // Maybe panic: it is a severe bug if we try to attach while generation is null.
-            let generation = self
-                .generation
-                .expect("Attempted to enter attached state without a generation");
-
-            let wanted_conf = attached_location_conf(generation, &self.shard, &self.config);
+            let wanted_conf = attached_location_conf(self.generation, &self.shard, &self.config);
             match self.observed.locations.get(&node_id) {
                 Some(conf) if conf.conf.as_ref() == Some(&wanted_conf) => {}
                 Some(_) | None => {
-                    dirty_nodes.insert(node_id);
+                    return true;
                 }
             }
         }
@@ -591,7 +532,7 @@ impl TenantState {
             match self.observed.locations.get(node_id) {
                 Some(conf) if conf.conf.as_ref() == Some(&wanted_conf) => {}
                 Some(_) | None => {
-                    dirty_nodes.insert(*node_id);
+                    return true;
                 }
             }
         }
@@ -599,18 +540,17 @@ impl TenantState {
         for node_id in self.observed.locations.keys() {
             if self.intent.attached != Some(*node_id) && !self.intent.secondary.contains(node_id) {
                 // We have observed state that isn't part of our intent: need to clean it up.
-                dirty_nodes.insert(*node_id);
+                return true;
             }
         }
 
-        dirty_nodes.retain(|node_id| {
-            nodes
-                .get(node_id)
-                .map(|n| n.is_available())
-                .unwrap_or(false)
-        });
+        // Even if there is no pageserver work to be done, if we have a pending notification to computes,
+        // wake up a reconciler to send it.
+        if self.pending_compute_notification {
+            return true;
+        }
 
-        !dirty_nodes.is_empty()
+        false
     }
 
     #[allow(clippy::too_many_arguments)]
@@ -632,20 +572,15 @@ impl TenantState {
             let node = pageservers
                 .get(node_id)
                 .expect("Nodes may not be removed while referenced");
-            if observed_loc.conf.is_none() && node.is_available() {
+            if observed_loc.conf.is_none()
+                && !matches!(node.availability, NodeAvailability::Offline)
+            {
                 dirty_observed = true;
                 break;
             }
         }
 
-        let active_nodes_dirty = self.dirty(pageservers);
-
-        // Even if there is no pageserver work to be done, if we have a pending notification to computes,
-        // wake up a reconciler to send it.
-        let do_reconcile =
-            active_nodes_dirty || dirty_observed || self.pending_compute_notification;
-
-        if !do_reconcile {
+        if !self.dirty() && !dirty_observed {
             tracing::info!("Not dirty, no reconciliation needed.");
             return None;
         }
@@ -661,10 +596,6 @@ impl TenantState {
         // Reconcile already in flight for the current sequence?
         if let Some(handle) = &self.reconciler {
             if handle.sequence == self.sequence {
-                tracing::info!(
-                    "Reconciliation already in progress for sequence {:?}",
-                    self.sequence,
-                );
                 return Some(ReconcilerWaiter {
                     tenant_shard_id: self.tenant_shard_id,
                     seq_wait: self.waiter.clone(),
@@ -675,21 +606,6 @@ impl TenantState {
             }
         }
 
-        // Build list of nodes from which the reconciler should detach
-        let mut detach = Vec::new();
-        for node_id in self.observed.locations.keys() {
-            if self.intent.get_attached() != &Some(*node_id)
-                && !self.intent.secondary.contains(node_id)
-            {
-                detach.push(
-                    pageservers
-                        .get(node_id)
-                        .expect("Intent references non-existent pageserver")
-                        .clone(),
-                )
-            }
-        }
-
         // Reconcile in flight for a stale sequence?  Our sequence's task will wait for it before
         // doing our sequence's work.
         let old_handle = self.reconciler.take();
@@ -699,20 +615,15 @@ impl TenantState {
             return None;
         };
 
-        // Advance the sequence before spawning a reconciler, so that sequence waiters
-        // can distinguish between before+after the reconcile completes.
-        self.sequence = self.sequence.next();
-
         let reconciler_cancel = cancel.child_token();
-        let reconciler_intent = TargetState::from_intent(pageservers, &self.intent);
         let mut reconciler = Reconciler {
             tenant_shard_id: self.tenant_shard_id,
             shard: self.shard,
             generation: self.generation,
-            intent: reconciler_intent,
-            detach,
+            intent: TargetState::from_intent(&self.intent),
             config: self.config.clone(),
             observed: self.observed.clone(),
+            pageservers: pageservers.clone(),
             compute_hook: compute_hook.clone(),
             service_config: service_config.clone(),
             _gate_guard: gate_guard,
@@ -805,17 +716,6 @@ impl TenantState {
         })
     }
 
-    /// Called when a ReconcileResult has been emitted and the service is updating
-    /// our state: if the result is from a sequence >= my ReconcileHandle, then drop
-    /// the handle to indicate there is no longer a reconciliation in progress.
-    pub(crate) fn reconcile_complete(&mut self, sequence: Sequence) {
-        if let Some(reconcile_handle) = &self.reconciler {
-            if reconcile_handle.sequence <= sequence {
-                self.reconciler = None;
-            }
-        }
-    }
-
     // If we had any state at all referring to this node ID, drop it.  Does not
     // attempt to reschedule.
     pub(crate) fn deref_node(&mut self, node_id: NodeId) {
@@ -836,8 +736,13 @@ impl TenantState {
             shard_number: self.tenant_shard_id.shard_number.0 as i32,
             shard_count: self.tenant_shard_id.shard_count.literal() as i32,
             shard_stripe_size: self.shard.stripe_size.0 as i32,
-            generation: self.generation.map(|g| g.into().unwrap_or(0) as i32),
-            generation_pageserver: self.intent.get_attached().map(|n| n.0 as i64),
+            generation: self.generation.into().unwrap_or(0) as i32,
+            generation_pageserver: self
+                .intent
+                .get_attached()
+                .map(|n| n.0 as i64)
+                .unwrap_or(i64::MAX),
+
             placement_policy: serde_json::to_string(&self.policy).unwrap(),
             config: serde_json::to_string(&self.config).unwrap(),
             splitting: SplitState::default(),
@@ -847,10 +752,7 @@ impl TenantState {
 
 #[cfg(test)]
 pub(crate) mod tests {
-    use pageserver_api::{
-        controller_api::NodeAvailability,
-        shard::{ShardCount, ShardNumber},
-    };
+    use pageserver_api::shard::{ShardCount, ShardNumber};
     use utils::id::TenantId;
 
     use crate::scheduler::test_utils::make_test_nodes;
@@ -903,16 +805,11 @@ pub(crate) mod tests {
         assert_ne!(attached_node_id, secondary_node_id);
 
         // Notifying the attached node is offline should demote it to a secondary
-        let changed = tenant_state.intent.demote_attached(attached_node_id);
+        let changed = tenant_state.intent.notify_offline(attached_node_id);
         assert!(changed);
-        assert!(tenant_state.intent.attached.is_none());
-        assert_eq!(tenant_state.intent.secondary.len(), 2);
 
         // Update the scheduler state to indicate the node is offline
-        nodes
-            .get_mut(&attached_node_id)
-            .unwrap()
-            .set_availability(NodeAvailability::Offline);
+        nodes.get_mut(&attached_node_id).unwrap().availability = NodeAvailability::Offline;
         scheduler.node_upsert(nodes.get(&attached_node_id).unwrap());
 
         // Scheduling the node should promote the still-available secondary node to attached
@@ -931,54 +828,4 @@ pub(crate) mod tests {
 
         Ok(())
     }
-
-    #[test]
-    fn intent_from_observed() -> anyhow::Result<()> {
-        let nodes = make_test_nodes(3);
-        let mut scheduler = Scheduler::new(nodes.values());
-
-        let mut tenant_state = make_test_tenant_shard(PlacementPolicy::Double(1));
-
-        tenant_state.observed.locations.insert(
-            NodeId(3),
-            ObservedStateLocation {
-                conf: Some(LocationConfig {
-                    mode: LocationConfigMode::AttachedMulti,
-                    generation: Some(2),
-                    secondary_conf: None,
-                    shard_number: tenant_state.shard.number.0,
-                    shard_count: tenant_state.shard.count.literal(),
-                    shard_stripe_size: tenant_state.shard.stripe_size.0,
-                    tenant_conf: TenantConfig::default(),
-                }),
-            },
-        );
-
-        tenant_state.observed.locations.insert(
-            NodeId(2),
-            ObservedStateLocation {
-                conf: Some(LocationConfig {
-                    mode: LocationConfigMode::AttachedStale,
-                    generation: Some(1),
-                    secondary_conf: None,
-                    shard_number: tenant_state.shard.number.0,
-                    shard_count: tenant_state.shard.count.literal(),
-                    shard_stripe_size: tenant_state.shard.stripe_size.0,
-                    tenant_conf: TenantConfig::default(),
-                }),
-            },
-        );
-
-        tenant_state.intent_from_observed(&mut scheduler);
-
-        // The highest generationed attached location gets used as attached
-        assert_eq!(tenant_state.intent.attached, Some(NodeId(3)));
-        // Other locations get used as secondary
-        assert_eq!(tenant_state.intent.secondary, vec![NodeId(2)]);
-
-        scheduler.consistency_check(nodes.values(), [&tenant_state].into_iter())?;
-
-        tenant_state.intent.clear(&mut scheduler);
-        Ok(())
-    }
 }

control_plane/src/attachment_service.rs

@@ -34,7 +34,7 @@ pub struct AttachmentService {
     client: reqwest::Client,
 }
 
-const COMMAND: &str = "storage_controller";
+const COMMAND: &str = "attachment_service";
 
 const ATTACHMENT_SERVICE_POSTGRES_VERSION: u32 = 16;
 
@@ -200,7 +200,7 @@ impl AttachmentService {
                 "localhost",
                 "-p",
                 &format!("{}", self.postgres_port),
-                DB_NAME,
+                &DB_NAME,
             ])
             .output()
             .await

control_plane/src/bin/neon_local.rs

@@ -15,7 +15,7 @@ use control_plane::pageserver::{PageServerNode, PAGESERVER_REMOTE_STORAGE_DIR};
 use control_plane::safekeeper::SafekeeperNode;
 use control_plane::{broker, local_env};
 use pageserver_api::controller_api::{
-    NodeAvailability, NodeConfigureRequest, NodeSchedulingPolicy, PlacementPolicy,
+    NodeAvailability, NodeConfigureRequest, NodeSchedulingPolicy,
 };
 use pageserver_api::models::{
     ShardParameters, TenantCreateRequest, TimelineCreateRequest, TimelineInfo,
@@ -435,11 +435,6 @@ async fn handle_tenant(
             let shard_stripe_size: Option<u32> =
                 create_match.get_one::<u32>("shard-stripe-size").cloned();
 
-            let placement_policy = match create_match.get_one::<String>("placement-policy") {
-                Some(s) if !s.is_empty() => serde_json::from_str::<PlacementPolicy>(s)?,
-                _ => PlacementPolicy::Single,
-            };
-
             let tenant_conf = PageServerNode::parse_config(tenant_conf)?;
 
             // If tenant ID was not specified, generate one
@@ -461,7 +456,6 @@ async fn handle_tenant(
                             .map(ShardStripeSize)
                             .unwrap_or(ShardParameters::DEFAULT_STRIPE_SIZE),
                     },
-                    placement_policy: Some(placement_policy),
                     config: tenant_conf,
                 })
                 .await?;
@@ -1030,7 +1024,7 @@ async fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Re
                         })
                         .collect::<Vec<_>>()
                 };
-            endpoint.reconfigure(pageservers, None).await?;
+            endpoint.reconfigure(pageservers).await?;
         }
         "stop" => {
             let endpoint_id = sub_args
@@ -1568,7 +1562,6 @@ fn cli() -> Command {
                     .help("Use this tenant in future CLI commands where tenant_id is needed, but not specified"))
                 .arg(Arg::new("shard-count").value_parser(value_parser!(u8)).long("shard-count").action(ArgAction::Set).help("Number of shards in the new tenant (default 1)"))
                 .arg(Arg::new("shard-stripe-size").value_parser(value_parser!(u32)).long("shard-stripe-size").action(ArgAction::Set).help("Sharding stripe size in pages"))
-                .arg(Arg::new("placement-policy").value_parser(value_parser!(String)).long("placement-policy").action(ArgAction::Set).help("Placement policy shards in this tenant"))
                 )
             .subcommand(Command::new("set-default").arg(tenant_id_arg.clone().required(true))
                 .about("Set a particular tenant as default in future CLI commands where tenant_id is needed, but not specified"))

control_plane/src/endpoint.rs

@@ -52,7 +52,6 @@ use compute_api::spec::RemoteExtSpec;
 use compute_api::spec::Role;
 use nix::sys::signal::kill;
 use nix::sys::signal::Signal;
-use pageserver_api::shard::ShardStripeSize;
 use serde::{Deserialize, Serialize};
 use url::Host;
 use utils::id::{NodeId, TenantId, TimelineId};
@@ -606,7 +605,7 @@ impl Endpoint {
         let conn_str = self.connstr("cloud_admin", "postgres");
         println!("Starting postgres node at '{}'", conn_str);
         if create_test_user {
-            let conn_str = self.connstr("test", "neondb");
+            let conn_str = self.connstr("user", "neondb");
             println!("Also at '{}'", conn_str);
         }
         let mut cmd = Command::new(self.env.neon_distrib_dir.join("compute_ctl"));
@@ -656,7 +655,7 @@ impl Endpoint {
         // Wait for it to start
         let mut attempt = 0;
         const ATTEMPT_INTERVAL: Duration = Duration::from_millis(100);
-        const MAX_ATTEMPTS: u32 = 10 * 90; // Wait up to 1.5 min
+        const MAX_ATTEMPTS: u32 = 10 * 30; // Wait up to 30 s
         loop {
             attempt += 1;
             match self.get_status().await {
@@ -736,11 +735,7 @@ impl Endpoint {
         }
     }
 
-    pub async fn reconfigure(
-        &self,
-        mut pageservers: Vec<(Host, u16)>,
-        stripe_size: Option<ShardStripeSize>,
-    ) -> Result<()> {
+    pub async fn reconfigure(&self, mut pageservers: Vec<(Host, u16)>) -> Result<()> {
         let mut spec: ComputeSpec = {
             let spec_path = self.endpoint_path().join("spec.json");
             let file = std::fs::File::open(spec_path)?;
@@ -770,9 +765,6 @@ impl Endpoint {
         let pageserver_connstr = Self::build_pageserver_connstr(&pageservers);
         assert!(!pageserver_connstr.is_empty());
         spec.pageserver_connstring = Some(pageserver_connstr);
-        if stripe_size.is_some() {
-            spec.shard_stripe_size = stripe_size.map(|s| s.0 as usize);
-        }
 
         let client = reqwest::Client::new();
         let response = client

control_plane/src/local_env.rs

@@ -232,7 +232,7 @@ impl LocalEnv {
         // run from the same location as neon_local.  This means that for compatibility
         // tests that run old pageserver/safekeeper, they still run latest attachment service.
         let neon_local_bin_dir = env::current_exe().unwrap().parent().unwrap().to_owned();
-        neon_local_bin_dir.join("storage_controller")
+        neon_local_bin_dir.join("attachment_service")
     }
 
     pub fn safekeeper_bin(&self) -> PathBuf {

control_plane/src/pageserver.rs

@@ -429,8 +429,6 @@ impl PageServerNode {
             generation,
             config,
             shard_parameters: ShardParameters::default(),
-            // Placement policy is not meaningful for creations not done via storage controller
-            placement_policy: None,
         };
         if !settings.is_empty() {
             bail!("Unrecognized tenant settings: {settings:?}")
@@ -539,11 +537,10 @@ impl PageServerNode {
         tenant_shard_id: TenantShardId,
         config: LocationConfig,
         flush_ms: Option<Duration>,
-        lazy: bool,
     ) -> anyhow::Result<()> {
         Ok(self
             .http_client
-            .location_config(tenant_shard_id, config, flush_ms, lazy)
+            .location_config(tenant_shard_id, config, flush_ms)
             .await?)
     }
 
@@ -608,7 +605,7 @@ impl PageServerNode {
                 eprintln!("connection error: {}", e);
             }
         });
-        let client = std::pin::pin!(client);
+        tokio::pin!(client);
 
         // Init base reader
         let (start_lsn, base_tarfile_path) = base;

docs/rfcs/002-storage.md

@@ -1,4 +1,4 @@
-# Neon storage node — alternative
+# Zenith storage node — alternative
 
 ## **Design considerations**
 

docs/rfcs/003-laptop-cli.md

@@ -1,6 +1,6 @@
 # Command line interface (end-user)
 
-Neon CLI as it is described here mostly resides on the same conceptual level as pg_ctl/initdb/pg_recvxlog/etc and replaces some of them in an opinionated way. I would also suggest bundling our patched postgres inside neon distribution at least at the start.
+Zenith CLI as it is described here mostly resides on the same conceptual level as pg_ctl/initdb/pg_recvxlog/etc and replaces some of them in an opinionated way. I would also suggest bundling our patched postgres inside zenith distribution at least at the start.
 
 This proposal is focused on managing local installations. For cluster operations, different tooling would be needed. The point of integration between the two is storage URL: no matter how complex cluster setup is it may provide an endpoint where the user may push snapshots.
 
@@ -8,40 +8,40 @@ The most important concept here is a snapshot, which can be created/pushed/pulle
 
 # Possible usage scenarios
 
-## Install neon, run a postgres
+## Install zenith, run a postgres
 
 ```
-> brew install pg-neon 
-> neon pg create # creates pgdata with default pattern pgdata$i
-> neon pg list
+> brew install pg-zenith 
+> zenith pg create # creates pgdata with default pattern pgdata$i
+> zenith pg list
 ID            PGDATA        USED    STORAGE            ENDPOINT
-primary1      pgdata1       0G      neon-local       localhost:5432
+primary1      pgdata1       0G      zenith-local       localhost:5432
 ```
 
-## Import standalone postgres to neon
+## Import standalone postgres to zenith
 
 ```
-> neon snapshot import --from=basebackup://replication@localhost:5432/ oldpg
+> zenith snapshot import --from=basebackup://replication@localhost:5432/ oldpg
 [====================------------] 60% | 20MB/s
-> neon snapshot list
+> zenith snapshot list
 ID          SIZE        PARENT
 oldpg       5G          -
 
-> neon pg create --snapshot oldpg
+> zenith pg create --snapshot oldpg
 Started postgres on localhost:5432
 
-> neon pg list
+> zenith pg list
 ID            PGDATA        USED    STORAGE            ENDPOINT
-primary1      pgdata1       5G      neon-local       localhost:5432
+primary1      pgdata1       5G      zenith-local       localhost:5432
 
-> neon snapshot destroy oldpg
+> zenith snapshot destroy oldpg
 Ok
 ```
 
 Also, we may start snapshot import implicitly by looking at snapshot schema
 
 ```
-> neon pg create --snapshot basebackup://replication@localhost:5432/
+> zenith pg create --snapshot basebackup://replication@localhost:5432/
 Downloading snapshot... Done.
 Started postgres on localhost:5432
 Destroying snapshot... Done.
@@ -52,39 +52,39 @@ Destroying snapshot... Done.
 Since we may export the whole snapshot as one big file (tar of basebackup, maybe with some manifest) it may be shared over conventional means: http, ssh, [git+lfs](https://docs.github.com/en/github/managing-large-files/about-git-large-file-storage).
 
 ```
-> neon pg create --snapshot http://learn-postgres.com/movies_db.neon movies
+> zenith pg create --snapshot http://learn-postgres.com/movies_db.zenith movies
 ```
 
 ## Create snapshot and push it to the cloud
 
 ```
-> neon snapshot create pgdata1@snap1
-> neon snapshot push --to ssh://stas@neon.tech pgdata1@snap1
+> zenith snapshot create pgdata1@snap1
+> zenith snapshot push --to ssh://stas@zenith.tech pgdata1@snap1
 ```
 
 ## Rollback database to the snapshot
 
-One way to rollback the database is just to init a new database from the snapshot and destroy the old one. But creating a new database from a snapshot would require a copy of that snapshot which is time consuming operation. Another option that would be cool to support is the ability to create the copy-on-write database from the snapshot without copying data, and store updated pages in a separate location, however that way would have performance implications. So to properly rollback the database to the older state we have `neon pg checkout`.
+One way to rollback the database is just to init a new database from the snapshot and destroy the old one. But creating a new database from a snapshot would require a copy of that snapshot which is time consuming operation. Another option that would be cool to support is the ability to create the copy-on-write database from the snapshot without copying data, and store updated pages in a separate location, however that way would have performance implications. So to properly rollback the database to the older state we have `zenith pg checkout`.
 
 ```
-> neon pg list
+> zenith pg list
 ID            PGDATA        USED    STORAGE            ENDPOINT
-primary1      pgdata1       5G      neon-local       localhost:5432
+primary1      pgdata1       5G      zenith-local       localhost:5432
 
-> neon snapshot create pgdata1@snap1
+> zenith snapshot create pgdata1@snap1
 
-> neon snapshot list
+> zenith snapshot list
 ID                    SIZE        PARENT
 oldpg                 5G          -
 pgdata1@snap1         6G          -
 pgdata1@CURRENT       6G          -
 
-> neon pg checkout pgdata1@snap1
+> zenith pg checkout pgdata1@snap1
 Stopping postgres on pgdata1.
 Rolling back pgdata1@CURRENT to pgdata1@snap1.
 Starting postgres on pgdata1.
 
-> neon snapshot list
+> zenith snapshot list
 ID                    SIZE        PARENT
 oldpg                 5G          -
 pgdata1@snap1         6G          -
@@ -99,7 +99,7 @@ Some notes: pgdata1@CURRENT -- implicit snapshot representing the current state
 PITR area acts like a continuous snapshot where you can reset the database to any point in time within this area (by area I mean some TTL period or some size limit, both possibly infinite).
 
 ```
-> neon pitr create --storage s3tank --ttl 30d --name pitr_last_month
+> zenith pitr create --storage s3tank --ttl 30d --name pitr_last_month
 ```
 
 Resetting the database to some state in past would require creating a snapshot on some lsn / time in this pirt area.
@@ -108,29 +108,29 @@ Resetting the database to some state in past would require creating a snapshot o
 
 ## storage
 
-Storage is either neon pagestore or s3. Users may create a database in a pagestore and create/move *snapshots* and *pitr regions* in both pagestore and s3. Storage is a concept similar to `git remote`. After installation, I imagine one local storage is available by default.
+Storage is either zenith pagestore or s3. Users may create a database in a pagestore and create/move *snapshots* and *pitr regions* in both pagestore and s3. Storage is a concept similar to `git remote`. After installation, I imagine one local storage is available by default.
 
-**neon storage attach** -t [native|s3] -c key=value -n name
+**zenith storage attach** -t [native|s3] -c key=value -n name
 
-Attaches/initializes storage. For --type=s3, user credentials and path should be provided. For --type=native we may support --path=/local/path and --url=neon.tech/stas/mystore. Other possible term for native is 'zstore'.
+Attaches/initializes storage. For --type=s3, user credentials and path should be provided. For --type=native we may support --path=/local/path and --url=zenith.tech/stas/mystore. Other possible term for native is 'zstore'.
 
 
-**neon storage list**
+**zenith storage list**
 
 Show currently attached storages. For example:
 
 ```
-> neon storage list
+> zenith storage list
 NAME            USED    TYPE                OPTIONS          PATH
-local           5.1G    neon-local                         /opt/neon/store/local
-local.compr     20.4G   neon-local        compression=on    /opt/neon/store/local.compr
-zcloud          60G     neon-remote                        neon.tech/stas/mystore
+local           5.1G    zenith-local                         /opt/zenith/store/local
+local.compr     20.4G   zenith-local        compression=on    /opt/zenith/store/local.compr
+zcloud          60G     zenith-remote                        zenith.tech/stas/mystore
 s3tank          80G     S3
 ```
 
-**neon storage detach**
+**zenith storage detach**
 
-**neon storage show**
+**zenith storage show**
 
 
 
@@ -140,29 +140,29 @@ Manages postgres data directories and can start postgres instances with proper c
 
 Pg is a term for a single postgres running on some data. I'm trying to avoid separation of datadir management and postgres instance management -- both that concepts bundled here together.
 
-**neon pg create** [--no-start --snapshot --cow] -s storage-name -n pgdata
+**zenith pg create** [--no-start --snapshot --cow] -s storage-name -n pgdata
 
 Creates (initializes) new data directory in given storage and starts postgres. I imagine that storage for this operation may be only local and data movement to remote location happens through snapshots/pitr.
 
 --no-start: just init datadir without creating 
 
---snapshot snap: init from the snapshot. Snap is a name or URL (neon.tech/stas/mystore/snap1)
+--snapshot snap: init from the snapshot. Snap is a name or URL (zenith.tech/stas/mystore/snap1)
 
 --cow: initialize Copy-on-Write data directory on top of some snapshot (makes sense if it is a snapshot of currently running a database)
 
-**neon pg destroy**
+**zenith pg destroy**
 
-**neon pg start** [--replica] pgdata
+**zenith pg start** [--replica] pgdata
 
 Start postgres with proper extensions preloaded/installed.
 
-**neon pg checkout**
+**zenith pg checkout**
 
 Rollback data directory to some previous snapshot. 
 
-**neon pg stop** pg_id
+**zenith pg stop** pg_id
 
-**neon pg list**
+**zenith pg list**
 
 ```
 ROLE                 PGDATA        USED    STORAGE            ENDPOINT
@@ -173,7 +173,7 @@ primary              my_pg2        3.2G    local.compr        localhost:5435
 -                    my_pg3        9.2G    local.compr        -
 ```
 
-**neon pg show**
+**zenith pg show**
 
 ```
 my_pg:
@@ -194,7 +194,7 @@ my_pg:
 
 ```
 
-**neon pg start-rest/graphql** pgdata
+**zenith pg start-rest/graphql** pgdata
 
 Starts REST/GraphQL proxy on top of postgres master. Not sure we should do that, just an idea.
 
@@ -203,35 +203,35 @@ Starts REST/GraphQL proxy on top of postgres master. Not sure we should do that,
 
 Snapshot creation is cheap -- no actual data is copied, we just start retaining old pages. Snapshot size means the amount of retained data, not all data. Snapshot name looks like pgdata_name@tag_name. tag_name is set by the user during snapshot creation. There are some reserved tag names: CURRENT represents the current state of the data directory; HEAD{i} represents the data directory state that resided in the database before i-th checkout.
 
-**neon snapshot create** pgdata_name@snap_name
+**zenith snapshot create** pgdata_name@snap_name
 
 Creates a new snapshot in the same storage where pgdata_name exists.
 
-**neon snapshot push** --to url pgdata_name@snap_name
+**zenith snapshot push** --to url pgdata_name@snap_name
 
-Produces binary stream of a given snapshot. Under the hood starts temp read-only postgres over this snapshot and sends basebackup stream. Receiving side should start `neon snapshot recv` before push happens. If url has some special schema like neon:// receiving side may require auth start `neon snapshot recv` on the go.
+Produces binary stream of a given snapshot. Under the hood starts temp read-only postgres over this snapshot and sends basebackup stream. Receiving side should start `zenith snapshot recv` before push happens. If url has some special schema like zenith:// receiving side may require auth start `zenith snapshot recv` on the go.
 
-**neon snapshot recv**
+**zenith snapshot recv**
 
 Starts a port listening for a basebackup stream, prints connection info to stdout (so that user may use that in push command), and expects data on that socket.
 
-**neon snapshot pull** --from url or path
+**zenith snapshot pull** --from url or path
 
-Connects to a remote neon/s3/file and pulls snapshot. The remote site should be neon service or files in our format.
+Connects to a remote zenith/s3/file and pulls snapshot. The remote site should be zenith service or files in our format.
 
-**neon snapshot import** --from basebackup://<...>  or path
+**zenith snapshot import** --from basebackup://<...>  or path
 
 Creates a new snapshot out of running postgres via basebackup protocol or basebackup files.
 
-**neon snapshot export**
+**zenith snapshot export**
 
-Starts read-only postgres over this snapshot and exports data in some format (pg_dump, or COPY TO on some/all tables). One of the options may be neon own format which is handy for us (but I think just tar of basebackup would be okay).
+Starts read-only postgres over this snapshot and exports data in some format (pg_dump, or COPY TO on some/all tables). One of the options may be zenith own format which is handy for us (but I think just tar of basebackup would be okay).
 
-**neon snapshot diff** snap1 snap2
+**zenith snapshot diff** snap1 snap2
 
 Shows size of data changed between two snapshots. We also may provide options to diff schema/data in tables. To do that start temp read-only postgreses.
 
-**neon snapshot destroy**
+**zenith snapshot destroy**
 
 ## pitr
 
@@ -239,7 +239,7 @@ Pitr represents wal stream and ttl policy for that stream
 
 XXX: any suggestions on a better name?
 
-**neon pitr create** name
+**zenith pitr create** name
 
 --ttl = inf | period
 
@@ -247,21 +247,21 @@ XXX: any suggestions on a better name?
 
 --storage = storage_name
 
-**neon pitr extract-snapshot** pitr_name --lsn xxx
+**zenith pitr extract-snapshot** pitr_name --lsn xxx
 
 Creates a snapshot out of some lsn in PITR area. The obtained snapshot may be managed with snapshot routines (move/send/export)
 
-**neon pitr gc** pitr_name
+**zenith pitr gc** pitr_name
 
 Force garbage collection on some PITR area.
 
-**neon pitr list**
+**zenith pitr list**
 
-**neon pitr destroy**
+**zenith pitr destroy**
 
 
 ## console
 
-**neon console**
+**zenith console**
 
 Opens browser targeted at web console with the more or less same functionality as described here.

docs/rfcs/004-durability.md

@@ -6,7 +6,7 @@ When do we consider the WAL record as durable, so that we can
 acknowledge the commit to the client and be reasonably certain that we
 will not lose the transaction?
 
-Neon uses a group of WAL safekeeper nodes to hold the generated WAL.
+Zenith uses a group of WAL safekeeper nodes to hold the generated WAL.
 A WAL record is considered durable, when it has been written to a
 majority of WAL safekeeper nodes. In this document, I use 5
 safekeepers, because I have five fingers. A WAL record is durable,

docs/rfcs/005-zenith_local.md

@@ -1,23 +1,23 @@
-# Neon local
+# Zenith local
 
-Here I list some objectives to keep in mind when discussing neon-local design and a proposal that brings all components together.  Your comments on both parts are very welcome.
+Here I list some objectives to keep in mind when discussing zenith-local design and a proposal that brings all components together.  Your comments on both parts are very welcome.
 
 #### Why do we need it?
 - For distribution - this easy to use binary will help us to build adoption among developers.
 - For internal use - to test all components together.
 
-In my understanding, we consider it to be just a mock-up version of neon-cloud.
+In my understanding, we consider it to be just a mock-up version of zenith-cloud.
 > Question: How much should we care about durability and security issues for a local setup?
 
 
 #### Why is it better than a simple local postgres?
 
-- Easy one-line setup. As simple as `cargo install neon && neon start`
+- Easy one-line setup. As simple as `cargo install zenith && zenith start`
 
 - Quick and cheap creation of compute nodes over the same storage.
 > Question: How can we describe a use-case for this feature?
 
-- Neon-local can work with S3 directly. 
+- Zenith-local can work with S3 directly. 
 
 - Push and pull images (snapshots) to remote S3 to exchange data with other users.
 
@@ -31,50 +31,50 @@ Ideally, just one binary that incorporates all elements we need.
 
 #### Components:
 
-- **neon-CLI** - interface for end-users.  Turns commands to REST requests and handles responses to show them in a user-friendly way.  
-CLI proposal is here https://github.com/neondatabase/rfcs/blob/003-laptop-cli.md/003-laptop-cli.md
-WIP code is here: https://github.com/neondatabase/postgres/tree/main/pageserver/src/bin/cli
+- **zenith-CLI** - interface for end-users.  Turns commands to REST requests and handles responses to show them in a user-friendly way.  
+CLI proposal is here https://github.com/libzenith/rfcs/blob/003-laptop-cli.md/003-laptop-cli.md
+WIP code is here: https://github.com/libzenith/postgres/tree/main/pageserver/src/bin/cli
 
-- **neon-console** - WEB UI with same functionality as CLI.
+- **zenith-console** - WEB UI with same functionality as CLI.
 >Note: not for the first release.
 
-- **neon-local** - entrypoint. Service that starts all other components and handles REST API requests. See REST API proposal below.
-    > Idea: spawn all other components as child processes, so that we could shutdown everything by stopping neon-local.
+- **zenith-local** - entrypoint. Service that starts all other components and handles REST API requests. See REST API proposal below.
+    > Idea: spawn all other components as child processes, so that we could shutdown everything by stopping zenith-local.
 
-- **neon-pageserver** - consists of a storage and WAL-replaying service (modified PG in current implementation).
+- **zenith-pageserver** - consists of a storage and WAL-replaying service (modified PG in current implementation).
 > Question: Probably, for local setup we should be able to bypass page-storage and interact directly with S3 to avoid double caching in shared buffers and page-server?
 
-WIP code is here: https://github.com/neondatabase/postgres/tree/main/pageserver/src
+WIP code is here: https://github.com/libzenith/postgres/tree/main/pageserver/src
 
-- **neon-S3** - stores base images of the database and WAL in S3 object storage. Import and export images from/to neon.
+- **zenith-S3** - stores base images of the database and WAL in S3 object storage. Import and export images from/to zenith.
 > Question: How should it operate in a local setup? Will we manage it ourselves or ask user to provide credentials for existing S3 object storage (i.e. minio)?
 > Question: Do we use it together with local page store or they are interchangeable?
 
 WIP code is ???
 
-- **neon-safekeeper** - receives WAL from postgres, stores it durably, answers to Postgres that "sync" is succeed.
+- **zenith-safekeeper** - receives WAL from postgres, stores it durably, answers to Postgres that "sync" is succeed.
 > Question: How should it operate in a local setup? In my understanding it should push WAL directly to S3 (if we use it) or store all data locally (if we use local page storage). The latter option seems meaningless (extra overhead and no gain), but it is still good to test the system.
 
-WIP code is here: https://github.com/neondatabase/postgres/tree/main/src/bin/safekeeper
+WIP code is here: https://github.com/libzenith/postgres/tree/main/src/bin/safekeeper
 
-- **neon-computenode** - bottomless PostgreSQL, ideally upstream, but for a start - our modified version. User can quickly create and destroy them and work with it as a regular postgres database.
+- **zenith-computenode** - bottomless PostgreSQL, ideally upstream, but for a start - our modified version. User can quickly create and destroy them and work with it as a regular postgres database.
  
- WIP code is in main branch and here: https://github.com/neondatabase/postgres/commits/compute_node
+ WIP code is in main branch and here: https://github.com/libzenith/postgres/commits/compute_node
 
 #### REST API:
 
 Service endpoint: `http://localhost:3000`
 
 Resources:
-- /storages - Where data lives: neon-pageserver or neon-s3
-- /pgs - Postgres - neon-computenode
+- /storages - Where data lives: zenith-pageserver or zenith-s3
+- /pgs - Postgres - zenith-computenode
 - /snapshots - snapshots **TODO**
 
->Question: Do we want to extend this API to manage neon components? I.e. start page-server, manage safekeepers and so on? Or they will be hardcoded to just start once and for all?
+>Question: Do we want to extend this API to manage zenith components? I.e. start page-server, manage safekeepers and so on? Or they will be hardcoded to just start once and for all?
 
 Methods and their mapping to CLI:
 
-- /storages - neon-pageserver or neon-s3
+- /storages - zenith-pageserver or zenith-s3
 
 CLI  | REST API
 ------------- | -------------
@@ -84,7 +84,7 @@ storage list | GET /storages
 storage show -n name | GET /storages/:storage_name 
 
 
-- /pgs - neon-computenode
+- /pgs - zenith-computenode
 
 CLI  | REST API
 ------------- | -------------

docs/rfcs/006-laptop-cli-v2-CLI.md

@@ -1,45 +1,45 @@
-Neon CLI allows you to operate database clusters (catalog clusters) and their commit history locally and in the cloud. Since ANSI calls them catalog clusters and cluster is a loaded term in the modern infrastructure we will call it "catalog".
+Zenith CLI allows you to operate database clusters (catalog clusters) and their commit history locally and in the cloud. Since ANSI calls them catalog clusters and cluster is a loaded term in the modern infrastructure we will call it "catalog".
 
 # CLI v2 (after chatting with Carl)
 
-Neon introduces the notion of a repository.
+Zenith introduces the notion of a repository.
 
 ```bash
-neon init
-neon clone neon://neon.tech/piedpiper/northwind -- clones a repo to the northwind directory
+zenith init
+zenith clone zenith://zenith.tech/piedpiper/northwind -- clones a repo to the northwind directory
 ```
 
 Once you have a cluster catalog you can explore it
 
 ```bash
-neon log -- returns a list of commits
-neon status -- returns if there are changes in the catalog that can be committed
-neon commit -- commits the changes and generates a new commit hash
-neon branch experimental <hash> -- creates a branch called testdb based on a given commit hash
+zenith log -- returns a list of commits
+zenith status -- returns if there are changes in the catalog that can be committed
+zenith commit -- commits the changes and generates a new commit hash
+zenith branch experimental <hash> -- creates a branch called testdb based on a given commit hash
 ```
 
 To make changes in the catalog you need to run compute nodes
 
 ```bash
 -- here is how you a compute node
-neon start /home/pipedpiper/northwind:main -- starts a compute instance
-neon start neon://neon.tech/northwind:main -- starts a compute instance in the cloud
+zenith start /home/pipedpiper/northwind:main -- starts a compute instance
+zenith start zenith://zenith.tech/northwind:main -- starts a compute instance in the cloud
 -- you can start a compute node against any hash or branch
-neon start /home/pipedpiper/northwind:experimental --port 8008 -- start another compute instance (on different port)
+zenith start /home/pipedpiper/northwind:experimental --port 8008 -- start another compute instance (on different port)
 -- you can start a compute node against any hash or branch
-neon start /home/pipedpiper/northwind:<hash> --port 8009 -- start another compute instance (on different port)
+zenith start /home/pipedpiper/northwind:<hash> --port 8009 -- start another compute instance (on different port)
 
 -- After running some DML you can run 
--- neon status and see how there are two WAL streams one on top of 
+-- zenith status and see how there are two WAL streams one on top of 
 -- the main branch
-neon status 
+zenith status 
 -- and another on top of the experimental branch
-neon status -b experimental
+zenith status -b experimental
 
 -- you can commit each branch separately
-neon commit main
+zenith commit main
 -- or
-neon commit -c /home/pipedpiper/northwind:experimental
+zenith commit -c /home/pipedpiper/northwind:experimental
 ```
 
 Starting compute instances against cloud environments
@@ -47,18 +47,18 @@ Starting compute instances against cloud environments
 ```bash
 -- you can start a compute instance against the cloud environment
 -- in this case all of the changes will be streamed into the cloud
-neon start https://neon:tecj/pipedpiper/northwind:main
-neon start https://neon:tecj/pipedpiper/northwind:main
-neon status -c https://neon:tecj/pipedpiper/northwind:main
-neon commit -c https://neon:tecj/pipedpiper/northwind:main
-neon branch -c https://neon:tecj/pipedpiper/northwind:<hash> experimental
+zenith start https://zenith:tech/pipedpiper/northwind:main
+zenith start https://zenith:tech/pipedpiper/northwind:main
+zenith status -c https://zenith:tech/pipedpiper/northwind:main
+zenith commit -c https://zenith:tech/pipedpiper/northwind:main
+zenith branch -c https://zenith:tech/pipedpiper/northwind:<hash> experimental
 ```
 
 Pushing data into the cloud
 
 ```bash
 -- pull all the commits from the cloud
-neon pull
+zenith pull
 -- push all the commits to the cloud
-neon push
+zenith push
 ```

docs/rfcs/006-laptop-cli-v2-repository-structure.md

@@ -1,14 +1,14 @@
 # Repository format
 
-A Neon repository is similar to a traditional PostgreSQL backup
+A Zenith repository is similar to a traditional PostgreSQL backup
 archive, like a WAL-G bucket or pgbarman backup catalogue. It holds
 multiple versions of a PostgreSQL database cluster.
 
-The distinguishing feature is that you can launch a Neon Postgres
+The distinguishing feature is that you can launch a Zenith Postgres
 server directly against a branch in the repository, without having to
-"restore" it first. Also, Neon manages the storage automatically,
+"restore" it first. Also, Zenith manages the storage automatically,
 there is no separation between full and incremental backups nor WAL
-archive. Neon relies heavily on the WAL, and uses concepts similar
+archive. Zenith relies heavily on the WAL, and uses concepts similar
 to incremental backups and WAL archiving internally, but it is hidden
 from the user.
 
@@ -19,15 +19,15 @@ efficient. Just something to get us started.
 
 The repository directory looks like this:
 
-    .neon/timelines/4543be3daeab2ed4e58a285cbb8dd1fce6970f8c/wal/
-    .neon/timelines/4543be3daeab2ed4e58a285cbb8dd1fce6970f8c/snapshots/<lsn>/
-    .neon/timelines/4543be3daeab2ed4e58a285cbb8dd1fce6970f8c/history
+    .zenith/timelines/4543be3daeab2ed4e58a285cbb8dd1fce6970f8c/wal/
+    .zenith/timelines/4543be3daeab2ed4e58a285cbb8dd1fce6970f8c/snapshots/<lsn>/
+    .zenith/timelines/4543be3daeab2ed4e58a285cbb8dd1fce6970f8c/history
     
-    .neon/refs/branches/mybranch
-    .neon/refs/tags/foo
-    .neon/refs/tags/bar
+    .zenith/refs/branches/mybranch
+    .zenith/refs/tags/foo
+    .zenith/refs/tags/bar
     
-    .neon/datadirs/<timeline uuid>
+    .zenith/datadirs/<timeline uuid>
 
 ### Timelines
 
@@ -39,7 +39,7 @@ All WAL is generated on a timeline. You can launch a read-only node
 against a tag or arbitrary LSN on a timeline, but in order to write,
 you need to create a timeline.
 
-Each timeline is stored in a directory under .neon/timelines. It
+Each timeline is stored in a directory under .zenith/timelines. It
 consists of a WAL archive, containing all the WAL in the standard
 PostgreSQL format, under the wal/ subdirectory.
 
@@ -66,18 +66,18 @@ contains the UUID of the timeline (and LSN, for tags).
 
 ### Datadirs
 
-.neon/datadirs contains PostgreSQL data directories. You can launch
+.zenith/datadirs contains PostgreSQL data directories. You can launch
 a Postgres instance on one of them with:
 
 ```
-  postgres -D .neon/datadirs/4543be3daeab2ed4e58a285cbb8dd1fce6970f8c
+  postgres -D .zenith/datadirs/4543be3daeab2ed4e58a285cbb8dd1fce6970f8c
 ```
 
 All the actual data is kept in the timeline directories, under
-.neon/timelines. The data directories are only needed for active
+.zenith/timelines. The data directories are only needed for active
 PostgreQSL instances. After an instance is stopped, the data directory
-can be safely removed. "neon start" will recreate it quickly from
-the data in .neon/timelines, if it's missing.
+can be safely removed. "zenith start" will recreate it quickly from
+the data in .zenith/timelines, if it's missing.
 
 ## Version 2
 
@@ -103,14 +103,14 @@ more advanced. The exact format is TODO. But it should support:
 
 ### Garbage collection
 
-When you run "neon gc", old timelines that are no longer needed are
+When you run "zenith gc", old timelines that are no longer needed are
 removed. That involves collecting the list of "unreachable" objects,
 starting from the named branches and tags.
 
 Also, if enough WAL has been generated on a timeline since last
 snapshot, a new snapshot or delta is created.
 
-### neon push/pull
+### zenith push/pull
 
 Compare the tags and branches on both servers, and copy missing ones.
 For each branch, compare the timeline it points to in both servers. If
@@ -123,7 +123,7 @@ every time you start up an instance? Then you would detect that the
 timelines have diverged. That would match with the "epoch" concept
 that we have in the WAL safekeeper
 
-### neon checkout/commit
+### zenith checkout/commit
 
 In this format, there is no concept of a "working tree", and hence no
 concept of checking out or committing. All modifications are done on
@@ -134,7 +134,7 @@ You can easily fork off a temporary timeline to emulate a "working tree".
 You can later remove it and have it garbage collected, or to "commit",
 re-point the branch to the new timeline.
 
-If we want to have a worktree and "neon checkout/commit" concept, we can
+If we want to have a worktree and "zenith checkout/commit" concept, we can
 emulate that with a temporary timeline. Create the temporary timeline at
-"neon checkout", and have "neon commit" modify the branch to point to
+"zenith checkout", and have "zenith commit" modify the branch to point to
 the new timeline.

docs/rfcs/007-serverless-on-laptop.md

@@ -4,27 +4,27 @@ How it works now
 1. Create repository, start page server on it
 
 ```
-$ neon init
+$ zenith init
 ...
 created main branch
-new neon repository was created in .neon
+new zenith repository was created in .zenith
 
-$ neon pageserver start
-Starting pageserver at '127.0.0.1:64000' in .neon
+$ zenith pageserver start
+Starting pageserver at '127.0.0.1:64000' in .zenith
 Page server started
 ```
 
 2. Create a branch, and start a Postgres instance on it
 
 ```
-$ neon branch heikki main
+$ zenith branch heikki main
 branching at end of WAL: 0/15ECF68
 
-$ neon pg create heikki
+$ zenith pg create heikki
 Initializing Postgres on timeline 76cf9279915be7797095241638e64644...
-Extracting base backup to create postgres instance: path=.neon/pgdatadirs/pg1 port=55432
+Extracting base backup to create postgres instance: path=.zenith/pgdatadirs/pg1 port=55432
 
-$ neon pg start pg1
+$ zenith pg start pg1
 Starting postgres node at 'host=127.0.0.1 port=55432 user=heikki'
 waiting for server to start.... done
 server started
@@ -52,20 +52,20 @@ serverless on your laptop, so that the workflow becomes just:
 1. Create repository, start page server on it (same as before)
 
 ```
-$ neon init
+$ zenith init
 ...
 created main branch
-new neon repository was created in .neon
+new zenith repository was created in .zenith
 
-$ neon pageserver start
-Starting pageserver at '127.0.0.1:64000' in .neon
+$ zenith pageserver start
+Starting pageserver at '127.0.0.1:64000' in .zenith
 Page server started
 ```
 
 2. Create branch
 
 ```
-$ neon branch heikki main
+$ zenith branch heikki main
 branching at end of WAL: 0/15ECF68
 ```
 

docs/rfcs/008-push-pull.md

@@ -7,22 +7,22 @@ Here is a proposal about implementing push/pull mechanics between pageservers. W
 The origin represents connection info for some remote pageserver. Let's use here same commands as git uses except using explicit list subcommand (git uses `origin -v` for that).
 
 ```
-neon origin add <name> <connection_uri>
-neon origin list
-neon origin remove <name>
+zenith origin add <name> <connection_uri>
+zenith origin list
+zenith origin remove <name>
 ```
 
 Connection URI a string of form `postgresql://user:pass@hostname:port` (https://www.postgresql.org/docs/13/libpq-connect.html#id-1.7.3.8.3.6). We can start with libpq password auth and later add support for client certs or require ssh as transport or invent some other kind of transport.
 
-Behind the scenes, this commands may update toml file inside .neon directory.
+Behind the scenes, this commands may update toml file inside .zenith directory.
 
 ## Push
 
 ### Pushing branch
 
 ```
-neon push mybranch cloudserver # push to eponymous branch in cloudserver
-neon push mybranch cloudserver:otherbranch # push to a different branch in cloudserver
+zenith push mybranch cloudserver # push to eponymous branch in cloudserver
+zenith push mybranch cloudserver:otherbranch # push to a different branch in cloudserver
 ```
 
 Exact mechanics would be slightly different in the following situations:

docs/rfcs/009-snapshot-first-storage-cli.md

@@ -2,7 +2,7 @@ While working on export/import commands, I understood that they fit really well
 
 We may think about backups as snapshots in a different format (i.e plain pgdata format, basebackup tar format, WAL-G format (if they want to support it) and so on). They use same storage API, the only difference is the code that packs/unpacks files.
 
-Even if neon aims to maintains durability using it's own snapshots, backups will be useful for uploading data from postgres to neon.
+Even if zenith aims to maintains durability using it's own snapshots, backups will be useful for uploading data from postgres to zenith.
 
 So here is an attempt to design consistent CLI for different usage scenarios:
 
@@ -16,8 +16,8 @@ Save`storage_dest` and other parameters in config.
 Push snapshots to `storage_dest` in background.
 
 ```
-neon init --storage_dest=S3_PREFIX
-neon start
+zenith init --storage_dest=S3_PREFIX
+zenith start
 ```
 
 #### 2. Restart pageserver (manually or crash-recovery).
@@ -25,7 +25,7 @@ Take `storage_dest` from pageserver config, start pageserver from latest snapsho
 Push snapshots to `storage_dest` in background.
 
 ```
-neon start
+zenith start
 ```
 
 #### 3. Import.
@@ -35,22 +35,22 @@ Do not save `snapshot_path` and `snapshot_format` in config, as it is a one-time
 Save`storage_dest` parameters in config.
 Push snapshots to `storage_dest` in background.
 ```
-//I.e. we want to start neon on top of existing $PGDATA and use s3 as a persistent storage.
-neon init --snapshot_path=FILE_PREFIX --snapshot_format=pgdata --storage_dest=S3_PREFIX
-neon start
+//I.e. we want to start zenith on top of existing $PGDATA and use s3 as a persistent storage.
+zenith init --snapshot_path=FILE_PREFIX --snapshot_format=pgdata --storage_dest=S3_PREFIX
+zenith start
 ```
 How to pass credentials needed for `snapshot_path`?
 
 #### 4. Export.
 Manually push snapshot to `snapshot_path` which differs from `storage_dest`
-Optionally set `snapshot_format`, which can be plain pgdata format or neon format.
+Optionally set `snapshot_format`, which can be plain pgdata format or zenith format.
 ```
-neon export --snapshot_path=FILE_PREFIX --snapshot_format=pgdata
+zenith export --snapshot_path=FILE_PREFIX --snapshot_format=pgdata
 ```
 
 #### Notes and questions
 - safekeeper s3_offload should use same (similar) syntax for storage. How to set it in UI?
-- Why do we need `neon init` as a separate command? Can't we init everything at first start?
+- Why do we need `zenith init` as a separate command? Can't we init everything at first start?
 - We can think of better names for all options.
 - Export to plain postgres format will be useless, if we are not 100% compatible on page level.
 I can recall at least one such difference - PD_WAL_LOGGED flag in pages.

docs/rfcs/013-term-history.md

@@ -9,7 +9,7 @@ receival and this might lag behind `term`; safekeeper switches to epoch `n` when
 it has received all committed log records from all `< n` terms. This roughly
 corresponds to proposed in
 
-https://github.com/neondatabase/rfcs/pull/3/files
+https://github.com/zenithdb/rfcs/pull/3/files
 
 
 This makes our biggest our difference from Raft. In Raft, every log record is

docs/rfcs/014-safekeepers-gossip.md

@@ -1,6 +1,6 @@
 # Safekeeper gossip
 
-Extracted from this [PR](https://github.com/neondatabase/rfcs/pull/13)
+Extracted from this [PR](https://github.com/zenithdb/rfcs/pull/13)
 
 ## Motivation
 

docs/rfcs/015-storage-messaging.md

@@ -2,7 +2,7 @@
 
 Created on 19.01.22
 
-Initially created [here](https://github.com/neondatabase/rfcs/pull/16) by @kelvich.
+Initially created [here](https://github.com/zenithdb/rfcs/pull/16) by @kelvich.
 
 That it is an alternative to (014-safekeeper-gossip)[]
 
@@ -292,4 +292,4 @@ But with an etcd we are in a bit different situation:
 1. We don't need persistency and strong consistency guarantees for the data we store in the etcd
 2. etcd uses Grpc as a protocol, and messages are pretty simple
 
-So it looks like implementing in-mem store with etcd interface is straightforward thing _if we will want that in future_. At the same time, we can avoid implementing it right now, and we will be able to run local neon installation with etcd running somewhere in the background (as opposed to building and running console, which in turn requires Postgres).
+So it looks like implementing in-mem store with etcd interface is straightforward thing _if we will want that in future_. At the same time, we can avoid implementing it right now, and we will be able to run local zenith installation with etcd running somewhere in the background (as opposed to building and running console, which in turn requires Postgres).

libs/pageserver_api/src/controller_api.rs

@@ -125,45 +125,5 @@ impl From<NodeSchedulingPolicy> for String {
     }
 }
 
-/// Controls how tenant shards are mapped to locations on pageservers, e.g. whether
-/// to create secondary locations.
-#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)]
-pub enum PlacementPolicy {
-    /// Cheapest way to attach a tenant: just one pageserver, no secondary
-    Single,
-    /// Production-ready way to attach a tenant: one attached pageserver and
-    /// some number of secondaries.
-    Double(usize),
-    /// Create one secondary mode locations. This is useful when onboarding
-    /// a tenant, or for an idle tenant that we might want to bring online quickly.
-    Secondary,
-
-    /// Do not attach to any pageservers.  This is appropriate for tenants that
-    /// have been idle for a long time, where we do not mind some delay in making
-    /// them available in future.
-    Detached,
-}
-
 #[derive(Serialize, Deserialize, Debug)]
 pub struct TenantShardMigrateResponse {}
-
-#[cfg(test)]
-mod test {
-    use super::*;
-    use serde_json;
-
-    /// Check stability of PlacementPolicy's serialization
-    #[test]
-    fn placement_policy_encoding() -> anyhow::Result<()> {
-        let v = PlacementPolicy::Double(1);
-        let encoded = serde_json::to_string(&v)?;
-        assert_eq!(encoded, "{\"Double\":1}");
-        assert_eq!(serde_json::from_str::<PlacementPolicy>(&encoded)?, v);
-
-        let v = PlacementPolicy::Single;
-        let encoded = serde_json::to_string(&v)?;
-        assert_eq!(encoded, "\"Single\"");
-        assert_eq!(serde_json::from_str::<PlacementPolicy>(&encoded)?, v);
-        Ok(())
-    }
-}

libs/pageserver_api/src/models.rs

@@ -14,6 +14,7 @@ use byteorder::{BigEndian, ReadBytesExt};
 use postgres_ffi::BLCKSZ;
 use serde::{Deserialize, Serialize};
 use serde_with::serde_as;
+use strum_macros;
 use utils::{
     completion,
     history_buffer::HistoryBufferWithDropCounter,
@@ -21,7 +22,6 @@ use utils::{
     lsn::Lsn,
 };
 
-use crate::controller_api::PlacementPolicy;
 use crate::{
     reltag::RelTag,
     shard::{ShardCount, ShardStripeSize, TenantShardId},
@@ -243,11 +243,6 @@ pub struct TenantCreateRequest {
     #[serde(skip_serializing_if = "ShardParameters::is_unsharded")]
     pub shard_parameters: ShardParameters,
 
-    // This parameter is only meaningful in requests sent to the storage controller
-    #[serde(default)]
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub placement_policy: Option<PlacementPolicy>,
-
     #[serde(flatten)]
     pub config: TenantConfig, // as we have a flattened field, we should reject all unknown fields in it
 }
@@ -441,8 +436,6 @@ pub struct TenantShardLocation {
 #[serde(deny_unknown_fields)]
 pub struct TenantLocationConfigResponse {
     pub shards: Vec<TenantShardLocation>,
-    // If the shards' ShardCount count is >1, stripe_size will be set.
-    pub stripe_size: Option<ShardStripeSize>,
 }
 
 #[derive(Serialize, Deserialize, Debug)]
@@ -1084,6 +1077,7 @@ impl PagestreamBeMessage {
 
 #[cfg(test)]
 mod tests {
+    use bytes::Buf;
     use serde_json::json;
 
     use super::*;

libs/pageserver_api/src/shard.rs

@@ -6,6 +6,7 @@ use crate::{
 };
 use hex::FromHex;
 use serde::{Deserialize, Serialize};
+use thiserror;
 use utils::id::TenantId;
 
 #[derive(Ord, PartialOrd, Eq, PartialEq, Clone, Copy, Serialize, Deserialize, Debug, Hash)]
@@ -655,7 +656,10 @@ fn key_to_shard_number(count: ShardCount, stripe_size: ShardStripeSize, key: &Ke
 
 #[cfg(test)]
 mod tests {
-    use utils::Hex;
+    use std::str::FromStr;
+
+    use bincode;
+    use utils::{id::TenantId, Hex};
 
     use super::*;
 

libs/postgres_backend/src/lib.rs

@@ -6,6 +6,7 @@
 #![deny(clippy::undocumented_unsafe_blocks)]
 use anyhow::Context;
 use bytes::Bytes;
+use futures::pin_mut;
 use serde::{Deserialize, Serialize};
 use std::io::ErrorKind;
 use std::net::SocketAddr;
@@ -377,7 +378,8 @@ impl<IO: AsyncRead + AsyncWrite + Unpin> PostgresBackend<IO> {
         &mut self,
         cx: &mut std::task::Context<'_>,
     ) -> Poll<Result<(), std::io::Error>> {
-        let flush_fut = std::pin::pin!(self.flush());
+        let flush_fut = self.flush();
+        pin_mut!(flush_fut);
         flush_fut.poll(cx)
     }
 

libs/postgres_backend/tests/simple_select.rs

@@ -72,19 +72,14 @@ async fn simple_select() {
     }
 }
 
-static KEY: Lazy<rustls::pki_types::PrivateKeyDer<'static>> = Lazy::new(|| {
+static KEY: Lazy<rustls::PrivateKey> = Lazy::new(|| {
     let mut cursor = Cursor::new(include_bytes!("key.pem"));
-    let key = rustls_pemfile::rsa_private_keys(&mut cursor)
-        .next()
-        .unwrap()
-        .unwrap();
-    rustls::pki_types::PrivateKeyDer::Pkcs1(key)
+    rustls::PrivateKey(rustls_pemfile::rsa_private_keys(&mut cursor).unwrap()[0].clone())
 });
 
-static CERT: Lazy<rustls::pki_types::CertificateDer<'static>> = Lazy::new(|| {
+static CERT: Lazy<rustls::Certificate> = Lazy::new(|| {
     let mut cursor = Cursor::new(include_bytes!("cert.pem"));
-    let cert = rustls_pemfile::certs(&mut cursor).next().unwrap().unwrap();
-    cert
+    rustls::Certificate(rustls_pemfile::certs(&mut cursor).unwrap()[0].clone())
 });
 
 // test that basic select with ssl works
@@ -93,8 +88,9 @@ async fn simple_select_ssl() {
     let (client_sock, server_sock) = make_tcp_pair().await;
 
     let server_cfg = rustls::ServerConfig::builder()
+        .with_safe_defaults()
         .with_no_client_auth()
-        .with_single_cert(vec![CERT.clone()], KEY.clone_key())
+        .with_single_cert(vec![CERT.clone()], KEY.clone())
         .unwrap();
     let tls_config = Some(Arc::new(server_cfg));
     let pgbackend =
@@ -106,9 +102,10 @@ async fn simple_select_ssl() {
     });
 
     let client_cfg = rustls::ClientConfig::builder()
+        .with_safe_defaults()
         .with_root_certificates({
             let mut store = rustls::RootCertStore::empty();
-            store.add(CERT.clone()).unwrap();
+            store.add(&CERT).unwrap();
             store
         })
         .with_no_client_auth();

libs/remote_storage/src/local_fs.rs

@@ -623,7 +623,9 @@ fn file_exists(file_path: &Utf8Path) -> anyhow::Result<bool> {
 mod fs_tests {
     use super::*;
 
+    use bytes::Bytes;
     use camino_tempfile::tempdir;
+    use futures_util::Stream;
     use std::{collections::HashMap, io::Write};
 
     async fn read_and_check_metadata(

libs/remote_storage/src/s3_bucket.rs

@@ -1040,7 +1040,7 @@ mod tests {
             Some("test/prefix/"),
             Some("/test/prefix/"),
         ];
-        let expected_outputs = [
+        let expected_outputs = vec![
             vec!["", "some/path", "some/path"],
             vec!["/", "/some/path", "/some/path"],
             vec![

libs/utils/src/auth.rs

@@ -1,6 +1,7 @@
 // For details about authentication see docs/authentication.md
 
 use arc_swap::ArcSwap;
+use serde;
 use std::{borrow::Cow, fmt::Display, fs, sync::Arc};
 
 use anyhow::Result;

libs/utils/src/completion.rs

@@ -4,9 +4,7 @@ use tokio_util::task::{task_tracker::TaskTrackerToken, TaskTracker};
 ///
 /// Can be cloned, moved and kept around in futures as "guard objects".
 #[derive(Clone)]
-pub struct Completion {
-    _token: TaskTrackerToken,
-}
+pub struct Completion(TaskTrackerToken);
 
 /// Barrier will wait until all clones of [`Completion`] have been dropped.
 #[derive(Clone)]
@@ -51,5 +49,5 @@ pub fn channel() -> (Completion, Barrier) {
     tracker.close();
 
     let token = tracker.token();
-    (Completion { _token: token }, Barrier(tracker))
+    (Completion(token), Barrier(tracker))
 }

libs/utils/src/generation.rs

@@ -45,7 +45,7 @@ impl Generation {
         Self::Broken
     }
 
-    pub const fn new(v: u32) -> Self {
+    pub fn new(v: u32) -> Self {
         Self::Valid(v)
     }
 

libs/utils/src/http/endpoint.rs

@@ -9,7 +9,7 @@ use metrics::{register_int_counter, Encoder, IntCounter, TextEncoder};
 use once_cell::sync::Lazy;
 use routerify::ext::RequestExt;
 use routerify::{Middleware, RequestInfo, Router, RouterBuilder};
-use tracing::{debug, info, info_span, warn, Instrument};
+use tracing::{self, debug, info, info_span, warn, Instrument};
 
 use std::future::Future;
 use std::str::FromStr;
@@ -156,10 +156,6 @@ pub struct ChannelWriter {
     buffer: BytesMut,
     pub tx: mpsc::Sender<std::io::Result<Bytes>>,
     written: usize,
-    /// Time spent waiting for the channel to make progress. It is not the same as time to upload a
-    /// buffer because we cannot know anything about that, but this should allow us to understand
-    /// the actual time taken without the time spent `std::thread::park`ed.
-    wait_time: std::time::Duration,
 }
 
 impl ChannelWriter {
@@ -172,7 +168,6 @@ impl ChannelWriter {
             buffer: BytesMut::with_capacity(buf_len).split_off(buf_len / 2),
             tx,
             written: 0,
-            wait_time: std::time::Duration::ZERO,
         }
     }
 
@@ -185,8 +180,6 @@ impl ChannelWriter {
         tracing::trace!(n, "flushing");
         let ready = self.buffer.split().freeze();
 
-        let wait_started_at = std::time::Instant::now();
-
         // not ideal to call from blocking code to block_on, but we are sure that this
         // operation does not spawn_blocking other tasks
         let res: Result<(), ()> = tokio::runtime::Handle::current().block_on(async {
@@ -199,9 +192,6 @@ impl ChannelWriter {
             // sending it to the client.
             Ok(())
         });
-
-        self.wait_time += wait_started_at.elapsed();
-
         if res.is_err() {
             return Err(std::io::ErrorKind::BrokenPipe.into());
         }
@@ -212,10 +202,6 @@ impl ChannelWriter {
     pub fn flushed_bytes(&self) -> usize {
         self.written
     }
-
-    pub fn wait_time(&self) -> std::time::Duration {
-        self.wait_time
-    }
 }
 
 impl std::io::Write for ChannelWriter {
@@ -266,52 +252,22 @@ async fn prometheus_metrics_handler(_req: Request<Body>) -> Result<Response<Body
 
     let span = info_span!("blocking");
     tokio::task::spawn_blocking(move || {
-        // there are situations where we lose scraped metrics under load, try to gather some clues
-        // since all nodes are queried this, keep the message count low.
-        let spawned_at = std::time::Instant::now();
-
         let _span = span.entered();
-
         let metrics = metrics::gather();
-
-        let gathered_at = std::time::Instant::now();
-
         let res = encoder
             .encode(&metrics, &mut writer)
             .and_then(|_| writer.flush().map_err(|e| e.into()));
 
-        // this instant is not when we finally got the full response sent, sending is done by hyper
-        // in another task.
-        let encoded_at = std::time::Instant::now();
-
-        let spawned_in = spawned_at - started_at;
-        let collected_in = gathered_at - spawned_at;
-        // remove the wait time here in case the tcp connection was clogged
-        let encoded_in = encoded_at - gathered_at - writer.wait_time();
-        let total = encoded_at - started_at;
-
         match res {
             Ok(()) => {
                 tracing::info!(
                     bytes = writer.flushed_bytes(),
-                    total_ms = total.as_millis(),
-                    spawning_ms = spawned_in.as_millis(),
-                    collection_ms = collected_in.as_millis(),
-                    encoding_ms = encoded_in.as_millis(),
+                    elapsed_ms = started_at.elapsed().as_millis(),
                     "responded /metrics"
                 );
             }
             Err(e) => {
-                // there is a chance that this error is not the BrokenPipe we generate in the writer
-                // for "closed connection", but it is highly unlikely.
-                tracing::warn!(
-                    after_bytes = writer.flushed_bytes(),
-                    total_ms = total.as_millis(),
-                    spawning_ms = spawned_in.as_millis(),
-                    collection_ms = collected_in.as_millis(),
-                    encoding_ms = encoded_in.as_millis(),
-                    "failed to write out /metrics response: {e:?}"
-                );
+                tracing::warn!("failed to write out /metrics response: {e:#}");
                 // semantics of this error are quite... unclear. we want to error the stream out to
                 // abort the response to somehow notify the client that we failed.
                 //

libs/utils/src/lsn.rs

@@ -415,6 +415,7 @@ mod tests {
 
     use super::*;
 
+    use serde::ser::Serialize;
     use serde_assert::{Deserializer, Serializer, Token, Tokens};
 
     #[test]

libs/utils/src/seqwait.rs

@@ -1,6 +1,6 @@
 #![warn(missing_docs)]
 
-use std::cmp::{Eq, Ordering};
+use std::cmp::{Eq, Ordering, PartialOrd};
 use std::collections::BinaryHeap;
 use std::fmt::Debug;
 use std::mem;
@@ -249,6 +249,7 @@ where
 mod tests {
     use super::*;
     use std::sync::Arc;
+    use std::time::Duration;
 
     impl MonotonicCounter<i32> for i32 {
         fn cnt_advance(&mut self, val: i32) {

libs/utils/src/simple_rcu.rs

@@ -221,7 +221,7 @@ impl RcuWaitList {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use std::sync::Mutex;
+    use std::sync::{Arc, Mutex};
     use std::time::Duration;
 
     #[tokio::test]

libs/utils/src/sync/heavier_once_cell.rs

@@ -239,6 +239,7 @@ mod tests {
     use std::{
         convert::Infallible,
         pin::{pin, Pin},
+        sync::atomic::{AtomicUsize, Ordering},
         time::Duration,
     };
 

pageserver/client/src/mgmt_api.rs

@@ -7,7 +7,7 @@ use utils::{
 
 pub mod util;
 
-#[derive(Debug, Clone)]
+#[derive(Debug)]
 pub struct Client {
     mgmt_api_endpoint: String,
     authorization_header: Option<String>,
@@ -24,9 +24,6 @@ pub enum Error {
 
     #[error("pageserver API: {1}")]
     ApiError(StatusCode, String),
-
-    #[error("Cancelled")]
-    Cancelled,
 }
 
 pub type Result<T> = std::result::Result<T, Error>;
@@ -254,30 +251,21 @@ impl Client {
         tenant_shard_id: TenantShardId,
         config: LocationConfig,
         flush_ms: Option<std::time::Duration>,
-        lazy: bool,
     ) -> Result<()> {
         let req_body = TenantLocationConfigRequest {
             tenant_id: tenant_shard_id,
             config,
         };
-
-        let mut path = reqwest::Url::parse(&format!(
+        let path = format!(
             "{}/v1/tenant/{}/location_config",
             self.mgmt_api_endpoint, tenant_shard_id
-        ))
-        // Should always work: mgmt_api_endpoint is configuration, not user input.
-        .expect("Cannot build URL");
-
-        if lazy {
-            path.query_pairs_mut().append_pair("lazy", "true");
-        }
-
-        if let Some(flush_ms) = flush_ms {
-            path.query_pairs_mut()
-                .append_pair("flush_ms", &format!("{}", flush_ms.as_millis()));
-        }
-
-        self.request(Method::PUT, path, &req_body).await?;
+        );
+        let path = if let Some(flush_ms) = flush_ms {
+            format!("{}?flush_ms={}", path, flush_ms.as_millis())
+        } else {
+            path
+        };
+        self.request(Method::PUT, &path, &req_body).await?;
         Ok(())
     }
 
@@ -290,21 +278,6 @@ impl Client {
             .map_err(Error::ReceiveBody)
     }
 
-    pub async fn get_location_config(
-        &self,
-        tenant_shard_id: TenantShardId,
-    ) -> Result<Option<LocationConfig>> {
-        let path = format!(
-            "{}/v1/location_config/{tenant_shard_id}",
-            self.mgmt_api_endpoint
-        );
-        self.request(Method::GET, &path, ())
-            .await?
-            .json()
-            .await
-            .map_err(Error::ReceiveBody)
-    }
-
     pub async fn timeline_create(
         &self,
         tenant_shard_id: TenantShardId,

pageserver/compaction/src/compact_tiered.rs

@@ -63,7 +63,7 @@ pub async fn compact_tiered<E: CompactionJobExecutor>(
         );
 
         // Identify the range of LSNs that belong to this level. We assume that
-        // each file in this level spans an LSN range up to 1.75x target file
+        // each file in this level span an LSN range up to 1.75x target file
         // size. That should give us enough slop that if we created a slightly
         // oversized L0 layer, e.g. because flushing the in-memory layer was
         // delayed for some reason, we don't consider the oversized layer to
@@ -248,6 +248,7 @@ enum CompactionStrategy {
     CreateImage,
 }
 
+#[allow(dead_code)] // Todo
 struct CompactionJob<E: CompactionJobExecutor> {
     key_range: Range<E::Key>,
     lsn_range: Range<Lsn>,
@@ -344,7 +345,7 @@ where
     ///
     /// TODO: Currently, this is called exactly once for the level, and we
     /// decide whether to create new image layers to cover the whole level, or
-    /// write a new set of deltas. In the future, this should try to partition
+    /// write a new set of delta. In the future, this should try to partition
     /// the key space, and make the decision separately for each partition.
     async fn divide_job(&mut self, job_id: JobId, ctx: &E::RequestContext) -> anyhow::Result<()> {
         let job = &self.jobs[job_id.0];
@@ -708,6 +709,18 @@ where
     }
 }
 
+// Sliding window through keyspace and values
+//
+// This is used to decide what layer to write next, from the beginning of the window.
+//
+// Candidates:
+//
+// 1. Create an image layer, snapping to previous images
+// 2. Create a delta layer, snapping to previous images
+// 3. Create an image layer, snapping to
+//
+//
+
 // Take previous partitioning, based on the image layers below.
 //
 // Candidate is at the front:
@@ -726,10 +739,6 @@ struct WindowElement<K> {
     last_key: K,  // inclusive
     accum_size: u64,
 }
-
-// Sliding window through keyspace and values
-//
-// This is used to decide what layer to write next, from the beginning of the window.
 struct Window<K> {
     elems: VecDeque<WindowElement<K>>,
 

pageserver/compaction/src/helpers.rs

@@ -6,6 +6,7 @@ use futures::future::BoxFuture;
 use futures::{Stream, StreamExt};
 use itertools::Itertools;
 use pin_project_lite::pin_project;
+use std::cmp::Ord;
 use std::collections::BinaryHeap;
 use std::collections::VecDeque;
 use std::future::Future;

pageserver/compaction/src/identify_levels.rs

@@ -1,5 +1,5 @@
-//! An LSM tree consists of multiple levels, each exponentially larger than the
-//! previous level. And each level consists of multiple "tiers". With tiered
+//! An LSM tree consists of multiple levels, each exponential larger than the
+//! previous level. And each level consists of be multiple "tiers". With tiered
 //! compaction, a level is compacted when it has accumulated more than N tiers,
 //! forming one tier on the next level.
 //!
@@ -170,6 +170,13 @@ where
     })
 }
 
+// helper struct used in depth()
+struct Event<K> {
+    key: K,
+    layer_idx: usize,
+    start: bool,
+}
+
 impl<L> Level<L> {
     /// Count the number of deltas stacked on each other.
     pub fn depth<K>(&self) -> u64
@@ -177,11 +184,6 @@ impl<L> Level<L> {
         K: CompactionKey,
         L: CompactionLayer<K>,
     {
-        struct Event<K> {
-            key: K,
-            layer_idx: usize,
-            start: bool,
-        }
         let mut events: Vec<Event<K>> = Vec::new();
         for (idx, l) in self.layers.iter().enumerate() {
             events.push(Event {
@@ -200,7 +202,7 @@ impl<L> Level<L> {
         // Sweep the key space left to right. Stop at each distinct key, and
         // count the number of deltas on top of the highest image at that key.
         //
-        // This is a little inefficient, as we walk through the active_set on
+        // This is a little enefficient, as we walk through the active_set on
         // every key. We could increment/decrement a counter on each step
         // instead, but that'd require a bit more complex bookkeeping.
         let mut active_set: BTreeSet<(Lsn, bool, usize)> = BTreeSet::new();
@@ -234,7 +236,6 @@ impl<L> Level<L> {
                 }
             }
         }
-        debug_assert_eq!(active_set, BTreeSet::new());
         max_depth
     }
 }

pageserver/compaction/src/interface.rs

@@ -4,12 +4,12 @@
 //! All the heavy lifting is done by the create_image and create_delta
 //! functions that the implementor provides.
 use async_trait::async_trait;
-use futures::Future;
 use pageserver_api::{key::Key, keyspace::key_range_size};
 use std::ops::Range;
 use utils::lsn::Lsn;
 
 /// Public interface. This is the main thing that the implementor needs to provide
+#[async_trait]
 pub trait CompactionJobExecutor {
     // Type system.
     //
@@ -17,7 +17,8 @@ pub trait CompactionJobExecutor {
     // compaction doesn't distinguish whether they are stored locally or
     // remotely.
     //
-    // The keyspace is defined by the CompactionKey trait.
+    // The keyspace is defined by CompactionKey trait.
+    //
     type Key: CompactionKey;
 
     type Layer: CompactionLayer<Self::Key> + Clone;
@@ -34,27 +35,27 @@ pub trait CompactionJobExecutor {
     // ----
 
     /// Return all layers that overlap the given bounding box.
-    fn get_layers(
+    async fn get_layers(
         &mut self,
         key_range: &Range<Self::Key>,
         lsn_range: &Range<Lsn>,
         ctx: &Self::RequestContext,
-    ) -> impl Future<Output = anyhow::Result<Vec<Self::Layer>>> + Send;
+    ) -> anyhow::Result<Vec<Self::Layer>>;
 
-    fn get_keyspace(
+    async fn get_keyspace(
         &mut self,
         key_range: &Range<Self::Key>,
         lsn: Lsn,
         ctx: &Self::RequestContext,
-    ) -> impl Future<Output = anyhow::Result<CompactionKeySpace<Self::Key>>> + Send;
+    ) -> anyhow::Result<CompactionKeySpace<Self::Key>>;
 
     /// NB: This is a pretty expensive operation. In the real pageserver
     /// implementation, it downloads the layer, and keeps it resident
     /// until the DeltaLayer is dropped.
-    fn downcast_delta_layer(
+    async fn downcast_delta_layer(
         &self,
         layer: &Self::Layer,
-    ) -> impl Future<Output = anyhow::Result<Option<Self::DeltaLayer>>> + Send;
+    ) -> anyhow::Result<Option<Self::DeltaLayer>>;
 
     // ----
     // Functions to execute the plan
@@ -62,33 +63,33 @@ pub trait CompactionJobExecutor {
 
     /// Create a new image layer, materializing all the values in the key range,
     /// at given 'lsn'.
-    fn create_image(
+    async fn create_image(
         &mut self,
         lsn: Lsn,
         key_range: &Range<Self::Key>,
         ctx: &Self::RequestContext,
-    ) -> impl Future<Output = anyhow::Result<()>> + Send;
+    ) -> anyhow::Result<()>;
 
     /// Create a new delta layer, containing all the values from 'input_layers'
     /// in the given key and LSN range.
-    fn create_delta(
+    async fn create_delta(
         &mut self,
         lsn_range: &Range<Lsn>,
         key_range: &Range<Self::Key>,
         input_layers: &[Self::DeltaLayer],
         ctx: &Self::RequestContext,
-    ) -> impl Future<Output = anyhow::Result<()>> + Send;
+    ) -> anyhow::Result<()>;
 
     /// Delete a layer. The compaction implementation will call this only after
     /// all the create_image() or create_delta() calls that deletion of this
     /// layer depends on have finished. But if the implementor has extra lazy
-    /// background tasks, like uploading the index json file to remote storage.
+    /// background tasks, like uploading the index json file to remote storage,
     /// it is the implementation's responsibility to track those.
-    fn delete_layer(
+    async fn delete_layer(
         &mut self,
         layer: &Self::Layer,
         ctx: &Self::RequestContext,
-    ) -> impl Future<Output = anyhow::Result<()>> + Send;
+    ) -> anyhow::Result<()>;
 }
 
 pub trait CompactionKey: std::cmp::Ord + Clone + Copy + std::fmt::Display {

pageserver/compaction/src/simulator.rs

@@ -429,6 +429,7 @@ impl From<&Arc<MockImageLayer>> for MockLayer {
     }
 }
 
+#[async_trait]
 impl interface::CompactionJobExecutor for MockTimeline {
     type Key = Key;
     type Layer = MockLayer;

pageserver/src/config.rs

@@ -20,6 +20,7 @@ use std::num::NonZeroUsize;
 use std::str::FromStr;
 use std::sync::Arc;
 use std::time::Duration;
+use toml_edit;
 use toml_edit::{Document, Item};
 
 use camino::{Utf8Path, Utf8PathBuf};
@@ -211,9 +212,9 @@ pub struct PageServerConf {
 
     pub log_format: LogFormat,
 
-    /// Number of tenants which will be concurrently loaded from remote storage proactively on startup or attach.
-    ///
-    /// A lower value implicitly deprioritizes loading such tenants, vs. other work in the system.
+    /// Number of tenants which will be concurrently loaded from remote storage proactively on startup,
+    /// does not limit tenants loaded in response to client I/O.  A lower value implicitly deprioritizes
+    /// loading such tenants, vs. other work in the system.
     pub concurrent_tenant_warmup: ConfigurableSemaphore,
 
     /// Number of concurrent [`Tenant::gather_size_inputs`](crate::tenant::Tenant::gather_size_inputs) allowed.
@@ -1202,7 +1203,10 @@ impl ConfigurableSemaphore {
 
 #[cfg(test)]
 mod tests {
-    use std::{fs, num::NonZeroU32};
+    use std::{
+        fs,
+        num::{NonZeroU32, NonZeroUsize},
+    };
 
     use camino_tempfile::{tempdir, Utf8TempDir};
     use pageserver_api::models::EvictionPolicy;

pageserver/src/consumption_metrics/metrics/tests.rs

@@ -1,5 +1,7 @@
 use super::*;
 use std::collections::HashMap;
+use std::time::SystemTime;
+use utils::lsn::Lsn;
 
 #[test]
 fn startup_collected_timeline_metrics_before_advancing() {

pageserver/src/context.rs

@@ -88,16 +88,13 @@
 
 use crate::task_mgr::TaskKind;
 
-pub(crate) mod optional_counter;
-
 // The main structure of this module, see module-level comment.
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub struct RequestContext {
     task_kind: TaskKind,
     download_behavior: DownloadBehavior,
     access_stats_behavior: AccessStatsBehavior,
     page_content_kind: PageContentKind,
-    pub micros_spent_throttled: optional_counter::MicroSecondsCounterU32,
 }
 
 /// The kind of access to the page cache.
@@ -153,7 +150,6 @@ impl RequestContextBuilder {
                 download_behavior: DownloadBehavior::Download,
                 access_stats_behavior: AccessStatsBehavior::Update,
                 page_content_kind: PageContentKind::Unknown,
-                micros_spent_throttled: Default::default(),
             },
         }
     }
@@ -167,7 +163,6 @@ impl RequestContextBuilder {
                 download_behavior: original.download_behavior,
                 access_stats_behavior: original.access_stats_behavior,
                 page_content_kind: original.page_content_kind,
-                micros_spent_throttled: Default::default(),
             },
         }
     }

pageserver/src/context/optional_counter.rs

@@ -1,101 +0,0 @@
-use std::{
-    sync::atomic::{AtomicU32, Ordering},
-    time::Duration,
-};
-
-#[derive(Debug)]
-pub struct CounterU32 {
-    inner: AtomicU32,
-}
-impl Default for CounterU32 {
-    fn default() -> Self {
-        Self {
-            inner: AtomicU32::new(u32::MAX),
-        }
-    }
-}
-impl CounterU32 {
-    pub fn open(&self) -> Result<(), &'static str> {
-        match self
-            .inner
-            .compare_exchange(u32::MAX, 0, Ordering::Relaxed, Ordering::Relaxed)
-        {
-            Ok(_) => Ok(()),
-            Err(_) => Err("open() called on clsoed state"),
-        }
-    }
-    pub fn close(&self) -> Result<u32, &'static str> {
-        match self.inner.swap(u32::MAX, Ordering::Relaxed) {
-            u32::MAX => Err("close() called on closed state"),
-            x => Ok(x),
-        }
-    }
-
-    pub fn add(&self, count: u32) -> Result<(), &'static str> {
-        if count == 0 {
-            return Ok(());
-        }
-        let mut had_err = None;
-        self.inner
-            .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |cur| match cur {
-                u32::MAX => {
-                    had_err = Some("add() called on closed state");
-                    None
-                }
-                x => {
-                    let (new, overflowed) = x.overflowing_add(count);
-                    if new == u32::MAX || overflowed {
-                        had_err = Some("add() overflowed the counter");
-                        None
-                    } else {
-                        Some(new)
-                    }
-                }
-            })
-            .map_err(|_| had_err.expect("we set it whenever the function returns None"))
-            .map(|_| ())
-    }
-}
-
-#[derive(Default, Debug)]
-pub struct MicroSecondsCounterU32 {
-    inner: CounterU32,
-}
-
-impl MicroSecondsCounterU32 {
-    pub fn open(&self) -> Result<(), &'static str> {
-        self.inner.open()
-    }
-    pub fn add(&self, duration: Duration) -> Result<(), &'static str> {
-        match duration.as_micros().try_into() {
-            Ok(x) => self.inner.add(x),
-            Err(_) => Err("add(): duration conversion error"),
-        }
-    }
-    pub fn close_and_checked_sub_from(&self, from: Duration) -> Result<Duration, &'static str> {
-        let val = self.inner.close()?;
-        let val = Duration::from_micros(val as u64);
-        let subbed = match from.checked_sub(val) {
-            Some(v) => v,
-            None => return Err("Duration::checked_sub"),
-        };
-        Ok(subbed)
-    }
-}
-
-#[cfg(test)]
-mod tests {
-
-    use super::*;
-
-    #[test]
-    fn test_basic() {
-        let counter = MicroSecondsCounterU32::default();
-        counter.open().unwrap();
-        counter.add(Duration::from_micros(23)).unwrap();
-        let res = counter
-            .close_and_checked_sub_from(Duration::from_micros(42))
-            .unwrap();
-        assert_eq!(res, Duration::from_micros(42 - 23));
-    }
-}

pageserver/src/deletion_queue.rs

@@ -20,9 +20,10 @@ use remote_storage::{GenericRemoteStorage, RemotePath};
 use serde::Deserialize;
 use serde::Serialize;
 use thiserror::Error;
+use tokio;
 use tokio_util::sync::CancellationToken;
 use tracing::Instrument;
-use tracing::{debug, error};
+use tracing::{self, debug, error};
 use utils::crashsafe::path_with_suffix_extension;
 use utils::generation::Generation;
 use utils::id::TimelineId;
@@ -725,7 +726,7 @@ mod test {
     use camino::Utf8Path;
     use hex_literal::hex;
     use pageserver_api::shard::ShardIndex;
-    use std::io::ErrorKind;
+    use std::{io::ErrorKind, time::Duration};
     use tracing::info;
 
     use remote_storage::{RemoteStorageConfig, RemoteStorageKind};
@@ -734,7 +735,10 @@ mod test {
     use crate::{
         control_plane_client::RetryForeverError,
         repository::Key,
-        tenant::{harness::TenantHarness, storage_layer::DeltaFileName},
+        tenant::{
+            harness::TenantHarness, remote_timeline_client::remote_timeline_path,
+            storage_layer::DeltaFileName,
+        },
     };
 
     use super::*;
@@ -1157,8 +1161,13 @@ mod test {
 pub(crate) mod mock {
     use tracing::info;
 
+    use crate::tenant::remote_timeline_client::remote_layer_path;
+
     use super::*;
-    use std::sync::atomic::{AtomicUsize, Ordering};
+    use std::sync::{
+        atomic::{AtomicUsize, Ordering},
+        Arc,
+    };
 
     pub struct ConsumerState {
         rx: tokio::sync::mpsc::UnboundedReceiver<ListWriterQueueMessage>,

pageserver/src/disk_usage_eviction_task.rs

@@ -58,7 +58,6 @@ use utils::{completion, id::TimelineId};
 
 use crate::{
     config::PageServerConf,
-    metrics::disk_usage_based_eviction::METRICS,
     task_mgr::{self, TaskKind, BACKGROUND_RUNTIME},
     tenant::{
         self,
@@ -66,6 +65,7 @@ use crate::{
         remote_timeline_client::LayerFileMetadata,
         secondary::SecondaryTenant,
         storage_layer::{AsLayerDesc, EvictionError, Layer, LayerFileName},
+        Timeline,
     },
 };
 
@@ -409,23 +409,13 @@ pub(crate) async fn disk_usage_eviction_task_iteration_impl<U: Usage>(
         "running disk usage based eviction due to pressure"
     );
 
-    let (candidates, collection_time) = {
-        let started_at = std::time::Instant::now();
+    let candidates =
         match collect_eviction_candidates(tenant_manager, eviction_order, cancel).await? {
             EvictionCandidates::Cancelled => {
                 return Ok(IterationOutcome::Cancelled);
             }
-            EvictionCandidates::Finished(partitioned) => (partitioned, started_at.elapsed()),
-        }
-    };
-
-    METRICS.layers_collected.inc_by(candidates.len() as u64);
-
-    tracing::info!(
-        elapsed_ms = collection_time.as_millis(),
-        total_layers = candidates.len(),
-        "collection completed"
-    );
+            EvictionCandidates::Finished(partitioned) => partitioned,
+        };
 
     // Debug-log the list of candidates
     let now = SystemTime::now();
@@ -456,10 +446,9 @@ pub(crate) async fn disk_usage_eviction_task_iteration_impl<U: Usage>(
     // the tenant's min-resident-size threshold, print a warning, and memorize the disk
     // usage at that point, in 'usage_planned_min_resident_size_respecting'.
 
-    let (evicted_amount, usage_planned) =
-        select_victims(&candidates, usage_pre).into_amount_and_planned();
+    let selection = select_victims(&candidates, usage_pre);
 
-    METRICS.layers_selected.inc_by(evicted_amount as u64);
+    let (evicted_amount, usage_planned) = selection.into_amount_and_planned();
 
     // phase2: evict layers
 
@@ -488,15 +477,9 @@ pub(crate) async fn disk_usage_eviction_task_iteration_impl<U: Usage>(
             if let Some(next) = next {
                 match next {
                     Ok(Ok(file_size)) => {
-                        METRICS.layers_evicted.inc();
                         usage_assumed.add_available_bytes(file_size);
                     }
-                    Ok(Err((
-                        file_size,
-                        EvictionError::NotFound
-                        | EvictionError::Downloaded
-                        | EvictionError::Timeout,
-                    ))) => {
+                    Ok(Err((file_size, EvictionError::NotFound | EvictionError::Downloaded))) => {
                         evictions_failed.file_sizes += file_size;
                         evictions_failed.count += 1;
                     }
@@ -512,10 +495,7 @@ pub(crate) async fn disk_usage_eviction_task_iteration_impl<U: Usage>(
 
             // calling again when consumed_all is fine as evicted is fused.
             let Some((_partition, candidate)) = evicted.next() else {
-                if !consumed_all {
-                    tracing::info!("all evictions started, waiting");
-                    consumed_all = true;
-                }
+                consumed_all = true;
                 continue;
             };
 
@@ -523,15 +503,11 @@ pub(crate) async fn disk_usage_eviction_task_iteration_impl<U: Usage>(
                 EvictionLayer::Attached(layer) => {
                     let file_size = layer.layer_desc().file_size;
                     js.spawn(async move {
-                        // have a low eviction waiting timeout because our LRU calculations go stale fast;
-                        // also individual layer evictions could hang because of bugs and we do not want to
-                        // pause disk_usage_based_eviction for such.
-                        let timeout = std::time::Duration::from_secs(5);
-
-                        match layer.evict_and_wait(timeout).await {
-                            Ok(()) => Ok(file_size),
-                            Err(e) => Err((file_size, e)),
-                        }
+                        layer
+                            .evict_and_wait()
+                            .await
+                            .map(|()| file_size)
+                            .map_err(|e| (file_size, e))
                     });
                 }
                 EvictionLayer::Secondary(layer) => {
@@ -553,30 +529,6 @@ pub(crate) async fn disk_usage_eviction_task_iteration_impl<U: Usage>(
         (usage_assumed, evictions_failed)
     };
 
-    let started_at = std::time::Instant::now();
-
-    let evict_layers = async move {
-        let mut evict_layers = std::pin::pin!(evict_layers);
-
-        let maximum_expected = std::time::Duration::from_secs(10);
-
-        let res = tokio::time::timeout(maximum_expected, &mut evict_layers).await;
-        let tuple = if let Ok(tuple) = res {
-            tuple
-        } else {
-            let elapsed = started_at.elapsed();
-            tracing::info!(elapsed_ms = elapsed.as_millis(), "still ongoing");
-            evict_layers.await
-        };
-
-        let elapsed = started_at.elapsed();
-        tracing::info!(elapsed_ms = elapsed.as_millis(), "completed");
-        tuple
-    };
-
-    let evict_layers =
-        evict_layers.instrument(tracing::info_span!("evict_layers", layers=%evicted_amount));
-
     let (usage_assumed, evictions_failed) = tokio::select! {
         tuple = evict_layers => { tuple },
         _ = cancel.cancelled() => {
@@ -811,8 +763,6 @@ async fn collect_eviction_candidates(
     eviction_order: EvictionOrder,
     cancel: &CancellationToken,
 ) -> anyhow::Result<EvictionCandidates> {
-    const LOG_DURATION_THRESHOLD: std::time::Duration = std::time::Duration::from_secs(10);
-
     // get a snapshot of the list of tenants
     let tenants = tenant::mgr::list_tenants()
         .await
@@ -841,8 +791,6 @@ async fn collect_eviction_candidates(
             continue;
         }
 
-        let started_at = std::time::Instant::now();
-
         // collect layers from all timelines in this tenant
         //
         // If one of the timelines becomes `!is_active()` during the iteration,
@@ -857,7 +805,6 @@ async fn collect_eviction_candidates(
             }
             let info = tl.get_local_layers_for_disk_usage_eviction().await;
             debug!(tenant_id=%tl.tenant_shard_id.tenant_id, shard_id=%tl.tenant_shard_id.shard_slug(), timeline_id=%tl.timeline_id, "timeline resident layers count: {}", info.resident_layers.len());
-
             tenant_candidates.extend(info.resident_layers.into_iter());
             max_layer_size = max_layer_size.max(info.max_layer_size.unwrap_or(0));
 
@@ -923,25 +870,7 @@ async fn collect_eviction_candidates(
                     (partition, candidate)
                 });
 
-        METRICS
-            .tenant_layer_count
-            .observe(tenant_candidates.len() as f64);
-
         candidates.extend(tenant_candidates);
-
-        let elapsed = started_at.elapsed();
-        METRICS
-            .tenant_collection_time
-            .observe(elapsed.as_secs_f64());
-
-        if elapsed > LOG_DURATION_THRESHOLD {
-            tracing::info!(
-                tenant_id=%tenant.tenant_shard_id().tenant_id,
-                shard_id=%tenant.tenant_shard_id().shard_slug(),
-                elapsed_ms = elapsed.as_millis(),
-                "collection took longer than threshold"
-            );
-        }
     }
 
     // Note: the same tenant ID might be hit twice, if it transitions from attached to
@@ -956,11 +885,11 @@ async fn collect_eviction_candidates(
         },
     );
 
-    for tenant in secondary_tenants {
+    for secondary_tenant in secondary_tenants {
         // for secondary tenants we use a sum of on_disk layers and already evicted layers. this is
         // to prevent repeated disk usage based evictions from completely draining less often
         // updating secondaries.
-        let (mut layer_info, total_layers) = tenant.get_layers_for_eviction();
+        let (mut layer_info, total_layers) = secondary_tenant.get_layers_for_eviction();
 
         debug_assert!(
             total_layers >= layer_info.resident_layers.len(),
@@ -968,8 +897,6 @@ async fn collect_eviction_candidates(
             layer_info.resident_layers.len()
         );
 
-        let started_at = std::time::Instant::now();
-
         layer_info
             .resident_layers
             .sort_unstable_by_key(|layer_info| std::cmp::Reverse(layer_info.last_activity_ts));
@@ -991,27 +918,9 @@ async fn collect_eviction_candidates(
                     )
                 });
 
-        METRICS
-            .tenant_layer_count
-            .observe(tenant_candidates.len() as f64);
         candidates.extend(tenant_candidates);
 
         tokio::task::yield_now().await;
-
-        let elapsed = started_at.elapsed();
-
-        METRICS
-            .tenant_collection_time
-            .observe(elapsed.as_secs_f64());
-
-        if elapsed > LOG_DURATION_THRESHOLD {
-            tracing::info!(
-                tenant_id=%tenant.tenant_shard_id().tenant_id,
-                shard_id=%tenant.tenant_shard_id().shard_slug(),
-                elapsed_ms = elapsed.as_millis(),
-                "collection took longer than threshold"
-            );
-        }
     }
 
     debug_assert!(MinResidentSizePartition::Above < MinResidentSizePartition::Below,
@@ -1088,6 +997,30 @@ impl<U: Usage> VictimSelection<U> {
     }
 }
 
+struct TimelineKey(Arc<Timeline>);
+
+impl PartialEq for TimelineKey {
+    fn eq(&self, other: &Self) -> bool {
+        Arc::ptr_eq(&self.0, &other.0)
+    }
+}
+
+impl Eq for TimelineKey {}
+
+impl std::hash::Hash for TimelineKey {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        Arc::as_ptr(&self.0).hash(state);
+    }
+}
+
+impl std::ops::Deref for TimelineKey {
+    type Target = Timeline;
+
+    fn deref(&self) -> &Self::Target {
+        self.0.as_ref()
+    }
+}
+
 /// A totally ordered f32 subset we can use with sorting functions.
 pub(crate) mod finite_f32 {
 

pageserver/src/http/openapi_spec.yml

@@ -579,12 +579,6 @@ paths:
         required: false
         schema:
           type: integer
-      - name: lazy
-        in: query
-        required: false
-        schema:
-          type: boolean
-        description: Set to true for attaches to queue up until activated by compute. Eager (false) is the default.
     put:
       description: |
         Configures a _tenant location_, that is how a particular pageserver handles
@@ -1339,10 +1333,6 @@ components:
           type: array
           items:
             $ref: "#/components/schemas/TenantShardLocation"
-        stripe_size:
-          description: If multiple shards are present, this field contains the sharding stripe size, else it is null.
-          type: integer
-          nullable: true
     TenantShardLocation:
       type: object
       required:

pageserver/src/http/routes.rs

@@ -14,7 +14,6 @@ use hyper::header;
 use hyper::StatusCode;
 use hyper::{Body, Request, Response, Uri};
 use metrics::launch_timestamp::LaunchTimestamp;
-use pageserver_api::models::LocationConfig;
 use pageserver_api::models::LocationConfigListResponse;
 use pageserver_api::models::ShardParameters;
 use pageserver_api::models::TenantDetails;
@@ -817,7 +816,13 @@ async fn tenant_attach_handler(
 
     let tenant = state
         .tenant_manager
-        .upsert_location(tenant_shard_id, location_conf, None, SpawnMode::Eager, &ctx)
+        .upsert_location(
+            tenant_shard_id,
+            location_conf,
+            None,
+            SpawnMode::Normal,
+            &ctx,
+        )
         .await?;
 
     let Some(tenant) = tenant else {
@@ -1413,7 +1418,6 @@ async fn put_tenant_location_config_handler(
 
     let request_data: TenantLocationConfigRequest = json_request(&mut request).await?;
     let flush = parse_query_param(&request, "flush_ms")?.map(Duration::from_millis);
-    let lazy = parse_query_param(&request, "lazy")?.unwrap_or(false);
     check_permission(&request, Some(tenant_shard_id.tenant_id))?;
 
     let ctx = RequestContext::new(TaskKind::MgmtRequest, DownloadBehavior::Warn);
@@ -1444,20 +1448,17 @@ async fn put_tenant_location_config_handler(
     let location_conf =
         LocationConf::try_from(&request_data.config).map_err(ApiError::BadRequest)?;
 
-    // lazy==true queues up for activation or jumps the queue like normal when a compute connects,
-    // similar to at startup ordering.
-    let spawn_mode = if lazy {
-        tenant::SpawnMode::Lazy
-    } else {
-        tenant::SpawnMode::Eager
-    };
-
-    let tenant = state
+    let attached = state
         .tenant_manager
-        .upsert_location(tenant_shard_id, location_conf, flush, spawn_mode, &ctx)
-        .await?;
-    let stripe_size = tenant.as_ref().map(|t| t.get_shard_stripe_size());
-    let attached = tenant.is_some();
+        .upsert_location(
+            tenant_shard_id,
+            location_conf,
+            flush,
+            tenant::SpawnMode::Normal,
+            &ctx,
+        )
+        .await?
+        .is_some();
 
     if let Some(_flush_ms) = flush {
         match state
@@ -1479,20 +1480,12 @@ async fn put_tenant_location_config_handler(
     // This API returns a vector of pageservers where the tenant is attached: this is
     // primarily for use in the sharding service.  For compatibilty, we also return this
     // when called directly on a pageserver, but the payload is always zero or one shards.
-    let mut response = TenantLocationConfigResponse {
-        shards: Vec::new(),
-        stripe_size: None,
-    };
+    let mut response = TenantLocationConfigResponse { shards: Vec::new() };
     if attached {
         response.shards.push(TenantShardLocation {
             shard_id: tenant_shard_id,
             node_id: state.conf.id,
-        });
-        if tenant_shard_id.shard_count.count() > 1 {
-            // Stripe size should be set if we are attached
-            debug_assert!(stripe_size.is_some());
-            response.stripe_size = stripe_size;
-        }
+        })
     }
 
     json_response(StatusCode::OK, response)
@@ -1520,29 +1513,6 @@ async fn list_location_config_handler(
     json_response(StatusCode::OK, result)
 }
 
-async fn get_location_config_handler(
-    request: Request<Body>,
-    _cancel: CancellationToken,
-) -> Result<Response<Body>, ApiError> {
-    let state = get_state(&request);
-    let tenant_shard_id: TenantShardId = parse_request_param(&request, "tenant_shard_id")?;
-    let slot = state.tenant_manager.get(tenant_shard_id);
-
-    let Some(slot) = slot else {
-        return Err(ApiError::NotFound(
-            anyhow::anyhow!("Tenant shard not found").into(),
-        ));
-    };
-
-    let result: Option<LocationConfig> = match slot {
-        TenantSlot::Attached(t) => Some(t.get_location_conf()),
-        TenantSlot::Secondary(s) => Some(s.get_location_conf()),
-        TenantSlot::InProgress(_) => None,
-    };
-
-    json_response(StatusCode::OK, result)
-}
-
 // Do a time travel recovery on the given tenant/tenant shard. Tenant needs to be detached
 // (from all pageservers) as it invalidates consistency assumptions.
 async fn tenant_time_travel_remote_storage_handler(
@@ -2247,9 +2217,6 @@ pub fn make_router(
         .get("/v1/location_config", |r| {
             api_handler(r, list_location_config_handler)
         })
-        .get("/v1/location_config/:tenant_id", |r| {
-            api_handler(r, get_location_config_handler)
-        })
         .put(
             "/v1/tenant/:tenant_shard_id/time_travel_remote_storage",
             |r| api_handler(r, tenant_time_travel_remote_storage_handler),

pageserver/src/metrics.rs

@@ -11,7 +11,6 @@ use once_cell::sync::Lazy;
 use pageserver_api::shard::TenantShardId;
 use strum::{EnumCount, IntoEnumIterator, VariantNames};
 use strum_macros::{EnumVariantNames, IntoStaticStr};
-use tracing::warn;
 use utils::id::TimelineId;
 
 /// Prometheus histogram buckets (in seconds) for operations in the critical
@@ -1006,39 +1005,15 @@ impl GlobalAndPerTimelineHistogram {
     }
 }
 
-struct GlobalAndPerTimelineHistogramTimer<'a, 'c> {
+struct GlobalAndPerTimelineHistogramTimer<'a> {
     h: &'a GlobalAndPerTimelineHistogram,
-    ctx: &'c RequestContext,
     start: std::time::Instant,
-    op: SmgrQueryType,
 }
 
-impl<'a, 'c> Drop for GlobalAndPerTimelineHistogramTimer<'a, 'c> {
+impl<'a> Drop for GlobalAndPerTimelineHistogramTimer<'a> {
     fn drop(&mut self) {
         let elapsed = self.start.elapsed();
-        let ex_throttled = self
-            .ctx
-            .micros_spent_throttled
-            .close_and_checked_sub_from(elapsed);
-        let ex_throttled = match ex_throttled {
-            Ok(res) => res,
-            Err(error) => {
-                use utils::rate_limit::RateLimit;
-                static LOGGED: Lazy<Mutex<enum_map::EnumMap<SmgrQueryType, RateLimit>>> =
-                    Lazy::new(|| {
-                        Mutex::new(enum_map::EnumMap::from_array(std::array::from_fn(|_| {
-                            RateLimit::new(Duration::from_secs(10))
-                        })))
-                    });
-                let mut guard = LOGGED.lock().unwrap();
-                let rate_limit = &mut guard[self.op];
-                rate_limit.call(|| {
-                    warn!(op=?self.op, error, "error deducting time spent throttled; this message is logged at a global rate limit");
-                });
-                elapsed
-            }
-        };
-        self.h.observe(ex_throttled.as_secs_f64());
+        self.h.observe(elapsed.as_secs_f64());
     }
 }
 
@@ -1050,7 +1025,6 @@ impl<'a, 'c> Drop for GlobalAndPerTimelineHistogramTimer<'a, 'c> {
     strum_macros::EnumCount,
     strum_macros::EnumIter,
     strum_macros::FromRepr,
-    enum_map::Enum,
 )]
 #[strum(serialize_all = "snake_case")]
 pub enum SmgrQueryType {
@@ -1156,35 +1130,11 @@ impl SmgrQueryTimePerTimeline {
         });
         Self { metrics }
     }
-    pub(crate) fn start_timer<'c: 'a, 'a>(
-        &'a self,
-        op: SmgrQueryType,
-        ctx: &'c RequestContext,
-    ) -> impl Drop + '_ {
+    pub(crate) fn start_timer(&self, op: SmgrQueryType) -> impl Drop + '_ {
         let metric = &self.metrics[op as usize];
-        let start = Instant::now();
-        match ctx.micros_spent_throttled.open() {
-            Ok(()) => (),
-            Err(error) => {
-                use utils::rate_limit::RateLimit;
-                static LOGGED: Lazy<Mutex<enum_map::EnumMap<SmgrQueryType, RateLimit>>> =
-                    Lazy::new(|| {
-                        Mutex::new(enum_map::EnumMap::from_array(std::array::from_fn(|_| {
-                            RateLimit::new(Duration::from_secs(10))
-                        })))
-                    });
-                let mut guard = LOGGED.lock().unwrap();
-                let rate_limit = &mut guard[op];
-                rate_limit.call(|| {
-                    warn!(?op, error, "error opening micros_spent_throttled; this message is logged at a global rate limit");
-                });
-            }
-        }
         GlobalAndPerTimelineHistogramTimer {
             h: metric,
-            ctx,
-            start,
-            op,
+            start: std::time::Instant::now(),
         }
     }
 }
@@ -1195,11 +1145,6 @@ mod smgr_query_time_tests {
     use strum::IntoEnumIterator;
     use utils::id::{TenantId, TimelineId};
 
-    use crate::{
-        context::{DownloadBehavior, RequestContext},
-        task_mgr::TaskKind,
-    };
-
     // Regression test, we used hard-coded string constants before using an enum.
     #[test]
     fn op_label_name() {
@@ -1248,8 +1193,7 @@ mod smgr_query_time_tests {
             let (pre_global, pre_per_tenant_timeline) = get_counts();
             assert_eq!(pre_per_tenant_timeline, 0);
 
-            let ctx = RequestContext::new(TaskKind::UnitTest, DownloadBehavior::Download);
-            let timer = metrics.start_timer(*op, &ctx);
+            let timer = metrics.start_timer(*op);
             drop(timer);
 
             let (post_global, post_per_tenant_timeline) = get_counts();
@@ -1971,16 +1915,17 @@ impl Drop for TimelineMetrics {
         let tenant_id = &self.tenant_id;
         let timeline_id = &self.timeline_id;
         let shard_id = &self.shard_id;
-        let _ = LAST_RECORD_LSN.remove_label_values(&[tenant_id, shard_id, timeline_id]);
+        let _ = LAST_RECORD_LSN.remove_label_values(&[tenant_id, &shard_id, timeline_id]);
         {
             RESIDENT_PHYSICAL_SIZE_GLOBAL.sub(self.resident_physical_size_get());
-            let _ = RESIDENT_PHYSICAL_SIZE.remove_label_values(&[tenant_id, shard_id, timeline_id]);
+            let _ =
+                RESIDENT_PHYSICAL_SIZE.remove_label_values(&[tenant_id, &shard_id, timeline_id]);
         }
-        let _ = CURRENT_LOGICAL_SIZE.remove_label_values(&[tenant_id, shard_id, timeline_id]);
+        let _ = CURRENT_LOGICAL_SIZE.remove_label_values(&[tenant_id, &shard_id, timeline_id]);
         if let Some(metric) = Lazy::get(&DIRECTORY_ENTRIES_COUNT) {
-            let _ = metric.remove_label_values(&[tenant_id, shard_id, timeline_id]);
+            let _ = metric.remove_label_values(&[tenant_id, &shard_id, timeline_id]);
         }
-        let _ = EVICTIONS.remove_label_values(&[tenant_id, shard_id, timeline_id]);
+        let _ = EVICTIONS.remove_label_values(&[tenant_id, &shard_id, timeline_id]);
 
         self.evictions_with_low_residence_duration
             .write()
@@ -2529,64 +2474,6 @@ pub(crate) mod tenant_throttling {
     }
 }
 
-pub(crate) mod disk_usage_based_eviction {
-    use super::*;
-
-    pub(crate) struct Metrics {
-        pub(crate) tenant_collection_time: Histogram,
-        pub(crate) tenant_layer_count: Histogram,
-        pub(crate) layers_collected: IntCounter,
-        pub(crate) layers_selected: IntCounter,
-        pub(crate) layers_evicted: IntCounter,
-    }
-
-    impl Default for Metrics {
-        fn default() -> Self {
-            let tenant_collection_time = register_histogram!(
-                "pageserver_disk_usage_based_eviction_tenant_collection_seconds",
-                "Time spent collecting layers from a tenant -- not normalized by collected layer amount",
-                vec![0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1.0, 5.0, 10.0]
-            )
-            .unwrap();
-
-            let tenant_layer_count = register_histogram!(
-                "pageserver_disk_usage_based_eviction_tenant_collected_layers",
-                "Amount of layers gathered from a tenant",
-                vec![5.0, 50.0, 500.0, 5000.0, 50000.0]
-            )
-            .unwrap();
-
-            let layers_collected = register_int_counter!(
-                "pageserver_disk_usage_based_eviction_collected_layers_total",
-                "Amount of layers collected"
-            )
-            .unwrap();
-
-            let layers_selected = register_int_counter!(
-                "pageserver_disk_usage_based_eviction_select_layers_total",
-                "Amount of layers selected"
-            )
-            .unwrap();
-
-            let layers_evicted = register_int_counter!(
-                "pageserver_disk_usage_based_eviction_evicted_layers_total",
-                "Amount of layers successfully evicted"
-            )
-            .unwrap();
-
-            Self {
-                tenant_collection_time,
-                tenant_layer_count,
-                layers_collected,
-                layers_selected,
-                layers_evicted,
-            }
-        }
-    }
-
-    pub(crate) static METRICS: Lazy<Metrics> = Lazy::new(Metrics::default);
-}
-
 pub fn preinitialize_metrics() {
     // Python tests need these and on some we do alerting.
     //
@@ -2621,7 +2508,6 @@ pub fn preinitialize_metrics() {
     Lazy::force(&TENANT_MANAGER);
 
     Lazy::force(&crate::tenant::storage_layer::layer::LAYER_IMPL_METRICS);
-    Lazy::force(&disk_usage_based_eviction::METRICS);
 
     // countervecs
     [&BACKGROUND_LOOP_PERIOD_OVERRUN_COUNT]

pageserver/src/page_cache.rs

@@ -73,6 +73,7 @@
 
 use std::{
     collections::{hash_map::Entry, HashMap},
+    convert::TryInto,
     sync::{
         atomic::{AtomicU64, AtomicU8, AtomicUsize, Ordering},
         Arc, Weak,
@@ -261,9 +262,7 @@ pub struct PageCache {
     size_metrics: &'static PageCacheSizeMetrics,
 }
 
-struct PinnedSlotsPermit {
-    _permit: tokio::sync::OwnedSemaphorePermit,
-}
+struct PinnedSlotsPermit(tokio::sync::OwnedSemaphorePermit);
 
 ///
 /// PageReadGuard is a "lease" on a buffer, for reading. The page is kept locked
@@ -559,9 +558,9 @@ impl PageCache {
         )
         .await
         {
-            Ok(res) => Ok(PinnedSlotsPermit {
-                _permit: res.expect("this semaphore is never closed"),
-            }),
+            Ok(res) => Ok(PinnedSlotsPermit(
+                res.expect("this semaphore is never closed"),
+            )),
             Err(_timeout) => {
                 crate::metrics::page_cache_errors_inc(
                     crate::metrics::PageCacheErrorKind::AcquirePinnedSlotTimeout,

pageserver/src/page_service.rs

@@ -27,7 +27,7 @@ use pageserver_api::models::{
 };
 use pageserver_api::shard::ShardIndex;
 use pageserver_api::shard::ShardNumber;
-use postgres_backend::{is_expected_io_error, AuthType, PostgresBackend, QueryError};
+use postgres_backend::{self, is_expected_io_error, AuthType, PostgresBackend, QueryError};
 use pq_proto::framed::ConnectionError;
 use pq_proto::FeStartupPacket;
 use pq_proto::{BeMessage, FeMessage, RowDescriptor};
@@ -44,6 +44,7 @@ use tokio::io::AsyncWriteExt;
 use tokio::io::{AsyncRead, AsyncWrite};
 use tokio_util::io::StreamReader;
 use tokio_util::sync::CancellationToken;
+use tracing::field;
 use tracing::*;
 use utils::id::ConnectionId;
 use utils::sync::gate::GateGuard;
@@ -910,7 +911,7 @@ impl PageServerHandler {
         let timeline = self.get_timeline_shard_zero(tenant_id, timeline_id).await?;
         let _timer = timeline
             .query_metrics
-            .start_timer(metrics::SmgrQueryType::GetRelExists, ctx);
+            .start_timer(metrics::SmgrQueryType::GetRelExists);
 
         let latest_gc_cutoff_lsn = timeline.get_latest_gc_cutoff_lsn();
         let lsn =
@@ -938,7 +939,7 @@ impl PageServerHandler {
 
         let _timer = timeline
             .query_metrics
-            .start_timer(metrics::SmgrQueryType::GetRelSize, ctx);
+            .start_timer(metrics::SmgrQueryType::GetRelSize);
 
         let latest_gc_cutoff_lsn = timeline.get_latest_gc_cutoff_lsn();
         let lsn =
@@ -966,7 +967,7 @@ impl PageServerHandler {
 
         let _timer = timeline
             .query_metrics
-            .start_timer(metrics::SmgrQueryType::GetDbSize, ctx);
+            .start_timer(metrics::SmgrQueryType::GetDbSize);
 
         let latest_gc_cutoff_lsn = timeline.get_latest_gc_cutoff_lsn();
         let lsn =
@@ -1114,10 +1115,7 @@ impl PageServerHandler {
         ctx: &RequestContext,
     ) -> Result<PagestreamBeMessage, PageStreamError> {
         let timeline = match self.get_cached_timeline_for_page(req) {
-            Ok(tl) => {
-                set_tracing_field_shard_id(tl);
-                tl
-            }
+            Ok(tl) => tl,
             Err(key) => {
                 match self
                     .load_timeline_for_page(tenant_id, timeline_id, key)
@@ -1142,9 +1140,12 @@ impl PageServerHandler {
             }
         };
 
+        // load_timeline_for_page sets shard_id, but get_cached_timeline_for_page doesn't
+        set_tracing_field_shard_id(timeline);
+
         let _timer = timeline
             .query_metrics
-            .start_timer(metrics::SmgrQueryType::GetPageAtLsn, ctx);
+            .start_timer(metrics::SmgrQueryType::GetPageAtLsn);
 
         let latest_gc_cutoff_lsn = timeline.get_latest_gc_cutoff_lsn();
         let lsn =
@@ -1172,7 +1173,7 @@ impl PageServerHandler {
 
         let _timer = timeline
             .query_metrics
-            .start_timer(metrics::SmgrQueryType::GetSlruSegment, ctx);
+            .start_timer(metrics::SmgrQueryType::GetSlruSegment);
 
         let latest_gc_cutoff_lsn = timeline.get_latest_gc_cutoff_lsn();
         let lsn =

pageserver/src/pgdatadir_mapping.rs

@@ -15,7 +15,6 @@ use crate::walrecord::NeonWalRecord;
 use anyhow::{ensure, Context};
 use bytes::{Buf, Bytes, BytesMut};
 use enum_map::Enum;
-use itertools::Itertools;
 use pageserver_api::key::{
     dbdir_key_range, is_rel_block_key, is_slru_block_key, rel_block_to_key, rel_dir_to_key,
     rel_key_range, rel_size_to_key, relmap_file_key, slru_block_to_key, slru_dir_to_key,
@@ -1499,7 +1498,7 @@ impl<'a> DatadirModification<'a> {
             return Ok(());
         }
 
-        let mut writer = self.tline.writer().await;
+        let writer = self.tline.writer().await;
 
         // Flush relation and  SLRU data blocks, keep metadata.
         let mut retained_pending_updates = HashMap::<_, Vec<_>>::new();
@@ -1538,22 +1537,14 @@ impl<'a> DatadirModification<'a> {
     /// All the modifications in this atomic update are stamped by the specified LSN.
     ///
     pub async fn commit(&mut self, ctx: &RequestContext) -> anyhow::Result<()> {
-        let mut writer = self.tline.writer().await;
+        let writer = self.tline.writer().await;
 
         let pending_nblocks = self.pending_nblocks;
         self.pending_nblocks = 0;
 
         if !self.pending_updates.is_empty() {
-            // The put_batch call below expects expects the inputs to be sorted by Lsn,
-            // so we do that first.
-            let lsn_ordered_batch: Vec<(Key, Lsn, Value)> = self
-                .pending_updates
-                .drain()
-                .map(|(key, vals)| vals.into_iter().map(move |(lsn, val)| (key, lsn, val)))
-                .kmerge_by(|lhs, rhs| lhs.1 .0 < rhs.1 .0)
-                .collect();
-
-            writer.put_batch(lsn_ordered_batch, ctx).await?;
+            writer.put_batch(&self.pending_updates, ctx).await?;
+            self.pending_updates.clear();
         }
 
         if !self.pending_deletions.is_empty() {
@@ -1686,7 +1677,7 @@ struct RelDirectory {
     rels: HashSet<(Oid, u8)>,
 }
 
-#[derive(Debug, Serialize, Deserialize, Default, PartialEq)]
+#[derive(Debug, Serialize, Deserialize, Default)]
 pub(crate) struct AuxFilesDirectory {
     pub(crate) files: HashMap<String, Bytes>,
 }

pageserver/src/repository.rs

@@ -37,6 +37,7 @@ impl Value {
 mod test {
     use super::*;
 
+    use bytes::Bytes;
     use utils::bin_ser::BeSer;
 
     macro_rules! roundtrip {

pageserver/src/tenant.rs

@@ -22,7 +22,6 @@ use pageserver_api::models;
 use pageserver_api::models::TimelineState;
 use pageserver_api::models::WalRedoManagerStatus;
 use pageserver_api::shard::ShardIdentity;
-use pageserver_api::shard::ShardStripeSize;
 use pageserver_api::shard::TenantShardId;
 use remote_storage::DownloadError;
 use remote_storage::GenericRemoteStorage;
@@ -110,6 +109,7 @@ pub use pageserver_api::models::TenantState;
 use tokio::sync::Semaphore;
 
 static INIT_DB_SEMAPHORE: Lazy<Semaphore> = Lazy::new(|| Semaphore::new(8));
+use toml_edit;
 use utils::{
     crashsafe,
     generation::Generation,
@@ -152,6 +152,7 @@ pub(crate) mod ephemeral_file;
 pub mod layer_map;
 
 pub mod metadata;
+mod par_fsync;
 pub mod remote_timeline_client;
 pub mod storage_layer;
 
@@ -226,11 +227,7 @@ pub(crate) struct TenantPreload {
 /// When we spawn a tenant, there is a special mode for tenant creation that
 /// avoids trying to read anything from remote storage.
 pub(crate) enum SpawnMode {
-    /// Activate as soon as possible
-    Eager,
-    /// Lazy activation in the background, with the option to skip the queue if the need comes up
-    Lazy,
-    /// Tenant has been created during the lifetime of this process
+    Normal,
     Create,
 }
 
@@ -703,37 +700,41 @@ impl Tenant {
                     .and_then(|x| x.initial_tenant_load_remote.take());
 
                 enum AttachType<'a> {
-                    /// We are attaching this tenant lazily in the background.
-                    Warmup {
-                        _permit: tokio::sync::SemaphorePermit<'a>,
-                        during_startup: bool
-                    },
-                    /// We are attaching this tenant as soon as we can, because for example an
-                    /// endpoint tried to access it.
+                    // During pageserver startup, we are attaching this tenant lazily in the background
+                    Warmup(tokio::sync::SemaphorePermit<'a>),
+                    // During pageserver startup, we are attaching this tenant as soon as we can,
+                    // because a client tried to access it.
                     OnDemand,
-                    /// During normal operations after startup, we are attaching a tenant, and
-                    /// eager attach was requested.
+                    // During normal operations after startup, we are attaching a tenant.
                     Normal,
                 }
 
-                let attach_type = if matches!(mode, SpawnMode::Lazy) {
-                    // Before doing any I/O, wait for at least one of:
-                    // - A client attempting to access to this tenant (on-demand loading)
-                    // - A permit becoming available in the warmup semaphore (background warmup)
-
+                // Before doing any I/O, wait for either or:
+                // - A client to attempt to access to this tenant (on-demand loading)
+                // - A permit to become available in the warmup semaphore (background warmup)
+                //
+                // Some-ness of init_order is how we know if we're attaching during startup or later
+                // in process lifetime.
+                let attach_type = if init_order.is_some() {
                     tokio::select!(
-                        permit = tenant_clone.activate_now_sem.acquire() => {
-                            let _ = permit.expect("activate_now_sem is never closed");
+                        _ = tenant_clone.activate_now_sem.acquire() => {
                             tracing::info!("Activating tenant (on-demand)");
                             AttachType::OnDemand
                         },
-                        permit = conf.concurrent_tenant_warmup.inner().acquire() => {
-                            let _permit = permit.expect("concurrent_tenant_warmup semaphore is never closed");
-                            tracing::info!("Activating tenant (warmup)");
-                            AttachType::Warmup {
-                                _permit,
-                                during_startup: init_order.is_some()
+                        permit_result = conf.concurrent_tenant_warmup.inner().acquire() => {
+                            match permit_result {
+                                Ok(p) => {
+                                    tracing::info!("Activating tenant (warmup)");
+                                    AttachType::Warmup(p)
+                                }
+                                Err(_) => {
+                                    // This is unexpected: the warmup semaphore should stay alive
+                                    // for the lifetime of init_order.  Log a warning and proceed.
+                                    tracing::warn!("warmup_limit semaphore unexpectedly closed");
+                                    AttachType::Normal
+                                }
                             }
+
                         }
                         _ = tenant_clone.cancel.cancelled() => {
                             // This is safe, but should be pretty rare: it is interesting if a tenant
@@ -748,8 +749,6 @@ impl Tenant {
                         },
                     )
                 } else {
-                    // SpawnMode::{Create,Eager} always cause jumping ahead of the
-                    // concurrent_tenant_warmup queue
                     AttachType::Normal
                 };
 
@@ -757,7 +756,7 @@ impl Tenant {
                     (SpawnMode::Create, _) => {
                         None
                     },
-                    (SpawnMode::Eager | SpawnMode::Lazy, Some(remote_storage)) => {
+                    (SpawnMode::Normal, Some(remote_storage)) => {
                         let _preload_timer = TENANT.preload.start_timer();
                         let res = tenant_clone
                             .preload(remote_storage, task_mgr::shutdown_token())
@@ -770,7 +769,7 @@ impl Tenant {
                             }
                         }
                     }
-                    (_, None) => {
+                    (SpawnMode::Normal, None) => {
                         let _preload_timer = TENANT.preload.start_timer();
                         None
                     }
@@ -829,7 +828,7 @@ impl Tenant {
                 let attached = {
                     let _attach_timer = match mode {
                         SpawnMode::Create => None,
-                        SpawnMode::Eager | SpawnMode::Lazy => Some(TENANT.attach.start_timer()),
+                        SpawnMode::Normal => {Some(TENANT.attach.start_timer())}
                     };
                     tenant_clone.attach(preload, mode, &ctx).await
                 };
@@ -851,7 +850,7 @@ impl Tenant {
                 // It also prevents the warmup proccess competing with the concurrency limit on
                 // logical size calculations: if logical size calculation semaphore is saturated,
                 // then warmup will wait for that before proceeding to the next tenant.
-                if matches!(attach_type, AttachType::Warmup { during_startup: true, .. }) {
+                if let AttachType::Warmup(_permit) = attach_type {
                     let mut futs: FuturesUnordered<_> = tenant_clone.timelines.lock().unwrap().values().cloned().map(|t| t.await_initial_logical_size()).collect();
                     tracing::info!("Waiting for initial logical sizes while warming up...");
                     while futs.next().await.is_some() {}
@@ -924,7 +923,7 @@ impl Tenant {
                 deleting: false,
                 timelines: HashMap::new(),
             },
-            (None, _) => {
+            (None, SpawnMode::Normal) => {
                 anyhow::bail!("local-only deployment is no longer supported, https://github.com/neondatabase/neon/issues/5624");
             }
         };
@@ -2087,10 +2086,6 @@ impl Tenant {
         &self.tenant_shard_id
     }
 
-    pub(crate) fn get_shard_stripe_size(&self) -> ShardStripeSize {
-        self.shard_identity.stripe_size
-    }
-
     pub(crate) fn get_generation(&self) -> Generation {
         self.generation
     }
@@ -2387,7 +2382,7 @@ impl Tenant {
             self.tenant_shard_id,
             self.generation,
             self.shard_identity,
-            self.walredo_mgr.clone(),
+            self.walredo_mgr.as_ref().map(Arc::clone),
             resources,
             pg_version,
             state,
@@ -3596,18 +3591,25 @@ pub async fn dump_layerfile_from_path(
 #[cfg(test)]
 pub(crate) mod harness {
     use bytes::{Bytes, BytesMut};
+    use camino::Utf8PathBuf;
     use once_cell::sync::OnceCell;
     use pageserver_api::models::ShardParameters;
     use pageserver_api::shard::ShardIndex;
+    use std::fs;
+    use std::sync::Arc;
     use utils::logging;
+    use utils::lsn::Lsn;
 
     use crate::deletion_queue::mock::MockDeletionQueue;
     use crate::walredo::apply_neon;
-    use crate::{repository::Key, walrecord::NeonWalRecord};
+    use crate::{
+        config::PageServerConf, repository::Key, tenant::Tenant, walrecord::NeonWalRecord,
+    };
 
     use super::*;
+    use crate::tenant::config::{TenantConf, TenantConfOpt};
     use hex_literal::hex;
-    use utils::id::TenantId;
+    use utils::id::{TenantId, TimelineId};
 
     pub const TIMELINE_ID: TimelineId =
         TimelineId::from_array(hex!("11223344556677881122334455667788"));
@@ -3679,10 +3681,7 @@ pub(crate) mod harness {
     }
 
     impl TenantHarness {
-        pub fn create_custom(
-            test_name: &'static str,
-            tenant_conf: TenantConf,
-        ) -> anyhow::Result<Self> {
+        pub fn create(test_name: &'static str) -> anyhow::Result<Self> {
             setup_logging();
 
             let repo_dir = PageServerConf::test_repo_dir(test_name);
@@ -3694,6 +3693,14 @@ pub(crate) mod harness {
             // OK in a test.
             let conf: &'static PageServerConf = Box::leak(Box::new(conf));
 
+            // Disable automatic GC and compaction to make the unit tests more deterministic.
+            // The tests perform them manually if needed.
+            let tenant_conf = TenantConf {
+                gc_period: Duration::ZERO,
+                compaction_period: Duration::ZERO,
+                ..TenantConf::default()
+            };
+
             let tenant_id = TenantId::generate();
             let tenant_shard_id = TenantShardId::unsharded(tenant_id);
             fs::create_dir_all(conf.tenant_path(&tenant_shard_id))?;
@@ -3721,18 +3728,6 @@ pub(crate) mod harness {
             })
         }
 
-        pub fn create(test_name: &'static str) -> anyhow::Result<Self> {
-            // Disable automatic GC and compaction to make the unit tests more deterministic.
-            // The tests perform them manually if needed.
-            let tenant_conf = TenantConf {
-                gc_period: Duration::ZERO,
-                compaction_period: Duration::ZERO,
-                ..TenantConf::default()
-            };
-
-            Self::create_custom(test_name, tenant_conf)
-        }
-
         pub fn span(&self) -> tracing::Span {
             info_span!("TenantHarness", tenant_id=%self.tenant_shard_id.tenant_id, shard_id=%self.tenant_shard_id.shard_slug())
         }
@@ -3774,7 +3769,7 @@ pub(crate) mod harness {
             let preload = tenant
                 .preload(&self.remote_storage, CancellationToken::new())
                 .await?;
-            tenant.attach(Some(preload), SpawnMode::Eager, ctx).await?;
+            tenant.attach(Some(preload), SpawnMode::Normal, ctx).await?;
 
             tenant.state.send_replace(TenantState::Active);
             for timeline in tenant.timelines.lock().unwrap().values() {
@@ -3840,12 +3835,13 @@ mod tests {
     use crate::keyspace::KeySpaceAccum;
     use crate::repository::{Key, Value};
     use crate::tenant::harness::*;
-    use crate::tenant::timeline::CompactFlags;
     use crate::DEFAULT_PG_VERSION;
     use bytes::BytesMut;
     use hex_literal::hex;
+    use once_cell::sync::Lazy;
     use pageserver_api::keyspace::KeySpace;
     use rand::{thread_rng, Rng};
+    use tokio_util::sync::CancellationToken;
 
     static TEST_KEY: Lazy<Key> =
         Lazy::new(|| Key::from_slice(&hex!("010000000033333333444444445500000001")));
@@ -3857,7 +3853,7 @@ mod tests {
             .create_test_timeline(TIMELINE_ID, Lsn(0x08), DEFAULT_PG_VERSION, &ctx)
             .await?;
 
-        let mut writer = tline.writer().await;
+        let writer = tline.writer().await;
         writer
             .put(
                 *TEST_KEY,
@@ -3869,7 +3865,7 @@ mod tests {
         writer.finish_write(Lsn(0x10));
         drop(writer);
 
-        let mut writer = tline.writer().await;
+        let writer = tline.writer().await;
         writer
             .put(
                 *TEST_KEY,
@@ -3935,7 +3931,7 @@ mod tests {
         let tline = tenant
             .create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)
             .await?;
-        let mut writer = tline.writer().await;
+        let writer = tline.writer().await;
 
         #[allow(non_snake_case)]
         let TEST_KEY_A: Key = Key::from_hex("110000000033333333444444445500000001").unwrap();
@@ -3969,7 +3965,7 @@ mod tests {
         let newtline = tenant
             .get_timeline(NEW_TIMELINE_ID, true)
             .expect("Should have a local timeline");
-        let mut new_writer = newtline.writer().await;
+        let new_writer = newtline.writer().await;
         new_writer
             .put(TEST_KEY_A, Lsn(0x40), &test_value("bar at 0x40"), &ctx)
             .await?;
@@ -4001,7 +3997,7 @@ mod tests {
     ) -> anyhow::Result<()> {
         let mut lsn = start_lsn;
         {
-            let mut writer = tline.writer().await;
+            let writer = tline.writer().await;
             // Create a relation on the timeline
             writer
                 .put(
@@ -4026,7 +4022,7 @@ mod tests {
         }
         tline.freeze_and_flush().await?;
         {
-            let mut writer = tline.writer().await;
+            let writer = tline.writer().await;
             writer
                 .put(
                     *TEST_KEY,
@@ -4389,7 +4385,7 @@ mod tests {
             .create_test_timeline(TIMELINE_ID, Lsn(0x08), DEFAULT_PG_VERSION, &ctx)
             .await?;
 
-        let mut writer = tline.writer().await;
+        let writer = tline.writer().await;
         writer
             .put(
                 *TEST_KEY,
@@ -4406,7 +4402,7 @@ mod tests {
             .compact(&CancellationToken::new(), EnumSet::empty(), &ctx)
             .await?;
 
-        let mut writer = tline.writer().await;
+        let writer = tline.writer().await;
         writer
             .put(
                 *TEST_KEY,
@@ -4423,7 +4419,7 @@ mod tests {
             .compact(&CancellationToken::new(), EnumSet::empty(), &ctx)
             .await?;
 
-        let mut writer = tline.writer().await;
+        let writer = tline.writer().await;
         writer
             .put(
                 *TEST_KEY,
@@ -4440,7 +4436,7 @@ mod tests {
             .compact(&CancellationToken::new(), EnumSet::empty(), &ctx)
             .await?;
 
-        let mut writer = tline.writer().await;
+        let writer = tline.writer().await;
         writer
             .put(
                 *TEST_KEY,
@@ -4497,7 +4493,7 @@ mod tests {
         for _ in 0..repeat {
             for _ in 0..key_count {
                 test_key.field6 = blknum;
-                let mut writer = timeline.writer().await;
+                let writer = timeline.writer().await;
                 writer
                     .put(
                         test_key,
@@ -4645,145 +4641,6 @@ mod tests {
         Ok(())
     }
 
-    // Test that vectored get handles layer gaps correctly
-    // by advancing into the next ancestor timeline if required.
-    //
-    // The test generates timelines that look like the diagram below.
-    // We leave a gap in one of the L1 layers at `gap_at_key` (`/` in the diagram).
-    // The reconstruct data for that key lies in the ancestor timeline (`X` in the diagram).
-    //
-    // ```
-    //-------------------------------+
-    //                          ...  |
-    //               [   L1   ]      |
-    //     [ / L1   ]                | Child Timeline
-    // ...                           |
-    // ------------------------------+
-    //     [ X L1   ]                | Parent Timeline
-    // ------------------------------+
-    // ```
-    #[tokio::test]
-    async fn test_get_vectored_key_gap() -> anyhow::Result<()> {
-        let tenant_conf = TenantConf {
-            // Make compaction deterministic
-            gc_period: Duration::ZERO,
-            compaction_period: Duration::ZERO,
-            // Encourage creation of L1 layers
-            checkpoint_distance: 16 * 1024,
-            compaction_target_size: 8 * 1024,
-            ..TenantConf::default()
-        };
-
-        let harness = TenantHarness::create_custom("test_get_vectored_key_gap", tenant_conf)?;
-        let (tenant, ctx) = harness.load().await;
-
-        let mut current_key = Key::from_hex("010000000033333333444444445500000000").unwrap();
-        let gap_at_key = current_key.add(100);
-        let mut current_lsn = Lsn(0x10);
-
-        const KEY_COUNT: usize = 10_000;
-
-        let timeline_id = TimelineId::generate();
-        let current_timeline = tenant
-            .create_test_timeline(timeline_id, current_lsn, DEFAULT_PG_VERSION, &ctx)
-            .await?;
-
-        current_lsn += 0x100;
-
-        let mut writer = current_timeline.writer().await;
-        writer
-            .put(
-                gap_at_key,
-                current_lsn,
-                &Value::Image(test_img(&format!("{} at {}", gap_at_key, current_lsn))),
-                &ctx,
-            )
-            .await?;
-        writer.finish_write(current_lsn);
-        drop(writer);
-
-        let mut latest_lsns = HashMap::new();
-        latest_lsns.insert(gap_at_key, current_lsn);
-
-        current_timeline.freeze_and_flush().await?;
-
-        let child_timeline_id = TimelineId::generate();
-
-        tenant
-            .branch_timeline_test(
-                &current_timeline,
-                child_timeline_id,
-                Some(current_lsn),
-                &ctx,
-            )
-            .await?;
-        let child_timeline = tenant
-            .get_timeline(child_timeline_id, true)
-            .expect("Should have the branched timeline");
-
-        for i in 0..KEY_COUNT {
-            if current_key == gap_at_key {
-                current_key = current_key.next();
-                continue;
-            }
-
-            current_lsn += 0x10;
-
-            let mut writer = child_timeline.writer().await;
-            writer
-                .put(
-                    current_key,
-                    current_lsn,
-                    &Value::Image(test_img(&format!("{} at {}", current_key, current_lsn))),
-                    &ctx,
-                )
-                .await?;
-            writer.finish_write(current_lsn);
-            drop(writer);
-
-            latest_lsns.insert(current_key, current_lsn);
-            current_key = current_key.next();
-
-            // Flush every now and then to encourage layer file creation.
-            if i % 500 == 0 {
-                child_timeline.freeze_and_flush().await?;
-            }
-        }
-
-        child_timeline.freeze_and_flush().await?;
-        let mut flags = EnumSet::new();
-        flags.insert(CompactFlags::ForceRepartition);
-        child_timeline
-            .compact(&CancellationToken::new(), flags, &ctx)
-            .await?;
-
-        let key_near_end = {
-            let mut tmp = current_key;
-            tmp.field6 -= 10;
-            tmp
-        };
-
-        let key_near_gap = {
-            let mut tmp = gap_at_key;
-            tmp.field6 -= 10;
-            tmp
-        };
-
-        let read = KeySpace {
-            ranges: vec![key_near_gap..gap_at_key.next(), key_near_end..current_key],
-        };
-        let results = child_timeline
-            .get_vectored_impl(read.clone(), current_lsn, &ctx)
-            .await?;
-
-        for (key, img_res) in results {
-            let expected = test_img(&format!("{} at {}", key, latest_lsns[&key]));
-            assert_eq!(img_res?, expected);
-        }
-
-        Ok(())
-    }
-
     #[tokio::test]
     async fn test_random_updates() -> anyhow::Result<()> {
         let harness = TenantHarness::create("test_random_updates")?;
@@ -4807,7 +4664,7 @@ mod tests {
         for blknum in 0..NUM_KEYS {
             lsn = Lsn(lsn.0 + 0x10);
             test_key.field6 = blknum as u32;
-            let mut writer = tline.writer().await;
+            let writer = tline.writer().await;
             writer
                 .put(
                     test_key,
@@ -4828,7 +4685,7 @@ mod tests {
                 lsn = Lsn(lsn.0 + 0x10);
                 let blknum = thread_rng().gen_range(0..NUM_KEYS);
                 test_key.field6 = blknum as u32;
-                let mut writer = tline.writer().await;
+                let writer = tline.writer().await;
                 writer
                     .put(
                         test_key,
@@ -4896,7 +4753,7 @@ mod tests {
         for blknum in 0..NUM_KEYS {
             lsn = Lsn(lsn.0 + 0x10);
             test_key.field6 = blknum as u32;
-            let mut writer = tline.writer().await;
+            let writer = tline.writer().await;
             writer
                 .put(
                     test_key,
@@ -4925,7 +4782,7 @@ mod tests {
                 lsn = Lsn(lsn.0 + 0x10);
                 let blknum = thread_rng().gen_range(0..NUM_KEYS);
                 test_key.field6 = blknum as u32;
-                let mut writer = tline.writer().await;
+                let writer = tline.writer().await;
                 writer
                     .put(
                         test_key,
@@ -5002,7 +4859,7 @@ mod tests {
                 lsn = Lsn(lsn.0 + 0x10);
                 let blknum = thread_rng().gen_range(0..NUM_KEYS);
                 test_key.field6 = blknum as u32;
-                let mut writer = tline.writer().await;
+                let writer = tline.writer().await;
                 writer
                     .put(
                         test_key,

pageserver/src/tenant/blob_io.rs

@@ -12,7 +12,7 @@
 //! len >= 128: 1XXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
 //!
 use bytes::{BufMut, BytesMut};
-use tokio_epoll_uring::{BoundedBuf, IoBuf, Slice};
+use tokio_epoll_uring::{BoundedBuf, Slice};
 
 use crate::context::RequestContext;
 use crate::page_cache::PAGE_SZ;
@@ -127,7 +127,7 @@ impl<const BUFFERED: bool> BlobWriter<BUFFERED> {
     /// You need to make sure that the internal buffer is empty, otherwise
     /// data will be written in wrong order.
     #[inline(always)]
-    async fn write_all_unbuffered<B: BoundedBuf<Buf = Buf>, Buf: IoBuf + Send>(
+    async fn write_all_unbuffered<B: BoundedBuf>(
         &mut self,
         src_buf: B,
     ) -> (B::Buf, Result<(), Error>) {
@@ -162,10 +162,7 @@ impl<const BUFFERED: bool> BlobWriter<BUFFERED> {
     }
 
     /// Internal, possibly buffered, write function
-    async fn write_all<B: BoundedBuf<Buf = Buf>, Buf: IoBuf + Send>(
-        &mut self,
-        src_buf: B,
-    ) -> (B::Buf, Result<(), Error>) {
+    async fn write_all<B: BoundedBuf>(&mut self, src_buf: B) -> (B::Buf, Result<(), Error>) {
         if !BUFFERED {
             assert!(self.buf.is_empty());
             return self.write_all_unbuffered(src_buf).await;
@@ -213,10 +210,7 @@ impl<const BUFFERED: bool> BlobWriter<BUFFERED> {
 
     /// Write a blob of data. Returns the offset that it was written to,
     /// which can be used to retrieve the data later.
-    pub async fn write_blob<B: BoundedBuf<Buf = Buf>, Buf: IoBuf + Send>(
-        &mut self,
-        srcbuf: B,
-    ) -> (B::Buf, Result<u64, Error>) {
+    pub async fn write_blob<B: BoundedBuf>(&mut self, srcbuf: B) -> (B::Buf, Result<u64, Error>) {
         let offset = self.offset;
 
         let len = srcbuf.bytes_init();

pageserver/src/tenant/config.rs

@@ -52,10 +52,7 @@ pub mod defaults {
     pub const DEFAULT_PITR_INTERVAL: &str = "7 days";
     pub const DEFAULT_WALRECEIVER_CONNECT_TIMEOUT: &str = "10 seconds";
     pub const DEFAULT_WALRECEIVER_LAGGING_WAL_TIMEOUT: &str = "10 seconds";
-    // The default limit on WAL lag should be set to avoid causing disconnects under high throughput
-    // scenarios: since the broker stats are updated ~1/s, a value of 1GiB should be sufficient for
-    // throughputs up to 1GiB/s per timeline.
-    pub const DEFAULT_MAX_WALRECEIVER_LSN_WAL_LAG: u64 = 1024 * 1024 * 1024;
+    pub const DEFAULT_MAX_WALRECEIVER_LSN_WAL_LAG: u64 = 10 * 1024 * 1024;
     pub const DEFAULT_EVICTIONS_LOW_RESIDENCE_DURATION_METRIC_THRESHOLD: &str = "24 hour";
 
     pub const DEFAULT_INGEST_BATCH_SIZE: u64 = 100;

pageserver/src/tenant/delete.rs

@@ -420,7 +420,7 @@ impl DeleteTenantFlow {
             .expect("cant be stopping or broken");
 
         tenant
-            .attach(preload, super::SpawnMode::Eager, ctx)
+            .attach(preload, super::SpawnMode::Normal, ctx)
             .await
             .context("attach")?;
 

pageserver/src/tenant/disk_btree.rs

@@ -18,19 +18,11 @@
 //! - An Iterator interface would be more convenient for the callers than the
 //!   'visit' function
 //!
-use async_stream::try_stream;
 use byteorder::{ReadBytesExt, BE};
 use bytes::{BufMut, Bytes, BytesMut};
 use either::Either;
-use futures::Stream;
 use hex;
-use std::{
-    cmp::Ordering,
-    io,
-    iter::Rev,
-    ops::{Range, RangeInclusive},
-    result,
-};
+use std::{cmp::Ordering, io, result};
 use thiserror::Error;
 use tracing::error;
 
@@ -259,90 +251,6 @@ where
         Ok(result)
     }
 
-    /// Return a stream which yields all key, value pairs from the index
-    /// starting from the first key greater or equal to `start_key`.
-    ///
-    /// Note that this is a copy of [`Self::visit`].
-    /// TODO: Once the sequential read path is removed this will become
-    /// the only index traversal method.
-    pub fn get_stream_from<'a>(
-        &'a self,
-        start_key: &'a [u8; L],
-        ctx: &'a RequestContext,
-    ) -> impl Stream<Item = std::result::Result<(Vec<u8>, u64), DiskBtreeError>> + 'a {
-        try_stream! {
-            let mut stack = Vec::new();
-            stack.push((self.root_blk, None));
-            let block_cursor = self.reader.block_cursor();
-            while let Some((node_blknum, opt_iter)) = stack.pop() {
-                // Locate the node.
-                let node_buf = block_cursor
-                    .read_blk(self.start_blk + node_blknum, ctx)
-                    .await?;
-
-                let node = OnDiskNode::deparse(node_buf.as_ref())?;
-                let prefix_len = node.prefix_len as usize;
-                let suffix_len = node.suffix_len as usize;
-
-                assert!(node.num_children > 0);
-
-                let mut keybuf = Vec::new();
-                keybuf.extend(node.prefix);
-                keybuf.resize(prefix_len + suffix_len, 0);
-
-                let mut iter: Either<Range<usize>, Rev<RangeInclusive<usize>>> = if let Some(iter) = opt_iter {
-                    iter
-                } else {
-                    // Locate the first match
-                    let idx = match node.binary_search(start_key, keybuf.as_mut_slice()) {
-                        Ok(idx) => idx,
-                        Err(idx) => {
-                            if node.level == 0 {
-                                // Imagine that the node contains the following keys:
-                                //
-                                // 1
-                                // 3  <-- idx
-                                // 5
-                                //
-                                // If the search key is '2' and there is exact match,
-                                // the binary search would return the index of key
-                                // '3'. That's cool, '3' is the first key to return.
-                                idx
-                            } else {
-                                // This is an internal page, so each key represents a lower
-                                // bound for what's in the child page. If there is no exact
-                                // match, we have to return the *previous* entry.
-                                //
-                                // 1  <-- return this
-                                // 3  <-- idx
-                                // 5
-                                idx.saturating_sub(1)
-                            }
-                        }
-                    };
-                    Either::Left(idx..node.num_children.into())
-                };
-
-                // idx points to the first match now. Keep going from there
-                while let Some(idx) = iter.next() {
-                    let key_off = idx * suffix_len;
-                    let suffix = &node.keys[key_off..key_off + suffix_len];
-                    keybuf[prefix_len..].copy_from_slice(suffix);
-                    let value = node.value(idx);
-                    #[allow(clippy::collapsible_if)]
-                    if node.level == 0 {
-                        // leaf
-                        yield (keybuf.clone(), value.to_u64());
-                    } else {
-                        stack.push((node_blknum, Some(iter)));
-                        stack.push((value.to_blknum(), None));
-                        break;
-                    }
-                }
-            }
-        }
-    }
-
     ///
     /// Scan the tree, starting from 'search_key', in the given direction. 'visitor'
     /// will be called for every key >= 'search_key' (or <= 'search_key', if scanning
@@ -792,6 +700,8 @@ impl<const L: usize> BuildNode<L> {
 #[cfg(test)]
 pub(crate) mod tests {
     use super::*;
+    use crate::context::DownloadBehavior;
+    use crate::task_mgr::TaskKind;
     use crate::tenant::block_io::{BlockCursor, BlockLease, BlockReaderRef};
     use rand::Rng;
     use std::collections::BTreeMap;

pageserver/src/tenant/ephemeral_file.rs

@@ -300,7 +300,7 @@ mod tests {
     use super::*;
     use crate::context::DownloadBehavior;
     use crate::task_mgr::TaskKind;
-    use crate::tenant::block_io::BlockReaderRef;
+    use crate::tenant::block_io::{BlockCursor, BlockReaderRef};
     use rand::{thread_rng, RngCore};
     use std::fs;
     use std::str::FromStr;

pageserver/src/tenant/layer_map.rs

@@ -460,22 +460,15 @@ impl LayerMap {
         }
     }
 
-    pub fn range_search(&self, key_range: Range<Key>, end_lsn: Lsn) -> RangeSearchResult {
-        let version = match self.historic.get().unwrap().get_version(end_lsn.0 - 1) {
-            Some(version) => version,
-            None => {
-                let mut result = RangeSearchResult::new();
-                result.not_found.add_range(key_range);
-                return result;
-            }
-        };
+    pub fn range_search(&self, key_range: Range<Key>, end_lsn: Lsn) -> Option<RangeSearchResult> {
+        let version = self.historic.get().unwrap().get_version(end_lsn.0 - 1)?;
 
         let raw_range = key_range.start.to_i128()..key_range.end.to_i128();
         let delta_changes = version.delta_coverage.range_overlaps(&raw_range);
         let image_changes = version.image_coverage.range_overlaps(&raw_range);
 
         let collector = RangeSearchCollector::new(key_range, end_lsn, delta_changes, image_changes);
-        collector.collect()
+        Some(collector.collect())
     }
 
     /// Start a batch of updates, applied on drop
@@ -1002,13 +995,8 @@ mod tests {
         let layer_map = LayerMap::default();
         let range = Key::from_i128(100)..Key::from_i128(200);
 
-        let res = layer_map.range_search(range.clone(), Lsn(100));
-        assert_eq!(
-            res.not_found.to_keyspace(),
-            KeySpace {
-                ranges: vec![range]
-            }
-        );
+        let res = layer_map.range_search(range, Lsn(100));
+        assert!(res.is_none());
     }
 
     #[test]
@@ -1045,7 +1033,7 @@ mod tests {
         for start in 0..60 {
             for end in (start + 1)..60 {
                 let range = Key::from_i128(start)..Key::from_i128(end);
-                let result = layer_map.range_search(range.clone(), Lsn(100));
+                let result = layer_map.range_search(range.clone(), Lsn(100)).unwrap();
                 let expected = brute_force_range_search(&layer_map, range, Lsn(100));
 
                 assert_range_search_result_eq(result, expected);

pageserver/src/tenant/mgr.rs

@@ -595,7 +595,7 @@ pub async fn init_tenant_mgr(
             shard_identity,
             Some(init_order.clone()),
             &TENANTS,
-            SpawnMode::Lazy,
+            SpawnMode::Normal,
             &ctx,
         ) {
             Ok(tenant) => {
@@ -1106,9 +1106,9 @@ impl TenantManager {
 
                 // Edge case: if we were called with SpawnMode::Create, but a Tenant already existed, then
                 // the caller thinks they're creating but the tenant already existed.  We must switch to
-                // Eager mode so that when starting this Tenant we properly probe remote storage for timelines,
+                // Normal mode so that when starting this Tenant we properly probe remote storage for timelines,
                 // rather than assuming it to be empty.
-                spawn_mode = SpawnMode::Eager;
+                spawn_mode = SpawnMode::Normal;
             }
             Some(TenantSlot::Secondary(state)) => {
                 info!("Shutting down secondary tenant");
@@ -1300,7 +1300,7 @@ impl TenantManager {
             shard_identity,
             None,
             self.tenants,
-            SpawnMode::Eager,
+            SpawnMode::Normal,
             ctx,
         )?;
 
@@ -1358,16 +1358,6 @@ impl TenantManager {
         }
     }
 
-    pub(crate) fn get(&self, tenant_shard_id: TenantShardId) -> Option<TenantSlot> {
-        let locked = self.tenants.read().unwrap();
-        match &*locked {
-            TenantsMap::Initializing => None,
-            TenantsMap::Open(map) | TenantsMap::ShuttingDown(map) => {
-                map.get(&tenant_shard_id).cloned()
-            }
-        }
-    }
-
     pub(crate) async fn delete_tenant(
         &self,
         tenant_shard_id: TenantShardId,
@@ -1531,7 +1521,7 @@ impl TenantManager {
                 *child_shard,
                 child_location_conf,
                 None,
-                SpawnMode::Eager,
+                SpawnMode::Normal,
                 ctx,
             )
             .await?;
@@ -2074,7 +2064,7 @@ pub(crate) async fn load_tenant(
         shard_identity,
         None,
         &TENANTS,
-        SpawnMode::Eager,
+        SpawnMode::Normal,
         ctx,
     )
     .with_context(|| format!("Failed to schedule tenant processing in path {tenant_path:?}"))?;
@@ -2658,7 +2648,7 @@ pub(crate) async fn immediate_gc(
 
     let tenant = guard
         .get(&tenant_shard_id)
-        .cloned()
+        .map(Arc::clone)
         .with_context(|| format!("tenant {tenant_shard_id}"))
         .map_err(|e| ApiError::NotFound(e.into()))?;
 

pageserver/src/tenant/par_fsync.rs

@@ -0,0 +1,84 @@
+use std::{
+    io,
+    sync::atomic::{AtomicUsize, Ordering},
+};
+
+use camino::{Utf8Path, Utf8PathBuf};
+
+fn fsync_path(path: &Utf8Path) -> io::Result<()> {
+    // TODO use VirtualFile::fsync_all once we fully go async.
+    let file = std::fs::File::open(path)?;
+    file.sync_all()
+}
+
+fn parallel_worker(paths: &[Utf8PathBuf], next_path_idx: &AtomicUsize) -> io::Result<()> {
+    while let Some(path) = paths.get(next_path_idx.fetch_add(1, Ordering::Relaxed)) {
+        fsync_path(path)?;
+    }
+
+    Ok(())
+}
+
+fn fsync_in_thread_pool(paths: &[Utf8PathBuf]) -> io::Result<()> {
+    // TODO: remove this function in favor of `par_fsync_async` once we asyncify everything.
+
+    /// Use at most this number of threads.
+    /// Increasing this limit will
+    /// - use more memory
+    /// - increase the cost of spawn/join latency
+    const MAX_NUM_THREADS: usize = 64;
+    let num_threads = paths.len().min(MAX_NUM_THREADS);
+    let next_path_idx = AtomicUsize::new(0);
+
+    std::thread::scope(|s| -> io::Result<()> {
+        let mut handles = vec![];
+        // Spawn `num_threads - 1`, as the current thread is also a worker.
+        for _ in 1..num_threads {
+            handles.push(s.spawn(|| parallel_worker(paths, &next_path_idx)));
+        }
+
+        parallel_worker(paths, &next_path_idx)?;
+
+        for handle in handles {
+            handle.join().unwrap()?;
+        }
+
+        Ok(())
+    })
+}
+
+/// Parallel fsync all files. Can be used in non-async context as it is using rayon thread pool.
+pub fn par_fsync(paths: &[Utf8PathBuf]) -> io::Result<()> {
+    if paths.len() == 1 {
+        fsync_path(&paths[0])?;
+        return Ok(());
+    }
+
+    fsync_in_thread_pool(paths)
+}
+
+/// Parallel fsync asynchronously.
+pub async fn par_fsync_async(paths: &[Utf8PathBuf]) -> io::Result<()> {
+    const MAX_CONCURRENT_FSYNC: usize = 64;
+    let mut next = paths.iter().peekable();
+    let mut js = tokio::task::JoinSet::new();
+    loop {
+        while js.len() < MAX_CONCURRENT_FSYNC && next.peek().is_some() {
+            let next = next.next().expect("just peeked");
+            let next = next.to_owned();
+            js.spawn_blocking(move || fsync_path(&next));
+        }
+
+        // now the joinset has been filled up, wait for next to complete
+        if let Some(res) = js.join_next().await {
+            res??;
+        } else {
+            // last item had already completed
+            assert!(
+                next.peek().is_none(),
+                "joinset emptied, we shouldn't have more work"
+            );
+            return Ok(());
+        }
+    }
+}

pageserver/src/tenant/remote_timeline_client.rs

@@ -1791,12 +1791,14 @@ mod tests {
         context::RequestContext,
         tenant::{
             harness::{TenantHarness, TIMELINE_ID},
-            Tenant, Timeline,
+            storage_layer::Layer,
+            Generation, Tenant, Timeline,
         },
         DEFAULT_PG_VERSION,
     };
 
     use std::collections::HashSet;
+    use utils::lsn::Lsn;
 
     pub(super) fn dummy_contents(name: &str) -> Vec<u8> {
         format!("contents for {name}").into()

pageserver/src/tenant/remote_timeline_client/download.rs

@@ -14,14 +14,14 @@ use tokio::io::{AsyncSeekExt, AsyncWriteExt};
 use tokio_util::io::StreamReader;
 use tokio_util::sync::CancellationToken;
 use tracing::warn;
-use utils::backoff;
+use utils::{backoff, crashsafe};
 
 use crate::config::PageServerConf;
 use crate::span::debug_assert_current_span_has_tenant_and_timeline_id;
 use crate::tenant::remote_timeline_client::{remote_layer_path, remote_timelines_path};
 use crate::tenant::storage_layer::LayerFileName;
 use crate::tenant::Generation;
-use crate::virtual_file::{on_fatal_io_error, MaybeFatalIo, VirtualFile};
+use crate::virtual_file::on_fatal_io_error;
 use crate::TEMP_FILE_SUFFIX;
 use remote_storage::{DownloadError, GenericRemoteStorage, ListingMode};
 use utils::crashsafe::path_with_suffix_extension;
@@ -50,8 +50,9 @@ pub async fn download_layer_file<'a>(
 ) -> Result<u64, DownloadError> {
     debug_assert_current_span_has_tenant_and_timeline_id();
 
-    let timeline_path = conf.timeline_path(&tenant_shard_id, &timeline_id);
-    let local_path = timeline_path.join(layer_file_name.file_name());
+    let local_path = conf
+        .timeline_path(&tenant_shard_id, &timeline_id)
+        .join(layer_file_name.file_name());
 
     let remote_path = remote_layer_path(
         &tenant_shard_id.tenant_id,
@@ -148,21 +149,10 @@ pub async fn download_layer_file<'a>(
         .with_context(|| format!("rename download layer file to {local_path}"))
         .map_err(DownloadError::Other)?;
 
-    // We use fatal_err() below because the after the rename above,
-    // the in-memory state of the filesystem already has the layer file in its final place,
-    // and subsequent pageserver code could think it's durable while it really isn't.
-    let work = async move {
-        let timeline_dir = VirtualFile::open(&timeline_path)
-            .await
-            .fatal_err("VirtualFile::open for timeline dir fsync");
-        timeline_dir
-            .sync_all()
-            .await
-            .fatal_err("VirtualFile::sync_all timeline dir");
-    };
-    crate::virtual_file::io_engine::get()
-        .spawn_blocking_and_block_on_if_std(work)
-        .await;
+    crashsafe::fsync_async(&local_path)
+        .await
+        .with_context(|| format!("fsync layer file {local_path}"))
+        .map_err(DownloadError::Other)?;
 
     tracing::debug!("download complete: {local_path}");
 
@@ -171,7 +161,7 @@ pub async fn download_layer_file<'a>(
 
 const TEMP_DOWNLOAD_EXTENSION: &str = "temp_download";
 
-pub(crate) fn is_temp_download_file(path: &Utf8Path) -> bool {
+pub fn is_temp_download_file(path: &Utf8Path) -> bool {
     let extension = path.extension();
     match extension {
         Some(TEMP_DOWNLOAD_EXTENSION) => true,

pageserver/src/tenant/secondary.rs

@@ -32,7 +32,7 @@ use remote_storage::GenericRemoteStorage;
 
 use tokio_util::sync::CancellationToken;
 use tracing::instrument;
-use utils::{completion::Barrier, id::TimelineId, sync::gate::Gate};
+use utils::{completion::Barrier, fs_ext, id::TimelineId, sync::gate::Gate};
 
 enum DownloadCommand {
     Download(TenantShardId),
@@ -121,10 +121,6 @@ impl SecondaryTenant {
         })
     }
 
-    pub(crate) fn tenant_shard_id(&self) -> TenantShardId {
-        self.tenant_shard_id
-    }
-
     pub(crate) async fn shutdown(&self) {
         self.cancel.cancel();
 
@@ -168,17 +164,16 @@ impl SecondaryTenant {
         self.detail.lock().unwrap().get_layers_for_eviction(self)
     }
 
-    /// Cancellation safe, but on cancellation the eviction will go through
     #[instrument(skip_all, fields(tenant_id=%self.tenant_shard_id.tenant_id, shard_id=%self.tenant_shard_id.shard_slug(), timeline_id=%timeline_id, name=%name))]
     pub(crate) async fn evict_layer(
-        self: &Arc<Self>,
+        &self,
         conf: &PageServerConf,
         timeline_id: TimelineId,
         name: LayerFileName,
     ) {
         debug_assert_current_span_has_tenant_id();
 
-        let guard = match self.gate.enter() {
+        let _guard = match self.gate.enter() {
             Ok(g) => g,
             Err(_) => {
                 tracing::debug!("Dropping layer evictions, secondary tenant shutting down",);
@@ -192,57 +187,35 @@ impl SecondaryTenant {
             .timeline_path(&self.tenant_shard_id, &timeline_id)
             .join(name.file_name());
 
-        let this = self.clone();
+        // We tolerate ENOENT, because between planning eviction and executing
+        // it, the secondary downloader could have seen an updated heatmap that
+        // resulted in a layer being deleted.
+        // Other local I/O errors are process-fatal: these should never happen.
+        tokio::fs::remove_file(path)
+            .await
+            .or_else(fs_ext::ignore_not_found)
+            .fatal_err("Deleting layer during eviction");
 
-        // spawn it to be cancellation safe
-        tokio::task::spawn_blocking(move || {
-            let _guard = guard;
-            // We tolerate ENOENT, because between planning eviction and executing
-            // it, the secondary downloader could have seen an updated heatmap that
-            // resulted in a layer being deleted.
-            // Other local I/O errors are process-fatal: these should never happen.
-            let deleted = std::fs::remove_file(path);
-
-            let not_found = deleted
-                .as_ref()
-                .is_err_and(|x| x.kind() == std::io::ErrorKind::NotFound);
-
-            let deleted = if not_found {
-                false
-            } else {
-                deleted
-                    .map(|()| true)
-                    .fatal_err("Deleting layer during eviction")
-            };
-
-            if !deleted {
-                // skip updating accounting and putting perhaps later timestamp
-                return;
-            }
-
-            // Update the timeline's state.  This does not have to be synchronized with
-            // the download process, because:
-            // - If downloader is racing with us to remove a file (e.g. because it is
-            //   removed from heatmap), then our mutual .remove() operations will both
-            //   succeed.
-            // - If downloader is racing with us to download the object (this would require
-            //   multiple eviction iterations to race with multiple download iterations), then
-            //   if we remove it from the state, the worst that happens is the downloader
-            //   downloads it again before re-inserting, or we delete the file but it remains
-            //   in the state map (in which case it will be downloaded if this secondary
-            //   tenant transitions to attached and tries to access it)
-            //
-            // The important assumption here is that the secondary timeline state does not
-            // have to 100% match what is on disk, because it's a best-effort warming
-            // of the cache.
-            let mut detail = this.detail.lock().unwrap();
-            if let Some(timeline_detail) = detail.timelines.get_mut(&timeline_id) {
-                timeline_detail.on_disk_layers.remove(&name);
-                timeline_detail.evicted_at.insert(name, now);
-            }
-        })
-        .await
-        .expect("secondary eviction should not have panicked");
+        // Update the timeline's state.  This does not have to be synchronized with
+        // the download process, because:
+        // - If downloader is racing with us to remove a file (e.g. because it is
+        //   removed from heatmap), then our mutual .remove() operations will both
+        //   succeed.
+        // - If downloader is racing with us to download the object (this would require
+        //   multiple eviction iterations to race with multiple download iterations), then
+        //   if we remove it from the state, the worst that happens is the downloader
+        //   downloads it again before re-inserting, or we delete the file but it remains
+        //   in the state map (in which case it will be downloaded if this secondary
+        //   tenant transitions to attached and tries to access it)
+        //
+        // The important assumption here is that the secondary timeline state does not
+        // have to 100% match what is on disk, because it's a best-effort warming
+        // of the cache.
+        let mut detail = self.detail.lock().unwrap();
+        if let Some(timeline_detail) = detail.timelines.get_mut(&timeline_id) {
+            timeline_detail.on_disk_layers.remove(&name);
+            timeline_detail.evicted_at.insert(name, now);
+        }
     }
 }
 

pageserver/src/tenant/secondary/downloader.rs

@@ -16,8 +16,7 @@ use crate::{
         config::SecondaryLocationConfig,
         debug_assert_current_span_has_tenant_and_timeline_id,
         remote_timeline_client::{
-            index::LayerFileMetadata, is_temp_download_file, FAILED_DOWNLOAD_WARN_THRESHOLD,
-            FAILED_REMOTE_OP_RETRIES,
+            index::LayerFileMetadata, FAILED_DOWNLOAD_WARN_THRESHOLD, FAILED_REMOTE_OP_RETRIES,
         },
         span::debug_assert_current_span_has_tenant_id,
         storage_layer::LayerFileName,
@@ -789,7 +788,7 @@ async fn init_timeline_state(
             // Secondary mode doesn't use local metadata files, but they might have been left behind by an attached tenant.
             warn!(path=?dentry.path(), "found legacy metadata file, these should have been removed in load_tenant_config");
             continue;
-        } else if crate::is_temporary(&file_path) || is_temp_download_file(&file_path) {
+        } else if crate::is_temporary(&file_path) {
             // Temporary files are frequently left behind from restarting during downloads
             tracing::info!("Cleaning up temporary file {file_path}");
             if let Err(e) = tokio::fs::remove_file(&file_path)

pageserver/src/tenant/secondary/heatmap_uploader.rs

@@ -18,6 +18,7 @@ use crate::{
 };
 
 use futures::Future;
+use md5;
 use pageserver_api::shard::TenantShardId;
 use rand::Rng;
 use remote_storage::{GenericRemoteStorage, TimeoutOrCancel};

pageserver/src/tenant/storage_layer.rs

@@ -72,7 +72,7 @@ where
 /// the same ValueReconstructState struct in the next 'get_value_reconstruct_data'
 /// call, to collect more records.
 ///
-#[derive(Debug, Default)]
+#[derive(Debug)]
 pub struct ValueReconstructState {
     pub records: Vec<(Lsn, NeonWalRecord)>,
     pub img: Option<(Lsn, Bytes)>,

pageserver/src/tenant/storage_layer/delta_layer.rs

@@ -46,7 +46,6 @@ use crate::{DELTA_FILE_MAGIC, STORAGE_FORMAT_VERSION};
 use anyhow::{anyhow, bail, ensure, Context, Result};
 use bytes::BytesMut;
 use camino::{Utf8Path, Utf8PathBuf};
-use futures::StreamExt;
 use pageserver_api::keyspace::KeySpace;
 use pageserver_api::models::LayerAccessKind;
 use pageserver_api::shard::TenantShardId;
@@ -848,33 +847,10 @@ impl DeltaLayerInner {
         reconstruct_state: &mut ValuesReconstructState,
         ctx: &RequestContext,
     ) -> Result<(), GetVectoredError> {
-        let block_reader = FileBlockReader::new(&self.file, self.file_id);
-        let index_reader = DiskBtreeReader::<_, DELTA_KEY_SIZE>::new(
-            self.index_start_blk,
-            self.index_root_blk,
-            block_reader,
-        );
-
-        let planner = VectoredReadPlanner::new(
-            self.max_vectored_read_bytes
-                .expect("Layer is loaded with max vectored bytes config")
-                .0
-                .into(),
-        );
-
-        let data_end_offset = self.index_start_blk as u64 * PAGE_SZ as u64;
-
-        let reads = Self::plan_reads(
-            keyspace,
-            lsn_range,
-            data_end_offset,
-            index_reader,
-            planner,
-            reconstruct_state,
-            ctx,
-        )
-        .await
-        .map_err(GetVectoredError::Other)?;
+        let reads = self
+            .plan_reads(keyspace, lsn_range, reconstruct_state, ctx)
+            .await
+            .map_err(GetVectoredError::Other)?;
 
         self.do_reads_and_update_state(reads, reconstruct_state)
             .await;
@@ -882,64 +858,73 @@ impl DeltaLayerInner {
         Ok(())
     }
 
-    async fn plan_reads<Reader>(
+    async fn plan_reads(
+        &self,
         keyspace: KeySpace,
         lsn_range: Range<Lsn>,
-        data_end_offset: u64,
-        index_reader: DiskBtreeReader<Reader, DELTA_KEY_SIZE>,
-        mut planner: VectoredReadPlanner,
         reconstruct_state: &mut ValuesReconstructState,
         ctx: &RequestContext,
-    ) -> anyhow::Result<Vec<VectoredRead>>
-    where
-        Reader: BlockReader,
-    {
-        let ctx = RequestContextBuilder::extend(ctx)
-            .page_content_kind(PageContentKind::DeltaLayerBtreeNode)
-            .build();
+    ) -> anyhow::Result<Vec<VectoredRead>> {
+        let mut planner = VectoredReadPlanner::new(
+            self.max_vectored_read_bytes
+                .expect("Layer is loaded with max vectored bytes config")
+                .0
+                .into(),
+        );
+
+        let block_reader = FileBlockReader::new(&self.file, self.file_id);
+        let tree_reader = DiskBtreeReader::<_, DELTA_KEY_SIZE>::new(
+            self.index_start_blk,
+            self.index_root_blk,
+            block_reader,
+        );
 
         for range in keyspace.ranges.iter() {
             let mut range_end_handled = false;
 
             let start_key = DeltaKey::from_key_lsn(&range.start, lsn_range.start);
-            let index_stream = index_reader.get_stream_from(&start_key.0, &ctx);
-            let mut index_stream = std::pin::pin!(index_stream);
+            tree_reader
+                .visit(
+                    &start_key.0,
+                    VisitDirection::Forwards,
+                    |raw_key, value| {
+                        let key = Key::from_slice(&raw_key[..KEY_SIZE]);
+                        let lsn = DeltaKey::extract_lsn_from_buf(raw_key);
+                        let blob_ref = BlobRef(value);
 
-            while let Some(index_entry) = index_stream.next().await {
-                let (raw_key, value) = index_entry?;
-                let key = Key::from_slice(&raw_key[..KEY_SIZE]);
-                let lsn = DeltaKey::extract_lsn_from_buf(&raw_key);
-                let blob_ref = BlobRef(value);
+                        assert!(key >= range.start && lsn >= lsn_range.start);
 
-                // Lsns are not monotonically increasing across keys, so we don't assert on them.
-                assert!(key >= range.start);
+                        let cached_lsn = reconstruct_state.get_cached_lsn(&key);
+                        let flag = {
+                            if cached_lsn >= Some(lsn) {
+                                BlobFlag::Ignore
+                            } else if blob_ref.will_init() {
+                                BlobFlag::Replaces
+                            } else {
+                                BlobFlag::None
+                            }
+                        };
 
-                let outside_lsn_range = !lsn_range.contains(&lsn);
-                let below_cached_lsn = reconstruct_state.get_cached_lsn(&key) >= Some(lsn);
-
-                let flag = {
-                    if outside_lsn_range || below_cached_lsn {
-                        BlobFlag::Ignore
-                    } else if blob_ref.will_init() {
-                        BlobFlag::ReplaceAll
-                    } else {
-                        // Usual path: add blob to the read
-                        BlobFlag::None
-                    }
-                };
-
-                if key >= range.end || (key.next() == range.end && lsn >= lsn_range.end) {
-                    planner.handle_range_end(blob_ref.pos());
-                    range_end_handled = true;
-                    break;
-                } else {
-                    planner.handle(key, lsn, blob_ref.pos(), flag);
-                }
-            }
+                        if key >= range.end || (key.next() == range.end && lsn >= lsn_range.end) {
+                            planner.handle_range_end(blob_ref.pos());
+                            range_end_handled = true;
+                            false
+                        } else {
+                            planner.handle(key, lsn, blob_ref.pos(), flag);
+                            true
+                        }
+                    },
+                    &RequestContextBuilder::extend(ctx)
+                        .page_content_kind(PageContentKind::DeltaLayerBtreeNode)
+                        .build(),
+                )
+                .await
+                .map_err(|err| anyhow!(err))?;
 
             if !range_end_handled {
-                tracing::info!("Handling range end fallback at {}", data_end_offset);
-                planner.handle_range_end(data_end_offset);
+                let payload_end = self.index_start_blk as u64 * PAGE_SZ as u64;
+                tracing::info!("Handling range end fallback at {}", payload_end);
+                planner.handle_range_end(payload_end);
             }
         }
 
@@ -1205,131 +1190,3 @@ impl<'a> pageserver_compaction::interface::CompactionDeltaEntry<'a, Key> for Del
         self.size
     }
 }
-
-#[cfg(test)]
-mod test {
-    use std::collections::BTreeMap;
-
-    use super::*;
-    use crate::{
-        context::DownloadBehavior, task_mgr::TaskKind, tenant::disk_btree::tests::TestDisk,
-    };
-
-    /// Construct an index for a fictional delta layer and and then
-    /// traverse in order to plan vectored reads for a query. Finally,
-    /// verify that the traversal fed the right index key and value
-    /// pairs into the planner.
-    #[tokio::test]
-    async fn test_delta_layer_index_traversal() {
-        let base_key = Key {
-            field1: 0,
-            field2: 1663,
-            field3: 12972,
-            field4: 16396,
-            field5: 0,
-            field6: 246080,
-        };
-
-        // Populate the index with some entries
-        let entries: BTreeMap<Key, Vec<Lsn>> = BTreeMap::from([
-            (base_key, vec![Lsn(1), Lsn(5), Lsn(25), Lsn(26), Lsn(28)]),
-            (base_key.add(1), vec![Lsn(2), Lsn(5), Lsn(10), Lsn(50)]),
-            (base_key.add(2), vec![Lsn(2), Lsn(5), Lsn(10), Lsn(50)]),
-            (base_key.add(5), vec![Lsn(10), Lsn(15), Lsn(16), Lsn(20)]),
-        ]);
-
-        let mut disk = TestDisk::default();
-        let mut writer = DiskBtreeBuilder::<_, DELTA_KEY_SIZE>::new(&mut disk);
-
-        let mut disk_offset = 0;
-        for (key, lsns) in &entries {
-            for lsn in lsns {
-                let index_key = DeltaKey::from_key_lsn(key, *lsn);
-                let blob_ref = BlobRef::new(disk_offset, false);
-                writer
-                    .append(&index_key.0, blob_ref.0)
-                    .expect("In memory disk append should never fail");
-
-                disk_offset += 1;
-            }
-        }
-
-        // Prepare all the arguments for the call into `plan_reads` below
-        let (root_offset, _writer) = writer
-            .finish()
-            .expect("In memory disk finish should never fail");
-        let reader = DiskBtreeReader::<_, DELTA_KEY_SIZE>::new(0, root_offset, disk);
-        let planner = VectoredReadPlanner::new(100);
-        let mut reconstruct_state = ValuesReconstructState::new();
-        let ctx = RequestContext::new(TaskKind::UnitTest, DownloadBehavior::Error);
-
-        let keyspace = KeySpace {
-            ranges: vec![
-                base_key..base_key.add(3),
-                base_key.add(3)..base_key.add(100),
-            ],
-        };
-        let lsn_range = Lsn(2)..Lsn(40);
-
-        // Plan and validate
-        let vectored_reads = DeltaLayerInner::plan_reads(
-            keyspace.clone(),
-            lsn_range.clone(),
-            disk_offset,
-            reader,
-            planner,
-            &mut reconstruct_state,
-            &ctx,
-        )
-        .await
-        .expect("Read planning should not fail");
-
-        validate(keyspace, lsn_range, vectored_reads, entries);
-    }
-
-    fn validate(
-        keyspace: KeySpace,
-        lsn_range: Range<Lsn>,
-        vectored_reads: Vec<VectoredRead>,
-        index_entries: BTreeMap<Key, Vec<Lsn>>,
-    ) {
-        #[derive(Debug, PartialEq, Eq)]
-        struct BlobSpec {
-            key: Key,
-            lsn: Lsn,
-            at: u64,
-        }
-
-        let mut planned_blobs = Vec::new();
-        for read in vectored_reads {
-            for (at, meta) in read.blobs_at.as_slice() {
-                planned_blobs.push(BlobSpec {
-                    key: meta.key,
-                    lsn: meta.lsn,
-                    at: *at,
-                });
-            }
-        }
-
-        let mut expected_blobs = Vec::new();
-        let mut disk_offset = 0;
-        for (key, lsns) in index_entries {
-            for lsn in lsns {
-                let key_included = keyspace.ranges.iter().any(|range| range.contains(&key));
-                let lsn_included = lsn_range.contains(&lsn);
-
-                if key_included && lsn_included {
-                    expected_blobs.push(BlobSpec {
-                        key,
-                        lsn,
-                        at: disk_offset,
-                    });
-                }
-
-                disk_offset += 1;
-            }
-        }
-
-        assert_eq!(planned_blobs, expected_blobs);
-    }
-}

pageserver/src/tenant/storage_layer/image_layer.rs

@@ -55,7 +55,6 @@ use std::ops::Range;
 use std::os::unix::prelude::FileExt;
 use std::sync::Arc;
 use tokio::sync::OnceCell;
-use tokio_stream::StreamExt;
 use tracing::*;
 
 use utils::{
@@ -490,33 +489,35 @@ impl ImageLayerInner {
         let tree_reader =
             DiskBtreeReader::new(self.index_start_blk, self.index_root_blk, block_reader);
 
-        let ctx = RequestContextBuilder::extend(ctx)
-            .page_content_kind(PageContentKind::ImageLayerBtreeNode)
-            .build();
-
         for range in keyspace.ranges.iter() {
             let mut range_end_handled = false;
 
             let mut search_key: [u8; KEY_SIZE] = [0u8; KEY_SIZE];
             range.start.write_to_byte_slice(&mut search_key);
 
-            let index_stream = tree_reader.get_stream_from(&search_key, &ctx);
-            let mut index_stream = std::pin::pin!(index_stream);
+            tree_reader
+                .visit(
+                    &search_key,
+                    VisitDirection::Forwards,
+                    |raw_key, offset| {
+                        let key = Key::from_slice(&raw_key[..KEY_SIZE]);
+                        assert!(key >= range.start);
 
-            while let Some(index_entry) = index_stream.next().await {
-                let (raw_key, offset) = index_entry?;
-
-                let key = Key::from_slice(&raw_key[..KEY_SIZE]);
-                assert!(key >= range.start);
-
-                if key >= range.end {
-                    planner.handle_range_end(offset);
-                    range_end_handled = true;
-                    break;
-                } else {
-                    planner.handle(key, self.lsn, offset, BlobFlag::None);
-                }
-            }
+                        if key >= range.end {
+                            planner.handle_range_end(offset);
+                            range_end_handled = true;
+                            false
+                        } else {
+                            planner.handle(key, self.lsn, offset, BlobFlag::None);
+                            true
+                        }
+                    },
+                    &RequestContextBuilder::extend(ctx)
+                        .page_content_kind(PageContentKind::ImageLayerBtreeNode)
+                        .build(),
+                )
+                .await
+                .map_err(|err| GetVectoredError::Other(anyhow!(err)))?;
 
             if !range_end_handled {
                 let payload_end = self.index_start_blk as u64 * PAGE_SZ as u64;

pageserver/src/tenant/storage_layer/inmemory_layer.rs

@@ -336,17 +336,32 @@ impl InMemoryLayer {
 
     /// Common subroutine of the public put_wal_record() and put_page_image() functions.
     /// Adds the page version to the in-memory tree
-
     pub(crate) async fn put_value(
         &self,
         key: Key,
         lsn: Lsn,
-        buf: &[u8],
+        val: &Value,
         ctx: &RequestContext,
     ) -> Result<()> {
         let mut inner = self.inner.write().await;
         self.assert_writable();
-        self.put_value_locked(&mut inner, key, lsn, buf, ctx).await
+        self.put_value_locked(&mut inner, key, lsn, val, ctx).await
+    }
+
+    pub(crate) async fn put_values(
+        &self,
+        values: &HashMap<Key, Vec<(Lsn, Value)>>,
+        ctx: &RequestContext,
+    ) -> Result<()> {
+        let mut inner = self.inner.write().await;
+        self.assert_writable();
+        for (key, vals) in values {
+            for (lsn, val) in vals {
+                self.put_value_locked(&mut inner, *key, *lsn, val, ctx)
+                    .await?;
+            }
+        }
+        Ok(())
     }
 
     async fn put_value_locked(
@@ -354,16 +369,22 @@ impl InMemoryLayer {
         locked_inner: &mut RwLockWriteGuard<'_, InMemoryLayerInner>,
         key: Key,
         lsn: Lsn,
-        buf: &[u8],
+        val: &Value,
         ctx: &RequestContext,
     ) -> Result<()> {
         trace!("put_value key {} at {}/{}", key, self.timeline_id, lsn);
 
         let off = {
+            // Avoid doing allocations for "small" values.
+            // In the regression test suite, the limit of 256 avoided allocations in 95% of cases:
+            // https://github.com/neondatabase/neon/pull/5056#discussion_r1301975061
+            let mut buf = smallvec::SmallVec::<[u8; 256]>::new();
+            buf.clear();
+            val.ser_into(&mut buf)?;
             locked_inner
                 .file
                 .write_blob(
-                    buf,
+                    &buf,
                     &RequestContextBuilder::extend(ctx)
                         .page_content_kind(PageContentKind::InMemoryLayer)
                         .build(),
@@ -391,12 +412,7 @@ impl InMemoryLayer {
     pub async fn freeze(&self, end_lsn: Lsn) {
         let inner = self.inner.write().await;
 
-        assert!(
-            self.start_lsn < end_lsn,
-            "{} >= {}",
-            self.start_lsn,
-            end_lsn
-        );
+        assert!(self.start_lsn < end_lsn);
         self.end_lsn.set(end_lsn).expect("end_lsn set only once");
 
         for vec_map in inner.index.values() {

pageserver/src/tenant/storage_layer/layer.rs

@@ -8,7 +8,7 @@ use pageserver_api::shard::ShardIndex;
 use std::ops::Range;
 use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
 use std::sync::{Arc, Weak};
-use std::time::{Duration, SystemTime};
+use std::time::SystemTime;
 use tracing::Instrument;
 use utils::lsn::Lsn;
 use utils::sync::heavier_once_cell;
@@ -195,7 +195,6 @@ impl Layer {
         let downloaded = resident.expect("just initialized");
 
         // if the rename works, the path is as expected
-        // TODO: sync system call
         std::fs::rename(temp_path, owner.local_path())
             .with_context(|| format!("rename temporary file as correct path for {owner}"))?;
 
@@ -209,15 +208,10 @@ impl Layer {
     /// If for a bad luck or blocking of the executor, we miss the actual eviction and the layer is
     /// re-downloaded, [`EvictionError::Downloaded`] is returned.
     ///
-    /// Timeout is mandatory, because waiting for eviction is only needed for our tests; eviction
-    /// will happen regardless the future returned by this method completing unless there is a
-    /// read access (currently including [`Layer::keep_resident`]) before eviction gets to
-    /// complete.
-    ///
     /// Technically cancellation safe, but cancelling might shift the viewpoint of what generation
     /// of download-evict cycle on retry.
-    pub(crate) async fn evict_and_wait(&self, timeout: Duration) -> Result<(), EvictionError> {
-        self.0.evict_and_wait(timeout).await
+    pub(crate) async fn evict_and_wait(&self) -> Result<(), EvictionError> {
+        self.0.evict_and_wait().await
     }
 
     /// Delete the layer file when the `self` gets dropped, also try to schedule a remote index upload
@@ -369,7 +363,7 @@ impl Layer {
     ///
     /// Does not start local deletion, use [`Self::delete_on_drop`] for that
     /// separatedly.
-    #[cfg(any(feature = "testing", test))]
+    #[cfg(feature = "testing")]
     pub(crate) fn wait_drop(&self) -> impl std::future::Future<Output = ()> + 'static {
         let mut rx = self.0.status.subscribe();
 
@@ -638,7 +632,7 @@ impl LayerInner {
 
     /// Cancellation safe, however dropping the future and calling this method again might result
     /// in a new attempt to evict OR join the previously started attempt.
-    pub(crate) async fn evict_and_wait(&self, timeout: Duration) -> Result<(), EvictionError> {
+    pub(crate) async fn evict_and_wait(&self) -> Result<(), EvictionError> {
         use tokio::sync::broadcast::error::RecvError;
 
         assert!(self.have_remote_client);
@@ -658,22 +652,16 @@ impl LayerInner {
         if strong.is_some() {
             // drop the DownloadedLayer outside of the holding the guard
             drop(strong);
-
-            // idea here is that only one evicter should ever get to witness a strong reference,
-            // which means whenever get_or_maybe_download upgrades a weak, it must mark up a
-            // cancelled eviction and signal us, like it currently does.
-            //
-            // a second concurrent evict_and_wait will not see a strong reference.
             LAYER_IMPL_METRICS.inc_started_evictions();
         }
 
-        match tokio::time::timeout(timeout, rx.recv()).await {
-            Ok(Ok(Status::Evicted)) => Ok(()),
-            Ok(Ok(Status::Downloaded)) => Err(EvictionError::Downloaded),
-            Ok(Err(RecvError::Closed)) => {
+        match rx.recv().await {
+            Ok(Status::Evicted) => Ok(()),
+            Ok(Status::Downloaded) => Err(EvictionError::Downloaded),
+            Err(RecvError::Closed) => {
                 unreachable!("sender cannot be dropped while we are in &self method")
             }
-            Ok(Err(RecvError::Lagged(_))) => {
+            Err(RecvError::Lagged(_)) => {
                 // this is quite unlikely, but we are blocking a lot in the async context, so
                 // we might be missing this because we are stuck on a LIFO slot on a thread
                 // which is busy blocking for a 1TB database create_image_layers.
@@ -686,7 +674,6 @@ impl LayerInner {
                     None => Ok(()),
                 }
             }
-            Err(_timeout) => Err(EvictionError::Timeout),
         }
     }
 
@@ -976,7 +963,7 @@ impl LayerInner {
                 }
 
                 self.consecutive_failures.store(0, Ordering::Relaxed);
-                tracing::info!(size=%self.desc.file_size, "on-demand download successful");
+                tracing::info!("on-demand download successful");
 
                 Ok(permit)
             }
@@ -1208,9 +1195,6 @@ pub(crate) enum EvictionError {
     /// Evictions must always lose to downloads in races, and this time it happened.
     #[error("layer was downloaded instead")]
     Downloaded,
-
-    #[error("eviction did not happen within timeout")]
-    Timeout,
 }
 
 /// Error internal to the [`LayerInner::get_or_maybe_download`]

pageserver/src/tenant/storage_layer/layer/tests.rs

@@ -1,173 +1,13 @@
 use futures::StreamExt;
-use pageserver_api::key::CONTROLFILE_KEY;
 use tokio::task::JoinSet;
-use tracing::Instrument;
 use utils::{
     completion::{self, Completion},
     id::TimelineId,
 };
 
 use super::*;
-use crate::{context::DownloadBehavior, task_mgr::BACKGROUND_RUNTIME};
-use crate::{task_mgr::TaskKind, tenant::harness::TenantHarness};
-
-/// Used in tests to advance a future to wanted await point, and not futher.
-const ADVANCE: std::time::Duration = std::time::Duration::from_secs(3600);
-
-/// Used in tests to indicate forever long timeout; has to be longer than the amount of ADVANCE
-/// timeout uses to advance futures.
-const FOREVER: std::time::Duration = std::time::Duration::from_secs(ADVANCE.as_secs() * 24 * 7);
-
-/// Demonstrate the API and resident -> evicted -> resident -> deleted transitions.
-#[tokio::test]
-async fn smoke_test() {
-    let handle = BACKGROUND_RUNTIME.handle();
-
-    let h = TenantHarness::create("smoke_test").unwrap();
-    let span = h.span();
-    let download_span = span.in_scope(|| tracing::info_span!("downloading", timeline_id = 1));
-    let (tenant, _) = h.load().await;
-
-    let ctx = RequestContext::new(TaskKind::UnitTest, DownloadBehavior::Download);
-
-    let timeline = tenant
-        .create_test_timeline(TimelineId::generate(), Lsn(0x10), 14, &ctx)
-        .await
-        .unwrap();
-
-    let layer = {
-        let mut layers = {
-            let layers = timeline.layers.read().await;
-            layers.resident_layers().collect::<Vec<_>>().await
-        };
-
-        assert_eq!(layers.len(), 1);
-
-        layers.swap_remove(0)
-    };
-
-    // all layers created at pageserver are like `layer`, initialized with strong
-    // Arc<DownloadedLayer>.
-
-    let img_before = {
-        let mut data = ValueReconstructState::default();
-        layer
-            .get_value_reconstruct_data(CONTROLFILE_KEY, Lsn(0x10)..Lsn(0x11), &mut data, &ctx)
-            .await
-            .unwrap();
-        data.img
-            .take()
-            .expect("tenant harness writes the control file")
-    };
-
-    // important part is evicting the layer, which can be done when there are no more ResidentLayer
-    // instances -- there currently are none, only two `Layer` values, one in the layermap and on
-    // in scope.
-    layer.evict_and_wait(FOREVER).await.unwrap();
-
-    // double-evict returns an error, which is valid if both eviction_task and disk usage based
-    // eviction would both evict the same layer at the same time.
-
-    let e = layer.evict_and_wait(FOREVER).await.unwrap_err();
-    assert!(matches!(e, EvictionError::NotFound));
-
-    // on accesses when the layer is evicted, it will automatically be downloaded.
-    let img_after = {
-        let mut data = ValueReconstructState::default();
-        layer
-            .get_value_reconstruct_data(CONTROLFILE_KEY, Lsn(0x10)..Lsn(0x11), &mut data, &ctx)
-            .instrument(download_span.clone())
-            .await
-            .unwrap();
-        data.img.take().unwrap()
-    };
-
-    assert_eq!(img_before, img_after);
-
-    // evict_and_wait can timeout, but it doesn't cancel the evicting itself
-    //
-    // ZERO for timeout does not work reliably, so first take up all spawn_blocking slots to
-    // artificially slow it down.
-    let helper = SpawnBlockingPoolHelper::consume_all_spawn_blocking_threads(handle).await;
-
-    match layer
-        .evict_and_wait(std::time::Duration::ZERO)
-        .await
-        .unwrap_err()
-    {
-        EvictionError::Timeout => {
-            // expected, but note that the eviction is "still ongoing"
-            helper.release().await;
-            // exhaust spawn_blocking pool to ensure it is now complete
-            SpawnBlockingPoolHelper::consume_and_release_all_of_spawn_blocking_threads(handle)
-                .await;
-        }
-        other => unreachable!("{other:?}"),
-    }
-
-    // only way to query if a layer is resident is to acquire a ResidentLayer instance.
-    // Layer::keep_resident never downloads, but it might initialize if the layer file is found
-    // downloaded locally.
-    let none = layer.keep_resident().await.unwrap();
-    assert!(
-        none.is_none(),
-        "Expected none, because eviction removed the local file, found: {none:?}"
-    );
-
-    // plain downloading is rarely needed
-    layer
-        .download_and_keep_resident()
-        .instrument(download_span)
-        .await
-        .unwrap();
-
-    // last important part is deletion on drop: gc and compaction use it for compacted L0 layers
-    // or fully garbage collected layers. deletion means deleting the local file, and scheduling a
-    // deletion of the already unlinked from index_part.json remote file.
-    //
-    // marking a layer to be deleted on drop is irreversible; there is no technical reason against
-    // reversiblity, but currently it is not needed so it is not provided.
-    layer.delete_on_drop();
-
-    let path = layer.local_path().to_owned();
-
-    // wait_drop produces an unconnected to Layer future which will resolve when the
-    // LayerInner::drop has completed.
-    let mut wait_drop = std::pin::pin!(layer.wait_drop());
-
-    // paused time doesn't really work well with timeouts and evict_and_wait, so delay pausing
-    // until here
-    tokio::time::pause();
-    tokio::time::timeout(ADVANCE, &mut wait_drop)
-        .await
-        .expect_err("should had timed out because two strong references exist");
-
-    tokio::fs::metadata(&path)
-        .await
-        .expect("the local layer file still exists");
-
-    let rtc = timeline.remote_client.as_ref().unwrap();
-
-    {
-        let layers = &[layer];
-        let mut g = timeline.layers.write().await;
-        g.finish_gc_timeline(layers);
-        // this just updates the remote_physical_size for demonstration purposes
-        rtc.schedule_gc_update(layers).unwrap();
-    }
-
-    // when strong references are dropped, the file is deleted and remote deletion is scheduled
-    wait_drop.await;
-
-    let e = tokio::fs::metadata(&path)
-        .await
-        .expect_err("the local file is deleted");
-    assert_eq!(e.kind(), std::io::ErrorKind::NotFound);
-
-    rtc.wait_completion().await.unwrap();
-
-    assert_eq!(rtc.get_remote_physical_size(), 0);
-}
+use crate::task_mgr::BACKGROUND_RUNTIME;
+use crate::tenant::harness::TenantHarness;
 
 /// This test demonstrates a previous hang when a eviction and deletion were requested at the same
 /// time. Now both of them complete per Arc drop semantics.
@@ -201,10 +41,10 @@ async fn evict_and_wait_on_wanted_deleted() {
     let resident = layer.keep_resident().await.unwrap();
 
     {
-        let mut evict_and_wait = std::pin::pin!(layer.evict_and_wait(FOREVER));
+        let mut evict_and_wait = std::pin::pin!(layer.evict_and_wait());
 
         // drive the future to await on the status channel
-        tokio::time::timeout(ADVANCE, &mut evict_and_wait)
+        tokio::time::timeout(std::time::Duration::from_secs(3600), &mut evict_and_wait)
             .await
             .expect_err("should had been a timeout since we are holding the layer resident");
 
@@ -275,10 +115,10 @@ async fn residency_check_while_evict_and_wait_on_clogged_spawn_blocking() {
 
     let resident = layer.keep_resident().await.unwrap();
 
-    let mut evict_and_wait = std::pin::pin!(layer.evict_and_wait(FOREVER));
+    let mut evict_and_wait = std::pin::pin!(layer.evict_and_wait());
 
     // drive the future to await on the status channel
-    tokio::time::timeout(ADVANCE, &mut evict_and_wait)
+    tokio::time::timeout(std::time::Duration::from_secs(3600), &mut evict_and_wait)
         .await
         .expect_err("should had been a timeout since we are holding the layer resident");
     assert_eq!(1, LAYER_IMPL_METRICS.started_evictions.get());
@@ -298,7 +138,7 @@ async fn residency_check_while_evict_and_wait_on_clogged_spawn_blocking() {
 
     // because the keep_resident check alters wanted evicted without sending a message, we will
     // never get completed
-    let e = tokio::time::timeout(ADVANCE, &mut evict_and_wait)
+    let e = tokio::time::timeout(std::time::Duration::from_secs(3600), &mut evict_and_wait)
         .await
         .expect("no timeout, because keep_resident re-initialized")
         .expect_err("eviction should not have succeeded because re-initialized");
@@ -318,10 +158,9 @@ async fn residency_check_while_evict_and_wait_on_clogged_spawn_blocking() {
             .sum::<u64>()
     );
 
-    let mut second_eviction = std::pin::pin!(layer.evict_and_wait(FOREVER));
+    let mut second_eviction = std::pin::pin!(layer.evict_and_wait());
 
-    // advance to the wait on the queue
-    tokio::time::timeout(ADVANCE, &mut second_eviction)
+    tokio::time::timeout(std::time::Duration::from_secs(3600), &mut second_eviction)
         .await
         .expect_err("timeout because spawn_blocking is clogged");
 
@@ -332,12 +171,7 @@ async fn residency_check_while_evict_and_wait_on_clogged_spawn_blocking() {
 
     helper.release().await;
 
-    // the second_eviction gets to run here
-    //
-    // synchronize to be *strictly* after the second_eviction spawn_blocking run
-    SpawnBlockingPoolHelper::consume_and_release_all_of_spawn_blocking_threads(handle).await;
-
-    tokio::time::timeout(ADVANCE, &mut second_eviction)
+    tokio::time::timeout(std::time::Duration::from_secs(3600), &mut second_eviction)
         .await
         .expect("eviction goes through now that spawn_blocking is unclogged")
         .expect("eviction should succeed, because version matches");
@@ -427,49 +261,3 @@ impl SpawnBlockingPoolHelper {
             .await
     }
 }
-
-#[test]
-fn spawn_blocking_pool_helper_actually_works() {
-    // create a custom runtime for which we know and control how many blocking threads it has
-    //
-    // because the amount is not configurable for our helper, expect the same amount as
-    // BACKGROUND_RUNTIME using the tokio defaults would have.
-    let rt = tokio::runtime::Builder::new_current_thread()
-        .max_blocking_threads(512)
-        .enable_all()
-        .build()
-        .unwrap();
-
-    let handle = rt.handle();
-
-    rt.block_on(async move {
-        // this will not return until all threads are spun up and actually executing the code
-        // waiting on `consumed` to be `SpawnBlockingPoolHelper::release`'d.
-        let consumed = SpawnBlockingPoolHelper::consume_all_spawn_blocking_threads(handle).await;
-
-        println!("consumed");
-
-        let mut jh = std::pin::pin!(tokio::task::spawn_blocking(move || {
-            // this will not get to run before we release
-        }));
-
-        println!("spawned");
-
-        tokio::time::timeout(std::time::Duration::from_secs(1), &mut jh)
-            .await
-            .expect_err("the task should not have gotten to run yet");
-
-        println!("tried to join");
-
-        consumed.release().await;
-
-        println!("released");
-
-        tokio::time::timeout(std::time::Duration::from_secs(1), jh)
-            .await
-            .expect("no timeout")
-            .expect("no join error");
-
-        println!("joined");
-    });
-}

pageserver/src/tenant/tasks.rs

@@ -101,7 +101,6 @@ pub fn start_background_loops(
                     _ = completion::Barrier::maybe_wait(background_jobs_can_start) => {}
                 };
                 compaction_loop(tenant, cancel)
-                    // If you rename this span, change the RUST_LOG env variable in test_runner/performance/test_branch_creation.py
                     .instrument(info_span!("compaction_loop", tenant_id = %tenant_shard_id.tenant_id, shard_id = %tenant_shard_id.shard_slug()))
                     .await;
                 Ok(())
@@ -199,11 +198,7 @@ async fn compaction_loop(tenant: Arc<Tenant>, cancel: CancellationToken) {
                 }
             };
 
-            let elapsed = started_at.elapsed();
-            warn_when_period_overrun(elapsed, period, BackgroundLoopKind::Compaction);
-
-            // the duration is recorded by performance tests by enabling debug in this function
-            tracing::debug!(elapsed_ms=elapsed.as_millis(), "compaction iteration complete");
+            warn_when_period_overrun(started_at.elapsed(), period, BackgroundLoopKind::Compaction);
 
             // Perhaps we did no work and the walredo process has been idle for some time:
             // give it a chance to shut down to avoid leaving walredo process running indefinitely.
@@ -222,7 +217,7 @@ async fn compaction_loop(tenant: Arc<Tenant>, cancel: CancellationToken) {
                 }
                 let allowed_rps = tenant.timeline_get_throttle.steady_rps();
                 let delta = now - prev;
-                info!(
+                warn!(
                     n_seconds=%format_args!("{:.3}",
                     delta.as_secs_f64()),
                     count_accounted,

pageserver/src/tenant/throttle.rs

@@ -2,14 +2,14 @@ use std::{
     str::FromStr,
     sync::{
         atomic::{AtomicU64, Ordering},
-        Arc, Mutex,
+        Arc,
     },
     time::{Duration, Instant},
 };
 
 use arc_swap::ArcSwap;
 use enumset::EnumSet;
-use tracing::{error, warn};
+use tracing::error;
 
 use crate::{context::RequestContext, task_mgr::TaskKind};
 
@@ -157,19 +157,6 @@ where
                 .fetch_add(wait_time.as_micros() as u64, Ordering::Relaxed);
             let observation = Observation { wait_time };
             self.metric.observe_throttling(&observation);
-            match ctx.micros_spent_throttled.add(wait_time) {
-                Ok(res) => res,
-                Err(error) => {
-                    use once_cell::sync::Lazy;
-                    use utils::rate_limit::RateLimit;
-                    static WARN_RATE_LIMIT: Lazy<Mutex<RateLimit>> =
-                        Lazy::new(|| Mutex::new(RateLimit::new(Duration::from_secs(10))));
-                    let mut guard = WARN_RATE_LIMIT.lock().unwrap();
-                    guard.call(move || {
-                        warn!(error, "error adding time spent throttled; this message is logged at a global rate limit");
-                    });
-                }
-            }
         }
     }
 }