From eaf970180b8d5e0effc0739543aa3d2ca5b96e55 Mon Sep 17 00:00:00 2001 From: John Spray Date: Tue, 17 Oct 2023 10:06:27 +0100 Subject: [PATCH] Remove defunct "attach marker" code. --- libs/pageserver_api/src/models.rs | 1 - pageserver/src/config.rs | 8 +--- pageserver/src/tenant.rs | 47 ------------------- pageserver/src/tenant/mgr.rs | 20 ++------ .../src/tenant/remote_timeline_client.rs | 22 --------- 5 files changed, 5 insertions(+), 93 deletions(-) diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index b2064cb7fe..e667645c0a 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -110,7 +110,6 @@ impl TenantState { // So, return `Maybe` while Attaching, making Console wait for the attach task to finish. Self::Attaching | Self::Activating(ActivatingFrom::Attaching) => Maybe, // tenant mgr startup distinguishes attaching from loading via marker file. - // If it's loading, there is no attach marker file, i.e., attach had finished in the past. Self::Loading | Self::Activating(ActivatingFrom::Loading) => Attached, // We only reach Active after successful load / attach. // So, call atttachment status Attached. diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index fe62a7299a..a58c08e29b 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -33,8 +33,7 @@ use crate::disk_usage_eviction_task::DiskUsageEvictionTaskConfig; use crate::tenant::config::TenantConf; use crate::tenant::config::TenantConfOpt; use crate::tenant::{ - TENANTS_SEGMENT_NAME, TENANT_ATTACHING_MARKER_FILENAME, TENANT_DELETED_MARKER_FILE_NAME, - TIMELINES_SEGMENT_NAME, + TENANTS_SEGMENT_NAME, TENANT_DELETED_MARKER_FILE_NAME, TIMELINES_SEGMENT_NAME, }; use crate::{ IGNORED_TENANT_FILE_NAME, METADATA_FILE_NAME, TENANT_CONFIG_NAME, TENANT_LOCATION_CONFIG_NAME, @@ -633,11 +632,6 @@ impl PageServerConf { self.tenants_path().join(tenant_id.to_string()) } - pub fn tenant_attaching_mark_file_path(&self, tenant_id: &TenantId) -> Utf8PathBuf { - self.tenant_path(tenant_id) - .join(TENANT_ATTACHING_MARKER_FILENAME) - } - pub fn tenant_ignore_mark_file_path(&self, tenant_id: &TenantId) -> Utf8PathBuf { self.tenant_path(tenant_id).join(IGNORED_TENANT_FILE_NAME) } diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 7043271c0b..c99ac3c3fc 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -151,8 +151,6 @@ pub const TENANTS_SEGMENT_NAME: &str = "tenants"; /// Parts of the `.neon/tenants//timelines/` directory prefix. pub const TIMELINES_SEGMENT_NAME: &str = "timelines"; -pub const TENANT_ATTACHING_MARKER_FILENAME: &str = "attaching"; - pub const TENANT_DELETED_MARKER_FILE_NAME: &str = "deleted"; /// References to shared objects that are passed into each tenant, such @@ -735,18 +733,6 @@ impl Tenant { ) -> anyhow::Result<()> { span::debug_assert_current_span_has_tenant_id(); - let marker_file = self.conf.tenant_attaching_mark_file_path(&self.tenant_id); - if let AttachMarkerMode::Expect = expect_marker { - if !tokio::fs::try_exists(&marker_file) - .await - .context("check for existence of marker file")? - { - anyhow::bail!( - "implementation error: marker file should exist at beginning of this function" - ); - } - } - let remote_storage = match &self.remote_storage { Some(rs) => rs, None => { @@ -846,13 +832,6 @@ impl Tenant { .map_err(LoadLocalTimelineError::ResumeDeletion)?; } - if let AttachMarkerMode::Expect = expect_marker { - std::fs::remove_file(&marker_file) - .with_context(|| format!("unlink attach marker file {marker_file}"))?; - crashsafe::fsync(marker_file.parent().expect("marker file has parent dir")) - .context("fsync tenant directory after unlinking attach marker file")?; - } - crate::failpoint_support::sleep_millis_async!("attach-before-activate"); info!("Done"); @@ -3094,16 +3073,10 @@ fn remove_timeline_and_uninit_mark( Ok(()) } -pub(crate) enum CreateTenantFilesMode { - Create, - Attach, -} - pub(crate) async fn create_tenant_files( conf: &'static PageServerConf, location_conf: &LocationConf, tenant_id: &TenantId, - mode: CreateTenantFilesMode, ) -> anyhow::Result { let target_tenant_directory = conf.tenant_path(tenant_id); anyhow::ensure!( @@ -3126,7 +3099,6 @@ pub(crate) async fn create_tenant_files( conf, location_conf, tenant_id, - mode, &temporary_tenant_dir, &target_tenant_directory, ) @@ -3152,28 +3124,9 @@ async fn try_create_target_tenant_dir( conf: &'static PageServerConf, location_conf: &LocationConf, tenant_id: &TenantId, - mode: CreateTenantFilesMode, temporary_tenant_dir: &Utf8Path, target_tenant_directory: &Utf8Path, ) -> Result<(), anyhow::Error> { - match mode { - CreateTenantFilesMode::Create => {} // needs no attach marker, writing tenant conf + atomic rename of dir is good enough - CreateTenantFilesMode::Attach => { - let attach_marker_path = temporary_tenant_dir.join(TENANT_ATTACHING_MARKER_FILENAME); - let file = std::fs::OpenOptions::new() - .create_new(true) - .write(true) - .open(&attach_marker_path) - .with_context(|| { - format!("could not create attach marker file {attach_marker_path:?}") - })?; - file.sync_all().with_context(|| { - format!("could not sync attach marker file: {attach_marker_path:?}") - })?; - // fsync of the directory in which the file resides comes later in this function - } - } - let temporary_tenant_timelines_dir = rebase_directory( &conf.timelines_path(tenant_id), target_tenant_directory, diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index 6ce51f35b5..0b8a391d7e 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -26,10 +26,7 @@ use crate::deletion_queue::DeletionQueueClient; use crate::task_mgr::{self, TaskKind}; use crate::tenant::config::{AttachmentMode, LocationConf, LocationMode, TenantConfOpt}; use crate::tenant::delete::DeleteTenantFlow; -use crate::tenant::{ - create_tenant_files, AttachMarkerMode, AttachedTenantConf, CreateTenantFilesMode, Tenant, - TenantState, -}; +use crate::tenant::{create_tenant_files, AttachedTenantConf, Tenant, TenantState}; use crate::{InitializationOrder, IGNORED_TENANT_FILE_NAME, TEMP_FILE_SUFFIX}; use utils::crashsafe::path_with_suffix_extension; @@ -287,9 +284,7 @@ async fn init_load_tenant_configs( continue; } - // This case happens if we: - // * crash during attach before creating the attach marker file - // * crash during tenant delete before removing tenant directory + // This case happens if we crash during attachment before writing a config into the dir let is_empty = tenant_dir_path.is_empty_dir().with_context(|| { format!("Failed to check whether {tenant_dir_path:?} is an empty dir") })?; @@ -638,7 +633,7 @@ pub(crate) async fn create_tenant( // We're holding the tenants lock in write mode while doing local IO. // If this section ever becomes contentious, introduce a new `TenantState::Creating` // and do the work in that state. - super::create_tenant_files(conf, &location_conf, &tenant_id, CreateTenantFilesMode::Create).await?; + super::create_tenant_files(conf, &location_conf, &tenant_id).await?; // TODO: tenant directory remains on disk if we bail out from here on. // See https://github.com/neondatabase/neon/issues/4233 @@ -1076,17 +1071,10 @@ pub(crate) async fn attach_tenant( ) -> Result<(), TenantMapInsertError> { tenant_map_insert(tenant_id, || async { let location_conf = LocationConf::attached_single(tenant_conf, generation); - let tenant_dir = create_tenant_files(conf, &location_conf, &tenant_id, CreateTenantFilesMode::Attach).await?; + let tenant_dir = create_tenant_files(conf, &location_conf, &tenant_id).await?; // TODO: tenant directory remains on disk if we bail out from here on. // See https://github.com/neondatabase/neon/issues/4233 - // Without the attach marker, schedule_local_tenant_processing will treat the attached tenant as fully attached - let marker_file_exists = conf - .tenant_attaching_mark_file_path(&tenant_id) - .try_exists() - .context("check for attach marker file existence")?; - anyhow::ensure!(marker_file_exists, "create_tenant_files should have created the attach marker file"); - let attached_tenant = schedule_local_tenant_processing(conf, tenant_id, &tenant_dir, AttachedTenantConf::try_from(location_conf)?, resources, None, &TENANTS, ctx)?; // TODO: tenant object & its background loops remain, untracked in tenant map, if we fail here. // See https://github.com/neondatabase/neon/issues/4233 diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index 06e2292156..1810300e6a 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -170,36 +170,14 @@ //! - create [`RemoteLayer`](super::storage_layer::RemoteLayer) instances //! for layers that are referenced by `IndexPart` but not present locally //! - schedule uploads for layers that are only present locally. -//! - if the remote `IndexPart`'s metadata was newer than the metadata in -//! the local filesystem, write the remote metadata to the local filesystem //! - After the above is done for each timeline, open the tenant for business by //! transitioning it from `TenantState::Attaching` to `TenantState::Active` state. //! This starts the timelines' WAL-receivers and the tenant's GC & Compaction loops. //! -//! We keep track of the fact that a client is in `Attaching` state in a marker -//! file on the local disk. This is critical because, when we restart the pageserver, -//! we do not want to do the `List timelines` step for each tenant that has already -//! been successfully attached (for performance & cost reasons). -//! Instead, for a tenant without the attach marker file, we assume that the -//! local state is in sync or ahead of the remote state. This includes the list -//! of all of the tenant's timelines, which is particularly critical to be up-to-date: -//! if there's a timeline on the remote that the pageserver doesn't know about, -//! the GC will not consider its branch point, leading to data loss. -//! So, for a tenant with the attach marker file, we know that we do not yet have -//! persisted all the remote timeline's metadata files locally. To exclude the -//! risk above, we re-run the procedure for such tenants -//! //! # Operating Without Remote Storage //! //! If no remote storage configuration is provided, the [`RemoteTimelineClient`] is //! not created and the uploads are skipped. -//! Theoretically, it should be ok to remove and re-add remote storage configuration to -//! the pageserver config at any time, since it doesn't make a difference to -//! [`Timeline::load_layer_map`]. -//! Of course, the remote timeline dir must not change while we have de-configured -//! remote storage, i.e., the pageserver must remain the owner of the given prefix -//! in remote storage. -//! But note that we don't test any of this right now. //! //! [`Tenant::timeline_init_and_sync`]: super::Tenant::timeline_init_and_sync //! [`Timeline::load_layer_map`]: super::Timeline::load_layer_map