storcon: skip draining shard if it's secondary is lagging too much (#8644)

## Problem
Migrations of tenant shards with cold secondaries are holding up drains
in during production deployments.

## Summary of changes
If a secondary locations is lagging by more than 256MiB (configurable,
but that's the default), then skip cutting it over to the secondary as part of the node drain.
This commit is contained in:
Vlad Lazar
2024-08-09 15:45:07 +01:00
committed by GitHub
parent e6770d79fd
commit f5cef7bf7f
13 changed files with 666 additions and 110 deletions

View File

@@ -0,0 +1,225 @@
use std::{
collections::{BTreeMap, HashMap},
sync::Arc,
};
use pageserver_api::controller_api::NodeSchedulingPolicy;
use utils::{id::NodeId, shard::TenantShardId};
use crate::{
background_node_operations::OperationError, node::Node, scheduler::Scheduler,
tenant_shard::TenantShard,
};
pub(crate) struct TenantShardIterator<F> {
tenants_accessor: F,
inspected_all_shards: bool,
last_inspected_shard: Option<TenantShardId>,
}
/// A simple iterator which can be used in tandem with [`crate::service::Service`]
/// to iterate over all known tenant shard ids without holding the lock on the
/// service state at all times.
impl<F> TenantShardIterator<F>
where
F: Fn(Option<TenantShardId>) -> Option<TenantShardId>,
{
pub(crate) fn new(tenants_accessor: F) -> Self {
Self {
tenants_accessor,
inspected_all_shards: false,
last_inspected_shard: None,
}
}
/// Returns the next tenant shard id if one exists
pub(crate) fn next(&mut self) -> Option<TenantShardId> {
if self.inspected_all_shards {
return None;
}
match (self.tenants_accessor)(self.last_inspected_shard) {
Some(tid) => {
self.last_inspected_shard = Some(tid);
Some(tid)
}
None => {
self.inspected_all_shards = true;
None
}
}
}
/// Returns true when the end of the iterator is reached and false otherwise
pub(crate) fn finished(&self) -> bool {
self.inspected_all_shards
}
}
/// Check that the state of the node being drained is as expected:
/// node is present in memory and scheduling policy is set to [`NodeSchedulingPolicy::Draining`]
pub(crate) fn validate_node_state(
node_id: &NodeId,
nodes: Arc<HashMap<NodeId, Node>>,
) -> Result<(), OperationError> {
let node = nodes.get(node_id).ok_or(OperationError::NodeStateChanged(
format!("node {} was removed", node_id).into(),
))?;
let current_policy = node.get_scheduling();
if !matches!(current_policy, NodeSchedulingPolicy::Draining) {
// TODO(vlad): maybe cancel pending reconciles before erroring out. need to think
// about it
return Err(OperationError::NodeStateChanged(
format!("node {} changed state to {:?}", node_id, current_policy).into(),
));
}
Ok(())
}
/// Struct that houses a few utility methods for draining pageserver nodes
pub(crate) struct TenantShardDrain {
pub(crate) drained_node: NodeId,
pub(crate) tenant_shard_id: TenantShardId,
}
impl TenantShardDrain {
/// Check if the tenant shard under question is eligible for drainining:
/// it's primary attachment is on the node being drained
pub(crate) fn tenant_shard_eligible_for_drain(
&self,
tenants: &BTreeMap<TenantShardId, TenantShard>,
scheduler: &Scheduler,
) -> Option<NodeId> {
let tenant_shard = tenants.get(&self.tenant_shard_id)?;
if *tenant_shard.intent.get_attached() != Some(self.drained_node) {
return None;
}
match scheduler.node_preferred(tenant_shard.intent.get_secondary()) {
Some(node) => Some(node),
None => {
tracing::warn!(
tenant_id=%self.tenant_shard_id.tenant_id, shard_id=%self.tenant_shard_id.shard_slug(),
"No eligible secondary while draining {}", self.drained_node
);
None
}
}
}
/// Attempt to reschedule the tenant shard under question to one of its secondary locations
/// Returns an Err when the operation should be aborted and Ok(None) when the tenant shard
/// should be skipped.
pub(crate) fn reschedule_to_secondary<'a>(
&self,
destination: NodeId,
tenants: &'a mut BTreeMap<TenantShardId, TenantShard>,
scheduler: &mut Scheduler,
nodes: &Arc<HashMap<NodeId, Node>>,
) -> Result<Option<&'a mut TenantShard>, OperationError> {
let tenant_shard = match tenants.get_mut(&self.tenant_shard_id) {
Some(some) => some,
None => {
// Tenant shard was removed in the meantime.
// Skip to the next one, but don't fail the overall operation
return Ok(None);
}
};
if !nodes.contains_key(&destination) {
return Err(OperationError::NodeStateChanged(
format!("node {} was removed", destination).into(),
));
}
if !tenant_shard.intent.get_secondary().contains(&destination) {
tracing::info!(
tenant_id=%self.tenant_shard_id.tenant_id, shard_id=%self.tenant_shard_id.shard_slug(),
"Secondary moved away from {destination} during drain"
);
return Ok(None);
}
match tenant_shard.reschedule_to_secondary(Some(destination), scheduler) {
Err(e) => {
tracing::warn!(
tenant_id=%self.tenant_shard_id.tenant_id, shard_id=%self.tenant_shard_id.shard_slug(),
"Scheduling error when draining pageserver {} : {}", self.drained_node, e
);
Ok(None)
}
Ok(()) => {
let scheduled_to = tenant_shard.intent.get_attached();
tracing::info!(
tenant_id=%self.tenant_shard_id.tenant_id, shard_id=%self.tenant_shard_id.shard_slug(),
"Rescheduled shard while draining node {}: {} -> {:?}",
self.drained_node,
self.drained_node,
scheduled_to
);
Ok(Some(tenant_shard))
}
}
}
}
#[cfg(test)]
mod tests {
use std::sync::Arc;
use utils::{
id::TenantId,
shard::{ShardCount, ShardNumber, TenantShardId},
};
use super::TenantShardIterator;
#[test]
fn test_tenant_shard_iterator() {
let tenant_id = TenantId::generate();
let shard_count = ShardCount(8);
let mut tenant_shards = Vec::default();
for i in 0..shard_count.0 {
tenant_shards.push((
TenantShardId {
tenant_id,
shard_number: ShardNumber(i),
shard_count,
},
(),
))
}
let tenant_shards = Arc::new(tenant_shards);
let mut tid_iter = TenantShardIterator::new({
let tenants = tenant_shards.clone();
move |last_inspected_shard: Option<TenantShardId>| {
let entry = match last_inspected_shard {
Some(skip_past) => {
let mut cursor = tenants.iter().skip_while(|(tid, _)| *tid != skip_past);
cursor.nth(1)
}
None => tenants.first(),
};
entry.map(|(tid, _)| tid).copied()
}
});
let mut iterated_over = Vec::default();
while let Some(tid) = tid_iter.next() {
iterated_over.push((tid, ()));
}
assert_eq!(iterated_over, *tenant_shards);
}
}

