From e6557f4f919bc36470ebf8bb02ab6d9b29e90e31 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 21 Nov 2022 11:01:55 +0200 Subject: [PATCH] Silence test failures, where we operate on a tenant before it's loaded Saw a failure like this, from 'test_tenants_attached_after_download' and 'test_tenant_redownloads_truncated_file_on_startup': > test_runner/fixtures/neon_fixtures.py:1064: in verbose_error > res.raise_for_status() > /github/home/.cache/pypoetry/virtualenvs/neon-_pxWMzVK-py3.9/lib/python3.9/site-packages/requests/models.py:1021: in raise_for_status > raise HTTPError(http_error_msg, response=self) > E requests.exceptions.HTTPError: 404 Client Error: Not Found for url: http://localhost:18150/v1/tenant/2334c9c113a82b5dd1651a0a23c53448/timeline > > The above exception was the direct cause of the following exception: > test_runner/regress/test_tenants_with_remote_storage.py:185: in test_tenants_attached_after_download > restored_timelines = client.timeline_list(tenant_id) > test_runner/fixtures/neon_fixtures.py:1148: in timeline_list > self.verbose_error(res) > test_runner/fixtures/neon_fixtures.py:1070: in verbose_error > raise PageserverApiException(msg) from e > E fixtures.neon_fixtures.PageserverApiException: NotFound: Tenant 2334c9c113a82b5dd1651a0a23c53448 is not active. Current state: Loading These tests starts the pageserver, wait until assert_no_in_progress_downloads_for_tenant says that has_downloads_in_progress is false, and then call timeline_list on the tenant. But has_downloads_in_progress was only returned as true when the tenant was being attached, not when it was being loaded at pageserver startup. Change tenant_status API endpoint (/v1/tenant/:tenant_id) so that it returns has_downloads_in_progress=true also for tenants that are still in Loading state. --- libs/pageserver_api/src/models.rs | 21 ++++++++++++++++++--- pageserver/src/http/routes.rs | 4 ++-- pageserver/src/page_service.rs | 13 ++++--------- pageserver/src/tenant.rs | 21 +++++++++++++++++---- test_runner/regress/test_broken_timeline.py | 3 ++- 5 files changed, 43 insertions(+), 19 deletions(-) diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 93c88749e5..86b9be640e 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -23,13 +23,28 @@ pub enum TenantState { Active { background_jobs_running: bool, }, - /// A tenant is recognized by pageserver, but not yet ready to operate: - /// e.g. not present locally and being downloaded or being read into memory from the file system. + /// A tenant is recognized by pageserver, but it is being detached or the system is being + /// shut down. Paused, - /// A tenant is recognized by the pageserver, but no longer used for any operations, as failed to get activated. + /// A tenant is recognized by the pageserver, but can no longer used for any operations, + /// because it failed to get activated. Broken, } +impl TenantState { + pub fn has_in_progress_downloads(&self) -> bool { + match self { + Self::Loading => true, + Self::Attaching => true, + Self::Active { + background_jobs_running: _, + } => false, + Self::Paused => false, + Self::Broken => false, + } + } +} + /// A state of a timeline in pageserver's memory. #[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub enum TimelineState { diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 3f0a1aab01..e0a74ba2f0 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -396,7 +396,7 @@ async fn tenant_list_handler(request: Request) -> Result, A id: *id, state: *state, current_physical_size: None, - has_in_progress_downloads: Some(state == &TenantState::Attaching), + has_in_progress_downloads: Some(state.has_in_progress_downloads()), }) .collect::>() }) @@ -425,7 +425,7 @@ async fn tenant_status(request: Request) -> Result, ApiErro id: tenant_id, state, current_physical_size: Some(current_physical_size), - has_in_progress_downloads: Some(state == TenantState::Attaching), + has_in_progress_downloads: Some(state.has_in_progress_downloads()), }; Ok::<_, anyhow::Error>(tenant_info) diff --git a/pageserver/src/page_service.rs b/pageserver/src/page_service.rs index 5de11017c6..b829f05395 100644 --- a/pageserver/src/page_service.rs +++ b/pageserver/src/page_service.rs @@ -940,15 +940,10 @@ impl postgres_backend_async::Handler for PageServerHandler { /// all tenants are still loading. async fn get_active_tenant_with_timeout(tenant_id: TenantId) -> Result> { let tenant = tenant_mgr::get_tenant(tenant_id, false)?; - - match tokio::time::timeout(Duration::from_secs(30), tenant.wait_until_loaded()).await { - Ok(result) => { - let state = result?; - if !matches!(state, pageserver_api::models::TenantState::Active { .. }) { - anyhow::bail!("Tenant {tenant_id} is not active. Current state: {state:?}"); - } - Ok(tenant) - } + match tokio::time::timeout(Duration::from_secs(30), tenant.wait_to_become_active()).await { + Ok(wait_result) => wait_result + // no .context(), the error message is good enough and some tests depend on it + .map(move |()| tenant), Err(_) => anyhow::bail!("Timeout waiting for tenant {tenant_id} to become Active"), } } diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 5056c9ddc2..58844a62a4 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -1298,14 +1298,27 @@ impl Tenant { self.state.subscribe() } - pub async fn wait_until_loaded(&self) -> anyhow::Result { + pub async fn wait_to_become_active(&self) -> anyhow::Result<()> { let mut receiver = self.state.subscribe(); loop { let current_state = *receiver.borrow_and_update(); - if current_state != TenantState::Loading { - break Ok(current_state); + match current_state { + TenantState::Loading | TenantState::Attaching => { + // in these states, there's a chance that we can reach ::Active + receiver.changed().await?; + } + TenantState::Active { .. } => { + return Ok(()); + } + TenantState::Broken | TenantState::Paused => { + // There's no chance the tenant can transition back into ::Active + anyhow::bail!( + "Tenant {} will not become active. Current state: {:?}", + self.tenant_id, + current_state, + ); + } } - receiver.changed().await?; } } } diff --git a/test_runner/regress/test_broken_timeline.py b/test_runner/regress/test_broken_timeline.py index 421dcf3140..08945e6795 100644 --- a/test_runner/regress/test_broken_timeline.py +++ b/test_runner/regress/test_broken_timeline.py @@ -18,6 +18,7 @@ def test_broken_timeline(neon_env_builder: NeonEnvBuilder): ".*Failed to load delta layer.*", ".*could not find data for key.*", ".*is not active. Current state: Broken.*", + ".*will not become active. Current state: Broken.*", ".*failed to load metadata.*", ] ) @@ -78,7 +79,7 @@ def test_broken_timeline(neon_env_builder: NeonEnvBuilder): # First timeline would not get loaded into pageserver due to corrupt metadata file with pytest.raises( - Exception, match=f"Tenant {tenant1} is not active. Current state: Broken" + Exception, match=f"Tenant {tenant1} will not become active. Current state: Broken" ) as err: pg1.start() log.info(