From 982812614f9af47a3e24701640facf1f5daab9d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Sat, 13 Jun 2026 21:39:02 +0800 Subject: [PATCH] test: preserve background config reload assumptions (#3409) --- docs/architecture/migration-progress.md | 73 +++++++++++---------- rustfs/src/admin/service/config.rs | 47 +++++++++++++- rustfs/src/main.rs | 85 +++++++++++++++++-------- 3 files changed, 147 insertions(+), 58 deletions(-) diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index dd84c6a4e..aa5e00233 100644 --- a/docs/architecture/migration-progress.md +++ b/docs/architecture/migration-progress.md @@ -5,16 +5,16 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Current Context - Issue: [`rustfs/backlog#660`](https://github.com/rustfs/backlog/issues/660) -- Branch: `overtrue/arch-metrics-runtime-controller-status` -- Baseline: `origin/main` at `76b375c478a8c2f5c79e62e848156a55915ed769` -- PR type for this branch: `behavior-change` -- Runtime behavior changes: none; the metrics runtime controller/status surface - only returns read-only snapshot/reconcile data and never starts, stops, - resizes, or wakes collectors. -- Rust code changes: add metrics runtime status/controller snapshots and - reconcile plans with explicit no-op worker mutation. +- Branch: `overtrue/arch-config-reload-preservation` +- Baseline: `origin/main` at `6b1e114b39a51166a872388ad64f86ee3d910864` +- PR type for this branch: `test-only` +- Runtime behavior changes: none; config reload and shutdown behavior are + preserved while their no-worker-mutation and shutdown-order assumptions are + made testable. +- Rust code changes: add module-local config reload and background shutdown + plans plus focused tests for `TEST-BGC-002`. - CI/script changes: none. -- Docs changes: record `BGC-006` metrics runtime controller/status scope. +- Docs changes: record `TEST-BGC-002` config reload preservation coverage. ## Phase 0 Tasks @@ -425,48 +425,57 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block cancellation, and startup call shape. - Verification: focused metrics runtime tests, compile checks, formatting, migration guards, Rust risk scan, and pre-commit quality gate. +- [x] `TEST-BGC-002` Preserve config reload and shutdown assumptions. + - Acceptance: dynamic server-config reload reports no worker mutation for + scanner/heal runtime config, bucket lifecycle/replication config files are + not dynamic server-config reload targets, and background shutdown keeps + scanner before AHM while preserving the scanner-implies-AHM dependency. + - Must preserve: no scanner, heal, lifecycle, replication, audit, storage + class, peer-signal, readiness, or worker lifecycle behavior change. + - Verification: focused config reload and shutdown tests, compile checks, + formatting, diff hygiene, and Rust risk scan. ## Next PRs -1. `test-only`: add config-reload preservation coverage for scanner/heal/ - lifecycle/replication in `TEST-BGC-002`. -2. `behavior-change`: add another low-risk read-only status surface only after - preserving config-reload and shutdown assumptions. +1. `behavior-change`: add another low-risk read-only status surface now that + config-reload and shutdown assumptions have dedicated preservation tests. +2. `test-only`: extend preservation coverage only if the next controller touches + additional startup/shutdown or config reload assumptions. ## Pre-Push Review Log | Expert | Status | Notes | |---|---|---| -| Quality/architecture | pass | Confirmed metrics runtime follows the established typed desired/status/reconcile shape without adding a generic scheduler, registry, or admin route. | -| Migration preservation | pass | Confirmed reconcile reports `worker_mutation: none`, preserving collector grouping, interval parsing, tombstone cycles, cancellation, and metrics emission behavior. | -| Testing/verification | pass | Focused tests, compile checks, formatting, diff hygiene, migration guards, Rust risk scan, and `make pre-commit` passed. | +| Quality/architecture | pass | Confirmed the new plans are module-local test seams only and do not add a generic scheduler, registry, route, or public API. | +| Migration preservation | pass | Confirmed reload plans report `worker_mutation: none`, bucket lifecycle/replication remain outside dynamic server-config reload, and shutdown keeps scanner before AHM. | +| Testing/verification | pass | Focused tests, formatting, diff hygiene, migration guards, Rust risk scan, and `make pre-commit` passed. | ## Verification Notes -Passed on `76b375c478a8c2f5c79e62e848156a55915ed769`: -- `cargo test -p rustfs-obs metrics_runtime --lib`; 4 passed. -- `cargo check -p rustfs-obs`. -- `cargo check -p rustfs --lib`. +Passed on `6b1e114b39a51166a872388ad64f86ee3d910864`: +- `cargo test -p rustfs --bin rustfs background_shutdown_plan_keeps_scanner_before_ahm`; 1 passed. +- `cargo test -p rustfs background_config_reload_plan_never_mutates_workers`; 1 passed. - `cargo fmt --all`. - `cargo fmt --all --check`. - `git diff --check`. - `./scripts/check_architecture_migration_rules.sh`. - `./scripts/check_layer_dependencies.sh`. -- `./scripts/check_metrics_migration_refs.sh`. -- Rust risk scan for changed Rust files; only an existing doc-comment - `println!` example and an existing bounded scheduler numeric cast matched. -- `make pre-commit`; all checks passed, including nextest 5880 passed and 111 +- Rust risk scan for changed Rust files; only pre-existing test `expect`, + startup `expect`, import alias `as`, and startup `eprintln!` matches remain; + added-line risky-pattern scan found no matches. +- `make pre-commit`; all checks passed, including nextest 5882 passed and 111 skipped. Notes: -- This slice adds reconcile/status data only. It does not apply reconcile output - to the running metrics collectors. -- There is no admin/API exposure in this PR; future controller harness work - should stay isolated from scanner, heal, lifecycle, and replication. +- This slice does not make dynamic config reload start, stop, restart, resize, + or wake scanner/heal/lifecycle/replication workers. +- The shutdown plan preserves the existing scanner before AHM order and keeps + scanner shutdown implying AHM shutdown. ## Handoff Notes -- Next migration slice should start `TEST-BGC-002` config-reload preservation - coverage before broader controller surfaces. -- Keep scanner, heal, lifecycle, replication, disk health, and config reload out - of broad controller movement until dedicated preservation tests exist. +- Next migration slice can add another low-risk read-only background status + surface while keeping worker lifecycle mutations out of scope. +- Keep scanner, heal, lifecycle, replication, disk health, and config reload + behavior changes out of broad controller movement until each has dedicated + status snapshots. diff --git a/rustfs/src/admin/service/config.rs b/rustfs/src/admin/service/config.rs index 5f5d9509c..6afde6683 100644 --- a/rustfs/src/admin/service/config.rs +++ b/rustfs/src/admin/service/config.rs @@ -42,6 +42,22 @@ pub fn is_dynamic_config_subsystem(sub_system: &str) -> bool { ) } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum DynamicConfigWorkerMutation { + None, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct DynamicConfigReloadPlan { + worker_mutation: DynamicConfigWorkerMutation, +} + +fn dynamic_config_reload_plan(sub_system: &str) -> Option { + is_dynamic_config_subsystem(sub_system).then_some(DynamicConfigReloadPlan { + worker_mutation: DynamicConfigWorkerMutation::None, + }) +} + fn internal_error(message: impl Into) -> S3Error { S3Error::with_message(S3ErrorCode::InternalError, message.into()) } @@ -257,7 +273,7 @@ pub async fn validate_server_config(config: &ServerConfig, sub_system: Option<&s } pub async fn apply_dynamic_config_for_subsystem(config: &ServerConfig, sub_system: &str) -> S3Result { - if !is_dynamic_config_subsystem(sub_system) { + if dynamic_config_reload_plan(sub_system).is_none() { return Ok(false); } @@ -359,6 +375,10 @@ mod tests { use rustfs_config::oidc::{OIDC_CLIENT_ID, OIDC_CONFIG_URL, OIDC_SCOPES}; use rustfs_config::{HEAL_SUB_SYS, SCANNER_SUB_SYS}; use rustfs_config::{MQTT_BROKER, MQTT_QUEUE_DIR, MQTT_TOPIC, WEBHOOK_ENDPOINT, WEBHOOK_QUEUE_DIR}; + use rustfs_ecstore::bucket::metadata::{BUCKET_LIFECYCLE_CONFIG, BUCKET_REPLICATION_CONFIG}; + + const LIFECYCLE_RELOAD_LABEL: &str = "lifecycle"; + const REPLICATION_RELOAD_LABEL: &str = "replication"; #[test] fn dynamic_config_subsystems_match_runtime_apply_support() { @@ -371,6 +391,31 @@ mod tests { assert!(!is_dynamic_config_subsystem("notify_webhook")); } + #[test] + fn background_config_reload_plan_never_mutates_workers() { + for sub_system in [ + STORAGE_CLASS_SUB_SYS, + AUDIT_WEBHOOK_SUB_SYS, + AUDIT_MQTT_SUB_SYS, + SCANNER_SUB_SYS, + HEAL_SUB_SYS, + ] { + assert_eq!( + dynamic_config_reload_plan(sub_system).map(|plan| plan.worker_mutation), + Some(DynamicConfigWorkerMutation::None) + ); + } + + for bucket_config in [ + BUCKET_LIFECYCLE_CONFIG, + BUCKET_REPLICATION_CONFIG, + LIFECYCLE_RELOAD_LABEL, + REPLICATION_RELOAD_LABEL, + ] { + assert_eq!(dynamic_config_reload_plan(bucket_config), None); + } + } + #[test] fn validate_storage_class_kvs_rejects_invalid_parity() { let mut kvs = KVS::new(); diff --git a/rustfs/src/main.rs b/rustfs/src/main.rs index 8aa2eda1b..572158ecc 100644 --- a/rustfs/src/main.rs +++ b/rustfs/src/main.rs @@ -1011,6 +1011,23 @@ struct ProtocolShutdownSenders { sftp: Option, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum BackgroundShutdownStep { + DataScanner, + Ahm, +} + +fn background_shutdown_steps(enable_scanner: bool, enable_heal: bool) -> Vec { + let mut steps = Vec::with_capacity(2); + if enable_scanner { + steps.push(BackgroundShutdownStep::DataScanner); + } + if enable_heal || enable_scanner { + steps.push(BackgroundShutdownStep::Ahm); + } + steps +} + /// Handles the shutdown process of the server async fn handle_shutdown( state_manager: &ServiceStateManager, @@ -1044,33 +1061,37 @@ async fn handle_shutdown( let enable_heal = get_env_bool_with_aliases(ENV_HEAL_ENABLED, &[ENV_HEAL_ENABLED_DEPRECATED], true); // Stop background services based on what was enabled. - if enable_scanner { - info!( - target: "rustfs::main::handle_shutdown", - event = EVENT_BACKGROUND_SERVICE_SHUTDOWN, - component = LOG_COMPONENT_MAIN, - subsystem = LOG_SUBSYSTEM_STARTUP, - service = "data_scanner", - state = "stopping", - "Background service shutdown started" - ); - shutdown_background_services(); + let background_steps = background_shutdown_steps(enable_scanner, enable_heal); + for step in &background_steps { + match step { + BackgroundShutdownStep::DataScanner => { + info!( + target: "rustfs::main::handle_shutdown", + event = EVENT_BACKGROUND_SERVICE_SHUTDOWN, + component = LOG_COMPONENT_MAIN, + subsystem = LOG_SUBSYSTEM_STARTUP, + service = "data_scanner", + state = "stopping", + "Background service shutdown started" + ); + shutdown_background_services(); + } + BackgroundShutdownStep::Ahm => { + info!( + target: "rustfs::main::handle_shutdown", + event = EVENT_BACKGROUND_SERVICE_SHUTDOWN, + component = LOG_COMPONENT_MAIN, + subsystem = LOG_SUBSYSTEM_STARTUP, + service = "ahm", + state = "stopping", + "Background service shutdown started" + ); + shutdown_ahm_services(); + } + } } - if enable_heal || enable_scanner { - info!( - target: "rustfs::main::handle_shutdown", - event = EVENT_BACKGROUND_SERVICE_SHUTDOWN, - component = LOG_COMPONENT_MAIN, - subsystem = LOG_SUBSYSTEM_STARTUP, - service = "ahm", - state = "stopping", - "Background service shutdown started" - ); - shutdown_ahm_services(); - } - - if !enable_scanner && !enable_heal { + if background_steps.is_empty() { info!( target: "rustfs::main::handle_shutdown", event = EVENT_BACKGROUND_SERVICE_SHUTDOWN, @@ -1268,4 +1289,18 @@ mod tests { assert!(!DEFAULT_CREDENTIALS_WARNING_MESSAGE.contains(rustfs_credentials::DEFAULT_ACCESS_KEY)); assert!(!DEFAULT_CREDENTIALS_WARNING_MESSAGE.contains(rustfs_credentials::DEFAULT_SECRET_KEY)); } + + #[test] + fn background_shutdown_plan_keeps_scanner_before_ahm() { + assert_eq!( + background_shutdown_steps(true, true), + vec![BackgroundShutdownStep::DataScanner, BackgroundShutdownStep::Ahm] + ); + assert_eq!( + background_shutdown_steps(true, false), + vec![BackgroundShutdownStep::DataScanner, BackgroundShutdownStep::Ahm] + ); + assert_eq!(background_shutdown_steps(false, true), vec![BackgroundShutdownStep::Ahm]); + assert!(background_shutdown_steps(false, false).is_empty()); + } }