View File

@@ -4,6 +4,7 @@ use utils::seqwait::MonotonicCounter;
mod auth;
mod background_node_operations;
mod compute_hook;
mod drain_utils;
mod heartbeater;
pub mod http;
mod id_lock_map;

View File

@@ -92,6 +92,11 @@ struct Cli {
/// Chaos testing
#[arg(long)]
chaos_interval: Option<humantime::Duration>,
// Maximum acceptable lag for the secondary location while draining
// a pageserver
#[arg(long)]
max_secondary_lag_bytes: Option<u64>,
}
enum StrictMode {
@@ -279,6 +284,7 @@ async fn async_main() -> anyhow::Result<()> {
.unwrap_or(RECONCILER_CONCURRENCY_DEFAULT),
split_threshold: args.split_threshold,
neon_local_repo_dir: args.neon_local_repo_dir,
max_secondary_lag_bytes: args.max_secondary_lag_bytes,
};
// After loading secrets & config, but before starting anything else, apply database migrations

View File

@@ -39,6 +39,9 @@ pub(super) struct Reconciler {
/// to detach this tenant shard.
pub(crate) detach: Vec<Node>,
/// Configuration specific to this reconciler
pub(crate) reconciler_config: ReconcilerConfig,
pub(crate) config: TenantConfig,
pub(crate) observed: ObservedState,
@@ -73,6 +76,65 @@ pub(super) struct Reconciler {
pub(crate) persistence: Arc<Persistence>,
}
pub(crate) struct ReconcilerConfigBuilder {
config: ReconcilerConfig,
}
impl ReconcilerConfigBuilder {
pub(crate) fn new() -> Self {
Self {
config: ReconcilerConfig::default(),
}
}
pub(crate) fn secondary_warmup_timeout(self, value: Duration) -> Self {
Self {
config: ReconcilerConfig {
secondary_warmup_timeout: Some(value),
..self.config
},
}
}
pub(crate) fn secondary_download_request_timeout(self, value: Duration) -> Self {
Self {
config: ReconcilerConfig {
secondary_download_request_timeout: Some(value),
..self.config
},
}
}
pub(crate) fn build(self) -> ReconcilerConfig {
self.config
}
}
#[derive(Default, Debug, Copy, Clone)]
pub(crate) struct ReconcilerConfig {
// During live migration give up on warming-up the secondary
// after this timeout.
secondary_warmup_timeout: Option<Duration>,
// During live migrations this is the amount of time that
// the pagserver will hold our poll.
secondary_download_request_timeout: Option<Duration>,
}
impl ReconcilerConfig {
pub(crate) fn get_secondary_warmup_timeout(&self) -> Duration {
const SECONDARY_WARMUP_TIMEOUT_DEFAULT: Duration = Duration::from_secs(300);
self.secondary_warmup_timeout
.unwrap_or(SECONDARY_WARMUP_TIMEOUT_DEFAULT)
}
pub(crate) fn get_secondary_download_request_timeout(&self) -> Duration {
const SECONDARY_DOWNLOAD_REQUEST_TIMEOUT_DEFAULT: Duration = Duration::from_secs(20);
self.secondary_download_request_timeout
.unwrap_or(SECONDARY_DOWNLOAD_REQUEST_TIMEOUT_DEFAULT)
}
}
/// RAII resource units granted to a Reconciler, which it should keep alive until it finishes doing I/O
pub(crate) struct ReconcileUnits {
_sem_units: tokio::sync::OwnedSemaphorePermit,
@@ -300,11 +362,13 @@ impl Reconciler {
) -> Result<(), ReconcileError> {
// This is not the timeout for a request, but the total amount of time we're willing to wait
// for a secondary location to get up to date before
const TOTAL_DOWNLOAD_TIMEOUT: Duration = Duration::from_secs(300);
let total_download_timeout = self.reconciler_config.get_secondary_warmup_timeout();
// This the long-polling interval for the secondary download requests we send to destination pageserver
// during a migration.
const REQUEST_DOWNLOAD_TIMEOUT: Duration = Duration::from_secs(20);
let request_download_timeout = self
.reconciler_config
.get_secondary_download_request_timeout();
let started_at = Instant::now();
@@ -315,14 +379,14 @@ impl Reconciler {
client
.tenant_secondary_download(
tenant_shard_id,
Some(REQUEST_DOWNLOAD_TIMEOUT),
Some(request_download_timeout),
)
.await
},
&self.service_config.jwt_token,
1,
3,
REQUEST_DOWNLOAD_TIMEOUT * 2,
request_download_timeout * 2,
&self.cancel,
)
.await
@@ -350,7 +414,7 @@ impl Reconciler {
return Ok(());
} else if status == StatusCode::ACCEPTED {
let total_runtime = started_at.elapsed();
if total_runtime > TOTAL_DOWNLOAD_TIMEOUT {
if total_runtime > total_download_timeout {
tracing::warn!("Timed out after {}ms downloading layers to {node}. Progress so far: {}/{} layers, {}/{} bytes",
total_runtime.as_millis(),
progress.layers_downloaded,

View File

@@ -14,10 +14,11 @@ use crate::{
Drain, Fill, Operation, OperationError, OperationHandler, MAX_RECONCILES_PER_OPERATION,
},
compute_hook::NotifyError,
drain_utils::{self, TenantShardDrain, TenantShardIterator},
id_lock_map::{trace_exclusive_lock, trace_shared_lock, IdLockMap, TracingExclusiveGuard},
metrics::LeadershipStatusGroup,
persistence::{AbortShardSplitStatus, MetadataHealthPersistence, TenantFilter},
reconciler::{ReconcileError, ReconcileUnits},
reconciler::{ReconcileError, ReconcileUnits, ReconcilerConfig, ReconcilerConfigBuilder},
scheduler::{MaySchedule, ScheduleContext, ScheduleMode},
tenant_shard::{
MigrateAttachment, ReconcileNeeded, ReconcilerStatus, ScheduleOptimization,
@@ -325,6 +326,12 @@ pub struct Config {
// TODO: make this cfg(feature = "testing")
pub neon_local_repo_dir: Option<PathBuf>,
// Maximum acceptable download lag for the secondary location
// while draining a node. If the secondary location is lagging
// by more than the configured amount, then the secondary is not
// upgraded to primary.
pub max_secondary_lag_bytes: Option<u64>,
}
impl From<DatabaseError> for ApiError {
@@ -5187,11 +5194,22 @@ impl Service {
Ok(())
}
/// Wrap [`TenantShard`] reconciliation methods with acquisition of [`Gate`] and [`ReconcileUnits`],
/// Like [`Self::maybe_configured_reconcile_shard`], but uses the default reconciler
/// configuration
fn maybe_reconcile_shard(
&self,
shard: &mut TenantShard,
nodes: &Arc<HashMap<NodeId, Node>>,
) -> Option<ReconcilerWaiter> {
self.maybe_configured_reconcile_shard(shard, nodes, ReconcilerConfig::default())
}
/// Wrap [`TenantShard`] reconciliation methods with acquisition of [`Gate`] and [`ReconcileUnits`],
fn maybe_configured_reconcile_shard(
&self,
shard: &mut TenantShard,
nodes: &Arc<HashMap<NodeId, Node>>,
reconciler_config: ReconcilerConfig,
) -> Option<ReconcilerWaiter> {
let reconcile_needed = shard.get_reconcile_needed(nodes);
@@ -5241,6 +5259,7 @@ impl Service {
&self.result_tx,
nodes,
&self.compute_hook,
reconciler_config,
&self.config,
&self.persistence,
units,
@@ -5715,18 +5734,92 @@ impl Service {
self.gate.close().await;
}
/// Spot check the download lag for a secondary location of a shard.
/// Should be used as a heuristic, since it's not always precise: the
/// secondary might have not downloaded the new heat map yet and, hence,
/// is not aware of the lag.
///
/// Returns:
/// * Ok(None) if the lag could not be determined from the status,
/// * Ok(Some(_)) if the lag could be determind
/// * Err on failures to query the pageserver.
async fn secondary_lag(
&self,
secondary: &NodeId,
tenant_shard_id: TenantShardId,
) -> Result<Option<u64>, mgmt_api::Error> {
let nodes = self.inner.read().unwrap().nodes.clone();
let node = nodes.get(secondary).ok_or(mgmt_api::Error::ApiError(
StatusCode::NOT_FOUND,
format!("Node with id {} not found", secondary),
))?;
match node
.with_client_retries(
|client| async move { client.tenant_secondary_status(tenant_shard_id).await },
&self.config.jwt_token,
1,
3,
Duration::from_millis(250),
&self.cancel,
)
.await
{
Some(Ok(status)) => match status.heatmap_mtime {
Some(_) => Ok(Some(status.bytes_total - status.bytes_downloaded)),
None => Ok(None),
},
Some(Err(e)) => Err(e),
None => Err(mgmt_api::Error::Cancelled),
}
}
/// Drain a node by moving the shards attached to it as primaries.
/// This is a long running operation and it should run as a separate Tokio task.
pub(crate) async fn drain_node(
&self,
self: &Arc<Self>,
node_id: NodeId,
cancel: CancellationToken,
) -> Result<(), OperationError> {
let mut last_inspected_shard: Option<TenantShardId> = None;
let mut inspected_all_shards = false;
const MAX_SECONDARY_LAG_BYTES_DEFAULT: u64 = 256 * 1024 * 1024;
let max_secondary_lag_bytes = self
.config
.max_secondary_lag_bytes
.unwrap_or(MAX_SECONDARY_LAG_BYTES_DEFAULT);
// By default, live migrations are generous about the wait time for getting
// the secondary location up to speed. When draining, give up earlier in order
// to not stall the operation when a cold secondary is encountered.
const SECONDARY_WARMUP_TIMEOUT: Duration = Duration::from_secs(20);
const SECONDARY_DOWNLOAD_REQUEST_TIMEOUT: Duration = Duration::from_secs(5);
let reconciler_config = ReconcilerConfigBuilder::new()
.secondary_warmup_timeout(SECONDARY_WARMUP_TIMEOUT)
.secondary_download_request_timeout(SECONDARY_DOWNLOAD_REQUEST_TIMEOUT)
.build();
let mut waiters = Vec::new();
while !inspected_all_shards {
let mut tid_iter = TenantShardIterator::new({
let service = self.clone();
move |last_inspected_shard: Option<TenantShardId>| {
let locked = &service.inner.read().unwrap();
let tenants = &locked.tenants;
let entry = match last_inspected_shard {
Some(skip_past) => {
// Skip to the last seen tenant shard id
let mut cursor = tenants.iter().skip_while(|(tid, _)| **tid != skip_past);
// Skip past the last seen
cursor.nth(1)
}
None => tenants.first_key_value(),
};
entry.map(|(tid, _)| tid).copied()
}
});
while !tid_iter.finished() {
if cancel.is_cancelled() {
match self
.node_configure(node_id, None, Some(NodeSchedulingPolicy::Active))
@@ -5745,71 +5838,82 @@ impl Service {
}
}
{
let mut locked = self.inner.write().unwrap();
let (nodes, tenants, scheduler) = locked.parts_mut();
drain_utils::validate_node_state(&node_id, self.inner.read().unwrap().nodes.clone())?;
let node = nodes.get(&node_id).ok_or(OperationError::NodeStateChanged(
format!("node {node_id} was removed").into(),
))?;
let current_policy = node.get_scheduling();
if !matches!(current_policy, NodeSchedulingPolicy::Draining) {
// TODO(vlad): maybe cancel pending reconciles before erroring out. need to think
// about it
return Err(OperationError::NodeStateChanged(
format!("node {node_id} changed state to {current_policy:?}").into(),
));
}
let mut cursor = tenants.iter_mut().skip_while({
let skip_past = last_inspected_shard;
move |(tid, _)| match skip_past {
Some(last) => **tid != last,
None => false,
while waiters.len() < MAX_RECONCILES_PER_OPERATION {
let tid = match tid_iter.next() {
Some(tid) => tid,
None => {
break;
}
});
};
while waiters.len() < MAX_RECONCILES_PER_OPERATION {
let (tid, tenant_shard) = match cursor.next() {
Some(some) => some,
let tid_drain = TenantShardDrain {
drained_node: node_id,
tenant_shard_id: tid,
};
let dest_node_id = {
let locked = self.inner.read().unwrap();
match tid_drain
.tenant_shard_eligible_for_drain(&locked.tenants, &locked.scheduler)
{
Some(node_id) => node_id,
None => {
inspected_all_shards = true;
break;
continue;
}
};
}
};
// If the shard is not attached to the node being drained, skip it.
if *tenant_shard.intent.get_attached() != Some(node_id) {
last_inspected_shard = Some(*tid);
match self.secondary_lag(&dest_node_id, tid).await {
Ok(Some(lag)) if lag <= max_secondary_lag_bytes => {
// The secondary is reasonably up to date.
// Migrate to it
}
Ok(Some(lag)) => {
tracing::info!(
tenant_id=%tid.tenant_id, shard_id=%tid.shard_slug(),
"Secondary on node {dest_node_id} is lagging by {lag}. Skipping reconcile."
);
continue;
}
Ok(None) => {
tracing::info!(
tenant_id=%tid.tenant_id, shard_id=%tid.shard_slug(),
"Could not determine lag for secondary on node {dest_node_id}. Skipping reconcile."
);
continue;
}
Err(err) => {
tracing::warn!(
tenant_id=%tid.tenant_id, shard_id=%tid.shard_slug(),
"Failed to get secondary lag from node {dest_node_id}. Skipping reconcile: {err}"
);
continue;
}
}
match tenant_shard.reschedule_to_secondary(None, scheduler) {
Err(e) => {
tracing::warn!(
tenant_id=%tid.tenant_id, shard_id=%tid.shard_slug(),
"Scheduling error when draining pageserver {} : {e}", node_id
);
}
Ok(()) => {
let scheduled_to = tenant_shard.intent.get_attached();
tracing::info!(
tenant_id=%tid.tenant_id, shard_id=%tid.shard_slug(),
"Rescheduled shard while draining node {}: {} -> {:?}",
node_id,
node_id,
scheduled_to
);
{
let mut locked = self.inner.write().unwrap();
let (nodes, tenants, scheduler) = locked.parts_mut();
let rescheduled = tid_drain.reschedule_to_secondary(
dest_node_id,
tenants,
scheduler,
nodes,
)?;
let waiter = self.maybe_reconcile_shard(tenant_shard, nodes);
if let Some(some) = waiter {
waiters.push(some);
}
if let Some(tenant_shard) = rescheduled {
let waiter = self.maybe_configured_reconcile_shard(
tenant_shard,
nodes,
reconciler_config,
);
if let Some(some) = waiter {
waiters.push(some);
}
}
last_inspected_shard = Some(*tid);
}
}

View File

@@ -7,7 +7,7 @@ use std::{
use crate::{
metrics::{self, ReconcileCompleteLabelGroup, ReconcileOutcome},
persistence::TenantShardPersistence,
reconciler::ReconcileUnits,
reconciler::{ReconcileUnits, ReconcilerConfig},
scheduler::{AffinityScore, MaySchedule, RefCountUpdate, ScheduleContext},
service::ReconcileResultRequest,
};
@@ -1063,6 +1063,7 @@ impl TenantShard {
result_tx: &tokio::sync::mpsc::UnboundedSender<ReconcileResultRequest>,
pageservers: &Arc<HashMap<NodeId, Node>>,
compute_hook: &Arc<ComputeHook>,
reconciler_config: ReconcilerConfig,
service_config: &service::Config,
persistence: &Arc<Persistence>,
units: ReconcileUnits,
@@ -1101,6 +1102,7 @@ impl TenantShard {
generation: self.generation,
intent: reconciler_intent,
detach,
reconciler_config,
config: self.config.clone(),
observed: self.observed.clone(),
compute_hook: compute_hook.clone(),