From eb02f4619e7cccdab7c4553b6ad257994b9460a0 Mon Sep 17 00:00:00 2001 From: John Spray Date: Tue, 20 Feb 2024 16:34:12 +0000 Subject: [PATCH 01/15] tests: add a shutdown log noise case to test_location_conf_churn (#6828) This test does lots of shutdowns, and we may emit this layer warning during shutdown. Saw a spurious failure here: https://neon-github-public-dev.s3.amazonaws.com/reports/pr-6820/7964134049/index.html#/testresult/784218040583d963 --- test_runner/regress/test_pageserver_secondary.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test_runner/regress/test_pageserver_secondary.py b/test_runner/regress/test_pageserver_secondary.py index cbff01dc2a..8f694de2e1 100644 --- a/test_runner/regress/test_pageserver_secondary.py +++ b/test_runner/regress/test_pageserver_secondary.py @@ -74,16 +74,19 @@ def test_location_conf_churn(neon_env_builder: NeonEnvBuilder, seed: int): tenant_id = env.initial_tenant timeline_id = env.initial_timeline - # We will make no effort to avoid stale attachments for ps in env.pageservers: ps.allowed_errors.extend( [ + # We will make no effort to avoid stale attachments ".*Dropped remote consistent LSN updates.*", ".*Dropping stale deletions.*", # page_service_conn_main{peer_addr=[::1]:41176}: query handler for 'pagestream 3b19aec5038c796f64b430b30a555121 d07776761d44050b8aab511df1657d83' failed: Tenant 3b19aec5038c796f64b430b30a555121 not found ".*query handler.*Tenant.*not found.*", # page_service_conn_main{peer_addr=[::1]:45552}: query handler for 'pagestream 414ede7ad50f775a8e7d9ba0e43b9efc a43884be16f44b3626482b6981b2c745' failed: Tenant 414ede7ad50f775a8e7d9ba0e43b9efc is not active ".*query handler.*Tenant.*not active.*", + # this shutdown case is logged at WARN severity by the time it bubbles up to logical size calculation code + # WARN ...: initial size calculation failed: downloading failed, possibly for shutdown + ".*downloading failed, possibly for shutdown", ] ) From e49602ecf59ea0bc5be43990241c408be4de8d65 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Tue, 20 Feb 2024 17:52:23 +0100 Subject: [PATCH 02/15] feat(metrics): per-timeline metric for on-demand downloads, remove calls_started histogram (#6834) refs #6737 # Problem Before this PR, on-demand downloads weren't measured per tenant_id. This makes root-cause analysis of latency spikes harder, requiring us to resort to log scraping for ``` {neon_service="pageserver"} |= `downloading on-demand` |= `$tenant_id` ``` which can be expensive when zooming out in Grafana. Context: https://neondb.slack.com/archives/C033RQ5SPDH/p1707809037868189 # Solution / Changes - Remove the calls_started histogram - I did the dilegence, there are only 2 dashboards using this histogram, and in fact only one uses it as a histogram, the other just as a a counter. - [Link 1](https://github.com/neondatabase/grafana-dashboard-export/blob/8115b54d9fa14c76da831ae21bbfbb56cc59ffb5/neonprod/dashboards/hkXNF7oVz/dashboard-Z31XmM24k.yaml#L1454): `Pageserver Thrashing` dashboard, linked from playbook, will fix. - [Link 2](https://github.com/neondatabase/grafana-dashboard-export/blob/8115b54d9fa14c76da831ae21bbfbb56cc59ffb5/neonprod/dashboards/CEllzAO4z/dashboard-sJqfNFL4k.yaml#L599): one of my personal dashboards, unused for a long time, already broken in other ways, no need to fix. - replace `pageserver_remote_timeline_client_calls_unfinished` gauge with a counter pair - Required `Clone`-able `IntCounterPair`, made the necessary changes in the `libs/metrics` crate - fix tests to deal with the fallout A subsequent PR will remove a timeline-scoped metric to compensate. Note that we don't need additional global counters for the per-timeline counters affected by this PR; we can use the `remote_storage` histogram for those, which, conveniently, also include the secondary-mode downloads, which aren't covered by the remote timeline client metrics (should they?). --- libs/metrics/src/lib.rs | 14 +++ pageserver/src/metrics.rs | 99 ++++++------------- .../src/tenant/remote_timeline_client.rs | 22 ++--- test_runner/fixtures/metrics.py | 17 ++-- test_runner/fixtures/pageserver/http.py | 29 +++--- test_runner/fixtures/pageserver/utils.py | 36 +++++-- test_runner/regress/test_remote_storage.py | 44 +++------ 7 files changed, 125 insertions(+), 136 deletions(-) diff --git a/libs/metrics/src/lib.rs b/libs/metrics/src/lib.rs index 18786106d1..744fc18e61 100644 --- a/libs/metrics/src/lib.rs +++ b/libs/metrics/src/lib.rs @@ -201,6 +201,11 @@ impl GenericCounterPairVec

{ pub fn with_label_values(&self, vals: &[&str]) -> GenericCounterPair

{ self.get_metric_with_label_values(vals).unwrap() } + + pub fn remove_label_values(&self, res: &mut [Result<()>; 2], vals: &[&str]) { + res[0] = self.inc.remove_label_values(vals); + res[1] = self.dec.remove_label_values(vals); + } } impl GenericCounterPair

{ @@ -247,6 +252,15 @@ impl GenericCounterPair

{ } } +impl Clone for GenericCounterPair

{ + fn clone(&self) -> Self { + Self { + inc: self.inc.clone(), + dec: self.dec.clone(), + } + } +} + /// Guard returned by [`GenericCounterPair::guard`] pub struct GenericCounterPairGuard(GenericCounter

); diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index a0fda39605..ee0bd268cc 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -4,8 +4,8 @@ use metrics::{ register_counter_vec, register_gauge_vec, register_histogram, register_histogram_vec, register_int_counter, register_int_counter_pair_vec, register_int_counter_vec, register_int_gauge, register_int_gauge_vec, register_uint_gauge, register_uint_gauge_vec, - Counter, CounterVec, GaugeVec, Histogram, HistogramVec, IntCounter, IntCounterPairVec, - IntCounterVec, IntGauge, IntGaugeVec, UIntGauge, UIntGaugeVec, + Counter, CounterVec, GaugeVec, Histogram, HistogramVec, IntCounter, IntCounterPair, + IntCounterPairVec, IntCounterVec, IntGauge, IntGaugeVec, UIntGauge, UIntGaugeVec, }; use once_cell::sync::Lazy; use pageserver_api::shard::TenantShardId; @@ -1266,13 +1266,12 @@ pub(crate) static LIVE_CONNECTIONS_COUNT: Lazy = Lazy::new(|| { // remote storage metrics -/// NB: increment _after_ recording the current value into [`REMOTE_TIMELINE_CLIENT_CALLS_STARTED_HIST`]. -static REMOTE_TIMELINE_CLIENT_CALLS_UNFINISHED_GAUGE: Lazy = Lazy::new(|| { - register_int_gauge_vec!( - "pageserver_remote_timeline_client_calls_unfinished", - "Number of ongoing calls to remote timeline client. \ - Used to populate pageserver_remote_timeline_client_calls_started. \ - This metric is not useful for sampling from Prometheus, but useful in tests.", +static REMOTE_TIMELINE_CLIENT_CALLS: Lazy = Lazy::new(|| { + register_int_counter_pair_vec!( + "pageserver_remote_timeline_client_calls_started", + "Number of started calls to remote timeline client.", + "pageserver_remote_timeline_client_calls_finished", + "Number of finshed calls to remote timeline client.", &[ "tenant_id", "shard_id", @@ -1281,23 +1280,7 @@ static REMOTE_TIMELINE_CLIENT_CALLS_UNFINISHED_GAUGE: Lazy = Lazy:: "op_kind" ], ) - .expect("failed to define a metric") -}); - -static REMOTE_TIMELINE_CLIENT_CALLS_STARTED_HIST: Lazy = Lazy::new(|| { - register_histogram_vec!( - "pageserver_remote_timeline_client_calls_started", - "When calling a remote timeline client method, we record the current value \ - of the calls_unfinished gauge in this histogram. Plot the histogram \ - over time in a heatmap to visualize how many operations were ongoing \ - at a given instant. It gives you a better idea of the queue depth \ - than plotting the gauge directly, since operations may complete faster \ - than the sampling interval.", - &["file_kind", "op_kind"], - // The calls_unfinished gauge is an integer gauge, hence we have integer buckets. - vec![0.0, 1.0, 2.0, 4.0, 6.0, 8.0, 10.0, 15.0, 20.0, 40.0, 60.0, 80.0, 100.0, 500.0], - ) - .expect("failed to define a metric") + .unwrap() }); static REMOTE_TIMELINE_CLIENT_BYTES_STARTED_COUNTER: Lazy = @@ -2078,7 +2061,7 @@ pub(crate) struct RemoteTimelineClientMetrics { shard_id: String, timeline_id: String, remote_physical_size_gauge: Mutex>, - calls_unfinished_gauge: Mutex>, + calls: Mutex>, bytes_started_counter: Mutex>, bytes_finished_counter: Mutex>, } @@ -2089,7 +2072,7 @@ impl RemoteTimelineClientMetrics { tenant_id: tenant_shard_id.tenant_id.to_string(), shard_id: format!("{}", tenant_shard_id.shard_slug()), timeline_id: timeline_id.to_string(), - calls_unfinished_gauge: Mutex::new(HashMap::default()), + calls: Mutex::new(HashMap::default()), bytes_started_counter: Mutex::new(HashMap::default()), bytes_finished_counter: Mutex::new(HashMap::default()), remote_physical_size_gauge: Mutex::new(None), @@ -2129,15 +2112,15 @@ impl RemoteTimelineClientMetrics { .unwrap() } - fn calls_unfinished_gauge( + fn calls_counter_pair( &self, file_kind: &RemoteOpFileKind, op_kind: &RemoteOpKind, - ) -> IntGauge { - let mut guard = self.calls_unfinished_gauge.lock().unwrap(); + ) -> IntCounterPair { + let mut guard = self.calls.lock().unwrap(); let key = (file_kind.as_str(), op_kind.as_str()); let metric = guard.entry(key).or_insert_with(move || { - REMOTE_TIMELINE_CLIENT_CALLS_UNFINISHED_GAUGE + REMOTE_TIMELINE_CLIENT_CALLS .get_metric_with_label_values(&[ &self.tenant_id, &self.shard_id, @@ -2150,17 +2133,6 @@ impl RemoteTimelineClientMetrics { metric.clone() } - fn calls_started_hist( - &self, - file_kind: &RemoteOpFileKind, - op_kind: &RemoteOpKind, - ) -> Histogram { - let key = (file_kind.as_str(), op_kind.as_str()); - REMOTE_TIMELINE_CLIENT_CALLS_STARTED_HIST - .get_metric_with_label_values(&[key.0, key.1]) - .unwrap() - } - fn bytes_started_counter( &self, file_kind: &RemoteOpFileKind, @@ -2231,7 +2203,7 @@ impl RemoteTimelineClientMetrics { #[must_use] pub(crate) struct RemoteTimelineClientCallMetricGuard { /// Decremented on drop. - calls_unfinished_metric: Option, + calls_counter_pair: Option, /// If Some(), this references the bytes_finished metric, and we increment it by the given `u64` on drop. bytes_finished: Option<(IntCounter, u64)>, } @@ -2241,10 +2213,10 @@ impl RemoteTimelineClientCallMetricGuard { /// The caller vouches to do the metric updates manually. pub fn will_decrement_manually(mut self) { let RemoteTimelineClientCallMetricGuard { - calls_unfinished_metric, + calls_counter_pair, bytes_finished, } = &mut self; - calls_unfinished_metric.take(); + calls_counter_pair.take(); bytes_finished.take(); } } @@ -2252,10 +2224,10 @@ impl RemoteTimelineClientCallMetricGuard { impl Drop for RemoteTimelineClientCallMetricGuard { fn drop(&mut self) { let RemoteTimelineClientCallMetricGuard { - calls_unfinished_metric, + calls_counter_pair, bytes_finished, } = self; - if let Some(guard) = calls_unfinished_metric.take() { + if let Some(guard) = calls_counter_pair.take() { guard.dec(); } if let Some((bytes_finished_metric, value)) = bytes_finished { @@ -2288,10 +2260,8 @@ impl RemoteTimelineClientMetrics { op_kind: &RemoteOpKind, size: RemoteTimelineClientMetricsCallTrackSize, ) -> RemoteTimelineClientCallMetricGuard { - let calls_unfinished_metric = self.calls_unfinished_gauge(file_kind, op_kind); - self.calls_started_hist(file_kind, op_kind) - .observe(calls_unfinished_metric.get() as f64); - calls_unfinished_metric.inc(); // NB: inc after the histogram, see comment on underlying metric + let calls_counter_pair = self.calls_counter_pair(file_kind, op_kind); + calls_counter_pair.inc(); let bytes_finished = match size { RemoteTimelineClientMetricsCallTrackSize::DontTrackSize { reason: _reason } => { @@ -2305,7 +2275,7 @@ impl RemoteTimelineClientMetrics { } }; RemoteTimelineClientCallMetricGuard { - calls_unfinished_metric: Some(calls_unfinished_metric), + calls_counter_pair: Some(calls_counter_pair), bytes_finished, } } @@ -2319,12 +2289,8 @@ impl RemoteTimelineClientMetrics { op_kind: &RemoteOpKind, size: RemoteTimelineClientMetricsCallTrackSize, ) { - let calls_unfinished_metric = self.calls_unfinished_gauge(file_kind, op_kind); - debug_assert!( - calls_unfinished_metric.get() > 0, - "begin and end should cancel out" - ); - calls_unfinished_metric.dec(); + let calls_counter_pair = self.calls_counter_pair(file_kind, op_kind); + calls_counter_pair.dec(); match size { RemoteTimelineClientMetricsCallTrackSize::DontTrackSize { reason: _reason } => {} RemoteTimelineClientMetricsCallTrackSize::Bytes(size) => { @@ -2341,18 +2307,15 @@ impl Drop for RemoteTimelineClientMetrics { shard_id, timeline_id, remote_physical_size_gauge, - calls_unfinished_gauge, + calls, bytes_started_counter, bytes_finished_counter, } = self; - for ((a, b), _) in calls_unfinished_gauge.get_mut().unwrap().drain() { - let _ = REMOTE_TIMELINE_CLIENT_CALLS_UNFINISHED_GAUGE.remove_label_values(&[ - tenant_id, - shard_id, - timeline_id, - a, - b, - ]); + for ((a, b), _) in calls.get_mut().unwrap().drain() { + let mut res = [Ok(()), Ok(())]; + REMOTE_TIMELINE_CLIENT_CALLS + .remove_label_values(&mut res, &[tenant_id, shard_id, timeline_id, a, b]); + // don't care about results } for ((a, b), _) in bytes_started_counter.get_mut().unwrap().drain() { let _ = REMOTE_TIMELINE_CLIENT_BYTES_STARTED_COUNTER.remove_label_values(&[ diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index 547679c435..7d30745a0d 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -614,7 +614,7 @@ impl RemoteTimelineClient { metadata, ); let op = UploadOp::UploadMetadata(index_part, disk_consistent_lsn); - self.calls_unfinished_metric_begin(&op); + self.metric_begin(&op); upload_queue.queued_operations.push_back(op); upload_queue.latest_files_changes_since_metadata_upload_scheduled = 0; @@ -654,7 +654,7 @@ impl RemoteTimelineClient { metadata.generation, metadata.shard ); let op = UploadOp::UploadLayer(layer, metadata); - self.calls_unfinished_metric_begin(&op); + self.metric_begin(&op); upload_queue.queued_operations.push_back(op); } @@ -830,7 +830,7 @@ impl RemoteTimelineClient { let op = UploadOp::Delete(Delete { layers: with_metadata, }); - self.calls_unfinished_metric_begin(&op); + self.metric_begin(&op); upload_queue.queued_operations.push_back(op); } @@ -1520,10 +1520,10 @@ impl RemoteTimelineClient { .await; } - self.calls_unfinished_metric_end(&task.op); + self.metric_end(&task.op); } - fn calls_unfinished_metric_impl( + fn metric_impl( &self, op: &UploadOp, ) -> Option<( @@ -1560,17 +1560,17 @@ impl RemoteTimelineClient { Some(res) } - fn calls_unfinished_metric_begin(&self, op: &UploadOp) { - let (file_kind, op_kind, track_bytes) = match self.calls_unfinished_metric_impl(op) { + fn metric_begin(&self, op: &UploadOp) { + let (file_kind, op_kind, track_bytes) = match self.metric_impl(op) { Some(x) => x, None => return, }; let guard = self.metrics.call_begin(&file_kind, &op_kind, track_bytes); - guard.will_decrement_manually(); // in unfinished_ops_metric_end() + guard.will_decrement_manually(); // in metric_end(), see right below } - fn calls_unfinished_metric_end(&self, op: &UploadOp) { - let (file_kind, op_kind, track_bytes) = match self.calls_unfinished_metric_impl(op) { + fn metric_end(&self, op: &UploadOp) { + let (file_kind, op_kind, track_bytes) = match self.metric_impl(op) { Some(x) => x, None => return, }; @@ -1655,7 +1655,7 @@ impl RemoteTimelineClient { // Tear down queued ops for op in qi.queued_operations.into_iter() { - self.calls_unfinished_metric_end(&op); + self.metric_end(&op); // Dropping UploadOp::Barrier() here will make wait_completion() return with an Err() // which is exactly what we want to happen. drop(op); diff --git a/test_runner/fixtures/metrics.py b/test_runner/fixtures/metrics.py index f433db2167..fd4618ca6a 100644 --- a/test_runner/fixtures/metrics.py +++ b/test_runner/fixtures/metrics.py @@ -54,7 +54,7 @@ class MetricsGetter: return results[0].value def get_metrics_values( - self, names: list[str], filter: Optional[Dict[str, str]] = None + self, names: list[str], filter: Optional[Dict[str, str]] = None, absence_ok=False ) -> Dict[str, float]: """ When fetching multiple named metrics, it is more efficient to use this @@ -63,6 +63,10 @@ class MetricsGetter: Throws RuntimeError if no metrics matching `names` are found, or if not all of `names` are found: this method is intended for loading sets of metrics whose existence is coupled. + + If it's expected that there may be no results for some of the metrics, + specify `absence_ok=True`. The returned dict will then not contain values + for these metrics. """ metrics = self.get_metrics() samples = [] @@ -75,9 +79,10 @@ class MetricsGetter: raise RuntimeError(f"Multiple values found for {sample.name}") result[sample.name] = sample.value - if len(result) != len(names): - log.info(f"Metrics found: {metrics.metrics}") - raise RuntimeError(f"could not find all metrics {' '.join(names)}") + if not absence_ok: + if len(result) != len(names): + log.info(f"Metrics found: {metrics.metrics}") + raise RuntimeError(f"could not find all metrics {' '.join(names)}") return result @@ -98,7 +103,8 @@ def histogram(prefix_without_trailing_underscore: str) -> List[str]: PAGESERVER_PER_TENANT_REMOTE_TIMELINE_CLIENT_METRICS: Tuple[str, ...] = ( - "pageserver_remote_timeline_client_calls_unfinished", + "pageserver_remote_timeline_client_calls_started_total", + "pageserver_remote_timeline_client_calls_finished_total", "pageserver_remote_physical_size", "pageserver_remote_timeline_client_bytes_started_total", "pageserver_remote_timeline_client_bytes_finished_total", @@ -127,7 +133,6 @@ PAGESERVER_GLOBAL_METRICS: Tuple[str, ...] = ( *histogram("pageserver_getpage_get_reconstruct_data_seconds"), *histogram("pageserver_wait_lsn_seconds"), *histogram("pageserver_remote_operation_seconds"), - *histogram("pageserver_remote_timeline_client_calls_started"), *histogram("pageserver_io_operations_seconds"), "pageserver_tenant_states_count", ) diff --git a/test_runner/fixtures/pageserver/http.py b/test_runner/fixtures/pageserver/http.py index 6af3b6a912..d4583308ff 100644 --- a/test_runner/fixtures/pageserver/http.py +++ b/test_runner/fixtures/pageserver/http.py @@ -694,32 +694,33 @@ class PageserverHttpClient(requests.Session, MetricsGetter): }, ).value - def get_remote_timeline_client_metric( + def get_remote_timeline_client_queue_count( self, - metric_name: str, tenant_id: TenantId, timeline_id: TimelineId, file_kind: str, op_kind: str, - ) -> Optional[float]: - metrics = self.get_metrics() - matches = metrics.query_all( - name=metric_name, + ) -> Optional[int]: + metrics = [ + "pageserver_remote_timeline_client_calls_started_total", + "pageserver_remote_timeline_client_calls_finished_total", + ] + res = self.get_metrics_values( + metrics, filter={ "tenant_id": str(tenant_id), "timeline_id": str(timeline_id), "file_kind": str(file_kind), "op_kind": str(op_kind), }, + absence_ok=True, ) - if len(matches) == 0: - value = None - elif len(matches) == 1: - value = matches[0].value - assert value is not None - else: - assert len(matches) < 2, "above filter should uniquely identify metric" - return value + if len(res) != 2: + return None + inc, dec = [res[metric] for metric in metrics] + queue_count = int(inc) - int(dec) + assert queue_count >= 0 + return queue_count def layer_map_info( self, diff --git a/test_runner/fixtures/pageserver/utils.py b/test_runner/fixtures/pageserver/utils.py index 201a34f964..1812eb438d 100644 --- a/test_runner/fixtures/pageserver/utils.py +++ b/test_runner/fixtures/pageserver/utils.py @@ -1,5 +1,5 @@ import time -from typing import Any, Dict, List, Optional, Union +from typing import Any, Dict, List, Optional, Tuple, Union from mypy_boto3_s3.type_defs import ( DeleteObjectOutputTypeDef, @@ -221,16 +221,40 @@ def wait_for_upload_queue_empty( ): while True: all_metrics = pageserver_http.get_metrics() - tl = all_metrics.query_all( - "pageserver_remote_timeline_client_calls_unfinished", + started = all_metrics.query_all( + "pageserver_remote_timeline_client_calls_started_total", { "tenant_id": str(tenant_id), "timeline_id": str(timeline_id), }, ) - assert len(tl) > 0 - log.info(f"upload queue for {tenant_id}/{timeline_id}: {tl}") - if all(m.value == 0 for m in tl): + finished = all_metrics.query_all( + "pageserver_remote_timeline_client_calls_finished_total", + { + "tenant_id": str(tenant_id), + "timeline_id": str(timeline_id), + }, + ) + assert len(started) == len(finished) + # this is `started left join finished`; if match, subtracting start from finished, resulting in queue depth + remaining_labels = ["shard_id", "file_kind", "op_kind"] + tl: List[Tuple[Any, float]] = [] + for s in started: + found = False + for f in finished: + if all([s.labels[label] == f.labels[label] for label in remaining_labels]): + assert ( + not found + ), "duplicate match, remaining_labels don't uniquely identify sample" + tl.append((s.labels, int(s.value) - int(f.value))) + found = True + if not found: + tl.append((s.labels, int(s.value))) + assert len(tl) == len(started), "something broken with join logic" + log.info(f"upload queue for {tenant_id}/{timeline_id}:") + for labels, queue_count in tl: + log.info(f" {labels}: {queue_count}") + if all(queue_count == 0 for (_, queue_count) in tl): return time.sleep(0.2) diff --git a/test_runner/regress/test_remote_storage.py b/test_runner/regress/test_remote_storage.py index 32b4f54fbd..18eba6e1c3 100644 --- a/test_runner/regress/test_remote_storage.py +++ b/test_runner/regress/test_remote_storage.py @@ -274,15 +274,9 @@ def test_remote_storage_upload_queue_retries( wait_for_last_flush_lsn(env, endpoint, tenant_id, timeline_id) def get_queued_count(file_kind, op_kind): - val = client.get_remote_timeline_client_metric( - "pageserver_remote_timeline_client_calls_unfinished", - tenant_id, - timeline_id, - file_kind, - op_kind, + return client.get_remote_timeline_client_queue_count( + tenant_id, timeline_id, file_kind, op_kind ) - assert val is not None, "expecting metric to be present" - return int(val) # create some layers & wait for uploads to finish overwrite_data_and_wait_for_it_to_arrive_at_pageserver("a") @@ -434,7 +428,7 @@ def test_remote_timeline_client_calls_started_metric( assert timeline_id is not None for (file_kind, op_kind), observations in calls_started.items(): val = client.get_metric_value( - name="pageserver_remote_timeline_client_calls_started_count", + name="pageserver_remote_timeline_client_calls_started_total", filter={ "file_kind": str(file_kind), "op_kind": str(op_kind), @@ -537,16 +531,6 @@ def test_timeline_deletion_with_files_stuck_in_upload_queue( client = env.pageserver.http_client() - def get_queued_count(file_kind, op_kind): - val = client.get_remote_timeline_client_metric( - "pageserver_remote_timeline_client_calls_unfinished", - tenant_id, - timeline_id, - file_kind, - op_kind, - ) - return int(val) if val is not None else val - endpoint = env.endpoints.create_start("main", tenant_id=tenant_id) client.configure_failpoints(("before-upload-layer", "return")) @@ -580,7 +564,10 @@ def test_timeline_deletion_with_files_stuck_in_upload_queue( def assert_compacted_and_uploads_queued(): assert timeline_path.exists() assert len(list(timeline_path.glob("*"))) >= 8 - assert get_queued_count(file_kind="index", op_kind="upload") > 0 + assert ( + get_queued_count(client, tenant_id, timeline_id, file_kind="index", op_kind="upload") + > 0 + ) wait_until(20, 0.1, assert_compacted_and_uploads_queued) @@ -618,7 +605,10 @@ def test_timeline_deletion_with_files_stuck_in_upload_queue( assert len(filtered) == 0 # timeline deletion should kill ongoing uploads, so, the metric will be gone - assert get_queued_count(file_kind="index", op_kind="upload") is None + assert ( + get_queued_count(client, tenant_id, timeline_id, file_kind="index", op_kind="upload") + is None + ) # timeline deletion should be unblocking checkpoint ops checkpoint_thread.join(2.0) @@ -919,16 +909,8 @@ def get_queued_count( file_kind: str, op_kind: str, ): - val = client.get_remote_timeline_client_metric( - "pageserver_remote_timeline_client_calls_unfinished", - tenant_id, - timeline_id, - file_kind, - op_kind, - ) - if val is None: - return val - return int(val) + """The most important aspect of this function is shorter name & no return type so asserts are more concise.""" + return client.get_remote_timeline_client_queue_count(tenant_id, timeline_id, file_kind, op_kind) def assert_nothing_to_upload( From cbb599f353a7489e18201dbbcf8e7d596f9bfb66 Mon Sep 17 00:00:00 2001 From: Nikita Kalyanov <44959448+nikitakalyanov@users.noreply.github.com> Date: Tue, 20 Feb 2024 19:42:36 +0200 Subject: [PATCH 03/15] Add /terminate API (#6745) this is to speed up suspends, see https://github.com/neondatabase/cloud/issues/10284 ## Problem ## Summary of changes ## Checklist before requesting a review - [ ] I have performed a self-review of my code. - [ ] If it is a core feature, I have added thorough tests. - [ ] Do we need to implement analytics? if so did you add the relevant metrics to the dashboard? - [ ] If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section. ## Checklist before merging - [ ] Do not forget to reformat commit message to not include the above checklist --- compute_tools/src/bin/compute_ctl.rs | 25 +++++------ compute_tools/src/compute.rs | 16 +++++++ compute_tools/src/http/api.rs | 55 ++++++++++++++++++++++++ compute_tools/src/http/openapi_spec.yaml | 23 ++++++++++ control_plane/src/endpoint.rs | 4 +- libs/compute_api/src/responses.rs | 4 ++ 6 files changed, 114 insertions(+), 13 deletions(-) diff --git a/compute_tools/src/bin/compute_ctl.rs b/compute_tools/src/bin/compute_ctl.rs index a7e10d0aee..117919786e 100644 --- a/compute_tools/src/bin/compute_ctl.rs +++ b/compute_tools/src/bin/compute_ctl.rs @@ -45,7 +45,6 @@ use std::{thread, time::Duration}; use anyhow::{Context, Result}; use chrono::Utc; use clap::Arg; -use nix::sys::signal::{kill, Signal}; use signal_hook::consts::{SIGQUIT, SIGTERM}; use signal_hook::{consts::SIGINT, iterator::Signals}; use tracing::{error, info}; @@ -53,7 +52,9 @@ use url::Url; use compute_api::responses::ComputeStatus; -use compute_tools::compute::{ComputeNode, ComputeState, ParsedSpec, PG_PID, SYNC_SAFEKEEPERS_PID}; +use compute_tools::compute::{ + forward_termination_signal, ComputeNode, ComputeState, ParsedSpec, PG_PID, +}; use compute_tools::configurator::launch_configurator; use compute_tools::extension_server::get_pg_version; use compute_tools::http::api::launch_http_server; @@ -394,6 +395,15 @@ fn main() -> Result<()> { info!("synced safekeepers at lsn {lsn}"); } + let mut state = compute.state.lock().unwrap(); + if state.status == ComputeStatus::TerminationPending { + state.status = ComputeStatus::Terminated; + compute.state_changed.notify_all(); + // we were asked to terminate gracefully, don't exit to avoid restart + delay_exit = true + } + drop(state); + if let Err(err) = compute.check_for_core_dumps() { error!("error while checking for core dumps: {err:?}"); } @@ -523,16 +533,7 @@ fn cli() -> clap::Command { /// wait for termination which would be easy then. fn handle_exit_signal(sig: i32) { info!("received {sig} termination signal"); - let ss_pid = SYNC_SAFEKEEPERS_PID.load(Ordering::SeqCst); - if ss_pid != 0 { - let ss_pid = nix::unistd::Pid::from_raw(ss_pid as i32); - kill(ss_pid, Signal::SIGTERM).ok(); - } - let pg_pid = PG_PID.load(Ordering::SeqCst); - if pg_pid != 0 { - let pg_pid = nix::unistd::Pid::from_raw(pg_pid as i32); - kill(pg_pid, Signal::SIGTERM).ok(); - } + forward_termination_signal(); exit(1); } diff --git a/compute_tools/src/compute.rs b/compute_tools/src/compute.rs index 1c5363d048..142bb14fe5 100644 --- a/compute_tools/src/compute.rs +++ b/compute_tools/src/compute.rs @@ -28,6 +28,8 @@ use compute_api::responses::{ComputeMetrics, ComputeStatus}; use compute_api::spec::{ComputeFeature, ComputeMode, ComputeSpec}; use utils::measured_stream::MeasuredReader; +use nix::sys::signal::{kill, Signal}; + use remote_storage::{DownloadError, RemotePath}; use crate::checker::create_availability_check_data; @@ -1322,3 +1324,17 @@ LIMIT 100", Ok(remote_ext_metrics) } } + +pub fn forward_termination_signal() { + let ss_pid = SYNC_SAFEKEEPERS_PID.load(Ordering::SeqCst); + if ss_pid != 0 { + let ss_pid = nix::unistd::Pid::from_raw(ss_pid as i32); + kill(ss_pid, Signal::SIGTERM).ok(); + } + let pg_pid = PG_PID.load(Ordering::SeqCst); + if pg_pid != 0 { + let pg_pid = nix::unistd::Pid::from_raw(pg_pid as i32); + // use 'immediate' shutdown (SIGQUIT): https://www.postgresql.org/docs/current/server-shutdown.html + kill(pg_pid, Signal::SIGQUIT).ok(); + } +} diff --git a/compute_tools/src/http/api.rs b/compute_tools/src/http/api.rs index fa2c4cff28..f076951239 100644 --- a/compute_tools/src/http/api.rs +++ b/compute_tools/src/http/api.rs @@ -5,6 +5,7 @@ use std::net::SocketAddr; use std::sync::Arc; use std::thread; +use crate::compute::forward_termination_signal; use crate::compute::{ComputeNode, ComputeState, ParsedSpec}; use compute_api::requests::ConfigurationRequest; use compute_api::responses::{ComputeStatus, ComputeStatusResponse, GenericAPIError}; @@ -123,6 +124,17 @@ async fn routes(req: Request, compute: &Arc) -> Response { + info!("serving /terminate POST request"); + match handle_terminate_request(compute).await { + Ok(()) => Response::new(Body::empty()), + Err((msg, code)) => { + error!("error handling /terminate request: {msg}"); + render_json_error(&msg, code) + } + } + } + // download extension files from remote extension storage on demand (&Method::POST, route) if route.starts_with("/extension_server/") => { info!("serving {:?} POST request", route); @@ -297,6 +309,49 @@ fn render_json_error(e: &str, status: StatusCode) -> Response { .unwrap() } +async fn handle_terminate_request(compute: &Arc) -> Result<(), (String, StatusCode)> { + { + let mut state = compute.state.lock().unwrap(); + if state.status == ComputeStatus::Terminated { + return Ok(()); + } + if state.status != ComputeStatus::Empty && state.status != ComputeStatus::Running { + let msg = format!( + "invalid compute status for termination request: {:?}", + state.status.clone() + ); + return Err((msg, StatusCode::PRECONDITION_FAILED)); + } + state.status = ComputeStatus::TerminationPending; + compute.state_changed.notify_all(); + drop(state); + } + forward_termination_signal(); + info!("sent signal and notified waiters"); + + // Spawn a blocking thread to wait for compute to become Terminated. + // This is needed to do not block the main pool of workers and + // be able to serve other requests while some particular request + // is waiting for compute to finish configuration. + let c = compute.clone(); + task::spawn_blocking(move || { + let mut state = c.state.lock().unwrap(); + while state.status != ComputeStatus::Terminated { + state = c.state_changed.wait(state).unwrap(); + info!( + "waiting for compute to become Terminated, current status: {:?}", + state.status + ); + } + + Ok(()) + }) + .await + .unwrap()?; + info!("terminated Postgres"); + Ok(()) +} + // Main Hyper HTTP server function that runs it and blocks waiting on it forever. #[tokio::main] async fn serve(port: u16, state: Arc) { diff --git a/compute_tools/src/http/openapi_spec.yaml b/compute_tools/src/http/openapi_spec.yaml index cedc6ece8f..d2ec54299f 100644 --- a/compute_tools/src/http/openapi_spec.yaml +++ b/compute_tools/src/http/openapi_spec.yaml @@ -168,6 +168,29 @@ paths: schema: $ref: "#/components/schemas/GenericError" + /terminate: + post: + tags: + - Terminate + summary: Terminate Postgres and wait for it to exit + description: "" + operationId: terminate + responses: + 200: + description: Result + 412: + description: "wrong state" + content: + application/json: + schema: + $ref: "#/components/schemas/GenericError" + 500: + description: "Unexpected error" + content: + application/json: + schema: + $ref: "#/components/schemas/GenericError" + components: securitySchemes: JWT: diff --git a/control_plane/src/endpoint.rs b/control_plane/src/endpoint.rs index f1fe12e05f..ce8f035dfc 100644 --- a/control_plane/src/endpoint.rs +++ b/control_plane/src/endpoint.rs @@ -652,7 +652,9 @@ impl Endpoint { } ComputeStatus::Empty | ComputeStatus::ConfigurationPending - | ComputeStatus::Configuration => { + | ComputeStatus::Configuration + | ComputeStatus::TerminationPending + | ComputeStatus::Terminated => { bail!("unexpected compute status: {:?}", state.status) } } diff --git a/libs/compute_api/src/responses.rs b/libs/compute_api/src/responses.rs index 92bbf79cd4..fd0c90d447 100644 --- a/libs/compute_api/src/responses.rs +++ b/libs/compute_api/src/responses.rs @@ -52,6 +52,10 @@ pub enum ComputeStatus { // compute will exit soon or is waiting for // control-plane to terminate it. Failed, + // Termination requested + TerminationPending, + // Terminated Postgres + Terminated, } fn rfc3339_serialize(x: &Option>, s: S) -> Result From fcbe9fb1840b7628fd242eec3bfd0df83535d0f7 Mon Sep 17 00:00:00 2001 From: Vlad Lazar Date: Tue, 20 Feb 2024 19:42:54 +0000 Subject: [PATCH 04/15] test: adjust checkpoint distance in `test_layer_map` (#6842) https://github.com/neondatabase/neon/commit/587cb705b898565d459d044df84d1ac2633f00bf changed the layer rolling logic to more closely obey the `checkpoint_distance` config. Previously, this test was getting layers significantly larger than the 8K it was asking for. Now the payload in the layers is closer to 8K (which means more layers in total). Tweak the `checkpoint_distance` to get a number of layers more reasonable for this test. Note that we still get more layers than before (~8K vs ~5K). --- test_runner/performance/test_layer_map.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_runner/performance/test_layer_map.py b/test_runner/performance/test_layer_map.py index 6bd0d85fa2..9b20954d45 100644 --- a/test_runner/performance/test_layer_map.py +++ b/test_runner/performance/test_layer_map.py @@ -17,10 +17,10 @@ def test_layer_map(neon_env_builder: NeonEnvBuilder, zenbenchmark): tenant, _ = env.neon_cli.create_tenant( conf={ "gc_period": "0s", - "checkpoint_distance": "8192", + "checkpoint_distance": "16384", "compaction_period": "1 s", "compaction_threshold": "1", - "compaction_target_size": "8192", + "compaction_target_size": "16384", } ) From 04190a1fea389138f1851630d340030cf73758ef Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Tue, 20 Feb 2024 20:45:00 +0000 Subject: [PATCH 05/15] CI(test_runner): misc small changes (#6801) ## Problem A set of small changes that are too small to open a separate for each. A notable change is adding `pytest-repeat` plugin, which can help to ensure that a flaky test is fixed by running such a test several times. ## Summary of changes - Update Allure from 2.24.0 to 2.27.0 - Update Ruff from 0.1.11 to 0.2.2 (update `[tool.ruff]` section of `pyproject.toml` for it) - Install pytest-repeat plugin --- .../actions/allure-report-generate/action.yml | 4 +- poetry.lock | 52 ++++++++++++------- pyproject.toml | 17 ++++-- 3 files changed, 48 insertions(+), 25 deletions(-) diff --git a/.github/actions/allure-report-generate/action.yml b/.github/actions/allure-report-generate/action.yml index 79f054cb06..9a0c79a221 100644 --- a/.github/actions/allure-report-generate/action.yml +++ b/.github/actions/allure-report-generate/action.yml @@ -76,8 +76,8 @@ runs: rm -f ${ALLURE_ZIP} fi env: - ALLURE_VERSION: 2.24.0 - ALLURE_ZIP_SHA256: 60b1d6ce65d9ef24b23cf9c2c19fd736a123487c38e54759f1ed1a7a77353c90 + ALLURE_VERSION: 2.27.0 + ALLURE_ZIP_SHA256: b071858fb2fa542c65d8f152c5c40d26267b2dfb74df1f1608a589ecca38e777 # Potentially we could have several running build for the same key (for example, for the main branch), so we use improvised lock for this - name: Acquire lock diff --git a/poetry.lock b/poetry.lock index 8e1d713d29..347f0a16a7 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2025,6 +2025,20 @@ pytest = [ {version = ">=6.2.4", markers = "python_version >= \"3.10\""}, ] +[[package]] +name = "pytest-repeat" +version = "0.9.3" +description = "pytest plugin for repeating tests" +optional = false +python-versions = ">=3.7" +files = [ + {file = "pytest_repeat-0.9.3-py3-none-any.whl", hash = "sha256:26ab2df18226af9d5ce441c858f273121e92ff55f5bb311d25755b8d7abdd8ed"}, + {file = "pytest_repeat-0.9.3.tar.gz", hash = "sha256:ffd3836dfcd67bb270bec648b330e20be37d2966448c4148c4092d1e8aba8185"}, +] + +[package.dependencies] +pytest = "*" + [[package]] name = "pytest-rerunfailures" version = "13.0" @@ -2257,28 +2271,28 @@ pyasn1 = ">=0.1.3" [[package]] name = "ruff" -version = "0.1.11" +version = "0.2.2" description = "An extremely fast Python linter and code formatter, written in Rust." optional = false python-versions = ">=3.7" files = [ - {file = "ruff-0.1.11-py3-none-macosx_10_12_x86_64.macosx_11_0_arm64.macosx_10_12_universal2.whl", hash = "sha256:a7f772696b4cdc0a3b2e527fc3c7ccc41cdcb98f5c80fdd4f2b8c50eb1458196"}, - {file = "ruff-0.1.11-py3-none-macosx_10_12_x86_64.whl", hash = "sha256:934832f6ed9b34a7d5feea58972635c2039c7a3b434fe5ba2ce015064cb6e955"}, - {file = "ruff-0.1.11-py3-none-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:ea0d3e950e394c4b332bcdd112aa566010a9f9c95814844a7468325290aabfd9"}, - {file = "ruff-0.1.11-py3-none-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:9bd4025b9c5b429a48280785a2b71d479798a69f5c2919e7d274c5f4b32c3607"}, - {file = "ruff-0.1.11-py3-none-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:e1ad00662305dcb1e987f5ec214d31f7d6a062cae3e74c1cbccef15afd96611d"}, - {file = "ruff-0.1.11-py3-none-manylinux_2_17_ppc64.manylinux2014_ppc64.whl", hash = "sha256:4b077ce83f47dd6bea1991af08b140e8b8339f0ba8cb9b7a484c30ebab18a23f"}, - {file = "ruff-0.1.11-py3-none-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:c4a88efecec23c37b11076fe676e15c6cdb1271a38f2b415e381e87fe4517f18"}, - {file = "ruff-0.1.11-py3-none-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:5b25093dad3b055667730a9b491129c42d45e11cdb7043b702e97125bcec48a1"}, - {file = "ruff-0.1.11-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:231d8fb11b2cc7c0366a326a66dafc6ad449d7fcdbc268497ee47e1334f66f77"}, - {file = "ruff-0.1.11-py3-none-musllinux_1_2_aarch64.whl", hash = "sha256:09c415716884950080921dd6237767e52e227e397e2008e2bed410117679975b"}, - {file = "ruff-0.1.11-py3-none-musllinux_1_2_armv7l.whl", hash = "sha256:0f58948c6d212a6b8d41cd59e349751018797ce1727f961c2fa755ad6208ba45"}, - {file = "ruff-0.1.11-py3-none-musllinux_1_2_i686.whl", hash = "sha256:190a566c8f766c37074d99640cd9ca3da11d8deae2deae7c9505e68a4a30f740"}, - {file = "ruff-0.1.11-py3-none-musllinux_1_2_x86_64.whl", hash = "sha256:6464289bd67b2344d2a5d9158d5eb81025258f169e69a46b741b396ffb0cda95"}, - {file = "ruff-0.1.11-py3-none-win32.whl", hash = "sha256:9b8f397902f92bc2e70fb6bebfa2139008dc72ae5177e66c383fa5426cb0bf2c"}, - {file = "ruff-0.1.11-py3-none-win_amd64.whl", hash = "sha256:eb85ee287b11f901037a6683b2374bb0ec82928c5cbc984f575d0437979c521a"}, - {file = "ruff-0.1.11-py3-none-win_arm64.whl", hash = "sha256:97ce4d752f964ba559c7023a86e5f8e97f026d511e48013987623915431c7ea9"}, - {file = "ruff-0.1.11.tar.gz", hash = "sha256:f9d4d88cb6eeb4dfe20f9f0519bd2eaba8119bde87c3d5065c541dbae2b5a2cb"}, + {file = "ruff-0.2.2-py3-none-macosx_10_12_x86_64.macosx_11_0_arm64.macosx_10_12_universal2.whl", hash = "sha256:0a9efb032855ffb3c21f6405751d5e147b0c6b631e3ca3f6b20f917572b97eb6"}, + {file = "ruff-0.2.2-py3-none-macosx_10_12_x86_64.whl", hash = "sha256:d450b7fbff85913f866a5384d8912710936e2b96da74541c82c1b458472ddb39"}, + {file = "ruff-0.2.2-py3-none-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:ecd46e3106850a5c26aee114e562c329f9a1fbe9e4821b008c4404f64ff9ce73"}, + {file = "ruff-0.2.2-py3-none-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:5e22676a5b875bd72acd3d11d5fa9075d3a5f53b877fe7b4793e4673499318ba"}, + {file = "ruff-0.2.2-py3-none-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:1695700d1e25a99d28f7a1636d85bafcc5030bba9d0578c0781ba1790dbcf51c"}, + {file = "ruff-0.2.2-py3-none-manylinux_2_17_ppc64.manylinux2014_ppc64.whl", hash = "sha256:b0c232af3d0bd8f521806223723456ffebf8e323bd1e4e82b0befb20ba18388e"}, + {file = "ruff-0.2.2-py3-none-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:f63d96494eeec2fc70d909393bcd76c69f35334cdbd9e20d089fb3f0640216ca"}, + {file = "ruff-0.2.2-py3-none-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:6a61ea0ff048e06de273b2e45bd72629f470f5da8f71daf09fe481278b175001"}, + {file = "ruff-0.2.2-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:5e1439c8f407e4f356470e54cdecdca1bd5439a0673792dbe34a2b0a551a2fe3"}, + {file = "ruff-0.2.2-py3-none-musllinux_1_2_aarch64.whl", hash = "sha256:940de32dc8853eba0f67f7198b3e79bc6ba95c2edbfdfac2144c8235114d6726"}, + {file = "ruff-0.2.2-py3-none-musllinux_1_2_armv7l.whl", hash = "sha256:0c126da55c38dd917621552ab430213bdb3273bb10ddb67bc4b761989210eb6e"}, + {file = "ruff-0.2.2-py3-none-musllinux_1_2_i686.whl", hash = "sha256:3b65494f7e4bed2e74110dac1f0d17dc8e1f42faaa784e7c58a98e335ec83d7e"}, + {file = "ruff-0.2.2-py3-none-musllinux_1_2_x86_64.whl", hash = "sha256:1ec49be4fe6ddac0503833f3ed8930528e26d1e60ad35c2446da372d16651ce9"}, + {file = "ruff-0.2.2-py3-none-win32.whl", hash = "sha256:d920499b576f6c68295bc04e7b17b6544d9d05f196bb3aac4358792ef6f34325"}, + {file = "ruff-0.2.2-py3-none-win_amd64.whl", hash = "sha256:cc9a91ae137d687f43a44c900e5d95e9617cb37d4c989e462980ba27039d239d"}, + {file = "ruff-0.2.2-py3-none-win_arm64.whl", hash = "sha256:c9d15fc41e6054bfc7200478720570078f0b41c9ae4f010bcc16bd6f4d1aacdd"}, + {file = "ruff-0.2.2.tar.gz", hash = "sha256:e62ed7f36b3068a30ba39193a14274cd706bc486fad521276458022f7bccb31d"}, ] [[package]] @@ -2794,4 +2808,4 @@ cffi = ["cffi (>=1.11)"] [metadata] lock-version = "2.0" python-versions = "^3.9" -content-hash = "cab9cf8cbf8dcd52022acfdabfae4778be3ed5a4afda832bd9c074a50c746763" +content-hash = "af9d5b45310c12411bfe67cb9677d2236808d0780ca1bd81525d2763a928f7f9" diff --git a/pyproject.toml b/pyproject.toml index b498f8acce..6dff112a5e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -39,17 +39,21 @@ types-pytest-lazy-fixture = "^0.6.3.3" pytest-split = "^0.8.1" zstandard = "^0.21.0" httpx = {extras = ["http2"], version = "^0.26.0"} +pytest-repeat = "^0.9.3" [tool.poetry.group.dev.dependencies] mypy = "==1.3.0" -ruff = "^0.1.11" +ruff = "^0.2.2" [build-system] requires = ["poetry-core>=1.0.0"] build-backend = "poetry.core.masonry.api" [tool.mypy] -exclude = "^vendor/" +exclude = [ + "^vendor/", + "^target/", +] check_untyped_defs = true # Help mypy find imports when running against list of individual files. # Without this line it would behave differently when executed on the entire project. @@ -73,7 +77,13 @@ ignore_missing_imports = true [tool.ruff] target-version = "py39" -extend-exclude = ["vendor/"] +extend-exclude = [ + "vendor/", + "target/", +] +line-length = 100 # this setting is rather guidance, it won't fail if it can't make the shorter + +[tool.ruff.lint] ignore = [ "E501", # Line too long, we don't want to be too strict about it ] @@ -84,4 +94,3 @@ select = [ "W", # pycodestyle "B", # bugbear ] -line-length = 100 # this setting is rather guidance, it won't fail if it can't make the shorter From 3882f570016b21dc264418a32e51cc05536c3238 Mon Sep 17 00:00:00 2001 From: Alex Chi Z Date: Tue, 20 Feb 2024 19:20:42 -0500 Subject: [PATCH 06/15] neon_local: add flag to create test user and database (#6848) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This pull request adds two flags: `--update-catalog true` for `endpoint create`, and `--create-test-user true` for `endpoint start`. The former enables catalog updates for neon_superuser permission and many other things, while the latter adds the user `test` and the database `neondb` when setting up the database. A combination of these two flags will create a Postgres similar to the production environment so that it would be easier for us to test if extensions behave correctly when added to Neon Postgres. Example output: ``` ❯ cargo neon endpoint start main --create-test-user true Finished dev [unoptimized + debuginfo] target(s) in 0.22s Running `target/debug/neon_local endpoint start main --create-test-user true` Starting existing endpoint main... Starting postgres node at 'postgresql://cloud_admin@127.0.0.1:55432/postgres' Also at 'postgresql://user@127.0.0.1:55432/neondb' ``` --------- Signed-off-by: Alex Chi Z --- control_plane/src/bin/neon_local.rs | 31 +++++++++++++++++++ control_plane/src/endpoint.rs | 47 +++++++++++++++++++++++------ 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs index a155e9ebb2..5c0d008943 100644 --- a/control_plane/src/bin/neon_local.rs +++ b/control_plane/src/bin/neon_local.rs @@ -652,6 +652,10 @@ async fn handle_timeline(timeline_match: &ArgMatches, env: &mut local_env::Local let name = import_match .get_one::("node-name") .ok_or_else(|| anyhow!("No node name provided"))?; + let update_catalog = import_match + .get_one::("update-catalog") + .cloned() + .unwrap_or_default(); // Parse base inputs let base_tarfile = import_match @@ -694,6 +698,7 @@ async fn handle_timeline(timeline_match: &ArgMatches, env: &mut local_env::Local None, pg_version, ComputeMode::Primary, + !update_catalog, )?; println!("Done"); } @@ -831,6 +836,10 @@ async fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Re .get_one::("endpoint_id") .map(String::to_string) .unwrap_or_else(|| format!("ep-{branch_name}")); + let update_catalog = sub_args + .get_one::("update-catalog") + .cloned() + .unwrap_or_default(); let lsn = sub_args .get_one::("lsn") @@ -880,6 +889,7 @@ async fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Re http_port, pg_version, mode, + !update_catalog, )?; } "start" => { @@ -918,6 +928,11 @@ async fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Re .get(endpoint_id.as_str()) .ok_or_else(|| anyhow::anyhow!("endpoint {endpoint_id} not found"))?; + let create_test_user = sub_args + .get_one::("create-test-user") + .cloned() + .unwrap_or_default(); + cplane.check_conflicting_endpoints( endpoint.mode, endpoint.tenant_id, @@ -972,6 +987,7 @@ async fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Re pageservers, remote_ext_config, stripe_size.0 as usize, + create_test_user, ) .await?; } @@ -1457,6 +1473,18 @@ fn cli() -> Command { .required(false) .default_value("1"); + let update_catalog = Arg::new("update-catalog") + .value_parser(value_parser!(bool)) + .long("update-catalog") + .help("If set, will set up the catalog for neon_superuser") + .required(false); + + let create_test_user = Arg::new("create-test-user") + .value_parser(value_parser!(bool)) + .long("create-test-user") + .help("If set, will create test user `user` and `neondb` database. Requires `update-catalog = true`") + .required(false); + Command::new("Neon CLI") .arg_required_else_help(true) .version(GIT_VERSION) @@ -1517,6 +1545,7 @@ fn cli() -> Command { .arg(Arg::new("end-lsn").long("end-lsn") .help("Lsn the basebackup ends at")) .arg(pg_version_arg.clone()) + .arg(update_catalog.clone()) ) ).subcommand( Command::new("tenant") @@ -1630,6 +1659,7 @@ fn cli() -> Command { .required(false)) .arg(pg_version_arg.clone()) .arg(hot_standby_arg.clone()) + .arg(update_catalog) ) .subcommand(Command::new("start") .about("Start postgres.\n If the endpoint doesn't exist yet, it is created.") @@ -1637,6 +1667,7 @@ fn cli() -> Command { .arg(endpoint_pageserver_id_arg.clone()) .arg(safekeepers_arg) .arg(remote_ext_config_args) + .arg(create_test_user) ) .subcommand(Command::new("reconfigure") .about("Reconfigure the endpoint") diff --git a/control_plane/src/endpoint.rs b/control_plane/src/endpoint.rs index ce8f035dfc..bab7a70ce7 100644 --- a/control_plane/src/endpoint.rs +++ b/control_plane/src/endpoint.rs @@ -41,11 +41,15 @@ use std::net::SocketAddr; use std::net::TcpStream; use std::path::PathBuf; use std::process::Command; +use std::str::FromStr; use std::sync::Arc; use std::time::Duration; use anyhow::{anyhow, bail, Context, Result}; +use compute_api::spec::Database; +use compute_api::spec::PgIdent; use compute_api::spec::RemoteExtSpec; +use compute_api::spec::Role; use nix::sys::signal::kill; use nix::sys::signal::Signal; use serde::{Deserialize, Serialize}; @@ -122,6 +126,7 @@ impl ComputeControlPlane { http_port: Option, pg_version: u32, mode: ComputeMode, + skip_pg_catalog_updates: bool, ) -> Result> { let pg_port = pg_port.unwrap_or_else(|| self.get_port()); let http_port = http_port.unwrap_or_else(|| self.get_port() + 1); @@ -140,7 +145,7 @@ impl ComputeControlPlane { // before and after start are the same. So, skip catalog updates, // with this we basically test a case of waking up an idle compute, where // we also skip catalog updates in the cloud. - skip_pg_catalog_updates: true, + skip_pg_catalog_updates, features: vec![], }); @@ -155,7 +160,7 @@ impl ComputeControlPlane { http_port, pg_port, pg_version, - skip_pg_catalog_updates: true, + skip_pg_catalog_updates, features: vec![], })?, )?; @@ -500,6 +505,7 @@ impl Endpoint { pageservers: Vec<(Host, u16)>, remote_ext_config: Option<&String>, shard_stripe_size: usize, + create_test_user: bool, ) -> Result<()> { if self.status() == EndpointStatus::Running { anyhow::bail!("The endpoint is already running"); @@ -551,8 +557,26 @@ impl Endpoint { cluster_id: None, // project ID: not used name: None, // project name: not used state: None, - roles: vec![], - databases: vec![], + roles: if create_test_user { + vec![Role { + name: PgIdent::from_str("test").unwrap(), + encrypted_password: None, + options: None, + }] + } else { + Vec::new() + }, + databases: if create_test_user { + vec![Database { + name: PgIdent::from_str("neondb").unwrap(), + owner: PgIdent::from_str("test").unwrap(), + options: None, + restrict_conn: false, + invalid: false, + }] + } else { + Vec::new() + }, settings: None, postgresql_conf: Some(postgresql_conf), }, @@ -577,11 +601,16 @@ impl Endpoint { .open(self.endpoint_path().join("compute.log"))?; // Launch compute_ctl - println!("Starting postgres node at '{}'", self.connstr()); + let conn_str = self.connstr("cloud_admin", "postgres"); + println!("Starting postgres node at '{}'", conn_str); + if create_test_user { + 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")); cmd.args(["--http-port", &self.http_address.port().to_string()]) .args(["--pgdata", self.pgdata().to_str().unwrap()]) - .args(["--connstr", &self.connstr()]) + .args(["--connstr", &conn_str]) .args([ "--spec-path", self.endpoint_path().join("spec.json").to_str().unwrap(), @@ -785,13 +814,13 @@ impl Endpoint { Ok(()) } - pub fn connstr(&self) -> String { + pub fn connstr(&self, user: &str, db_name: &str) -> String { format!( "postgresql://{}@{}:{}/{}", - "cloud_admin", + user, self.pg_address.ip(), self.pg_address.port(), - "postgres" + db_name ) } } From 5d6083bfc61701877be2ae8b9d9d726a4d0e773b Mon Sep 17 00:00:00 2001 From: Vlad Lazar Date: Wed, 21 Feb 2024 09:49:46 +0000 Subject: [PATCH 07/15] pageserver: add vectored get implementation (#6576) This PR introduces a new vectored implementation of the read path. The search is basically a DFS if you squint at it long enough. LayerFringe tracks the next layers to visit and acts as our stack. Vertices are tuples of (layer, keyspace, lsn range). Continuously pop the top of the stack (most recent layer) and do all the reads for one layer at once. The search maintains a fringe (`LayerFringe`) which tracks all the layers that intersect the current keyspace being searched. Continuously pop the top of the fringe (layer with highest LSN) and get all the data required from the layer in one go. Said search is done on one timeline at a time. If data is still required for some keys, then search the ancestor timeline. Apart from the high level layer traversal, vectored variants have been introduced for grabbing data from each layer type. They still suffer from read amplification issues and that will be addressed in a different PR. You might notice that in some places we duplicate the code for the existing read path. All of that code will be removed when we switch the non-vectored read path to proxy into the vectored read path. In the meantime, we'll have to contend with the extra cruft for the sake of testing and gentle releasing. --- .github/workflows/build_and_test.yml | 1 + Cargo.lock | 1 + libs/pageserver_api/Cargo.toml | 1 + libs/pageserver_api/src/keyspace.rs | 52 ++- pageserver/src/basebackup.rs | 5 +- pageserver/src/config.rs | 24 ++ pageserver/src/tenant.rs | 175 +++++++-- pageserver/src/tenant/layer_map.rs | 118 ++++-- pageserver/src/tenant/storage_layer.rs | 282 +++++++++++++ .../src/tenant/storage_layer/delta_layer.rs | 139 ++++++- .../src/tenant/storage_layer/image_layer.rs | 78 +++- .../tenant/storage_layer/inmemory_layer.rs | 100 ++++- pageserver/src/tenant/storage_layer/layer.rs | 51 ++- .../src/tenant/storage_layer/layer_desc.rs | 2 +- pageserver/src/tenant/timeline.rs | 371 ++++++++++++++++-- test_runner/fixtures/neon_fixtures.py | 7 + test_runner/regress/test_compatibility.py | 4 + 17 files changed, 1284 insertions(+), 127 deletions(-) diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 2a1c79e437..1744616888 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -472,6 +472,7 @@ jobs: CHECK_ONDISK_DATA_COMPATIBILITY: nonempty BUILD_TAG: ${{ needs.tag.outputs.build-tag }} PAGESERVER_VIRTUAL_FILE_IO_ENGINE: std-fs + PAGESERVER_GET_VECTORED_IMPL: vectored # Temporary disable this step until we figure out why it's so flaky # Ref https://github.com/neondatabase/neon/issues/4540 diff --git a/Cargo.lock b/Cargo.lock index f25e3d1574..ac8cceb5f6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3552,6 +3552,7 @@ dependencies = [ "enum-map", "hex", "humantime-serde", + "itertools", "postgres_ffi", "rand 0.8.5", "serde", diff --git a/libs/pageserver_api/Cargo.toml b/libs/pageserver_api/Cargo.toml index 902af21965..938910caea 100644 --- a/libs/pageserver_api/Cargo.toml +++ b/libs/pageserver_api/Cargo.toml @@ -21,6 +21,7 @@ hex.workspace = true thiserror.workspace = true humantime-serde.workspace = true chrono.workspace = true +itertools.workspace = true workspace_hack.workspace = true diff --git a/libs/pageserver_api/src/keyspace.rs b/libs/pageserver_api/src/keyspace.rs index 396c801606..443ffdcf03 100644 --- a/libs/pageserver_api/src/keyspace.rs +++ b/libs/pageserver_api/src/keyspace.rs @@ -2,6 +2,7 @@ use postgres_ffi::BLCKSZ; use std::ops::Range; use crate::key::Key; +use itertools::Itertools; /// /// Represents a set of Keys, in a compact form. @@ -63,9 +64,36 @@ impl KeySpace { KeyPartitioning { parts } } - /// Update the keyspace such that it doesn't contain any range - /// that is overlapping with `other`. This can involve splitting or - /// removing of existing ranges. + /// Merge another keyspace into the current one. + /// Note: the keyspaces must not ovelap (enforced via assertions) + pub fn merge(&mut self, other: &KeySpace) { + let all_ranges = self + .ranges + .iter() + .merge_by(other.ranges.iter(), |lhs, rhs| lhs.start < rhs.start); + + let mut accum = KeySpaceAccum::new(); + let mut prev: Option<&Range> = None; + for range in all_ranges { + if let Some(prev) = prev { + let overlap = + std::cmp::max(range.start, prev.start) < std::cmp::min(range.end, prev.end); + assert!( + !overlap, + "Attempt to merge ovelapping keyspaces: {:?} overlaps {:?}", + prev, range + ); + } + + accum.add_range(range.clone()); + prev = Some(range); + } + + self.ranges = accum.to_keyspace().ranges; + } + + /// Remove all keys in `other` from `self`. + /// This can involve splitting or removing of existing ranges. pub fn remove_overlapping_with(&mut self, other: &KeySpace) { let (self_start, self_end) = match (self.start(), self.end()) { (Some(start), Some(end)) => (start, end), @@ -220,16 +248,7 @@ impl KeySpaceAccum { } pub fn consume_keyspace(&mut self) -> KeySpace { - if let Some(accum) = self.accum.take() { - self.ranges.push(accum); - } - - let mut prev_accum = KeySpaceAccum::new(); - std::mem::swap(self, &mut prev_accum); - - KeySpace { - ranges: prev_accum.ranges, - } + std::mem::take(self).to_keyspace() } pub fn size(&self) -> u64 { @@ -279,6 +298,13 @@ impl KeySpaceRandomAccum { } KeySpace { ranges } } + + pub fn consume_keyspace(&mut self) -> KeySpace { + let mut prev_accum = KeySpaceRandomAccum::new(); + std::mem::swap(self, &mut prev_accum); + + prev_accum.to_keyspace() + } } pub fn key_range_size(key_range: &Range) -> u32 { diff --git a/pageserver/src/basebackup.rs b/pageserver/src/basebackup.rs index 7edfab75d4..c862816b80 100644 --- a/pageserver/src/basebackup.rs +++ b/pageserver/src/basebackup.rs @@ -261,10 +261,7 @@ where let mut slru_builder = SlruSegmentsBuilder::new(&mut self.ar); for part in slru_partitions.parts { - let blocks = self - .timeline - .get_vectored(&part.ranges, self.lsn, self.ctx) - .await?; + let blocks = self.timeline.get_vectored(part, self.lsn, self.ctx).await?; for (key, block) in blocks { slru_builder.add_block(&key, block?).await?; diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index 6d71ff1dd4..6c00c55f39 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -33,6 +33,7 @@ use utils::{ use crate::disk_usage_eviction_task::DiskUsageEvictionTaskConfig; use crate::tenant::config::TenantConf; use crate::tenant::config::TenantConfOpt; +use crate::tenant::timeline::GetVectoredImpl; use crate::tenant::{ TENANTS_SEGMENT_NAME, TENANT_DELETED_MARKER_FILE_NAME, TIMELINES_SEGMENT_NAME, }; @@ -84,6 +85,8 @@ pub mod defaults { pub const DEFAULT_VIRTUAL_FILE_IO_ENGINE: &str = "std-fs"; + pub const DEFAULT_GET_VECTORED_IMPL: &str = "sequential"; + /// /// Default built-in configuration file. /// @@ -121,6 +124,8 @@ pub mod defaults { #virtual_file_io_engine = '{DEFAULT_VIRTUAL_FILE_IO_ENGINE}' +#get_vectored_impl = '{DEFAULT_GET_VECTORED_IMPL}' + [tenant_config] #checkpoint_distance = {DEFAULT_CHECKPOINT_DISTANCE} # in bytes #checkpoint_timeout = {DEFAULT_CHECKPOINT_TIMEOUT} @@ -256,6 +261,8 @@ pub struct PageServerConf { pub ingest_batch_size: u64, pub virtual_file_io_engine: virtual_file::IoEngineKind, + + pub get_vectored_impl: GetVectoredImpl, } /// We do not want to store this in a PageServerConf because the latter may be logged @@ -342,6 +349,8 @@ struct PageServerConfigBuilder { ingest_batch_size: BuilderValue, virtual_file_io_engine: BuilderValue, + + get_vectored_impl: BuilderValue, } impl Default for PageServerConfigBuilder { @@ -419,6 +428,8 @@ impl Default for PageServerConfigBuilder { ingest_batch_size: Set(DEFAULT_INGEST_BATCH_SIZE), virtual_file_io_engine: Set(DEFAULT_VIRTUAL_FILE_IO_ENGINE.parse().unwrap()), + + get_vectored_impl: Set(DEFAULT_GET_VECTORED_IMPL.parse().unwrap()), } } } @@ -579,6 +590,10 @@ impl PageServerConfigBuilder { self.virtual_file_io_engine = BuilderValue::Set(value); } + pub fn get_vectored_impl(&mut self, value: GetVectoredImpl) { + self.get_vectored_impl = BuilderValue::Set(value); + } + pub fn build(self) -> anyhow::Result { let concurrent_tenant_warmup = self .concurrent_tenant_warmup @@ -689,6 +704,9 @@ impl PageServerConfigBuilder { virtual_file_io_engine: self .virtual_file_io_engine .ok_or(anyhow!("missing virtual_file_io_engine"))?, + get_vectored_impl: self + .get_vectored_impl + .ok_or(anyhow!("missing get_vectored_impl"))?, }) } } @@ -943,6 +961,9 @@ impl PageServerConf { "virtual_file_io_engine" => { builder.virtual_file_io_engine(parse_toml_from_str("virtual_file_io_engine", item)?) } + "get_vectored_impl" => { + builder.get_vectored_impl(parse_toml_from_str("get_vectored_impl", item)?) + } _ => bail!("unrecognized pageserver option '{key}'"), } } @@ -1017,6 +1038,7 @@ impl PageServerConf { secondary_download_concurrency: defaults::DEFAULT_SECONDARY_DOWNLOAD_CONCURRENCY, ingest_batch_size: defaults::DEFAULT_INGEST_BATCH_SIZE, virtual_file_io_engine: DEFAULT_VIRTUAL_FILE_IO_ENGINE.parse().unwrap(), + get_vectored_impl: defaults::DEFAULT_GET_VECTORED_IMPL.parse().unwrap(), } } } @@ -1250,6 +1272,7 @@ background_task_maximum_delay = '334 s' secondary_download_concurrency: defaults::DEFAULT_SECONDARY_DOWNLOAD_CONCURRENCY, ingest_batch_size: defaults::DEFAULT_INGEST_BATCH_SIZE, virtual_file_io_engine: DEFAULT_VIRTUAL_FILE_IO_ENGINE.parse().unwrap(), + get_vectored_impl: defaults::DEFAULT_GET_VECTORED_IMPL.parse().unwrap(), }, "Correct defaults should be used when no config values are provided" ); @@ -1314,6 +1337,7 @@ background_task_maximum_delay = '334 s' secondary_download_concurrency: defaults::DEFAULT_SECONDARY_DOWNLOAD_CONCURRENCY, ingest_batch_size: 100, virtual_file_io_engine: DEFAULT_VIRTUAL_FILE_IO_ENGINE.parse().unwrap(), + get_vectored_impl: defaults::DEFAULT_GET_VECTORED_IMPL.parse().unwrap(), }, "Should be able to parse all basic config values correctly" ); diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index c646e5cf90..7021921b12 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -3877,6 +3877,7 @@ mod tests { 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; @@ -4514,6 +4515,61 @@ mod tests { Ok(()) } + async fn bulk_insert_compact_gc( + timeline: Arc, + ctx: &RequestContext, + mut lsn: Lsn, + repeat: usize, + key_count: usize, + ) -> anyhow::Result<()> { + let mut test_key = Key::from_hex("010000000033333333444444445500000000").unwrap(); + let mut blknum = 0; + + // Enforce that key range is monotonously increasing + let mut keyspace = KeySpaceAccum::new(); + + for _ in 0..repeat { + for _ in 0..key_count { + test_key.field6 = blknum; + let mut writer = timeline.writer().await; + writer + .put( + test_key, + lsn, + &Value::Image(test_img(&format!("{} at {}", blknum, lsn))), + ctx, + ) + .await?; + writer.finish_write(lsn); + drop(writer); + + keyspace.add_key(test_key); + + lsn = Lsn(lsn.0 + 0x10); + blknum += 1; + } + + let cutoff = timeline.get_last_record_lsn(); + + timeline + .update_gc_info( + Vec::new(), + cutoff, + Duration::ZERO, + &CancellationToken::new(), + ctx, + ) + .await?; + timeline.freeze_and_flush().await?; + timeline + .compact(&CancellationToken::new(), EnumSet::empty(), ctx) + .await?; + timeline.gc().await?; + } + + Ok(()) + } + // // Insert 1000 key-value pairs with increasing keys, flush, compact, GC. // Repeat 50 times. @@ -4526,49 +4582,98 @@ mod tests { .create_test_timeline(TIMELINE_ID, Lsn(0x08), DEFAULT_PG_VERSION, &ctx) .await?; - let mut lsn = Lsn(0x10); + let lsn = Lsn(0x10); + bulk_insert_compact_gc(tline.clone(), &ctx, lsn, 50, 10000).await?; - let mut keyspace = KeySpaceAccum::new(); + Ok(()) + } - let mut test_key = Key::from_hex("010000000033333333444444445500000000").unwrap(); - let mut blknum = 0; - for _ in 0..50 { - for _ in 0..10000 { - test_key.field6 = blknum; - let mut writer = tline.writer().await; - writer - .put( - test_key, - lsn, - &Value::Image(test_img(&format!("{} at {}", blknum, lsn))), - &ctx, - ) - .await?; - writer.finish_write(lsn); - drop(writer); + // Test the vectored get real implementation against a simple sequential implementation. + // + // The test generates a keyspace by repeatedly flushing the in-memory layer and compacting. + // Projected to 2D the key space looks like below. Lsn grows upwards on the Y axis and keys + // grow to the right on the X axis. + // [Delta] + // [Delta] + // [Delta] + // [Delta] + // ------------ Image --------------- + // + // After layer generation we pick the ranges to query as follows: + // 1. The beginning of each delta layer + // 2. At the seam between two adjacent delta layers + // + // There's one major downside to this test: delta layers only contains images, + // so the search can stop at the first delta layer and doesn't traverse any deeper. + #[tokio::test] + async fn test_get_vectored() -> anyhow::Result<()> { + let harness = TenantHarness::create("test_get_vectored")?; + let (tenant, ctx) = harness.load().await; + let tline = tenant + .create_test_timeline(TIMELINE_ID, Lsn(0x08), DEFAULT_PG_VERSION, &ctx) + .await?; - keyspace.add_key(test_key); + let lsn = Lsn(0x10); + bulk_insert_compact_gc(tline.clone(), &ctx, lsn, 50, 10000).await?; - lsn = Lsn(lsn.0 + 0x10); - blknum += 1; + let guard = tline.layers.read().await; + guard.layer_map().dump(true, &ctx).await?; + + let mut reads = Vec::new(); + let mut prev = None; + guard.layer_map().iter_historic_layers().for_each(|desc| { + if !desc.is_delta() { + prev = Some(desc.clone()); + return; } - let cutoff = tline.get_last_record_lsn(); + let start = desc.key_range.start; + let end = desc + .key_range + .start + .add(Timeline::MAX_GET_VECTORED_KEYS.try_into().unwrap()); + reads.push(KeySpace { + ranges: vec![start..end], + }); + if let Some(prev) = &prev { + if !prev.is_delta() { + return; + } + + let first_range = Key { + field6: prev.key_range.end.field6 - 4, + ..prev.key_range.end + }..prev.key_range.end; + + let second_range = desc.key_range.start..Key { + field6: desc.key_range.start.field6 + 4, + ..desc.key_range.start + }; + + reads.push(KeySpace { + ranges: vec![first_range, second_range], + }); + }; + + prev = Some(desc.clone()); + }); + + drop(guard); + + // Pick a big LSN such that we query over all the changes. + // Technically, u64::MAX - 1 is the largest LSN supported by the read path, + // but there seems to be a bug on the non-vectored search path which surfaces + // in that case. + let reads_lsn = Lsn(u64::MAX - 1000); + + for read in reads { + info!("Doing vectored read on {:?}", read); + + let vectored_res = tline.get_vectored_impl(read.clone(), reads_lsn, &ctx).await; tline - .update_gc_info( - Vec::new(), - cutoff, - Duration::ZERO, - &CancellationToken::new(), - &ctx, - ) - .await?; - tline.freeze_and_flush().await?; - tline - .compact(&CancellationToken::new(), EnumSet::empty(), &ctx) - .await?; - tline.gc().await?; + .validate_get_vectored_impl(&vectored_res, read, reads_lsn, &ctx) + .await; } Ok(()) diff --git a/pageserver/src/tenant/layer_map.rs b/pageserver/src/tenant/layer_map.rs index bb52e586d1..5f4814cc6b 100644 --- a/pageserver/src/tenant/layer_map.rs +++ b/pageserver/src/tenant/layer_map.rs @@ -52,8 +52,7 @@ use crate::repository::Key; use crate::tenant::storage_layer::InMemoryLayer; use anyhow::Result; use pageserver_api::keyspace::KeySpaceAccum; -use std::cmp::Ordering; -use std::collections::{BTreeMap, VecDeque}; +use std::collections::{HashMap, VecDeque}; use std::iter::Peekable; use std::ops::Range; use std::sync::Arc; @@ -147,43 +146,28 @@ impl Drop for BatchedUpdates<'_> { } /// Return value of LayerMap::search -#[derive(Eq, PartialEq, Debug)] +#[derive(Eq, PartialEq, Debug, Hash)] pub struct SearchResult { pub layer: Arc, pub lsn_floor: Lsn, } -pub struct OrderedSearchResult(SearchResult); - -impl Ord for OrderedSearchResult { - fn cmp(&self, other: &Self) -> Ordering { - self.0.lsn_floor.cmp(&other.0.lsn_floor) - } -} - -impl PartialOrd for OrderedSearchResult { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl PartialEq for OrderedSearchResult { - fn eq(&self, other: &Self) -> bool { - self.0.lsn_floor == other.0.lsn_floor - } -} - -impl Eq for OrderedSearchResult {} - +/// Return value of [`LayerMap::range_search`] +/// +/// Contains a mapping from a layer description to a keyspace +/// accumulator that contains all the keys which intersect the layer +/// from the original search space. Keys that were not found are accumulated +/// in a separate key space accumulator. +#[derive(Debug)] pub struct RangeSearchResult { - pub found: BTreeMap, + pub found: HashMap, pub not_found: KeySpaceAccum, } impl RangeSearchResult { fn new() -> Self { Self { - found: BTreeMap::new(), + found: HashMap::new(), not_found: KeySpaceAccum::new(), } } @@ -314,7 +298,7 @@ where Some(search_result) => self .result .found - .entry(OrderedSearchResult(search_result)) + .entry(search_result) .or_default() .add_range(covered_range), None => self.pad_range(covered_range), @@ -362,6 +346,35 @@ where } } +#[derive(PartialEq, Eq, Hash, Debug, Clone)] +pub enum InMemoryLayerHandle { + Open { + lsn_floor: Lsn, + end_lsn: Lsn, + }, + Frozen { + idx: usize, + lsn_floor: Lsn, + end_lsn: Lsn, + }, +} + +impl InMemoryLayerHandle { + pub fn get_lsn_floor(&self) -> Lsn { + match self { + InMemoryLayerHandle::Open { lsn_floor, .. } => *lsn_floor, + InMemoryLayerHandle::Frozen { lsn_floor, .. } => *lsn_floor, + } + } + + pub fn get_end_lsn(&self) -> Lsn { + match self { + InMemoryLayerHandle::Open { end_lsn, .. } => *end_lsn, + InMemoryLayerHandle::Frozen { end_lsn, .. } => *end_lsn, + } + } +} + impl LayerMap { /// /// Find the latest layer (by lsn.end) that covers the given @@ -556,6 +569,43 @@ impl LayerMap { self.historic.iter() } + /// Get a handle for the first in memory layer that matches the provided predicate. + /// The handle should be used with [`Self::get_in_memory_layer`] to retrieve the actual layer. + /// + /// Note: [`Self::find_in_memory_layer`] and [`Self::get_in_memory_layer`] should be called during + /// the same exclusive region established by holding the layer manager lock. + pub fn find_in_memory_layer(&self, mut pred: Pred) -> Option + where + Pred: FnMut(&Arc) -> bool, + { + if let Some(open) = &self.open_layer { + if pred(open) { + return Some(InMemoryLayerHandle::Open { + lsn_floor: open.get_lsn_range().start, + end_lsn: open.get_lsn_range().end, + }); + } + } + + let pos = self.frozen_layers.iter().rev().position(pred); + pos.map(|rev_idx| { + let idx = self.frozen_layers.len() - 1 - rev_idx; + InMemoryLayerHandle::Frozen { + idx, + lsn_floor: self.frozen_layers[idx].get_lsn_range().start, + end_lsn: self.frozen_layers[idx].get_lsn_range().end, + } + }) + } + + /// Get the layer pointed to by the provided handle. + pub fn get_in_memory_layer(&self, handle: &InMemoryLayerHandle) -> Option> { + match handle { + InMemoryLayerHandle::Open { .. } => self.open_layer.clone(), + InMemoryLayerHandle::Frozen { idx, .. } => self.frozen_layers.get(*idx).cloned(), + } + } + /// /// Divide the whole given range of keys into sub-ranges based on the latest /// image layer that covers each range at the specified lsn (inclusive). @@ -869,6 +919,8 @@ impl LayerMap { #[cfg(test)] mod tests { + use pageserver_api::keyspace::KeySpace; + use super::*; #[derive(Clone)] @@ -895,15 +947,15 @@ mod tests { fn assert_range_search_result_eq(lhs: RangeSearchResult, rhs: RangeSearchResult) { assert_eq!(lhs.not_found.to_keyspace(), rhs.not_found.to_keyspace()); - let lhs: Vec<_> = lhs + let lhs: HashMap = lhs .found .into_iter() - .map(|(search_result, accum)| (search_result.0, accum.to_keyspace())) + .map(|(search_result, accum)| (search_result, accum.to_keyspace())) .collect(); - let rhs: Vec<_> = rhs + let rhs: HashMap = rhs .found .into_iter() - .map(|(search_result, accum)| (search_result.0, accum.to_keyspace())) + .map(|(search_result, accum)| (search_result, accum.to_keyspace())) .collect(); assert_eq!(lhs, rhs); @@ -923,7 +975,7 @@ mod tests { Some(res) => { range_search_result .found - .entry(OrderedSearchResult(res)) + .entry(res) .or_default() .add_key(key); } diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index 2d92baccbe..73c018db31 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -8,15 +8,21 @@ pub(crate) mod layer; mod layer_desc; use crate::context::{AccessStatsBehavior, RequestContext}; +use crate::repository::Value; use crate::task_mgr::TaskKind; use crate::walrecord::NeonWalRecord; use bytes::Bytes; use enum_map::EnumMap; use enumset::EnumSet; use once_cell::sync::Lazy; +use pageserver_api::key::Key; +use pageserver_api::keyspace::{KeySpace, KeySpaceRandomAccum}; use pageserver_api::models::{ LayerAccessKind, LayerResidenceEvent, LayerResidenceEventReason, LayerResidenceStatus, }; +use std::cmp::{Ordering, Reverse}; +use std::collections::hash_map::Entry; +use std::collections::{BinaryHeap, HashMap}; use std::ops::Range; use std::sync::Mutex; use std::time::{Duration, SystemTime, UNIX_EPOCH}; @@ -34,6 +40,11 @@ pub use layer_desc::{PersistentLayerDesc, PersistentLayerKey}; pub(crate) use layer::{EvictionError, Layer, ResidentLayer}; +use super::layer_map::InMemoryLayerHandle; +use super::timeline::layer_manager::LayerManager; +use super::timeline::GetVectoredError; +use super::PageReconstructError; + pub fn range_overlaps(a: &Range, b: &Range) -> bool where T: PartialOrd, @@ -67,6 +78,277 @@ pub struct ValueReconstructState { pub img: Option<(Lsn, Bytes)>, } +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +pub(crate) enum ValueReconstructSituation { + Complete, + #[default] + Continue, +} + +/// Reconstruct data accumulated for a single key during a vectored get +#[derive(Debug, Default, Clone)] +pub(crate) struct VectoredValueReconstructState { + pub(crate) records: Vec<(Lsn, NeonWalRecord)>, + pub(crate) img: Option<(Lsn, Bytes)>, + + situation: ValueReconstructSituation, +} + +impl VectoredValueReconstructState { + fn get_cached_lsn(&self) -> Option { + self.img.as_ref().map(|img| img.0) + } +} + +impl From for ValueReconstructState { + fn from(mut state: VectoredValueReconstructState) -> Self { + // walredo expects the records to be descending in terms of Lsn + state.records.sort_by_key(|(lsn, _)| Reverse(*lsn)); + + ValueReconstructState { + records: state.records, + img: state.img, + } + } +} + +/// Bag of data accumulated during a vectored get +pub(crate) struct ValuesReconstructState { + pub(crate) keys: HashMap>, + + keys_done: KeySpaceRandomAccum, +} + +impl ValuesReconstructState { + pub(crate) fn new() -> Self { + Self { + keys: HashMap::new(), + keys_done: KeySpaceRandomAccum::new(), + } + } + + /// Associate a key with the error which it encountered and mark it as done + pub(crate) fn on_key_error(&mut self, key: Key, err: PageReconstructError) { + let previous = self.keys.insert(key, Err(err)); + if let Some(Ok(state)) = previous { + if state.situation == ValueReconstructSituation::Continue { + self.keys_done.add_key(key); + } + } + } + + /// Update the state collected for a given key. + /// Returns true if this was the last value needed for the key and false otherwise. + /// + /// If the key is done after the update, mark it as such. + pub(crate) fn update_key( + &mut self, + key: &Key, + lsn: Lsn, + value: Value, + ) -> ValueReconstructSituation { + let state = self + .keys + .entry(*key) + .or_insert(Ok(VectoredValueReconstructState::default())); + + if let Ok(state) = state { + let key_done = match state.situation { + ValueReconstructSituation::Complete => unreachable!(), + ValueReconstructSituation::Continue => match value { + Value::Image(img) => { + state.img = Some((lsn, img)); + true + } + Value::WalRecord(rec) => { + let reached_cache = + state.get_cached_lsn().map(|clsn| clsn + 1) == Some(lsn); + let will_init = rec.will_init(); + state.records.push((lsn, rec)); + will_init || reached_cache + } + }, + }; + + if key_done && state.situation == ValueReconstructSituation::Continue { + state.situation = ValueReconstructSituation::Complete; + self.keys_done.add_key(*key); + } + + state.situation + } else { + ValueReconstructSituation::Complete + } + } + + /// Returns the Lsn at which this key is cached if one exists. + /// The read path should go no further than this Lsn for the given key. + pub(crate) fn get_cached_lsn(&self, key: &Key) -> Option { + self.keys + .get(key) + .and_then(|k| k.as_ref().ok()) + .and_then(|state| state.get_cached_lsn()) + } + + /// Returns the key space describing the keys that have + /// been marked as completed since the last call to this function. + pub(crate) fn consume_done_keys(&mut self) -> KeySpace { + self.keys_done.consume_keyspace() + } +} + +impl Default for ValuesReconstructState { + fn default() -> Self { + Self::new() + } +} + +/// Description of layer to be read - the layer map can turn +/// this description into the actual layer. +#[derive(PartialEq, Eq, Hash, Debug, Clone)] +pub(crate) enum ReadableLayerDesc { + Persistent { + desc: PersistentLayerDesc, + lsn_floor: Lsn, + lsn_ceil: Lsn, + }, + InMemory { + handle: InMemoryLayerHandle, + lsn_ceil: Lsn, + }, +} + +/// Wraper for 'ReadableLayerDesc' sorted by Lsn +#[derive(Debug)] +struct ReadableLayerDescOrdered(ReadableLayerDesc); + +/// Data structure which maintains a fringe of layers for the +/// read path. The fringe is the set of layers which intersects +/// the current keyspace that the search is descending on. +/// Each layer tracks the keyspace that intersects it. +/// +/// The fringe must appear sorted by Lsn. Hence, it uses +/// a two layer indexing scheme. +#[derive(Debug)] +pub(crate) struct LayerFringe { + layers_by_lsn: BinaryHeap, + layers: HashMap, +} + +impl LayerFringe { + pub(crate) fn new() -> Self { + LayerFringe { + layers_by_lsn: BinaryHeap::new(), + layers: HashMap::new(), + } + } + + pub(crate) fn next_layer(&mut self) -> Option<(ReadableLayerDesc, KeySpace)> { + let handle = match self.layers_by_lsn.pop() { + Some(h) => h, + None => return None, + }; + + let removed = self.layers.remove_entry(&handle.0); + match removed { + Some((layer, keyspace)) => Some((layer, keyspace)), + None => unreachable!("fringe internals are always consistent"), + } + } + + pub(crate) fn update(&mut self, layer: ReadableLayerDesc, keyspace: KeySpace) { + let entry = self.layers.entry(layer.clone()); + match entry { + Entry::Occupied(mut entry) => { + entry.get_mut().merge(&keyspace); + } + Entry::Vacant(entry) => { + self.layers_by_lsn + .push(ReadableLayerDescOrdered(entry.key().clone())); + entry.insert(keyspace); + } + } + } +} + +impl Default for LayerFringe { + fn default() -> Self { + Self::new() + } +} + +impl Ord for ReadableLayerDescOrdered { + fn cmp(&self, other: &Self) -> Ordering { + let ord = self.0.get_lsn_ceil().cmp(&other.0.get_lsn_ceil()); + if ord == std::cmp::Ordering::Equal { + self.0 + .get_lsn_floor() + .cmp(&other.0.get_lsn_floor()) + .reverse() + } else { + ord + } + } +} + +impl PartialOrd for ReadableLayerDescOrdered { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl PartialEq for ReadableLayerDescOrdered { + fn eq(&self, other: &Self) -> bool { + self.0.get_lsn_floor() == other.0.get_lsn_floor() + && self.0.get_lsn_ceil() == other.0.get_lsn_ceil() + } +} + +impl Eq for ReadableLayerDescOrdered {} + +impl ReadableLayerDesc { + pub(crate) fn get_lsn_floor(&self) -> Lsn { + match self { + ReadableLayerDesc::Persistent { lsn_floor, .. } => *lsn_floor, + ReadableLayerDesc::InMemory { handle, .. } => handle.get_lsn_floor(), + } + } + + pub(crate) fn get_lsn_ceil(&self) -> Lsn { + match self { + ReadableLayerDesc::Persistent { lsn_ceil, .. } => *lsn_ceil, + ReadableLayerDesc::InMemory { lsn_ceil, .. } => *lsn_ceil, + } + } + + pub(crate) async fn get_values_reconstruct_data( + &self, + layer_manager: &LayerManager, + keyspace: KeySpace, + reconstruct_state: &mut ValuesReconstructState, + ctx: &RequestContext, + ) -> Result<(), GetVectoredError> { + match self { + ReadableLayerDesc::Persistent { desc, lsn_ceil, .. } => { + let layer = layer_manager.get_from_desc(desc); + layer + .get_values_reconstruct_data(keyspace, *lsn_ceil, reconstruct_state, ctx) + .await + } + ReadableLayerDesc::InMemory { handle, lsn_ceil } => { + let layer = layer_manager + .layer_map() + .get_in_memory_layer(handle) + .unwrap(); + + layer + .get_values_reconstruct_data(keyspace, *lsn_ceil, reconstruct_state, ctx) + .await + } + } + } +} + /// Return value from [`Layer::get_value_reconstruct_data`] #[derive(Clone, Copy, Debug)] pub enum ValueReconstructResult { diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 9a7bcbcebe..19eebf5531 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -35,16 +35,19 @@ use crate::tenant::blob_io::BlobWriter; use crate::tenant::block_io::{BlockBuf, BlockCursor, BlockLease, BlockReader, FileBlockReader}; use crate::tenant::disk_btree::{DiskBtreeBuilder, DiskBtreeReader, VisitDirection}; use crate::tenant::storage_layer::{Layer, ValueReconstructResult, ValueReconstructState}; -use crate::tenant::Timeline; +use crate::tenant::timeline::GetVectoredError; +use crate::tenant::{PageReconstructError, Timeline}; use crate::virtual_file::{self, VirtualFile}; use crate::{walrecord, TEMP_FILE_SUFFIX}; use crate::{DELTA_FILE_MAGIC, STORAGE_FORMAT_VERSION}; -use anyhow::{bail, ensure, Context, Result}; +use anyhow::{anyhow, bail, ensure, Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; +use pageserver_api::keyspace::KeySpace; use pageserver_api::models::LayerAccessKind; use pageserver_api::shard::TenantShardId; use rand::{distributions::Alphanumeric, Rng}; use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; use std::fs::File; use std::io::SeekFrom; use std::ops::Range; @@ -59,7 +62,10 @@ use utils::{ lsn::Lsn, }; -use super::{AsLayerDesc, LayerAccessStats, PersistentLayerDesc, ResidentLayer}; +use super::{ + AsLayerDesc, LayerAccessStats, PersistentLayerDesc, ResidentLayer, ValueReconstructSituation, + ValuesReconstructState, +}; /// /// Header stored in the beginning of the file @@ -818,6 +824,133 @@ impl DeltaLayerInner { } } + // Look up the keys in the provided keyspace and update + // the reconstruct state with whatever is found. + // + // If the key is cached, go no further than the cached Lsn. + // + // Currently, the index is visited for each range, but this + // can be further optimised to visit the index only once. + pub(super) async fn get_values_reconstruct_data( + &self, + keyspace: KeySpace, + end_lsn: Lsn, + reconstruct_state: &mut ValuesReconstructState, + ctx: &RequestContext, + ) -> Result<(), GetVectoredError> { + let file = &self.file; + let tree_reader = DiskBtreeReader::<_, DELTA_KEY_SIZE>::new( + self.index_start_blk, + self.index_root_blk, + file, + ); + + let mut offsets: BTreeMap> = BTreeMap::new(); + + for range in keyspace.ranges.iter() { + let mut ignore_key = None; + + // Scan the page versions backwards, starting from the last key in the range. + // to collect all the offsets at which need to be read. + let end_key = DeltaKey::from_key_lsn(&range.end, Lsn(end_lsn.0 - 1)); + tree_reader + .visit( + &end_key.0, + VisitDirection::Backwards, + |raw_key, value| { + let key = Key::from_slice(&raw_key[..KEY_SIZE]); + let entry_lsn = DeltaKey::extract_lsn_from_buf(raw_key); + + if entry_lsn >= end_lsn { + return true; + } + + if key < range.start { + return false; + } + + if key >= range.end { + return true; + } + + if Some(key) == ignore_key { + return true; + } + + if let Some(cached_lsn) = reconstruct_state.get_cached_lsn(&key) { + if entry_lsn <= cached_lsn { + return key != range.start; + } + } + + let blob_ref = BlobRef(value); + let lsns_at = offsets.entry(key).or_default(); + lsns_at.push((entry_lsn, blob_ref.pos())); + + if blob_ref.will_init() { + if key == range.start { + return false; + } else { + ignore_key = Some(key); + return true; + } + } + + true + }, + &RequestContextBuilder::extend(ctx) + .page_content_kind(PageContentKind::DeltaLayerBtreeNode) + .build(), + ) + .await + .map_err(|err| GetVectoredError::Other(anyhow!(err)))?; + } + + let ctx = &RequestContextBuilder::extend(ctx) + .page_content_kind(PageContentKind::DeltaLayerValue) + .build(); + + let cursor = file.block_cursor(); + let mut buf = Vec::new(); + for (key, lsns_at) in offsets { + for (lsn, block_offset) in lsns_at { + let res = cursor.read_blob_into_buf(block_offset, &mut buf, ctx).await; + + if let Err(e) = res { + reconstruct_state.on_key_error( + key, + PageReconstructError::from(anyhow!(e).context(format!( + "Failed to read blob from virtual file {}", + file.file.path + ))), + ); + + break; + } + + let value = Value::des(&buf); + if let Err(e) = value { + reconstruct_state.on_key_error( + key, + PageReconstructError::from(anyhow!(e).context(format!( + "Failed to deserialize file blob from virtual file {}", + file.file.path + ))), + ); + + break; + } + + let key_situation = reconstruct_state.update_key(&key, lsn, value.unwrap()); + if key_situation == ValueReconstructSituation::Complete { + break; + } + } + } + + Ok(()) + } + pub(super) async fn load_keys<'a>( &'a self, ctx: &RequestContext, diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 458131b572..b867cb0333 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -26,20 +26,22 @@ use crate::config::PageServerConf; use crate::context::{PageContentKind, RequestContext, RequestContextBuilder}; use crate::page_cache::PAGE_SZ; -use crate::repository::{Key, KEY_SIZE}; +use crate::repository::{Key, Value, KEY_SIZE}; use crate::tenant::blob_io::BlobWriter; use crate::tenant::block_io::{BlockBuf, BlockReader, FileBlockReader}; use crate::tenant::disk_btree::{DiskBtreeBuilder, DiskBtreeReader, VisitDirection}; use crate::tenant::storage_layer::{ LayerAccessStats, ValueReconstructResult, ValueReconstructState, }; -use crate::tenant::Timeline; +use crate::tenant::timeline::GetVectoredError; +use crate::tenant::{PageReconstructError, Timeline}; use crate::virtual_file::{self, VirtualFile}; use crate::{IMAGE_FILE_MAGIC, STORAGE_FORMAT_VERSION, TEMP_FILE_SUFFIX}; -use anyhow::{bail, ensure, Context, Result}; +use anyhow::{anyhow, bail, ensure, Context, Result}; use bytes::Bytes; use camino::{Utf8Path, Utf8PathBuf}; use hex; +use pageserver_api::keyspace::KeySpace; use pageserver_api::models::LayerAccessKind; use pageserver_api::shard::TenantShardId; use rand::{distributions::Alphanumeric, Rng}; @@ -59,7 +61,7 @@ use utils::{ }; use super::filename::ImageFileName; -use super::{AsLayerDesc, Layer, PersistentLayerDesc, ResidentLayer}; +use super::{AsLayerDesc, Layer, PersistentLayerDesc, ResidentLayer, ValuesReconstructState}; /// /// Header stored in the beginning of the file @@ -438,6 +440,74 @@ impl ImageLayerInner { Ok(ValueReconstructResult::Missing) } } + + // Look up the keys in the provided keyspace and update + // the reconstruct state with whatever is found. + pub(super) async fn get_values_reconstruct_data( + &self, + keyspace: KeySpace, + reconstruct_state: &mut ValuesReconstructState, + ctx: &RequestContext, + ) -> Result<(), GetVectoredError> { + let file = &self.file; + let tree_reader = DiskBtreeReader::new(self.index_start_blk, self.index_root_blk, file); + + let mut offsets = Vec::new(); + + for range in keyspace.ranges.iter() { + let mut search_key: [u8; KEY_SIZE] = [0u8; KEY_SIZE]; + range.start.write_to_byte_slice(&mut search_key); + + tree_reader + .visit( + &search_key, + VisitDirection::Forwards, + |raw_key, value| { + let key = Key::from_slice(&raw_key[..KEY_SIZE]); + assert!(key >= range.start); + + if !range.contains(&key) { + return false; + } + + offsets.push((key, value)); + + true + }, + &RequestContextBuilder::extend(ctx) + .page_content_kind(PageContentKind::ImageLayerBtreeNode) + .build(), + ) + .await + .map_err(|err| GetVectoredError::Other(anyhow!(err)))?; + } + + let ctx = &RequestContextBuilder::extend(ctx) + .page_content_kind(PageContentKind::ImageLayerValue) + .build(); + + let cursor = file.block_cursor(); + let mut buf = Vec::new(); + for (key, offset) in offsets { + let res = cursor.read_blob_into_buf(offset, &mut buf, ctx).await; + if let Err(e) = res { + reconstruct_state.on_key_error( + key, + PageReconstructError::from(anyhow!(e).context(format!( + "Failed to read blob from virtual file {}", + file.file.path + ))), + ); + + continue; + } + + let blob = Bytes::copy_from_slice(buf.as_slice()); + reconstruct_state.update_key(&key, self.lsn, Value::Image(blob)); + } + + Ok(()) + } } /// A builder object for constructing a new image layer. diff --git a/pageserver/src/tenant/storage_layer/inmemory_layer.rs b/pageserver/src/tenant/storage_layer/inmemory_layer.rs index 4b06a787ce..5f1db21d49 100644 --- a/pageserver/src/tenant/storage_layer/inmemory_layer.rs +++ b/pageserver/src/tenant/storage_layer/inmemory_layer.rs @@ -9,13 +9,15 @@ use crate::context::{PageContentKind, RequestContext, RequestContextBuilder}; use crate::repository::{Key, Value}; use crate::tenant::block_io::BlockReader; use crate::tenant::ephemeral_file::EphemeralFile; -use crate::tenant::storage_layer::{ValueReconstructResult, ValueReconstructState}; -use crate::tenant::Timeline; +use crate::tenant::storage_layer::ValueReconstructResult; +use crate::tenant::timeline::GetVectoredError; +use crate::tenant::{PageReconstructError, Timeline}; use crate::walrecord; -use anyhow::{ensure, Result}; +use anyhow::{anyhow, ensure, Result}; +use pageserver_api::keyspace::KeySpace; use pageserver_api::models::InMemoryLayerInfo; use pageserver_api::shard::TenantShardId; -use std::collections::HashMap; +use std::collections::{BinaryHeap, HashMap, HashSet}; use std::sync::{Arc, OnceLock}; use tracing::*; use utils::{bin_ser::BeSer, id::TimelineId, lsn::Lsn, vec_map::VecMap}; @@ -25,7 +27,10 @@ use std::fmt::Write as _; use std::ops::Range; use tokio::sync::{RwLock, RwLockWriteGuard}; -use super::{DeltaLayerWriter, ResidentLayer}; +use super::{ + DeltaLayerWriter, ResidentLayer, ValueReconstructSituation, ValueReconstructState, + ValuesReconstructState, +}; pub struct InMemoryLayer { conf: &'static PageServerConf, @@ -202,6 +207,91 @@ impl InMemoryLayer { Ok(ValueReconstructResult::Complete) } } + + // Look up the keys in the provided keyspace and update + // the reconstruct state with whatever is found. + // + // If the key is cached, go no further than the cached Lsn. + pub(crate) async fn get_values_reconstruct_data( + &self, + keyspace: KeySpace, + end_lsn: Lsn, + reconstruct_state: &mut ValuesReconstructState, + ctx: &RequestContext, + ) -> Result<(), GetVectoredError> { + let ctx = RequestContextBuilder::extend(ctx) + .page_content_kind(PageContentKind::InMemoryLayer) + .build(); + + let inner = self.inner.read().await; + let reader = inner.file.block_cursor(); + + #[derive(Eq, PartialEq, Ord, PartialOrd)] + struct BlockRead { + key: Key, + lsn: Lsn, + block_offset: u64, + } + + let mut planned_block_reads = BinaryHeap::new(); + + for range in keyspace.ranges.iter() { + let mut key = range.start; + while key < range.end { + if let Some(vec_map) = inner.index.get(&key) { + let lsn_range = match reconstruct_state.get_cached_lsn(&key) { + Some(cached_lsn) => (cached_lsn + 1)..end_lsn, + None => self.start_lsn..end_lsn, + }; + + let slice = vec_map.slice_range(lsn_range); + for (entry_lsn, pos) in slice.iter().rev() { + planned_block_reads.push(BlockRead { + key, + lsn: *entry_lsn, + block_offset: *pos, + }); + } + } + + key = key.next(); + } + } + + let keyspace_size = keyspace.total_size(); + + let mut completed_keys = HashSet::new(); + while completed_keys.len() < keyspace_size && !planned_block_reads.is_empty() { + let block_read = planned_block_reads.pop().unwrap(); + if completed_keys.contains(&block_read.key) { + continue; + } + + let buf = reader.read_blob(block_read.block_offset, &ctx).await; + if let Err(e) = buf { + reconstruct_state + .on_key_error(block_read.key, PageReconstructError::from(anyhow!(e))); + completed_keys.insert(block_read.key); + continue; + } + + let value = Value::des(&buf.unwrap()); + if let Err(e) = value { + reconstruct_state + .on_key_error(block_read.key, PageReconstructError::from(anyhow!(e))); + completed_keys.insert(block_read.key); + continue; + } + + let key_situation = + reconstruct_state.update_key(&block_read.key, block_read.lsn, value.unwrap()); + if key_situation == ValueReconstructSituation::Complete { + completed_keys.insert(block_read.key); + } + } + + Ok(()) + } } impl std::fmt::Display for InMemoryLayer { diff --git a/pageserver/src/tenant/storage_layer/layer.rs b/pageserver/src/tenant/storage_layer/layer.rs index bfcc031863..cc5b7ade6a 100644 --- a/pageserver/src/tenant/storage_layer/layer.rs +++ b/pageserver/src/tenant/storage_layer/layer.rs @@ -1,5 +1,6 @@ use anyhow::Context; use camino::{Utf8Path, Utf8PathBuf}; +use pageserver_api::keyspace::KeySpace; use pageserver_api::models::{ HistoricLayerInfo, LayerAccessKind, LayerResidenceEventReason, LayerResidenceStatus, }; @@ -16,13 +17,14 @@ use crate::config::PageServerConf; use crate::context::RequestContext; use crate::repository::Key; use crate::span::debug_assert_current_span_has_tenant_and_timeline_id; +use crate::tenant::timeline::GetVectoredError; use crate::tenant::{remote_timeline_client::LayerFileMetadata, Timeline}; use super::delta_layer::{self, DeltaEntry}; use super::image_layer; use super::{ AsLayerDesc, LayerAccessStats, LayerAccessStatsReset, LayerFileName, PersistentLayerDesc, - ValueReconstructResult, ValueReconstructState, + ValueReconstructResult, ValueReconstructState, ValuesReconstructState, }; use utils::generation::Generation; @@ -262,6 +264,29 @@ impl Layer { .with_context(|| format!("get_value_reconstruct_data for layer {self}")) } + pub(crate) async fn get_values_reconstruct_data( + &self, + keyspace: KeySpace, + end_lsn: Lsn, + reconstruct_data: &mut ValuesReconstructState, + ctx: &RequestContext, + ) -> Result<(), GetVectoredError> { + let layer = self + .0 + .get_or_maybe_download(true, Some(ctx)) + .await + .map_err(|err| GetVectoredError::Other(anyhow::anyhow!(err)))?; + + self.0 + .access_stats + .record_access(LayerAccessKind::GetValueReconstructData, ctx); + + layer + .get_values_reconstruct_data(keyspace, end_lsn, reconstruct_data, &self.0, ctx) + .instrument(tracing::debug_span!("get_values_reconstruct_data", layer=%self)) + .await + } + /// Download the layer if evicted. /// /// Will not error when the layer is already downloaded. @@ -1177,7 +1202,7 @@ pub(crate) enum EvictionError { /// Error internal to the [`LayerInner::get_or_maybe_download`] #[derive(Debug, thiserror::Error)] -enum DownloadError { +pub(crate) enum DownloadError { #[error("timeline has already shutdown")] TimelineShutdown, #[error("no remote storage configured")] @@ -1337,6 +1362,28 @@ impl DownloadedLayer { } } + async fn get_values_reconstruct_data( + &self, + keyspace: KeySpace, + end_lsn: Lsn, + reconstruct_data: &mut ValuesReconstructState, + owner: &Arc, + ctx: &RequestContext, + ) -> Result<(), GetVectoredError> { + use LayerKind::*; + + match self.get(owner, ctx).await.map_err(GetVectoredError::from)? { + Delta(d) => { + d.get_values_reconstruct_data(keyspace, end_lsn, reconstruct_data, ctx) + .await + } + Image(i) => { + i.get_values_reconstruct_data(keyspace, reconstruct_data, ctx) + .await + } + } + } + async fn dump(&self, owner: &Arc, ctx: &RequestContext) -> anyhow::Result<()> { use LayerKind::*; match self.get(owner, ctx).await? { diff --git a/pageserver/src/tenant/storage_layer/layer_desc.rs b/pageserver/src/tenant/storage_layer/layer_desc.rs index fa78e9fdb2..c375923e81 100644 --- a/pageserver/src/tenant/storage_layer/layer_desc.rs +++ b/pageserver/src/tenant/storage_layer/layer_desc.rs @@ -15,7 +15,7 @@ use utils::id::TenantId; /// A unique identifier of a persistent layer. This is different from `LayerDescriptor`, which is only used in the /// benchmarks. This struct contains all necessary information to find the image / delta layer. It also provides /// a unified way to generate layer information like file name. -#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] +#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Hash)] pub struct PersistentLayerDesc { pub tenant_shard_id: TenantShardId, pub timeline_id: TimelineId, diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 92e5b52c75..0f22284c55 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -16,7 +16,7 @@ use futures::stream::StreamExt; use itertools::Itertools; use once_cell::sync::Lazy; use pageserver_api::{ - keyspace::{key_range_size, KeySpaceAccum}, + keyspace::KeySpaceAccum, models::{ DownloadRemoteLayersTaskInfo, DownloadRemoteLayersTaskSpawnRequest, EvictionPolicy, LayerMapInfo, TimelineState, @@ -67,7 +67,7 @@ use crate::{ tenant::storage_layer::{ AsLayerDesc, DeltaLayerWriter, EvictionError, ImageLayerWriter, InMemoryLayer, Layer, LayerAccessStatsReset, LayerFileName, ResidentLayer, ValueReconstructResult, - ValueReconstructState, + ValueReconstructState, ValuesReconstructState, }, }; use crate::{ @@ -111,11 +111,11 @@ use self::layer_manager::LayerManager; use self::logical_size::LogicalSize; use self::walreceiver::{WalReceiver, WalReceiverConf}; -use super::config::TenantConf; -use super::remote_timeline_client::index::IndexPart; use super::remote_timeline_client::RemoteTimelineClient; use super::secondary::heatmap::{HeatMapLayer, HeatMapTimeline}; +use super::{config::TenantConf, storage_layer::ReadableLayerDesc}; use super::{debug_assert_current_span_has_tenant_and_timeline_id, AttachedTenantConf}; +use super::{remote_timeline_client::index::IndexPart, storage_layer::LayerFringe}; #[derive(Debug, PartialEq, Eq, Clone, Copy)] pub(super) enum FlushLoopState { @@ -472,6 +472,15 @@ pub(crate) enum GetVectoredError { #[error("Requested at invalid LSN: {0}")] InvalidLsn(Lsn), + + #[error("Requested key {0} not found")] + MissingKey(Key), + + #[error(transparent)] + GetReadyAncestorError(GetReadyAncestorError), + + #[error(transparent)] + Other(#[from] anyhow::Error), } #[derive(thiserror::Error, Debug)] @@ -579,6 +588,23 @@ impl From for PageReconstructError { } } +#[derive( + Eq, + PartialEq, + Debug, + Copy, + Clone, + strum_macros::EnumString, + strum_macros::Display, + serde_with::DeserializeFromStr, + serde_with::SerializeDisplay, +)] +#[strum(serialize_all = "kebab-case")] +pub enum GetVectoredImpl { + Sequential, + Vectored, +} + /// Public interface functions impl Timeline { /// Get the LSN where this branch was created @@ -708,7 +734,7 @@ impl Timeline { /// which actually vectorizes the read path. pub(crate) async fn get_vectored( &self, - key_ranges: &[Range], + keyspace: KeySpace, lsn: Lsn, ctx: &RequestContext, ) -> Result>, GetVectoredError> { @@ -716,10 +742,7 @@ impl Timeline { return Err(GetVectoredError::InvalidLsn(lsn)); } - let key_count = key_ranges - .iter() - .map(|range| key_range_size(range) as u64) - .sum(); + let key_count = keyspace.total_size().try_into().unwrap(); if key_count > Timeline::MAX_GET_VECTORED_KEYS { return Err(GetVectoredError::Oversized(key_count)); } @@ -728,33 +751,163 @@ impl Timeline { .throttle(ctx, key_count as usize) .await; - let _timer = crate::metrics::GET_VECTORED_LATENCY - .for_task_kind(ctx.task_kind()) - .map(|t| t.start_timer()); - - let mut values = BTreeMap::new(); - for range in key_ranges { + for range in &keyspace.ranges { let mut key = range.start; while key != range.end { assert!(!self.shard_identity.is_key_disposable(&key)); - - let block = self.get(key, lsn, ctx).await; - - if matches!( - block, - Err(PageReconstructError::Cancelled | PageReconstructError::AncestorStopping(_)) - ) { - return Err(GetVectoredError::Cancelled); - } - - values.insert(key, block); key = key.next(); } } + trace!( + "get vectored request for {:?}@{} from task kind {:?} will use {} implementation", + keyspace, + lsn, + ctx.task_kind(), + self.conf.get_vectored_impl + ); + + let _timer = crate::metrics::GET_VECTORED_LATENCY + .for_task_kind(ctx.task_kind()) + .map(|t| t.start_timer()); + + match self.conf.get_vectored_impl { + GetVectoredImpl::Sequential => { + self.get_vectored_sequential_impl(keyspace, lsn, ctx).await + } + GetVectoredImpl::Vectored => { + let vectored_res = self.get_vectored_impl(keyspace.clone(), lsn, ctx).await; + + self.validate_get_vectored_impl(&vectored_res, keyspace, lsn, ctx) + .await; + + vectored_res + } + } + } + + pub(super) async fn get_vectored_sequential_impl( + &self, + keyspace: KeySpace, + lsn: Lsn, + ctx: &RequestContext, + ) -> Result>, GetVectoredError> { + let mut values = BTreeMap::new(); + for range in keyspace.ranges { + let mut key = range.start; + while key != range.end { + let block = self.get(key, lsn, ctx).await; + + use PageReconstructError::*; + match block { + Err(Cancelled | AncestorStopping(_)) => { + return Err(GetVectoredError::Cancelled) + } + Err(Other(err)) if err.to_string().contains("could not find data for key") => { + return Err(GetVectoredError::MissingKey(key)) + } + _ => { + values.insert(key, block); + key = key.next(); + } + } + } + } + Ok(values) } + pub(super) async fn get_vectored_impl( + &self, + keyspace: KeySpace, + lsn: Lsn, + ctx: &RequestContext, + ) -> Result>, GetVectoredError> { + let mut reconstruct_state = ValuesReconstructState::new(); + + self.get_vectored_reconstruct_data(keyspace, lsn, &mut reconstruct_state, ctx) + .await?; + + let mut results: BTreeMap> = BTreeMap::new(); + for (key, res) in reconstruct_state.keys { + match res { + Err(err) => { + results.insert(key, Err(err)); + } + Ok(state) => { + let state = ValueReconstructState::from(state); + + let reconstruct_res = self.reconstruct_value(key, lsn, state).await; + results.insert(key, reconstruct_res); + } + } + } + + Ok(results) + } + + pub(super) async fn validate_get_vectored_impl( + &self, + vectored_res: &Result>, GetVectoredError>, + keyspace: KeySpace, + lsn: Lsn, + ctx: &RequestContext, + ) { + let sequential_res = self + .get_vectored_sequential_impl(keyspace.clone(), lsn, ctx) + .await; + + fn errors_match(lhs: &GetVectoredError, rhs: &GetVectoredError) -> bool { + use GetVectoredError::*; + match (lhs, rhs) { + (Cancelled, Cancelled) => true, + (_, Cancelled) => true, + (Oversized(l), Oversized(r)) => l == r, + (InvalidLsn(l), InvalidLsn(r)) => l == r, + (MissingKey(l), MissingKey(r)) => l == r, + (GetReadyAncestorError(_), GetReadyAncestorError(_)) => true, + (Other(_), Other(_)) => true, + _ => false, + } + } + + match (&sequential_res, vectored_res) { + (Err(seq_err), Ok(_)) => { + panic!(concat!("Sequential get failed with {}, but vectored get did not", + " - keyspace={:?} lsn={}"), + seq_err, keyspace, lsn) }, + (Ok(_), Err(vec_err)) => { + panic!(concat!("Vectored get failed with {}, but sequential get did not", + " - keyspace={:?} lsn={}"), + vec_err, keyspace, lsn) }, + (Err(seq_err), Err(vec_err)) => { + assert!(errors_match(seq_err, vec_err), + "Mismatched errors: {seq_err} != {vec_err} - keyspace={keyspace:?} lsn={lsn}")}, + (Ok(seq_values), Ok(vec_values)) => { + seq_values.iter().zip(vec_values.iter()).for_each(|((seq_key, seq_res), (vec_key, vec_res))| { + assert_eq!(seq_key, vec_key); + match (seq_res, vec_res) { + (Ok(seq_blob), Ok(vec_blob)) => { + assert_eq!(seq_blob, vec_blob, + "Image mismatch for key {seq_key} - keyspace={keyspace:?} lsn={lsn}"); + }, + (Err(err), Ok(_)) => { + panic!( + concat!("Sequential get failed with {} for key {}, but vectored get did not", + " - keyspace={:?} lsn={}"), + err, seq_key, keyspace, lsn) }, + (Ok(_), Err(err)) => { + panic!( + concat!("Vectored get failed with {} for key {}, but sequential get did not", + " - keyspace={:?} lsn={}"), + err, seq_key, keyspace, lsn) }, + (Err(_), Err(_)) => {} + } + }) + } + } + } + /// Get last or prev record separately. Same as get_last_record_rlsn().last/prev. pub(crate) fn get_last_record_lsn(&self) -> Lsn { self.last_record_lsn.load().last @@ -2547,6 +2700,170 @@ impl Timeline { } } + /// Get the data needed to reconstruct all keys in the provided keyspace + /// + /// The algorithm is as follows: + /// 1. While some keys are still not done and there's a timeline to visit: + /// 2. Visit the timeline (see [`Timeline::get_vectored_reconstruct_data_timeline`]: + /// 2.1: Build the fringe for the current keyspace + /// 2.2 Visit the newest layer from the fringe to collect all values for the range it + /// intersects + /// 2.3. Pop the timeline from the fringe + /// 2.4. If the fringe is empty, go back to 1 + async fn get_vectored_reconstruct_data( + &self, + mut keyspace: KeySpace, + request_lsn: Lsn, + reconstruct_state: &mut ValuesReconstructState, + ctx: &RequestContext, + ) -> Result<(), GetVectoredError> { + let mut timeline_owned: Arc; + let mut timeline = self; + + let mut cont_lsn = Lsn(request_lsn.0 + 1); + + loop { + if self.cancel.is_cancelled() { + return Err(GetVectoredError::Cancelled); + } + + let completed = Self::get_vectored_reconstruct_data_timeline( + timeline, + keyspace.clone(), + cont_lsn, + reconstruct_state, + &self.cancel, + ctx, + ) + .await?; + + keyspace.remove_overlapping_with(&completed); + if keyspace.total_size() == 0 || timeline.ancestor_timeline.is_none() { + break; + } + + cont_lsn = Lsn(timeline.ancestor_lsn.0 + 1); + timeline_owned = timeline + .get_ready_ancestor_timeline(ctx) + .await + .map_err(GetVectoredError::GetReadyAncestorError)?; + timeline = &*timeline_owned; + } + + if keyspace.total_size() != 0 { + return Err(GetVectoredError::MissingKey(keyspace.start().unwrap())); + } + + Ok(()) + } + + /// Collect the reconstruct data for a ketspace from the specified timeline. + /// + /// Maintain a fringe [`LayerFringe`] which tracks all the layers that intersect + /// the current keyspace. The current keyspace of the search at any given timeline + /// is the original keyspace minus all the keys that have been completed minus + /// any keys for which we couldn't find an intersecting layer. It's not tracked explicitly, + /// but if you merge all the keyspaces in the fringe, you get the "current keyspace". + /// + /// This is basically a depth-first search visitor implementation where a vertex + /// is the (layer, lsn range, key space) tuple. The fringe acts as the stack. + /// + /// At each iteration pop the top of the fringe (the layer with the highest Lsn) + /// and get all the required reconstruct data from the layer in one go. + async fn get_vectored_reconstruct_data_timeline( + timeline: &Timeline, + keyspace: KeySpace, + mut cont_lsn: Lsn, + reconstruct_state: &mut ValuesReconstructState, + cancel: &CancellationToken, + ctx: &RequestContext, + ) -> Result { + let mut unmapped_keyspace = keyspace.clone(); + let mut fringe = LayerFringe::new(); + + let mut completed_keyspace = KeySpace::default(); + + // Hold the layer map whilst visiting the timeline to prevent + // compaction, eviction and flushes from rendering the layers unreadable. + // + // TODO: Do we actually need to do this? In theory holding on + // to [`tenant::storage_layer::Layer`] should be enough. However, + // [`Timeline::get`] also holds the lock during IO, so more investigation + // is needed. + let guard = timeline.layers.read().await; + let layers = guard.layer_map(); + + 'outer: loop { + if cancel.is_cancelled() { + return Err(GetVectoredError::Cancelled); + } + + let keys_done_last_step = reconstruct_state.consume_done_keys(); + unmapped_keyspace.remove_overlapping_with(&keys_done_last_step); + completed_keyspace.merge(&keys_done_last_step); + + let in_memory_layer = layers.find_in_memory_layer(|l| { + let start_lsn = l.get_lsn_range().start; + cont_lsn > start_lsn + }); + + match in_memory_layer { + Some(l) => { + fringe.update( + ReadableLayerDesc::InMemory { + handle: l, + lsn_ceil: cont_lsn, + }, + unmapped_keyspace.clone(), + ); + } + None => { + for range in unmapped_keyspace.ranges.iter() { + let results = match layers.range_search(range.clone(), cont_lsn) { + Some(res) => res, + None => { + break 'outer; + } + }; + + results + .found + .into_iter() + .map(|(SearchResult { layer, lsn_floor }, keyspace_accum)| { + ( + ReadableLayerDesc::Persistent { + desc: (*layer).clone(), + lsn_floor, + lsn_ceil: cont_lsn, + }, + keyspace_accum.to_keyspace(), + ) + }) + .for_each(|(layer, keyspace)| fringe.update(layer, keyspace)); + } + } + } + + if let Some((layer_to_read, keyspace_to_read)) = fringe.next_layer() { + layer_to_read + .get_values_reconstruct_data( + &guard, + keyspace_to_read.clone(), + reconstruct_state, + ctx, + ) + .await?; + + unmapped_keyspace = keyspace_to_read; + cont_lsn = layer_to_read.get_lsn_floor(); + } else { + break; + } + } + + Ok(completed_keyspace) + } + /// # Cancel-safety /// /// This method is cancellation-safe. @@ -3263,7 +3580,7 @@ impl Timeline { || last_key_in_range { let results = self - .get_vectored(&key_request_accum.consume_keyspace().ranges, lsn, ctx) + .get_vectored(key_request_accum.consume_keyspace(), lsn, ctx) .await?; for (img_key, img) in results { diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 51b126b84b..ce5ef66d22 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -488,6 +488,11 @@ class NeonEnvBuilder: self.pageserver_virtual_file_io_engine: Optional[str] = pageserver_virtual_file_io_engine + self.pageserver_get_vectored_impl: Optional[str] = None + if os.getenv("PAGESERVER_GET_VECTORED_IMPL", "") == "vectored": + self.pageserver_get_vectored_impl = "vectored" + log.debug('Overriding pageserver get_vectored_impl config to "vectored"') + assert test_name.startswith( "test_" ), "Unexpectedly instantiated from outside a test function" @@ -1055,6 +1060,8 @@ class NeonEnv: } if self.pageserver_virtual_file_io_engine is not None: ps_cfg["virtual_file_io_engine"] = self.pageserver_virtual_file_io_engine + if config.pageserver_get_vectored_impl is not None: + ps_cfg["get_vectored_impl"] = config.pageserver_get_vectored_impl # Create a corresponding NeonPageserver object self.pageservers.append( diff --git a/test_runner/regress/test_compatibility.py b/test_runner/regress/test_compatibility.py index 465101f64f..0ea76d447e 100644 --- a/test_runner/regress/test_compatibility.py +++ b/test_runner/regress/test_compatibility.py @@ -226,6 +226,10 @@ def test_forward_compatibility( ) try: + # TODO: remove this once the previous pageserrver version understands + # the 'get_vectored_impl' config + neon_env_builder.pageserver_get_vectored_impl = None + neon_env_builder.num_safekeepers = 3 neon_local_binpath = neon_env_builder.neon_binpath env = neon_env_builder.from_repo_dir( From e7452d3756c3bd00a56cb3cd49dc991f5e533baf Mon Sep 17 00:00:00 2001 From: John Spray Date: Wed, 21 Feb 2024 09:54:25 +0000 Subject: [PATCH 08/15] storage controller: concurrency + deadlines during startup reconcile (#6823) ## Problem During startup_reconcile we do a couple of potentially-slow things: - Calling out to all nodes to read their locations - Calling out to the cloud control plane to notify it of all tenants' attached nodes The read of node locations was not being done concurrently across nodes, and neither operation was bounded by a well defined deadline. ## Summary of changes - Refactor the async parts of startup_reconcile into separate functions - Add concurrency and deadline to `scan_node_locations` - Add deadline to `compute_notify_many` - Run `cleanup_locations` in the background: there's no need for startup_reconcile to wait for this to complete. --- .../attachment_service/src/service.rs | 333 ++++++++++++------ 1 file changed, 234 insertions(+), 99 deletions(-) diff --git a/control_plane/attachment_service/src/service.rs b/control_plane/attachment_service/src/service.rs index 4082af3fe6..0236496c61 100644 --- a/control_plane/attachment_service/src/service.rs +++ b/control_plane/attachment_service/src/service.rs @@ -21,11 +21,11 @@ use pageserver_api::{ ReAttachRequest, ReAttachResponse, ReAttachResponseTenant, ValidateRequest, ValidateResponse, ValidateResponseTenant, }, - models, models::{ - LocationConfig, LocationConfigMode, ShardParameters, TenantConfig, TenantCreateRequest, - TenantLocationConfigRequest, TenantLocationConfigResponse, TenantShardLocation, - TenantShardSplitRequest, TenantShardSplitResponse, TimelineCreateRequest, TimelineInfo, + self, LocationConfig, LocationConfigListResponse, LocationConfigMode, ShardParameters, + TenantConfig, TenantCreateRequest, TenantLocationConfigRequest, + TenantLocationConfigResponse, TenantShardLocation, TenantShardSplitRequest, + TenantShardSplitResponse, TimelineCreateRequest, TimelineInfo, }, shard::{ShardCount, ShardIdentity, ShardNumber, ShardStripeSize, TenantShardId}, }; @@ -167,84 +167,53 @@ impl Service { /// Called once on startup, this function attempts to contact all pageservers to build an up-to-date /// view of the world, and determine which pageservers are responsive. #[instrument(skip_all)] - async fn startup_reconcile(&self) { + async fn startup_reconcile(self: &Arc) { // For all tenant shards, a vector of observed states on nodes (where None means // indeterminate, same as in [`ObservedStateLocation`]) let mut observed = HashMap::new(); let mut nodes_online = HashSet::new(); - // TODO: issue these requests concurrently - { - let nodes = { - let locked = self.inner.read().unwrap(); - locked.nodes.clone() - }; - for node in nodes.values() { - let http_client = reqwest::ClientBuilder::new() - .timeout(Duration::from_secs(5)) - .build() - .expect("Failed to construct HTTP client"); - let client = mgmt_api::Client::from_client( - http_client, - node.base_url(), - self.config.jwt_token.as_deref(), - ); + // Startup reconciliation does I/O to other services: whether they + // are responsive or not, we should aim to finish within our deadline, because: + // - If we don't, a k8s readiness hook watching /ready will kill us. + // - While we're waiting for startup reconciliation, we are not fully + // available for end user operations like creating/deleting tenants and timelines. + // + // We set multiple deadlines to break up the time available between the phases of work: this is + // arbitrary, but avoids a situation where the first phase could burn our entire timeout period. + let start_at = Instant::now(); + let node_scan_deadline = start_at + .checked_add(STARTUP_RECONCILE_TIMEOUT / 2) + .expect("Reconcile timeout is a modest constant"); - 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, - } - } + let compute_notify_deadline = start_at + .checked_add((STARTUP_RECONCILE_TIMEOUT / 4) * 3) + .expect("Reconcile timeout is a modest constant"); - let list_response = backoff::retry( - || client.list_location_config(), - is_fatal, - 1, - 5, - "Location config listing", - &self.cancel, - ) - .await; - let Some(list_response) = list_response else { - tracing::info!("Shutdown during startup_reconcile"); - return; - }; + // Accumulate a list of any tenant locations that ought to be detached + let mut cleanup = Vec::new(); - tracing::info!("Scanning shards on node {}...", node.id); - match list_response { - Err(e) => { - tracing::warn!("Could not contact pageserver {} ({e})", node.id); - // TODO: be more tolerant, do some retries, in case - // pageserver is being restarted at the same time as we are - } - Ok(listing) => { - tracing::info!( - "Received {} shard statuses from pageserver {}, setting it to Active", - listing.tenant_shards.len(), - node.id - ); - nodes_online.insert(node.id); + let node_listings = self.scan_node_locations(node_scan_deadline).await; + for (node_id, list_response) in node_listings { + let tenant_shards = list_response.tenant_shards; + tracing::info!( + "Received {} shard statuses from pageserver {}, setting it to Active", + tenant_shards.len(), + node_id + ); + nodes_online.insert(node_id); - for (tenant_shard_id, conf_opt) in listing.tenant_shards { - observed.insert(tenant_shard_id, (node.id, conf_opt)); - } - } - } + for (tenant_shard_id, conf_opt) in tenant_shards { + observed.insert(tenant_shard_id, (node_id, conf_opt)); } } - let mut cleanup = Vec::new(); - + // List of tenants for which we will attempt to notify compute of their location at startup let mut compute_notifications = Vec::new(); // Populate intent and observed states for all tenants, based on reported state on pageservers - let (shard_count, nodes) = { + let shard_count = { let mut locked = self.inner.write().unwrap(); let (nodes, tenants, scheduler) = locked.parts_mut(); @@ -288,18 +257,171 @@ impl Service { } } - (tenants.len(), nodes.clone()) + tenants.len() }; // TODO: if any tenant's intent now differs from its loaded generation_pageserver, we should clear that // generation_pageserver in the database. - // Clean up any tenants that were found on pageservers but are not known to us. + // Emit compute hook notifications for all tenants which are already stably attached. Other tenants + // will emit compute hook notifications when they reconcile. + // + // Ordering: we must complete these notification attempts before doing any other reconciliation for the + // tenants named here, because otherwise our calls to notify() might race with more recent values + // generated by reconciliation. + let notify_failures = self + .compute_notify_many(compute_notifications, compute_notify_deadline) + .await; + + // Compute notify is fallible. If it fails here, do not delay overall startup: set the + // flag on these shards that they have a pending notification. + // Update tenant state for any that failed to do their initial compute notify, so that they'll retry later. + { + let mut locked = self.inner.write().unwrap(); + for tenant_shard_id in notify_failures.into_iter() { + if let Some(shard) = locked.tenants.get_mut(&tenant_shard_id) { + shard.pending_compute_notification = true; + } + } + } + + // Finally, now that the service is up and running, launch reconcile operations for any tenants + // which require it: under normal circumstances this should only include tenants that were in some + // transient state before we restarted, or any tenants whose compute hooks failed above. + let reconcile_tasks = self.reconcile_all(); + // We will not wait for these reconciliation tasks to run here: we're now done with startup and + // normal operations may proceed. + + // Clean up any tenants that were found on pageservers but are not known to us. Do this in the + // background because it does not need to complete in order to proceed with other work. + if !cleanup.is_empty() { + tracing::info!("Cleaning up {} locations in the background", cleanup.len()); + tokio::task::spawn({ + let cleanup_self = self.clone(); + async move { cleanup_self.cleanup_locations(cleanup).await } + }); + } + + tracing::info!("Startup complete, spawned {reconcile_tasks} reconciliation tasks ({shard_count} shards total)"); + } + + /// Used during [`Self::startup_reconcile`]: issue GETs to all nodes concurrently, with a deadline. + /// + /// The result includes only nodes which responded within the deadline + async fn scan_node_locations( + &self, + deadline: Instant, + ) -> HashMap { + let nodes = { + let locked = self.inner.read().unwrap(); + locked.nodes.clone() + }; + + let mut node_results = HashMap::new(); + + let mut node_list_futs = FuturesUnordered::new(); + + for node in nodes.values() { + node_list_futs.push({ + async move { + let http_client = reqwest::ClientBuilder::new() + .timeout(Duration::from_secs(5)) + .build() + .expect("Failed to construct HTTP client"); + let client = mgmt_api::Client::from_client( + http_client, + node.base_url(), + self.config.jwt_token.as_deref(), + ); + + 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, + } + } + + tracing::info!("Scanning shards on node {}...", node.id); + let description = format!("List locations on {}", node.id); + let response = backoff::retry( + || client.list_location_config(), + is_fatal, + 1, + 5, + &description, + &self.cancel, + ) + .await; + + (node.id, response) + } + }); + } + + loop { + let (node_id, result) = tokio::select! { + next = node_list_futs.next() => { + match next { + Some(result) => result, + None =>{ + // We got results for all our nodes + break; + } + + } + }, + _ = tokio::time::sleep(deadline.duration_since(Instant::now())) => { + // Give up waiting for anyone who hasn't responded: we will yield the results that we have + tracing::info!("Reached deadline while waiting for nodes to respond to location listing requests"); + break; + } + }; + + let Some(list_response) = result else { + tracing::info!("Shutdown during startup_reconcile"); + break; + }; + + match list_response { + Err(e) => { + tracing::warn!("Could not scan node {} ({e})", node_id); + } + Ok(listing) => { + node_results.insert(node_id, listing); + } + } + } + + node_results + } + + /// Used during [`Self::startup_reconcile`]: detach a list of unknown-to-us tenants from pageservers. + /// + /// This is safe to run in the background, because if we don't have this TenantShardId in our map of + /// tenants, then it is probably something incompletely deleted before: we will not fight with any + /// other task trying to attach it. + #[instrument(skip_all)] + async fn cleanup_locations(&self, cleanup: Vec<(TenantShardId, NodeId)>) { + let nodes = self.inner.read().unwrap().nodes.clone(); + for (tenant_shard_id, node_id) in cleanup { // A node reported a tenant_shard_id which is unknown to us: detach it. - let node = nodes - .get(&node_id) - .expect("Always exists: only known nodes are scanned"); + let Some(node) = nodes.get(&node_id) else { + // This is legitimate; we run in the background and [`Self::startup_reconcile`] might have identified + // a location to clean up on a node that has since been removed. + tracing::info!( + "Not cleaning up location {node_id}/{tenant_shard_id}: node not found" + ); + continue; + }; + + if self.cancel.is_cancelled() { + break; + } let client = mgmt_api::Client::new(node.base_url(), self.config.jwt_token.as_deref()); match client @@ -332,21 +454,24 @@ impl Service { } } } + } - // Emit compute hook notifications for all tenants which are already stably attached. Other tenants - // will emit compute hook notifications when they reconcile. - // - // Ordering: we must complete these notification attempts before doing any other reconciliation for the - // tenants named here, because otherwise our calls to notify() might race with more recent values - // generated by reconciliation. - - // Compute notify is fallible. If it fails here, do not delay overall startup: set the - // flag on these shards that they have a pending notification. + /// Used during [`Self::startup_reconcile`]: issue many concurrent compute notifications. + /// + /// Returns a set of any shards for which notifications where not acked within the deadline. + async fn compute_notify_many( + &self, + notifications: Vec<(TenantShardId, NodeId)>, + deadline: Instant, + ) -> HashSet { let compute_hook = self.inner.read().unwrap().compute_hook.clone(); + let attempt_shards = notifications.iter().map(|i| i.0).collect::>(); + let mut success_shards = HashSet::new(); + // Construct an async stream of futures to invoke the compute notify function: we do this // in order to subsequently use .buffered() on the stream to execute with bounded parallelism. - let stream = futures::stream::iter(compute_notifications.into_iter()) + let mut stream = futures::stream::iter(notifications.into_iter()) .map(|(tenant_shard_id, node_id)| { let compute_hook = compute_hook.clone(); let cancel = self.cancel.clone(); @@ -357,33 +482,43 @@ impl Service { node_id=%node_id, "Failed to notify compute on startup for shard: {e}" ); - Some(tenant_shard_id) - } else { None + } else { + Some(tenant_shard_id) } } }) .buffered(compute_hook::API_CONCURRENCY); - let notify_results = stream.collect::>().await; - // Update tenant state for any that failed to do their initial compute notify, so that they'll retry later. - { - let mut locked = self.inner.write().unwrap(); - for tenant_shard_id in notify_results.into_iter().flatten() { - if let Some(shard) = locked.tenants.get_mut(&tenant_shard_id) { - shard.pending_compute_notification = true; + loop { + tokio::select! { + next = stream.next() => { + match next { + Some(Some(success_shard)) => { + // A notification succeeded + success_shards.insert(success_shard); + }, + Some(None) => { + // A notification that failed + }, + None => { + tracing::info!("Successfully sent all compute notifications"); + break; + } + } + }, + _ = tokio::time::sleep(deadline.duration_since(Instant::now())) => { + // Give up sending any that didn't succeed yet + tracing::info!("Reached deadline while sending compute notifications"); + break; } - } + }; } - // Finally, now that the service is up and running, launch reconcile operations for any tenants - // which require it: under normal circumstances this should only include tenants that were in some - // transient state before we restarted, or any tenants whose compute hooks failed above. - let reconcile_tasks = self.reconcile_all(); - // We will not wait for these reconciliation tasks to run here: we're now done with startup and - // normal operations may proceed. - - tracing::info!("Startup complete, spawned {reconcile_tasks} reconciliation tasks ({shard_count} shards total)"); + attempt_shards + .difference(&success_shards) + .cloned() + .collect() } /// Long running background task that periodically wakes up and looks for shards that need From e0af945f8f552c546bc114edce10aff35d990b5b Mon Sep 17 00:00:00 2001 From: Conrad Ludgate Date: Wed, 21 Feb 2024 10:04:09 +0000 Subject: [PATCH 09/15] proxy: improve error classification (#6841) ## Problem ## Summary of changes 1. Classify further cplane API errors 2. add 'serviceratelimit' and make a few of the timeout errors return that. 3. a few additional minor changes --- proxy/src/bin/pg_sni_router.rs | 14 +++----------- proxy/src/console/provider.rs | 18 +++++++++++++++++- proxy/src/context.rs | 12 +++++------- proxy/src/error.rs | 26 +++++--------------------- proxy/src/serverless/sql_over_http.rs | 19 +++++++------------ 5 files changed, 37 insertions(+), 52 deletions(-) diff --git a/proxy/src/bin/pg_sni_router.rs b/proxy/src/bin/pg_sni_router.rs index 43b805e8a1..5024ba3744 100644 --- a/proxy/src/bin/pg_sni_router.rs +++ b/proxy/src/bin/pg_sni_router.rs @@ -171,16 +171,8 @@ async fn task_main( .context("failed to set socket option")?; info!(%peer_addr, "serving"); - let mut ctx = - RequestMonitoring::new(session_id, peer_addr.ip(), "sni_router", "sni"); - handle_client( - &mut ctx, - dest_suffix, - tls_config, - tls_server_end_point, - socket, - ) - .await + let ctx = RequestMonitoring::new(session_id, peer_addr.ip(), "sni_router", "sni"); + handle_client(ctx, dest_suffix, tls_config, tls_server_end_point, socket).await } .unwrap_or_else(|e| { // Acknowledge that the task has finished with an error. @@ -248,7 +240,7 @@ async fn ssl_handshake( } async fn handle_client( - ctx: &mut RequestMonitoring, + mut ctx: RequestMonitoring, dest_suffix: Arc, tls_config: Arc, tls_server_end_point: TlsServerEndPoint, diff --git a/proxy/src/console/provider.rs b/proxy/src/console/provider.rs index 640444d14e..0b74cd90cc 100644 --- a/proxy/src/console/provider.rs +++ b/proxy/src/console/provider.rs @@ -87,6 +87,22 @@ pub mod errors { impl ReportableError for ApiError { fn get_error_kind(&self) -> crate::error::ErrorKind { match self { + ApiError::Console { + status: http::StatusCode::NOT_FOUND | http::StatusCode::NOT_ACCEPTABLE, + .. + } => crate::error::ErrorKind::User, + ApiError::Console { + status: http::StatusCode::LOCKED, + text, + } if text.contains("quota exceeded") + || text.contains("the limit for current plan reached") => + { + crate::error::ErrorKind::User + } + ApiError::Console { + status: http::StatusCode::TOO_MANY_REQUESTS, + .. + } => crate::error::ErrorKind::ServiceRateLimit, ApiError::Console { .. } => crate::error::ErrorKind::ControlPlane, ApiError::Transport(_) => crate::error::ErrorKind::ControlPlane, } @@ -222,7 +238,7 @@ pub mod errors { match self { WakeComputeError::BadComputeAddress(_) => crate::error::ErrorKind::ControlPlane, WakeComputeError::ApiError(e) => e.get_error_kind(), - WakeComputeError::TimeoutError => crate::error::ErrorKind::RateLimit, + WakeComputeError::TimeoutError => crate::error::ErrorKind::ServiceRateLimit, } } } diff --git a/proxy/src/context.rs b/proxy/src/context.rs index 0cea53ae63..e5caa5bd59 100644 --- a/proxy/src/context.rs +++ b/proxy/src/context.rs @@ -147,15 +147,13 @@ impl RequestMonitoring { self.success = true; } - pub fn log(&mut self) { + pub fn log(self) {} +} + +impl Drop for RequestMonitoring { + fn drop(&mut self) { if let Some(tx) = self.sender.take() { let _: Result<(), _> = tx.send(self.clone()); } } } - -impl Drop for RequestMonitoring { - fn drop(&mut self) { - self.log() - } -} diff --git a/proxy/src/error.rs b/proxy/src/error.rs index 69fe1ebc12..4614f3913d 100644 --- a/proxy/src/error.rs +++ b/proxy/src/error.rs @@ -37,9 +37,12 @@ pub enum ErrorKind { /// Network error between user and proxy. Not necessarily user error ClientDisconnect, - /// Proxy self-imposed rate limits + /// Proxy self-imposed user rate limits RateLimit, + /// Proxy self-imposed service-wise rate limits + ServiceRateLimit, + /// internal errors Service, @@ -54,25 +57,12 @@ pub enum ErrorKind { } impl ErrorKind { - pub fn to_str(&self) -> &'static str { - match self { - ErrorKind::User => "request failed due to user error", - ErrorKind::ClientDisconnect => "client disconnected", - ErrorKind::RateLimit => "request cancelled due to rate limit", - ErrorKind::Service => "internal service error", - ErrorKind::ControlPlane => "non-retryable control plane error", - ErrorKind::Postgres => "postgres error", - ErrorKind::Compute => { - "non-retryable compute connection error (or exhausted retry capacity)" - } - } - } - pub fn to_metric_label(&self) -> &'static str { match self { ErrorKind::User => "user", ErrorKind::ClientDisconnect => "clientdisconnect", ErrorKind::RateLimit => "ratelimit", + ErrorKind::ServiceRateLimit => "serviceratelimit", ErrorKind::Service => "service", ErrorKind::ControlPlane => "controlplane", ErrorKind::Postgres => "postgres", @@ -85,12 +75,6 @@ pub trait ReportableError: fmt::Display + Send + 'static { fn get_error_kind(&self) -> ErrorKind; } -impl ReportableError for tokio::time::error::Elapsed { - fn get_error_kind(&self) -> ErrorKind { - ErrorKind::RateLimit - } -} - impl ReportableError for tokio_postgres::error::Error { fn get_error_kind(&self) -> ErrorKind { if self.as_db_error().is_some() { diff --git a/proxy/src/serverless/sql_over_http.rs b/proxy/src/serverless/sql_over_http.rs index e49c1c4db9..63fe87eade 100644 --- a/proxy/src/serverless/sql_over_http.rs +++ b/proxy/src/serverless/sql_over_http.rs @@ -12,7 +12,7 @@ use hyper::StatusCode; use hyper::{Body, HeaderMap, Request}; use serde_json::json; use serde_json::Value; -use tokio::join; +use tokio::try_join; use tokio_postgres::error::DbError; use tokio_postgres::error::ErrorPosition; use tokio_postgres::GenericClient; @@ -32,11 +32,9 @@ use crate::auth::ComputeUserInfoParseError; use crate::config::ProxyConfig; use crate::config::TlsConfig; use crate::context::RequestMonitoring; -use crate::error::ReportableError; use crate::metrics::HTTP_CONTENT_LENGTH; use crate::metrics::NUM_CONNECTION_REQUESTS_GAUGE; use crate::proxy::NeonOptions; -use crate::serverless::backend::HttpConnError; use crate::DbName; use crate::RoleName; @@ -287,8 +285,10 @@ pub async fn handle( )? } }, - Err(e) => { - ctx.set_error_kind(e.get_error_kind()); + Err(_) => { + // TODO: when http error classification is done, distinguish between + // timeout on sql vs timeout in proxy/cplane + // ctx.set_error_kind(crate::error::ErrorKind::RateLimit); let message = format!( "HTTP-Connection timed out, execution time exeeded {} seconds", @@ -402,16 +402,11 @@ async fn handle_inner( // not strictly necessary to mark success here, // but it's just insurance for if we forget it somewhere else ctx.latency_timer.success(); - Ok::<_, HttpConnError>(client) + Ok::<_, anyhow::Error>(client) }; // Run both operations in parallel - let (payload_result, auth_and_connect_result) = - join!(fetch_and_process_request, authenticate_and_connect,); - - // Handle the results - let payload = payload_result?; // Handle errors appropriately - let mut client = auth_and_connect_result?; // Handle errors appropriately + let (payload, mut client) = try_join!(fetch_and_process_request, authenticate_and_connect)?; let mut response = Response::builder() .status(StatusCode::OK) From 428d9fe69e5e70fb2f633117c2137146dfb3d42b Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 21 Feb 2024 14:36:57 +0200 Subject: [PATCH 10/15] tests: Make test_vm_bit_clear_on_heap_lock more robust again. (#6714) When checking that the contents of the VM page in cache and in pageserver match, ignore the LSN on the page. It could be different, if the page was flushed from cache by a checkpoint, for example. Here's one such failure from the CI that this hopefully fixes: https://neon-github-public-dev.s3.amazonaws.com/reports/pr-6687/7847132649/index.html#suites/8545ca7650e609b2963d4035816a356b/5f9018db15ef4408/ In the passing, also remove some log.infos from the loop. I added them while developing the tests, but now they're just noise. --- test_runner/regress/test_vm_bits.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/test_runner/regress/test_vm_bits.py b/test_runner/regress/test_vm_bits.py index 1377bed6f6..eff103ca09 100644 --- a/test_runner/regress/test_vm_bits.py +++ b/test_runner/regress/test_vm_bits.py @@ -167,10 +167,14 @@ def test_vm_bit_clear_on_heap_lock(neon_env_builder: NeonEnvBuilder): # The VM page in shared buffer cache, and the same page as reconstructed # by the pageserver, should be equal. + # + # Ignore the LSN on the page though (first 8 bytes). If the dirty + # VM page is flushed from the cache for some reason, it gets WAL-logged, + # which changes the LSN on the page. cur.execute("select get_raw_page( 'vmtest_lock', 'vm', 0 )") - vm_page_in_cache = (cur.fetchall()[0][0])[:100].hex() + vm_page_in_cache = (cur.fetchall()[0][0])[8:100].hex() cur.execute("select get_raw_page_at_lsn( 'vmtest_lock', 'vm', 0, pg_current_wal_insert_lsn() )") - vm_page_at_pageserver = (cur.fetchall()[0][0])[:100].hex() + vm_page_at_pageserver = (cur.fetchall()[0][0])[8:100].hex() assert vm_page_at_pageserver == vm_page_in_cache @@ -201,16 +205,6 @@ def test_vm_bit_clear_on_heap_lock(neon_env_builder: NeonEnvBuilder): for _ in range(1000): cur.execute("select test_consume_xids(10000);") for _ in range(1000): - cur.execute( - "select get_raw_page_at_lsn( 'vmtest_lock', 'vm', 0, pg_current_wal_insert_lsn() )" - ) - page = (cur.fetchall()[0][0])[:100].hex() - log.info(f"VM page contents: {page}") - - cur.execute("select get_raw_page( 'vmtest_lock', 'vm', 0 )") - page = (cur.fetchall()[0][0])[:100].hex() - log.info(f"VM page contents in cache: {page}") - cur.execute("select min(datfrozenxid::text::int) from pg_database") datfrozenxid = int(cur.fetchall()[0][0]) log.info(f"datfrozenxid {datfrozenxid} locking_xid: {locking_xid}") From 84f027357d425110a50657f13b34a0c602111050 Mon Sep 17 00:00:00 2001 From: John Spray Date: Wed, 21 Feb 2024 14:12:35 +0000 Subject: [PATCH 11/15] pageserver: adjust checkpoint distance for sharded tenants (#6852) ## Problem Where the stripe size is the same order of magnitude as the checkpoint distance (such as with default settings), tenant shards can easily pass through `checkpoint_distance` bytes of LSN without actually ingesting anything. This results in emitting many tiny L0 delta layers. ## Summary of changes - Multiply checkpoint distance by shard count before comparing with LSN distance. This is a heuristic and does not guarantee that we won't emit small layers, but it fixes the issue for typical cases where the writes in a (checkpoint_distance * shard_count) range of LSN bytes are somewhat distributed across shards. - Add a test that checks the size of layers after ingesting to a sharded tenant; this fails before the fix. --------- Co-authored-by: Joonas Koivunen --- pageserver/src/tenant/timeline.rs | 8 ++- test_runner/fixtures/workload.py | 11 ++-- test_runner/regress/test_sharding.py | 83 +++++++++++++++++++++++++++- 3 files changed, 95 insertions(+), 7 deletions(-) diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 0f22284c55..6ee05116f8 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -5192,11 +5192,15 @@ impl<'a> TimelineWriter<'a> { // Rolling the open layer can be triggered by: // 1. The distance from the last LSN we rolled at. This bounds the amount of WAL that - // the safekeepers need to store. + // the safekeepers need to store. For sharded tenants, we multiply by shard count to + // account for how writes are distributed across shards: we expect each node to consume + // 1/count of the LSN on average. // 2. The size of the currently open layer. // 3. The time since the last roll. It helps safekeepers to regard pageserver as caught // up and suspend activity. - if distance >= self.get_checkpoint_distance().into() { + if distance + >= self.get_checkpoint_distance() as i128 * self.shard_identity.count.count() as i128 + { info!( "Will roll layer at {} with layer size {} due to LSN distance ({})", lsn, state.current_size, distance diff --git a/test_runner/fixtures/workload.py b/test_runner/fixtures/workload.py index f29a6cbf3c..1d5394dc1d 100644 --- a/test_runner/fixtures/workload.py +++ b/test_runner/fixtures/workload.py @@ -73,7 +73,7 @@ class Workload: self.env, endpoint, self.tenant_id, self.timeline_id, pageserver_id=pageserver_id ) - def write_rows(self, n, pageserver_id: Optional[int] = None): + def write_rows(self, n, pageserver_id: Optional[int] = None, upload: bool = True): endpoint = self.endpoint(pageserver_id) start = self.expect_rows end = start + n - 1 @@ -87,9 +87,12 @@ class Workload: """ ) - return last_flush_lsn_upload( - self.env, endpoint, self.tenant_id, self.timeline_id, pageserver_id=pageserver_id - ) + if upload: + return last_flush_lsn_upload( + self.env, endpoint, self.tenant_id, self.timeline_id, pageserver_id=pageserver_id + ) + else: + return False def churn_rows(self, n, pageserver_id: Optional[int] = None, upload=True): assert self.expect_rows >= n diff --git a/test_runner/regress/test_sharding.py b/test_runner/regress/test_sharding.py index 99b2ceb8bc..9e491d450c 100644 --- a/test_runner/regress/test_sharding.py +++ b/test_runner/regress/test_sharding.py @@ -4,7 +4,7 @@ from fixtures.neon_fixtures import ( tenant_get_shards, ) from fixtures.remote_storage import s3_storage -from fixtures.types import TenantShardId, TimelineId +from fixtures.types import Lsn, TenantShardId, TimelineId from fixtures.workload import Workload @@ -284,3 +284,84 @@ def test_sharding_split_smoke( ) env.attachment_service.consistency_check() + + +def test_sharding_ingest( + neon_env_builder: NeonEnvBuilder, +): + """ + Check behaviors related to ingest: + - That we generate properly sized layers + - TODO: that updates to remote_consistent_lsn are made correctly via safekeepers + """ + + # Set a small stripe size and checkpoint distance, so that we can exercise rolling logic + # without writing a lot of data. + expect_layer_size = 131072 + TENANT_CONF = { + # small checkpointing and compaction targets to ensure we generate many upload operations + "checkpoint_distance": f"{expect_layer_size}", + "compaction_target_size": f"{expect_layer_size}", + } + shard_count = 4 + neon_env_builder.num_pageservers = shard_count + env = neon_env_builder.init_start( + initial_tenant_conf=TENANT_CONF, + initial_tenant_shard_count=shard_count, + # A stripe size the same order of magnitude as layer size: this ensures that + # within checkpoint_distance some shards will have no data to ingest, if LSN + # contains sequential page writes. This test checks that this kind of + # scenario doesn't result in some shards emitting empty/tiny layers. + initial_tenant_shard_stripe_size=expect_layer_size // 8192, + ) + tenant_id = env.initial_tenant + timeline_id = env.initial_timeline + + workload = Workload(env, tenant_id, timeline_id) + workload.init() + workload.write_rows(512, upload=False) + workload.write_rows(512, upload=False) + workload.write_rows(512, upload=False) + workload.write_rows(512, upload=False) + workload.validate() + + small_layer_count = 0 + ok_layer_count = 0 + huge_layer_count = 0 + + # Inspect the resulting layer map, count how many layers are undersized. + for shard in env.attachment_service.locate(tenant_id): + pageserver = env.get_pageserver(shard["node_id"]) + shard_id = shard["shard_id"] + layer_map = pageserver.http_client().layer_map_info(shard_id, timeline_id) + + for layer in layer_map.historic_layers: + assert layer.layer_file_size is not None + if layer.layer_file_size < expect_layer_size // 2: + classification = "Small" + small_layer_count += 1 + elif layer.layer_file_size > expect_layer_size * 2: + classification = "Huge " + huge_layer_count += 1 + else: + classification = "OK " + ok_layer_count += 1 + + if layer.kind == "Delta": + assert layer.lsn_end is not None + lsn_size = Lsn(layer.lsn_end) - Lsn(layer.lsn_start) + else: + lsn_size = 0 + + log.info( + f"{classification} layer[{pageserver.id}]: {layer.layer_file_name} (size {layer.layer_file_size}, LSN distance {lsn_size})" + ) + + # Why an inexact check? + # - Because we roll layers on checkpoint_distance * shard_count, we expect to obey the target + # layer size on average, but it is still possible to write some tiny layers. + log.info(f"Totals: {small_layer_count} small layers, {ok_layer_count} ok layers") + assert float(small_layer_count) / float(ok_layer_count) < 0.25 + + # Each shard may emit up to one huge layer, because initdb ingest doesn't respect checkpoint_distance. + assert huge_layer_count <= shard_count From 7257ffbf75d8dd75f5e1bd5cf2b3f4a06555cde9 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Wed, 21 Feb 2024 16:57:30 +0200 Subject: [PATCH 12/15] feat: imitiation_only eviction_task policy (#6598) mostly reusing the existing and perhaps controversially sharing the histogram. in practice we don't configure this per-tenant. Cc: #5331 --- libs/pageserver_api/src/models.rs | 2 + pageserver/src/config.rs | 41 +++++- .../src/tenant/timeline/eviction_task.rs | 119 +++++++++++------- 3 files changed, 116 insertions(+), 46 deletions(-) diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 557a4d7de9..af3c8018c4 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -291,6 +291,7 @@ pub struct TenantConfig { pub enum EvictionPolicy { NoEviction, LayerAccessThreshold(EvictionPolicyLayerAccessThreshold), + OnlyImitiate(EvictionPolicyLayerAccessThreshold), } impl EvictionPolicy { @@ -298,6 +299,7 @@ impl EvictionPolicy { match self { EvictionPolicy::NoEviction => "NoEviction", EvictionPolicy::LayerAccessThreshold(_) => "LayerAccessThreshold", + EvictionPolicy::OnlyImitiate(_) => "OnlyImitiate", } } } diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index 6c00c55f39..34d9636673 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -1572,17 +1572,50 @@ threshold = "20m" eviction_order: crate::disk_usage_eviction_task::EvictionOrder::AbsoluteAccessed, }) ); + match &conf.default_tenant_conf.eviction_policy { - EvictionPolicy::NoEviction => panic!("Unexpected eviction opolicy tenant settings"), - EvictionPolicy::LayerAccessThreshold(eviction_thresold) => { - assert_eq!(eviction_thresold.period, Duration::from_secs(20 * 60)); - assert_eq!(eviction_thresold.threshold, Duration::from_secs(20 * 60)); + EvictionPolicy::LayerAccessThreshold(eviction_threshold) => { + assert_eq!(eviction_threshold.period, Duration::from_secs(20 * 60)); + assert_eq!(eviction_threshold.threshold, Duration::from_secs(20 * 60)); } + other => unreachable!("Unexpected eviction policy tenant settings: {other:?}"), } Ok(()) } + #[test] + fn parse_imitation_only_pageserver_config() { + let tempdir = tempdir().unwrap(); + let (workdir, pg_distrib_dir) = prepare_fs(&tempdir).unwrap(); + + let pageserver_conf_toml = format!( + r#"pg_distrib_dir = "{pg_distrib_dir}" +metric_collection_endpoint = "http://sample.url" +metric_collection_interval = "10min" +id = 222 + +[tenant_config] +evictions_low_residence_duration_metric_threshold = "20m" + +[tenant_config.eviction_policy] +kind = "OnlyImitiate" +period = "20m" +threshold = "20m" +"#, + ); + let toml: Document = pageserver_conf_toml.parse().unwrap(); + let conf = PageServerConf::parse_and_validate(&toml, &workdir).unwrap(); + + match &conf.default_tenant_conf.eviction_policy { + EvictionPolicy::OnlyImitiate(t) => { + assert_eq!(t.period, Duration::from_secs(20 * 60)); + assert_eq!(t.threshold, Duration::from_secs(20 * 60)); + } + other => unreachable!("Unexpected eviction policy tenant settings: {other:?}"), + } + } + fn prepare_fs(tempdir: &Utf8TempDir) -> anyhow::Result<(Utf8PathBuf, Utf8PathBuf)> { let tempdir_path = tempdir.path(); diff --git a/pageserver/src/tenant/timeline/eviction_task.rs b/pageserver/src/tenant/timeline/eviction_task.rs index 33ba234a63..127e351c14 100644 --- a/pageserver/src/tenant/timeline/eviction_task.rs +++ b/pageserver/src/tenant/timeline/eviction_task.rs @@ -85,6 +85,7 @@ impl Timeline { let policy = self.get_eviction_policy(); let period = match policy { EvictionPolicy::LayerAccessThreshold(lat) => lat.period, + EvictionPolicy::OnlyImitiate(lat) => lat.period, EvictionPolicy::NoEviction => Duration::from_secs(10), }; if random_init_delay(period, &cancel).await.is_err() { @@ -119,33 +120,45 @@ impl Timeline { ctx: &RequestContext, ) -> ControlFlow<(), Instant> { debug!("eviction iteration: {policy:?}"); - match policy { + let start = Instant::now(); + let (period, threshold) = match policy { EvictionPolicy::NoEviction => { // check again in 10 seconds; XXX config watch mechanism - ControlFlow::Continue(Instant::now() + Duration::from_secs(10)) + return ControlFlow::Continue(Instant::now() + Duration::from_secs(10)); } EvictionPolicy::LayerAccessThreshold(p) => { - let start = Instant::now(); match self.eviction_iteration_threshold(p, cancel, ctx).await { ControlFlow::Break(()) => return ControlFlow::Break(()), ControlFlow::Continue(()) => (), } - let elapsed = start.elapsed(); - crate::tenant::tasks::warn_when_period_overrun( - elapsed, - p.period, - BackgroundLoopKind::Eviction, - ); - crate::metrics::EVICTION_ITERATION_DURATION - .get_metric_with_label_values(&[ - &format!("{}", p.period.as_secs()), - &format!("{}", p.threshold.as_secs()), - ]) - .unwrap() - .observe(elapsed.as_secs_f64()); - ControlFlow::Continue(start + p.period) + (p.period, p.threshold) } - } + EvictionPolicy::OnlyImitiate(p) => { + if self.imitiate_only(p, cancel, ctx).await.is_break() { + return ControlFlow::Break(()); + } + (p.period, p.threshold) + } + }; + + let elapsed = start.elapsed(); + crate::tenant::tasks::warn_when_period_overrun( + elapsed, + period, + BackgroundLoopKind::Eviction, + ); + // FIXME: if we were to mix policies on a pageserver, we would have no way to sense this. I + // don't think that is a relevant fear however, and regardless the imitation should be the + // most costly part. + crate::metrics::EVICTION_ITERATION_DURATION + .get_metric_with_label_values(&[ + &format!("{}", period.as_secs()), + &format!("{}", threshold.as_secs()), + ]) + .unwrap() + .observe(elapsed.as_secs_f64()); + + ControlFlow::Continue(start + period) } async fn eviction_iteration_threshold( @@ -167,30 +180,6 @@ impl Timeline { _ = self.cancel.cancelled() => return ControlFlow::Break(()), }; - // If we evict layers but keep cached values derived from those layers, then - // we face a storm of on-demand downloads after pageserver restart. - // The reason is that the restart empties the caches, and so, the values - // need to be re-computed by accessing layers, which we evicted while the - // caches were filled. - // - // Solutions here would be one of the following: - // 1. Have a persistent cache. - // 2. Count every access to a cached value to the access stats of all layers - // that were accessed to compute the value in the first place. - // 3. Invalidate the caches at a period of < p.threshold/2, so that the values - // get re-computed from layers, thereby counting towards layer access stats. - // 4. Make the eviction task imitate the layer accesses that typically hit caches. - // - // We follow approach (4) here because in Neon prod deployment: - // - page cache is quite small => high churn => low hit rate - // => eviction gets correct access stats - // - value-level caches such as logical size & repatition have a high hit rate, - // especially for inactive tenants - // => eviction sees zero accesses for these - // => they cause the on-demand download storm on pageserver restart - // - // We should probably move to persistent caches in the future, or avoid - // having inactive tenants attached to pageserver in the first place. match self.imitate_layer_accesses(p, cancel, ctx).await { ControlFlow::Break(()) => return ControlFlow::Break(()), ControlFlow::Continue(()) => (), @@ -307,6 +296,52 @@ impl Timeline { ControlFlow::Continue(()) } + /// Like `eviction_iteration_threshold`, but without any eviction. Eviction will be done by + /// disk usage based eviction task. + async fn imitiate_only( + self: &Arc, + p: &EvictionPolicyLayerAccessThreshold, + cancel: &CancellationToken, + ctx: &RequestContext, + ) -> ControlFlow<()> { + let acquire_permit = crate::tenant::tasks::concurrent_background_tasks_rate_limit_permit( + BackgroundLoopKind::Eviction, + ctx, + ); + + let _permit = tokio::select! { + permit = acquire_permit => permit, + _ = cancel.cancelled() => return ControlFlow::Break(()), + _ = self.cancel.cancelled() => return ControlFlow::Break(()), + }; + + self.imitate_layer_accesses(p, cancel, ctx).await + } + + /// If we evict layers but keep cached values derived from those layers, then + /// we face a storm of on-demand downloads after pageserver restart. + /// The reason is that the restart empties the caches, and so, the values + /// need to be re-computed by accessing layers, which we evicted while the + /// caches were filled. + /// + /// Solutions here would be one of the following: + /// 1. Have a persistent cache. + /// 2. Count every access to a cached value to the access stats of all layers + /// that were accessed to compute the value in the first place. + /// 3. Invalidate the caches at a period of < p.threshold/2, so that the values + /// get re-computed from layers, thereby counting towards layer access stats. + /// 4. Make the eviction task imitate the layer accesses that typically hit caches. + /// + /// We follow approach (4) here because in Neon prod deployment: + /// - page cache is quite small => high churn => low hit rate + /// => eviction gets correct access stats + /// - value-level caches such as logical size & repatition have a high hit rate, + /// especially for inactive tenants + /// => eviction sees zero accesses for these + /// => they cause the on-demand download storm on pageserver restart + /// + /// We should probably move to persistent caches in the future, or avoid + /// having inactive tenants attached to pageserver in the first place. #[instrument(skip_all)] async fn imitate_layer_accesses( &self, From 41464325c7b84d90884dcff94d25551fbf03ecde Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Wed, 21 Feb 2024 17:20:59 +0200 Subject: [PATCH 13/15] fix: remaining missed cancellations and timeouts (#6843) As noticed in #6836 some occurances of error conversions were missed in #6697: - `std::io::Error` popped up by `tokio::io::copy_buf` containing `DownloadError` was turned into `DownloadError::Other` - similarly for secondary downloader errors These changes come at the loss of pathname context. Cc: #6096 --- libs/remote_storage/src/error.rs | 29 ++++++++++++++--- libs/remote_storage/src/support.rs | 8 ++++- .../tenant/remote_timeline_client/download.rs | 25 +++------------ pageserver/src/tenant/secondary/downloader.rs | 31 ++++++++++--------- 4 files changed, 52 insertions(+), 41 deletions(-) diff --git a/libs/remote_storage/src/error.rs b/libs/remote_storage/src/error.rs index 96f044e087..66422853e1 100644 --- a/libs/remote_storage/src/error.rs +++ b/libs/remote_storage/src/error.rs @@ -44,6 +44,26 @@ impl DownloadError { } } +impl From for DownloadError { + fn from(value: std::io::Error) -> Self { + let needs_unwrap = value.kind() == std::io::ErrorKind::Other + && value + .get_ref() + .and_then(|x| x.downcast_ref::()) + .is_some(); + + if needs_unwrap { + *value + .into_inner() + .expect("just checked") + .downcast::() + .expect("just checked") + } else { + DownloadError::Other(value.into()) + } + } +} + #[derive(Debug)] pub enum TimeTravelError { /// Validation or other error happened due to user input. @@ -142,13 +162,12 @@ impl std::fmt::Display for TimeoutOrCancel { impl std::error::Error for TimeoutOrCancel {} impl TimeoutOrCancel { - pub fn caused(error: &anyhow::Error) -> Option<&Self> { - error.root_cause().downcast_ref() - } - /// Returns true if the error was caused by [`TimeoutOrCancel::Cancel`]. pub fn caused_by_cancel(error: &anyhow::Error) -> bool { - Self::caused(error).is_some_and(Self::is_cancel) + error + .root_cause() + .downcast_ref::() + .is_some_and(Self::is_cancel) } pub fn is_cancel(&self) -> bool { diff --git a/libs/remote_storage/src/support.rs b/libs/remote_storage/src/support.rs index 20f193c6c8..d146b5445b 100644 --- a/libs/remote_storage/src/support.rs +++ b/libs/remote_storage/src/support.rs @@ -73,6 +73,8 @@ where if !*this.hit { if let Poll::Ready(e) = this.cancellation.poll(cx) { *this.hit = true; + + // most likely this will be a std::io::Error wrapping a DownloadError let e = Err(std::io::Error::from(e)); return Poll::Ready(Some(e)); } @@ -130,6 +132,8 @@ mod tests { .is_some_and(|e| matches!(e, DownloadError::Cancelled)), "{inner:?}" ); + let e = DownloadError::from(e); + assert!(matches!(e, DownloadError::Cancelled), "{e:?}"); tokio::select! { _ = stream.next() => unreachable!("no timeout ever happens as we were already cancelled"), @@ -146,7 +150,7 @@ mod tests { let stream = DownloadStream::new(cancel_or_timeout(timeout, cancel.clone()), inner); let mut stream = std::pin::pin!(stream); - // because the stream uses 120s timeout we are paused, we advance to 120s right away. + // because the stream uses 120s timeout and we are paused, we advance to 120s right away. let first = stream.next(); let e = first.await.expect("there must be some").unwrap_err(); @@ -158,6 +162,8 @@ mod tests { .is_some_and(|e| matches!(e, DownloadError::Timeout)), "{inner:?}" ); + let e = DownloadError::from(e); + assert!(matches!(e, DownloadError::Timeout), "{e:?}"); cancel.cancel(); diff --git a/pageserver/src/tenant/remote_timeline_client/download.rs b/pageserver/src/tenant/remote_timeline_client/download.rs index c70267474e..962cf5d12e 100644 --- a/pageserver/src/tenant/remote_timeline_client/download.rs +++ b/pageserver/src/tenant/remote_timeline_client/download.rs @@ -88,14 +88,7 @@ pub async fn download_layer_file<'a>( let mut reader = tokio_util::io::StreamReader::new(download.download_stream); - let bytes_amount = tokio::io::copy_buf(&mut reader, &mut destination_file) - .await - .with_context(|| { - format!( - "download layer at remote path '{remote_path:?}' into file {temp_file_path:?}" - ) - }) - .map_err(DownloadError::Other); + let bytes_amount = tokio::io::copy_buf(&mut reader, &mut destination_file).await; match bytes_amount { Ok(bytes_amount) => { @@ -107,7 +100,7 @@ pub async fn download_layer_file<'a>( on_fatal_io_error(&e, &format!("Removing temporary file {temp_file_path}")); } - Err(e) + Err(e.into()) } } }, @@ -245,10 +238,7 @@ async fn do_download_index_part( let stream = download.download_stream; let mut stream = StreamReader::new(stream); - tokio::io::copy_buf(&mut stream, &mut bytes) - .await - .with_context(|| format!("download index part at {remote_path:?}")) - .map_err(DownloadError::Other)?; + tokio::io::copy_buf(&mut stream, &mut bytes).await?; Ok(bytes) }, @@ -428,14 +418,7 @@ pub(crate) async fn download_initdb_tar_zst( let mut download = tokio_util::io::StreamReader::new(download.download_stream); let mut writer = tokio::io::BufWriter::with_capacity(super::BUFFER_SIZE, file); - // TODO: this consumption of the response body should be subject to timeout + cancellation, but - // not without thinking carefully about how to recover safely from cancelling a write to - // local storage (e.g. by writing into a temp file as we do in download_layer) - // FIXME: flip the weird error wrapping - tokio::io::copy_buf(&mut download, &mut writer) - .await - .with_context(|| format!("download initdb.tar.zst at {remote_path:?}")) - .map_err(DownloadError::Other)?; + tokio::io::copy_buf(&mut download, &mut writer).await?; let mut file = writer.into_inner(); diff --git a/pageserver/src/tenant/secondary/downloader.rs b/pageserver/src/tenant/secondary/downloader.rs index 6966cf7709..51ab421b58 100644 --- a/pageserver/src/tenant/secondary/downloader.rs +++ b/pageserver/src/tenant/secondary/downloader.rs @@ -438,8 +438,14 @@ impl From for UpdateError { fn from(value: std::io::Error) -> Self { if let Some(nix::errno::Errno::ENOSPC) = value.raw_os_error().map(nix::errno::from_i32) { UpdateError::NoSpace + } else if value + .get_ref() + .and_then(|x| x.downcast_ref::()) + .is_some() + { + UpdateError::from(DownloadError::from(value)) } else { - // An I/O error from e.g. tokio::io::copy is most likely a remote storage issue + // An I/O error from e.g. tokio::io::copy_buf is most likely a remote storage issue UpdateError::Other(anyhow::anyhow!(value)) } } @@ -672,20 +678,17 @@ impl<'a> TenantDownloader<'a> { .await { Ok(bytes) => bytes, - Err(e) => { - if let DownloadError::NotFound = e { - // A heatmap might be out of date and refer to a layer that doesn't exist any more. - // This is harmless: continue to download the next layer. It is expected during compaction - // GC. - tracing::debug!( - "Skipped downloading missing layer {}, raced with compaction/gc?", - layer.name - ); - continue; - } else { - return Err(e.into()); - } + Err(DownloadError::NotFound) => { + // A heatmap might be out of date and refer to a layer that doesn't exist any more. + // This is harmless: continue to download the next layer. It is expected during compaction + // GC. + tracing::debug!( + "Skipped downloading missing layer {}, raced with compaction/gc?", + layer.name + ); + continue; } + Err(e) => return Err(e.into()), }; if downloaded_bytes != layer.metadata.file_size { From 4de2f0f3e021cd7e84c5f8ef5251da13f0127c3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arpad=20M=C3=BCller?= Date: Wed, 21 Feb 2024 16:35:37 +0100 Subject: [PATCH 14/15] Implement a sharded time travel recovery endpoint (#6821) The sharding service didn't have support for S3 disaster recovery. This PR adds a new endpoint to the attachment service, which is slightly different from the endpoint on the pageserver, in that it takes the shard count history of the tenant as json parameters: we need to do time travel recovery for both the shard count at the target time and the shard count at the current moment in time, as well as the past shard counts that either still reference. Fixes #6604, part of https://github.com/neondatabase/cloud/issues/8233 --------- Co-authored-by: John Spray --- Cargo.lock | 1 + control_plane/attachment_service/Cargo.toml | 1 + control_plane/attachment_service/src/http.rs | 40 +++++- .../attachment_service/src/scheduler.rs | 5 +- .../attachment_service/src/service.rs | 92 +++++++++++- .../attachment_service/src/tenant_state.rs | 7 + control_plane/src/bin/neon_local.rs | 2 +- libs/pageserver_api/src/models.rs | 8 +- pageserver/client/src/mgmt_api.rs | 14 ++ test_runner/fixtures/neon_fixtures.py | 4 +- test_runner/fixtures/pageserver/http.py | 10 +- test_runner/fixtures/pageserver/utils.py | 4 +- test_runner/regress/test_sharding_service.py | 133 +++++++++++++++++- 13 files changed, 304 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac8cceb5f6..51c433cd07 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -284,6 +284,7 @@ dependencies = [ "diesel_migrations", "futures", "git-version", + "humantime", "hyper", "metrics", "once_cell", diff --git a/control_plane/attachment_service/Cargo.toml b/control_plane/attachment_service/Cargo.toml index 9e1c6377ee..bfdfd4c77d 100644 --- a/control_plane/attachment_service/Cargo.toml +++ b/control_plane/attachment_service/Cargo.toml @@ -18,6 +18,7 @@ clap.workspace = true futures.workspace = true git-version.workspace = true hyper.workspace = true +humantime.workspace = true once_cell.workspace = true pageserver_api.workspace = true pageserver_client.workspace = true diff --git a/control_plane/attachment_service/src/http.rs b/control_plane/attachment_service/src/http.rs index 67ab37dfc1..d85753bedc 100644 --- a/control_plane/attachment_service/src/http.rs +++ b/control_plane/attachment_service/src/http.rs @@ -4,7 +4,7 @@ use hyper::{Body, Request, Response}; use hyper::{StatusCode, Uri}; use pageserver_api::models::{ TenantCreateRequest, TenantLocationConfigRequest, TenantShardSplitRequest, - TimelineCreateRequest, + TenantTimeTravelRequest, TimelineCreateRequest, }; use pageserver_api::shard::TenantShardId; use pageserver_client::mgmt_api; @@ -12,7 +12,7 @@ use std::sync::Arc; use std::time::{Duration, Instant}; use utils::auth::SwappableJwtAuth; use utils::http::endpoint::{auth_middleware, request_span}; -use utils::http::request::parse_request_param; +use utils::http::request::{must_get_query_param, parse_request_param}; use utils::id::{TenantId, TimelineId}; use utils::{ @@ -180,6 +180,39 @@ async fn handle_tenant_location_config( ) } +async fn handle_tenant_time_travel_remote_storage( + service: Arc, + mut req: Request, +) -> Result, ApiError> { + let tenant_id: TenantId = parse_request_param(&req, "tenant_id")?; + let time_travel_req = json_request::(&mut req).await?; + + let timestamp_raw = must_get_query_param(&req, "travel_to")?; + let _timestamp = humantime::parse_rfc3339(×tamp_raw).map_err(|_e| { + ApiError::BadRequest(anyhow::anyhow!( + "Invalid time for travel_to: {timestamp_raw:?}" + )) + })?; + + let done_if_after_raw = must_get_query_param(&req, "done_if_after")?; + let _done_if_after = humantime::parse_rfc3339(&done_if_after_raw).map_err(|_e| { + ApiError::BadRequest(anyhow::anyhow!( + "Invalid time for done_if_after: {done_if_after_raw:?}" + )) + })?; + + service + .tenant_time_travel_remote_storage( + &time_travel_req, + tenant_id, + timestamp_raw, + done_if_after_raw, + ) + .await?; + + json_response(StatusCode::OK, ()) +} + async fn handle_tenant_delete( service: Arc, req: Request, @@ -477,6 +510,9 @@ pub fn make_router( .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) + }) // Timeline operations .delete("/v1/tenant/:tenant_id/timeline/:timeline_id", |r| { tenant_service_handler(r, handle_tenant_timeline_delete) diff --git a/control_plane/attachment_service/src/scheduler.rs b/control_plane/attachment_service/src/scheduler.rs index 39d8d0a260..fb3c7f634c 100644 --- a/control_plane/attachment_service/src/scheduler.rs +++ b/control_plane/attachment_service/src/scheduler.rs @@ -175,10 +175,7 @@ impl Scheduler { } } - pub(crate) fn schedule_shard( - &mut self, - hard_exclude: &[NodeId], - ) -> Result { + pub(crate) fn schedule_shard(&self, hard_exclude: &[NodeId]) -> Result { if self.nodes.is_empty() { return Err(ScheduleError::NoPageservers); } diff --git a/control_plane/attachment_service/src/service.rs b/control_plane/attachment_service/src/service.rs index 0236496c61..74e1296709 100644 --- a/control_plane/attachment_service/src/service.rs +++ b/control_plane/attachment_service/src/service.rs @@ -1,4 +1,5 @@ use std::{ + borrow::Cow, cmp::Ordering, collections::{BTreeMap, HashMap, HashSet}, str::FromStr, @@ -25,7 +26,7 @@ use pageserver_api::{ self, LocationConfig, LocationConfigListResponse, LocationConfigMode, ShardParameters, TenantConfig, TenantCreateRequest, TenantLocationConfigRequest, TenantLocationConfigResponse, TenantShardLocation, TenantShardSplitRequest, - TenantShardSplitResponse, TimelineCreateRequest, TimelineInfo, + TenantShardSplitResponse, TenantTimeTravelRequest, TimelineCreateRequest, TimelineInfo, }, shard::{ShardCount, ShardIdentity, ShardNumber, ShardStripeSize, TenantShardId}, }; @@ -1329,6 +1330,95 @@ impl Service { Ok(result) } + pub(crate) async fn tenant_time_travel_remote_storage( + &self, + time_travel_req: &TenantTimeTravelRequest, + tenant_id: TenantId, + timestamp: Cow<'_, str>, + done_if_after: Cow<'_, str>, + ) -> Result<(), ApiError> { + let node = { + let locked = self.inner.read().unwrap(); + // Just a sanity check to prevent misuse: the API expects that the tenant is fully + // detached everywhere, and nothing writes to S3 storage. Here, we verify that, + // but only at the start of the process, so it's really just to prevent operator + // mistakes. + for (shard_id, shard) in locked.tenants.range(TenantShardId::tenant_range(tenant_id)) { + if shard.intent.get_attached().is_some() || !shard.intent.get_secondary().is_empty() + { + return Err(ApiError::InternalServerError(anyhow::anyhow!( + "We want tenant to be attached in shard with tenant_shard_id={shard_id}" + ))); + } + let maybe_attached = shard + .observed + .locations + .iter() + .filter_map(|(node_id, observed_location)| { + observed_location + .conf + .as_ref() + .map(|loc| (node_id, observed_location, loc.mode)) + }) + .find(|(_, _, mode)| *mode != LocationConfigMode::Detached); + if let Some((node_id, _observed_location, mode)) = maybe_attached { + return Err(ApiError::InternalServerError(anyhow::anyhow!("We observed attached={mode:?} tenant in node_id={node_id} shard with tenant_shard_id={shard_id}"))); + } + } + let scheduler = &locked.scheduler; + // Right now we only perform the operation on a single node without parallelization + // TODO fan out the operation to multiple nodes for better performance + let node_id = scheduler.schedule_shard(&[])?; + let node = locked + .nodes + .get(&node_id) + .expect("Pageservers may not be deleted while lock is active"); + node.clone() + }; + + // The shard count is encoded in the remote storage's URL, so we need to handle all historically used shard counts + let mut counts = time_travel_req + .shard_counts + .iter() + .copied() + .collect::>() + .into_iter() + .collect::>(); + counts.sort_unstable(); + + for count in counts { + let shard_ids = (0..count.count()) + .map(|i| TenantShardId { + tenant_id, + shard_number: ShardNumber(i), + shard_count: count, + }) + .collect::>(); + for tenant_shard_id in shard_ids { + let client = + mgmt_api::Client::new(node.base_url(), self.config.jwt_token.as_deref()); + + tracing::info!("Doing time travel recovery for shard {tenant_shard_id}",); + + client + .tenant_time_travel_remote_storage( + tenant_shard_id, + ×tamp, + &done_if_after, + ) + .await + .map_err(|e| { + ApiError::InternalServerError(anyhow::anyhow!( + "Error doing time travel recovery for shard {tenant_shard_id} on node {}: {e}", + node.id + )) + })?; + } + } + + Ok(()) + } + pub(crate) async fn tenant_delete(&self, tenant_id: TenantId) -> Result { self.ensure_attached_wait(tenant_id).await?; diff --git a/control_plane/attachment_service/src/tenant_state.rs b/control_plane/attachment_service/src/tenant_state.rs index 4ec6fdca67..7970207e27 100644 --- a/control_plane/attachment_service/src/tenant_state.rs +++ b/control_plane/attachment_service/src/tenant_state.rs @@ -495,6 +495,13 @@ 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. + return true; + } + } + // 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 { diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs index 5c0d008943..f824003d01 100644 --- a/control_plane/src/bin/neon_local.rs +++ b/control_plane/src/bin/neon_local.rs @@ -616,7 +616,7 @@ async fn handle_timeline(timeline_match: &ArgMatches, env: &mut local_env::Local let tenant_id = get_tenant_id(create_match, env)?; let new_branch_name = create_match .get_one::("branch-name") - .ok_or_else(|| anyhow!("No branch name provided"))?; + .ok_or_else(|| anyhow!("No branch name provided"))?; // TODO let pg_version = create_match .get_one::("pg-version") diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index af3c8018c4..b68ab9fd59 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -344,7 +344,7 @@ impl ThrottleConfig { /// A flattened analog of a `pagesever::tenant::LocationMode`, which /// lists out all possible states (and the virtual "Detached" state) /// in a flat form rather than using rust-style enums. -#[derive(Serialize, Deserialize, Debug, Clone, Eq, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, Copy, Eq, PartialEq)] pub enum LocationConfigMode { AttachedSingle, AttachedMulti, @@ -408,6 +408,12 @@ pub struct TenantLocationConfigRequest { pub config: LocationConfig, // as we have a flattened field, we should reject all unknown fields in it } +#[derive(Serialize, Deserialize, Debug)] +#[serde(deny_unknown_fields)] +pub struct TenantTimeTravelRequest { + pub shard_counts: Vec, +} + #[derive(Serialize, Deserialize, Debug)] #[serde(deny_unknown_fields)] pub struct TenantShardLocation { diff --git a/pageserver/client/src/mgmt_api.rs b/pageserver/client/src/mgmt_api.rs index baea747d3c..969d0d99c0 100644 --- a/pageserver/client/src/mgmt_api.rs +++ b/pageserver/client/src/mgmt_api.rs @@ -217,6 +217,20 @@ impl Client { } } + pub async fn tenant_time_travel_remote_storage( + &self, + tenant_shard_id: TenantShardId, + timestamp: &str, + done_if_after: &str, + ) -> Result<()> { + let uri = format!( + "{}/v1/tenant/{tenant_shard_id}/time_travel_remote_storage?travel_to={timestamp}&done_if_after={done_if_after}", + self.mgmt_api_endpoint + ); + self.request(Method::PUT, &uri, ()).await?; + Ok(()) + } + pub async fn tenant_config(&self, req: &TenantConfigRequest) -> Result<()> { let uri = format!("{}/v1/tenant/config", self.mgmt_api_endpoint); self.request(Method::PUT, &uri, req).await?; diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index ce5ef66d22..79a4c7cde8 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -512,7 +512,7 @@ class NeonEnvBuilder: def init_start( self, - initial_tenant_conf: Optional[Dict[str, str]] = None, + initial_tenant_conf: Optional[Dict[str, Any]] = None, default_remote_storage_if_missing: bool = True, initial_tenant_shard_count: Optional[int] = None, initial_tenant_shard_stripe_size: Optional[int] = None, @@ -1497,7 +1497,7 @@ class NeonCli(AbstractNeonCli): self, tenant_id: Optional[TenantId] = None, timeline_id: Optional[TimelineId] = None, - conf: Optional[Dict[str, str]] = None, + conf: Optional[Dict[str, Any]] = None, shard_count: Optional[int] = None, shard_stripe_size: Optional[int] = None, set_default: bool = False, diff --git a/test_runner/fixtures/pageserver/http.py b/test_runner/fixtures/pageserver/http.py index d4583308ff..98eb89d30c 100644 --- a/test_runner/fixtures/pageserver/http.py +++ b/test_runner/fixtures/pageserver/http.py @@ -395,12 +395,20 @@ class PageserverHttpClient(requests.Session, MetricsGetter): tenant_id: Union[TenantId, TenantShardId], timestamp: datetime, done_if_after: datetime, + shard_counts: Optional[List[int]] = None, ): """ Issues a request to perform time travel operations on the remote storage """ + + if shard_counts is None: + shard_counts = [] + body: Dict[str, Any] = { + "shard_counts": shard_counts, + } res = self.put( - f"http://localhost:{self.port}/v1/tenant/{tenant_id}/time_travel_remote_storage?travel_to={timestamp.isoformat()}Z&done_if_after={done_if_after.isoformat()}Z" + f"http://localhost:{self.port}/v1/tenant/{tenant_id}/time_travel_remote_storage?travel_to={timestamp.isoformat()}Z&done_if_after={done_if_after.isoformat()}Z", + json=body, ) self.verbose_error(res) diff --git a/test_runner/fixtures/pageserver/utils.py b/test_runner/fixtures/pageserver/utils.py index 1812eb438d..225cfcd143 100644 --- a/test_runner/fixtures/pageserver/utils.py +++ b/test_runner/fixtures/pageserver/utils.py @@ -482,8 +482,8 @@ def tenant_delete_wait_completed( MANY_SMALL_LAYERS_TENANT_CONFIG = { "gc_period": "0s", "compaction_period": "0s", - "checkpoint_distance": f"{1024**2}", - "image_creation_threshold": "100", + "checkpoint_distance": 1024**2, + "image_creation_threshold": 100, } diff --git a/test_runner/regress/test_sharding_service.py b/test_runner/regress/test_sharding_service.py index d2334c7776..6525f9733f 100644 --- a/test_runner/regress/test_sharding_service.py +++ b/test_runner/regress/test_sharding_service.py @@ -1,13 +1,30 @@ import time from collections import defaultdict +from datetime import datetime, timezone +from typing import List from fixtures.log_helper import log -from fixtures.neon_fixtures import NeonEnv, NeonEnvBuilder +from fixtures.neon_fixtures import ( + NeonEnv, + NeonEnvBuilder, + PgBin, +) from fixtures.pageserver.http import PageserverHttpClient -from fixtures.pageserver.utils import tenant_delete_wait_completed, timeline_delete_wait_completed +from fixtures.pageserver.utils import ( + MANY_SMALL_LAYERS_TENANT_CONFIG, + enable_remote_storage_versioning, + list_prefix, + remote_storage_delete_key, + tenant_delete_wait_completed, + timeline_delete_wait_completed, +) from fixtures.pg_version import PgVersion +from fixtures.remote_storage import RemoteStorageKind, s3_storage from fixtures.types import TenantId, TimelineId -from fixtures.utils import wait_until +from fixtures.utils import run_pg_bench_small, wait_until +from mypy_boto3_s3.type_defs import ( + ObjectTypeDef, +) from pytest_httpserver import HTTPServer from werkzeug.wrappers.request import Request from werkzeug.wrappers.response import Response @@ -457,3 +474,113 @@ def test_sharding_service_debug_apis(neon_env_builder: NeonEnvBuilder): # Check that the 'drop' APIs didn't leave things in a state that would fail a consistency check: they're # meant to be unclean wrt the pageserver state, but not leave a broken storage controller behind. env.attachment_service.consistency_check() + + +def test_sharding_service_s3_time_travel_recovery( + neon_env_builder: NeonEnvBuilder, + pg_bin: PgBin, +): + """ + Test for S3 time travel + """ + + remote_storage_kind = s3_storage() + neon_env_builder.enable_pageserver_remote_storage(remote_storage_kind) + + # Mock S3 doesn't have versioning enabled by default, enable it + # (also do it before there is any writes to the bucket) + if remote_storage_kind == RemoteStorageKind.MOCK_S3: + remote_storage = neon_env_builder.pageserver_remote_storage + assert remote_storage, "remote storage not configured" + enable_remote_storage_versioning(remote_storage) + + neon_env_builder.num_pageservers = 1 + + env = neon_env_builder.init_start() + virtual_ps_http = PageserverHttpClient(env.attachment_service_port, lambda: True) + + tenant_id = TenantId.generate() + env.attachment_service.tenant_create( + tenant_id, + shard_count=2, + shard_stripe_size=8192, + tenant_config=MANY_SMALL_LAYERS_TENANT_CONFIG, + ) + + # Check that the consistency check passes + env.attachment_service.consistency_check() + + branch_name = "main" + timeline_id = env.neon_cli.create_timeline( + branch_name, + tenant_id=tenant_id, + ) + # Write some nontrivial amount of data into the endpoint and wait until it is uploaded + with env.endpoints.create_start("main", tenant_id=tenant_id) as endpoint: + run_pg_bench_small(pg_bin, endpoint.connstr()) + endpoint.safe_psql("CREATE TABLE created_foo(id integer);") + # last_flush_lsn_upload(env, endpoint, tenant_id, timeline_id) + + # Give the data time to be uploaded + time.sleep(4) + + # Detach the tenant + virtual_ps_http.tenant_location_conf( + tenant_id, + { + "mode": "Detached", + "secondary_conf": None, + "tenant_conf": {}, + "generation": None, + }, + ) + + time.sleep(4) + ts_before_disaster = datetime.now(tz=timezone.utc).replace(tzinfo=None) + time.sleep(4) + + # Simulate a "disaster": delete some random files from remote storage for one of the shards + assert env.pageserver_remote_storage + shard_id_for_list = "0002" + objects: List[ObjectTypeDef] = list_prefix( + env.pageserver_remote_storage, + f"tenants/{tenant_id}-{shard_id_for_list}/timelines/{timeline_id}/", + ).get("Contents", []) + assert len(objects) > 1 + log.info(f"Found {len(objects)} objects in remote storage") + should_delete = False + for obj in objects: + obj_key = obj["Key"] + should_delete = not should_delete + if not should_delete: + log.info(f"Keeping key on remote storage: {obj_key}") + continue + log.info(f"Deleting key from remote storage: {obj_key}") + remote_storage_delete_key(env.pageserver_remote_storage, obj_key) + pass + + time.sleep(4) + ts_after_disaster = datetime.now(tz=timezone.utc).replace(tzinfo=None) + time.sleep(4) + + # Do time travel recovery + virtual_ps_http.tenant_time_travel_remote_storage( + tenant_id, ts_before_disaster, ts_after_disaster, shard_counts=[2] + ) + time.sleep(4) + + # Attach the tenant again + virtual_ps_http.tenant_location_conf( + tenant_id, + { + "mode": "AttachedSingle", + "secondary_conf": None, + "tenant_conf": {}, + "generation": 100, + }, + ) + + with env.endpoints.create_start("main", tenant_id=tenant_id) as endpoint: + endpoint.safe_psql("SELECT * FROM created_foo;") + + env.attachment_service.consistency_check() From 532b0fa52b950730d9cc9f7a0089b31f4fc1fa42 Mon Sep 17 00:00:00 2001 From: John Spray Date: Wed, 21 Feb 2024 15:45:22 +0000 Subject: [PATCH 15/15] Revise CODEOWNERS (#6840) ## Problem - Current file has ambiguous ownership for some paths - The /control_plane/attachment_service is storage specific & updates there don't need to request reviews from other teams. ## Summary of changes - Define a single owning team per path, so that we can make reviews by that team mandatory in future. - Remove the top-level /control_plane as no one specific team owns neon_local, and we would rarely see a PR that exclusively touches that path. - Add an entry for /control_plane/attachment_service, which is newer storage-specific code. --- CODEOWNERS | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CODEOWNERS b/CODEOWNERS index e384dc39f1..5b601f0566 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -1,10 +1,10 @@ /compute_tools/ @neondatabase/control-plane @neondatabase/compute -/control_plane/ @neondatabase/compute @neondatabase/storage -/libs/pageserver_api/ @neondatabase/compute @neondatabase/storage +/control_plane/attachment_service @neondatabase/storage +/libs/pageserver_api/ @neondatabase/storage /libs/postgres_ffi/ @neondatabase/compute /libs/remote_storage/ @neondatabase/storage /libs/safekeeper_api/ @neondatabase/safekeepers -/libs/vm_monitor/ @neondatabase/autoscaling @neondatabase/compute +/libs/vm_monitor/ @neondatabase/autoscaling /pageserver/ @neondatabase/storage /pgxn/ @neondatabase/compute /proxy/ @neondatabase/proxy