From edbcb8224ebc96f9eedeb7d43ac60d71f3386cba Mon Sep 17 00:00:00 2001 From: "Lei, HUANG" <6406592+v0y4g3r@users.noreply.github.com> Date: Mon, 21 Sep 2026 09:19:07 +0000 Subject: [PATCH] fix: fail startup on duplicate region engine configs (#9281) * fix: reject duplicate region engine configurations Signed-off-by: Lei, HUANG * refactor: reuse canonical engine names in config validation Replace duplicated engine-name literals with the existing common-catalog constants so duplicate-config validation uses the shared engine names. Keep the TOML regression inputs independent to verify the public configuration tags. Signed-off-by: Lei, HUANG * fix: clarify zero-based duplicate engine config indices State explicitly that duplicate region engine configuration indices are zero-based so users can map them to the order of TOML entries. Preserve the existing index values and error classification. Signed-off-by: Lei, HUANG --------- Signed-off-by: Lei, HUANG --- config/config.md | 4 +- config/datanode.example.toml | 1 + config/standalone.example.toml | 1 + src/datanode/src/datanode.rs | 109 +++++++++++++++++++++++++++++++-- src/datanode/src/error.rs | 12 ++++ 5 files changed, 119 insertions(+), 8 deletions(-) diff --git a/config/config.md b/config/config.md index 7385c8efb0..16384b406f 100644 --- a/config/config.md +++ b/config/config.md @@ -170,7 +170,7 @@ | `storage.http_client.timeout` | String | `30s` | The total request timeout, applied from when the request starts connecting until the response body has finished.
Also considered a total deadline. | | `storage.http_client.pool_idle_timeout` | String | `90s` | The timeout for idle sockets being kept-alive. | | `storage.http_client.skip_ssl_validation` | Bool | `false` | To skip the ssl verification
**Security Notice**: Setting `skip_ssl_validation = true` disables certificate verification, making connections vulnerable to man-in-the-middle attacks. Only use this in development or trusted private networks. | -| `[[region_engine]]` | -- | -- | The region engine options. You can configure multiple region engines. | +| `[[region_engine]]` | -- | -- | The region engine options. You can configure multiple region engines.
Each engine type (mito, file, metric) may appear only once; duplicates cause startup to fail. | | `region_engine.mito` | -- | -- | The Mito engine options. | | `region_engine.mito.num_workers` | Integer | `8` | Number of region workers. | | `region_engine.mito.worker_channel_size` | Integer | `128` | Request channel size of each worker. | @@ -631,7 +631,7 @@ | `storage.http_client.timeout` | String | `30s` | The total request timeout, applied from when the request starts connecting until the response body has finished.
Also considered a total deadline. | | `storage.http_client.pool_idle_timeout` | String | `90s` | The timeout for idle sockets being kept-alive. | | `storage.http_client.skip_ssl_validation` | Bool | `false` | To skip the ssl verification
**Security Notice**: Setting `skip_ssl_validation = true` disables certificate verification, making connections vulnerable to man-in-the-middle attacks. Only use this in development or trusted private networks. | -| `[[region_engine]]` | -- | -- | The region engine options. You can configure multiple region engines. | +| `[[region_engine]]` | -- | -- | The region engine options. You can configure multiple region engines.
Each engine type (mito, file, metric) may appear only once; duplicates cause startup to fail. | | `region_engine.mito` | -- | -- | The Mito engine options. | | `region_engine.mito.num_workers` | Integer | `8` | Number of region workers. | | `region_engine.mito.worker_channel_size` | Integer | `128` | Request channel size of each worker. | diff --git a/config/datanode.example.toml b/config/datanode.example.toml index f99e1e124f..45baaefbdf 100644 --- a/config/datanode.example.toml +++ b/config/datanode.example.toml @@ -464,6 +464,7 @@ skip_ssl_validation = false # endpoint = "https://storage.googleapis.com" ## The region engine options. You can configure multiple region engines. +## Each engine type (mito, file, metric) may appear only once; duplicates cause startup to fail. [[region_engine]] ## The Mito engine options. diff --git a/config/standalone.example.toml b/config/standalone.example.toml index cc4be3f593..e59e7ba1b4 100644 --- a/config/standalone.example.toml +++ b/config/standalone.example.toml @@ -674,6 +674,7 @@ skip_ssl_validation = false # endpoint = "https://storage.googleapis.com" ## The region engine options. You can configure multiple region engines. +## Each engine type (mito, file, metric) may appear only once; duplicates cause startup to fail. [[region_engine]] ## The Mito engine options. diff --git a/src/datanode/src/datanode.rs b/src/datanode/src/datanode.rs index 15e83fc1c7..ae4b0723e1 100644 --- a/src/datanode/src/datanode.rs +++ b/src/datanode/src/datanode.rs @@ -14,11 +14,13 @@ //! Datanode implementation. +use std::collections::HashMap; use std::path::Path; use std::sync::Arc; use std::time::{Duration, Instant}; use common_base::Plugins; +use common_catalog::consts::{FILE_ENGINE, METRIC_ENGINE, MITO_ENGINE}; use common_datasource::object_store::LocalFileAccess; use common_error::ext::BoxedError; use common_greptimedb_telemetry::GreptimeDBTelemetryTask; @@ -63,9 +65,10 @@ use tokio::sync::Notify; use crate::config::{DatanodeOptions, RegionEngineConfig, StorageConfig}; use crate::error::{ self, BuildDatanodeSnafu, BuildMetricEngineSnafu, BuildMitoEngineSnafu, CreateDirSnafu, - DataFusionSnafu, GetMetadataSnafu, InvalidObjectStoreWalConfigSnafu, MissingCacheSnafu, - MissingNodeIdSnafu, ObjectStoreWalNotSupportedSnafu, OpenLogStoreSnafu, Result, - ShutdownInstanceSnafu, ShutdownServerSnafu, StartServerSnafu, + DataFusionSnafu, DuplicateRegionEngineConfigSnafu, GetMetadataSnafu, + InvalidObjectStoreWalConfigSnafu, MissingCacheSnafu, MissingNodeIdSnafu, + ObjectStoreWalNotSupportedSnafu, OpenLogStoreSnafu, Result, ShutdownInstanceSnafu, + ShutdownServerSnafu, StartServerSnafu, }; use crate::event_listener::{ NoopRegionServerEventListener, RegionServerEventListenerRef, RegionServerEventReceiver, @@ -253,6 +256,7 @@ impl DatanodeBuilder { } pub async fn build(mut self) -> Result { + validate_region_engine_config(&self.opts.region_engine)?; let node_id = self.opts.node_id.context(MissingNodeIdSnafu)?; set_default_prefix(self.opts.default_column_prefix.as_deref()) .map_err(BoxedError::new) @@ -710,6 +714,27 @@ impl DatanodeBuilder { } } +/// Rejects repeated engine types before their configs can silently overwrite each other. +fn validate_region_engine_config(configs: &[RegionEngineConfig]) -> Result<()> { + let mut engine_indices = HashMap::new(); + for (index, config) in configs.iter().enumerate() { + let engine = match config { + RegionEngineConfig::Mito(_) => MITO_ENGINE, + RegionEngineConfig::File(_) => FILE_ENGINE, + RegionEngineConfig::Metric(_) => METRIC_ENGINE, + }; + if let Some(first_index) = engine_indices.insert(engine, index) { + return DuplicateRegionEngineConfigSnafu { + engine, + first_index, + duplicate_index: index, + } + .fail(); + } + } + Ok(()) +} + /// Rejects an object store WAL config the log store cannot run on. fn validate_object_store_wal_config(config: &ObjectStoreWalConfig) -> Result<()> { let prefix = config.prefix.trim(); @@ -879,6 +904,7 @@ async fn open_all_regions( mod tests { use std::assert_matches; use std::collections::{BTreeMap, HashMap}; + use std::io::Write; use std::path::Path; use std::sync::Arc; use std::time::Duration; @@ -886,6 +912,7 @@ mod tests { use cache::build_datanode_cache_registry; use common_base::Plugins; use common_base::readable_size::ReadableSize; + use common_config::Configurable; use common_error::ext::ErrorExt; use common_error::status_code::StatusCode; use common_meta::cache::LayeredCacheRegistryBuilder; @@ -893,15 +920,17 @@ mod tests { use common_meta::key::datanode_table::DatanodeTableManager; use common_meta::kv_backend::KvBackendRef; use common_meta::kv_backend::memory::MemoryKvBackend; - use common_test_util::temp_dir::create_temp_dir; + use common_test_util::temp_dir::{create_named_temp_file, create_temp_dir}; use common_wal::config::DatanodeWalConfig; use common_wal::config::object_store::{ObjectStoreWalConfig, STANDALONE_GENERATION}; use mito2::engine::MITO_ENGINE_NAME; use store_api::region_request::RegionRequest; use store_api::storage::RegionId; - use crate::config::DatanodeOptions; - use crate::datanode::{DatanodeBuilder, validate_object_store_wal_config}; + use crate::config::{DatanodeOptions, RegionEngineConfig}; + use crate::datanode::{ + DatanodeBuilder, validate_object_store_wal_config, validate_region_engine_config, + }; use crate::error::Error; use crate::tests::{MockRegionEngine, mock_region_server}; @@ -1014,6 +1043,74 @@ mod tests { std::fs::read_dir(dir).unwrap().next().is_none() } + #[test] + fn test_validate_unique_region_engine_config() { + let mut opts = DatanodeOptions::default(); + validate_region_engine_config(&opts.region_engine).unwrap(); + + opts.region_engine + .push(RegionEngineConfig::Metric(Default::default())); + validate_region_engine_config(&opts.region_engine).unwrap(); + + // Omitted engines still use their defaults during engine construction. + validate_region_engine_config(&[]).unwrap(); + } + + #[tokio::test] + async fn test_build_rejects_duplicate_region_engine_config() { + for (engine, first_index) in [("mito", 0), ("file", 1), ("metric", 2)] { + let mut file = create_named_temp_file(); + write!( + file, + r#" + [[region_engine]] + [region_engine.mito] + global_write_buffer_size = "4GiB" + min_compaction_interval = "600s" + + [[region_engine]] + [region_engine.file] + + [[region_engine]] + [region_engine.metric] + + [[region_engine]] + [region_engine.{engine}] + "# + ) + .unwrap(); + let opts = DatanodeOptions::load_layered_options( + Some(file.path().to_str().unwrap()), + "DATANODE_DUPLICATE_REGION_ENGINE_UT", + ) + .unwrap(); + let data_home = create_temp_dir("duplicate-region-engine-config"); + let mut builder = datanode_builder( + data_home.path().to_str().unwrap(), + DatanodeWalConfig::default(), + Arc::new(MemoryKvBackend::new()), + ); + builder.opts.region_engine = opts.region_engine; + let err = build_err(builder).await; + + let Error::DuplicateRegionEngineConfig { + engine: actual_engine, + first_index: actual_first_index, + duplicate_index, + .. + } = &err + else { + panic!("unexpected error for {engine}: {err:?}"); + }; + assert_eq!(*actual_engine, engine); + assert_eq!(*actual_first_index, first_index); + assert_eq!(*duplicate_index, 3); + assert_eq!(err.status_code(), StatusCode::InvalidArguments); + // Rejected before the builder creates any storage or log store. + assert!(is_empty_dir(data_home.path())); + } + } + #[tokio::test] async fn test_build_rejects_invalid_object_store_wal_config() { let cases = [ diff --git a/src/datanode/src/error.rs b/src/datanode/src/error.rs index c391efffcf..b62708aa34 100644 --- a/src/datanode/src/error.rs +++ b/src/datanode/src/error.rs @@ -299,6 +299,17 @@ pub enum Error { location: Location, }, + #[snafu(display( + "Duplicate region engine config '{engine}' at region_engine[{first_index}] and region_engine[{duplicate_index}] (indices are zero-based); each engine type may be configured only once" + ))] + DuplicateRegionEngineConfig { + engine: &'static str, + first_index: usize, + duplicate_index: usize, + #[snafu(implicit)] + location: Location, + }, + #[snafu(display("Invalid object store WAL config, {} = {:?}: {}", field, value, reason))] InvalidObjectStoreWalConfig { field: &'static str, @@ -486,6 +497,7 @@ impl ErrorExt for Error { | GcConfigMismatch { .. } | ParseAddr { .. } | TomlFormat { .. } + | DuplicateRegionEngineConfig { .. } | InvalidObjectStoreWalConfig { .. } | BuildDatanode { .. } => StatusCode::InvalidArguments,