From e5c81fef86b47d319e98e24a8d3693d9659ada21 Mon Sep 17 00:00:00 2001 From: John Spray Date: Tue, 31 Oct 2023 11:44:35 +0000 Subject: [PATCH] tests: minor improvements (#5674) Minor changes from while I have been working on HA tests: - Manual pytest executions came with some warnings from `log.warn()` usage - When something fails in a generations-enabled test, it it useful to have a log from the attachment service of what attached when, and with which generation. --------- Co-authored-by: Joonas Koivunen --- control_plane/src/bin/attachment_service.rs | 26 +++++++++++++++++---- test_runner/fixtures/pageserver/utils.py | 4 ++-- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/control_plane/src/bin/attachment_service.rs b/control_plane/src/bin/attachment_service.rs index 0ccd9e75fc..3205703c7d 100644 --- a/control_plane/src/bin/attachment_service.rs +++ b/control_plane/src/bin/attachment_service.rs @@ -12,6 +12,7 @@ use hyper::{Body, Request, Response}; use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; use std::{collections::HashMap, sync::Arc}; +use utils::http::endpoint::request_span; use utils::logging::{self, LogFormat}; use utils::signals::{ShutdownSignals, Signal}; @@ -221,8 +222,25 @@ async fn handle_attach_hook(mut req: Request) -> Result, Ap generation: 0, }); - if attach_req.node_id.is_some() { + if let Some(attaching_pageserver) = attach_req.node_id.as_ref() { tenant_state.generation += 1; + tracing::info!( + tenant_id = %attach_req.tenant_id, + ps_id = %attaching_pageserver, + generation = %tenant_state.generation, + "issuing", + ); + } else if let Some(ps_id) = tenant_state.pageserver { + tracing::info!( + tenant_id = %attach_req.tenant_id, + %ps_id, + generation = %tenant_state.generation, + "dropping", + ); + } else { + tracing::info!( + tenant_id = %attach_req.tenant_id, + "no-op: tenant already has no pageserver"); } tenant_state.pageserver = attach_req.node_id; let generation = tenant_state.generation; @@ -240,9 +258,9 @@ async fn handle_attach_hook(mut req: Request) -> Result, Ap fn make_router(persistent_state: PersistentState) -> RouterBuilder { endpoint::make_router() .data(Arc::new(State::new(persistent_state))) - .post("/re-attach", handle_re_attach) - .post("/validate", handle_validate) - .post("/attach-hook", handle_attach_hook) + .post("/re-attach", |r| request_span(r, handle_re_attach)) + .post("/validate", |r| request_span(r, handle_validate)) + .post("/attach-hook", |r| request_span(r, handle_attach_hook)) } #[tokio::main] diff --git a/test_runner/fixtures/pageserver/utils.py b/test_runner/fixtures/pageserver/utils.py index e54b5408b4..007ff387f4 100644 --- a/test_runner/fixtures/pageserver/utils.py +++ b/test_runner/fixtures/pageserver/utils.py @@ -249,7 +249,7 @@ def assert_prefix_empty(neon_env_builder: "NeonEnvBuilder", prefix: Optional[str # this has been seen in the wild by tests with the below contradicting logging # https://neon-github-public-dev.s3.amazonaws.com/reports/pr-5322/6207777020/index.html#suites/3556ed71f2d69272a7014df6dcb02317/53b5c368b5a68865 # this seems like a mock_s3 issue - log.warn( + log.warning( f"contrading ListObjectsV2 response with KeyCount={keys} and Contents={objects}, CommonPrefixes={common_prefixes}, assuming this means KeyCount=0" ) keys = 0 @@ -257,7 +257,7 @@ def assert_prefix_empty(neon_env_builder: "NeonEnvBuilder", prefix: Optional[str # this has been seen in one case with mock_s3: # https://neon-github-public-dev.s3.amazonaws.com/reports/pr-4938/6000769714/index.html#suites/3556ed71f2d69272a7014df6dcb02317/ca01e4f4d8d9a11f # looking at moto impl, it might be there's a race with common prefix (sub directory) not going away with deletes - log.warn( + log.warning( f"contradicting ListObjectsV2 response with KeyCount={keys} and Contents={objects}, CommonPrefixes={common_prefixes}" )