From 6b86e3875c4912f3a7c27915b0bbd43399402847 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Thu, 11 Jun 2026 11:28:09 +0800 Subject: [PATCH] refactor(storage): remove old admin surfaces (#3340) --- crates/ecstore/src/config/com.rs | 54 ++++++++-- crates/ecstore/src/set_disk.rs | 37 ++----- crates/ecstore/src/sets.rs | 22 ----- crates/ecstore/src/store.rs | 22 ----- crates/ecstore/src/store/rebalance.rs | 6 +- crates/ecstore/src/store_api.rs | 1 - crates/ecstore/src/store_api/traits.rs | 7 -- docs/architecture/migration-progress.md | 125 ++++++++++++------------ 8 files changed, 115 insertions(+), 159 deletions(-) diff --git a/crates/ecstore/src/config/com.rs b/crates/ecstore/src/config/com.rs index a30cf9394..b2267b61f 100644 --- a/crates/ecstore/src/config/com.rs +++ b/crates/ecstore/src/config/com.rs @@ -31,6 +31,7 @@ use rustfs_config::notify::{ }; use rustfs_config::oidc::{IDENTITY_OPENID_KEYS, IDENTITY_OPENID_SUB_SYS, OIDC_REDIRECT_URI_DYNAMIC}; use rustfs_config::{COMMENT_KEY, DEFAULT_DELIMITER, ENABLE_KEY, EnableState, RUSTFS_REGION}; +use rustfs_storage_api::StorageAdminApi; use rustfs_utils::path::SLASH_SEPARATOR; use serde_json::{Map, Value}; use std::collections::{HashMap, HashSet}; @@ -277,7 +278,10 @@ fn new_server_config() -> Config { Config::new() } -async fn new_and_save_server_config(api: Arc) -> Result { +async fn new_and_save_server_config(api: Arc) -> Result +where + S: StorageAPI + StorageAdminApi, +{ let mut cfg = new_server_config(); lookup_configs(&mut cfg, api.clone()).await; save_server_config(api, &cfg).await?; @@ -1059,7 +1063,10 @@ pub async fn try_migrate_server_config(api: Arc) { } /// Handle the situation where the configuration file does not exist, create and save a new configuration -async fn handle_missing_config(api: Arc, context: &str) -> Result { +async fn handle_missing_config(api: Arc, context: &str) -> Result +where + S: StorageAPI + StorageAdminApi, +{ warn!("Configuration not found ({}): Start initializing new configuration", context); let cfg = if is_first_cluster_node_local().await { new_and_save_server_config(api.clone()).await? @@ -1078,7 +1085,10 @@ fn handle_config_read_error(err: Error, file_path: &str) -> Result { Err(err) } -pub async fn read_config_without_migrate(api: Arc) -> Result { +pub async fn read_config_without_migrate(api: Arc) -> Result +where + S: StorageAPI + StorageAdminApi, +{ let config_file = get_config_file(); // Try to read the configuration file @@ -1089,7 +1099,10 @@ pub async fn read_config_without_migrate(api: Arc) -> Result(api: Arc, data: &[u8]) -> Result { +async fn read_server_config(api: Arc, data: &[u8]) -> Result +where + S: StorageAPI + StorageAdminApi, +{ // If the provided data is empty, try to read from the file again if data.is_empty() { let config_file = get_config_file(); @@ -1141,14 +1154,20 @@ pub async fn save_server_config(api: Arc, cfg: &Config) -> Res save_config(api, &config_file, data).await } -pub async fn lookup_configs(cfg: &mut Config, api: Arc) { +pub async fn lookup_configs(cfg: &mut Config, api: Arc) +where + S: StorageAPI + StorageAdminApi, +{ // TODO: from etcd if let Err(err) = apply_dynamic_config(cfg, api).await { error!("apply_dynamic_config err {:?}", &err); } } -async fn apply_dynamic_config(cfg: &mut Config, api: Arc) -> Result<()> { +async fn apply_dynamic_config(cfg: &mut Config, api: Arc) -> Result<()> +where + S: StorageAPI + StorageAdminApi, +{ for key in SUB_SYSTEMS_DYNAMIC.iter() { apply_dynamic_config_for_sub_sys(cfg, api.clone(), key).await?; } @@ -1156,8 +1175,11 @@ async fn apply_dynamic_config(cfg: &mut Config, api: Arc) -> R Ok(()) } -async fn apply_dynamic_config_for_sub_sys(cfg: &mut Config, api: Arc, subsys: &str) -> Result<()> { - let set_drive_counts = api.set_drive_counts(); +async fn apply_dynamic_config_for_sub_sys(cfg: &mut Config, api: Arc, subsys: &str) -> Result<()> +where + S: StorageAPI + StorageAdminApi, +{ + let set_drive_counts = StorageAdminApi::set_drive_counts(api.as_ref()); if subsys == STORAGE_CLASS_SUB_SYS { let kvs = cfg.get_value(STORAGE_CLASS_SUB_SYS, DEFAULT_DELIMITER).unwrap_or_default(); @@ -1210,6 +1232,7 @@ mod tests { use rustfs_lock::client::LockClient; use rustfs_lock::client::local::LocalClient; use rustfs_lock::{LockError, LockInfo, LockResponse, LockStats}; + use rustfs_storage_api::StorageAdminApi; use serde_json::Value; use serial_test::serial; use std::collections::HashMap; @@ -1702,6 +1725,14 @@ mod tests { async fn new_ns_lock(&self, bucket: &str, object: &str) -> Result { self.set_disks.new_ns_lock(bucket, object).await } + } + + #[async_trait::async_trait] + impl StorageAdminApi for LockingConfigStorage { + type BackendInfo = rustfs_madmin::BackendInfo; + type StorageInfo = rustfs_madmin::StorageInfo; + type Disk = crate::disk::DiskStore; + type Error = Error; async fn backend_info(&self) -> rustfs_madmin::BackendInfo { panic!("unused in test") @@ -1715,12 +1746,15 @@ mod tests { panic!("unused in test") } - async fn get_disks(&self, _pool_idx: usize, _set_idx: usize) -> Result>> { + async fn disk_set_inventory( + &self, + _selector: rustfs_storage_api::DiskSetSelector, + ) -> Result>> { panic!("unused in test") } fn set_drive_counts(&self) -> Vec { - panic!("unused in test") + vec![self.set_disks.set_endpoints.len()] } } diff --git a/crates/ecstore/src/set_disk.rs b/crates/ecstore/src/set_disk.rs index 71238ef85..081fe9370 100644 --- a/crates/ecstore/src/set_disk.rs +++ b/crates/ecstore/src/set_disk.rs @@ -1613,29 +1613,6 @@ impl StorageAPI for SetDisks { Ok(NamespaceLockWrapper::new(set_lock, resource, self.locker_owner.clone())) } - - #[tracing::instrument(skip(self))] - async fn backend_info(&self) -> rustfs_madmin::BackendInfo { - unimplemented!() - } - #[tracing::instrument(skip(self))] - async fn storage_info(&self) -> rustfs_madmin::StorageInfo { - self.storage_info_snapshot().await - } - #[tracing::instrument(skip(self))] - async fn local_storage_info(&self) -> rustfs_madmin::StorageInfo { - self.local_storage_info_snapshot().await - } - - #[tracing::instrument(skip(self))] - async fn get_disks(&self, _pool_idx: usize, _set_idx: usize) -> Result>> { - Ok(self.disk_inventory().await) - } - - #[tracing::instrument(skip(self))] - fn set_drive_counts(&self) -> Vec { - unimplemented!() - } } #[async_trait::async_trait] @@ -1837,7 +1814,7 @@ impl ObjectOperations for SetDisks { } #[tracing::instrument(skip(self))] async fn delete_object_version(&self, bucket: &str, object: &str, fi: &FileInfo, force_del_marker: bool) -> Result<()> { - let disks = self.get_disks(0, 0).await?; + let disks = self.disk_inventory().await; let write_quorum = disks.len() / 2 + 1; let mut futures = Vec::with_capacity(disks.len()); @@ -2213,9 +2190,8 @@ impl ObjectOperations for SetDisks { .await .map_err(|e| to_object_err(e, vec![bucket, object]))?; - if let Ok(disks) = self.get_disks(0, 0).await { - record_capacity_scope_if_needed(opts.capacity_scope_token, &disks); - } + let disks = self.disk_inventory().await; + record_capacity_scope_if_needed(opts.capacity_scope_token, &disks); let mut oi = ObjectInfo::from_file_info(&fi, bucket, object, opts.versioned || opts.version_suspended); oi.replication_decision = goi.replication_decision; @@ -2243,9 +2219,8 @@ impl ObjectOperations for SetDisks { .await .map_err(|e| to_object_err(e, vec![bucket, object]))?; - if let Ok(disks) = self.get_disks(0, 0).await { - record_capacity_scope_if_needed(opts.capacity_scope_token, &disks); - } + let disks = self.disk_inventory().await; + record_capacity_scope_if_needed(opts.capacity_scope_token, &disks); let mut obj_info = ObjectInfo::from_file_info(&dfi, bucket, object, opts.versioned || opts.version_suspended); obj_info.size = goi.size; @@ -2519,7 +2494,7 @@ impl ObjectOperations for SetDisks { let event_name = EventName::LifecycleTransition.as_str(); let mut should_notify_transition = true; - let disks = self.get_disks(0, 0).await?; + let disks = self.disk_inventory().await; if let Err(err) = self.delete_object_version(bucket, object, &fi, false).await { should_notify_transition = false; diff --git a/crates/ecstore/src/sets.rs b/crates/ecstore/src/sets.rs index e5a79304d..586153c9d 100644 --- a/crates/ecstore/src/sets.rs +++ b/crates/ecstore/src/sets.rs @@ -901,28 +901,6 @@ impl StorageAPI for Sets { async fn new_ns_lock(&self, bucket: &str, object: &str) -> Result { self.disk_set[0].new_ns_lock(bucket, object).await } - #[tracing::instrument(skip(self))] - async fn backend_info(&self) -> rustfs_madmin::BackendInfo { - unimplemented!() - } - #[tracing::instrument(skip(self))] - async fn storage_info(&self) -> rustfs_madmin::StorageInfo { - self.storage_info_snapshot().await - } - #[tracing::instrument(skip(self))] - async fn local_storage_info(&self) -> rustfs_madmin::StorageInfo { - self.local_storage_info_snapshot().await - } - - #[tracing::instrument(skip(self))] - async fn get_disks(&self, _pool_idx: usize, _set_idx: usize) -> Result>> { - unimplemented!() - } - - #[tracing::instrument(skip(self))] - fn set_drive_counts(&self) -> Vec { - unimplemented!() - } } async fn _close_storage_disks(disks: &[Option]) { diff --git a/crates/ecstore/src/store.rs b/crates/ecstore/src/store.rs index 1a036d65f..cafa2ce18 100644 --- a/crates/ecstore/src/store.rs +++ b/crates/ecstore/src/store.rs @@ -706,28 +706,6 @@ impl StorageAPI for ECStore { async fn new_ns_lock(&self, bucket: &str, object: &str) -> Result { self.handle_new_ns_lock(bucket, object).await } - #[instrument(skip(self))] - async fn backend_info(&self) -> rustfs_madmin::BackendInfo { - self.handle_backend_info().await - } - #[instrument(skip(self))] - async fn storage_info(&self) -> rustfs_madmin::StorageInfo { - self.handle_storage_info().await - } - #[instrument(skip(self))] - async fn local_storage_info(&self) -> rustfs_madmin::StorageInfo { - self.handle_local_storage_info().await - } - - #[instrument(skip(self))] - async fn get_disks(&self, pool_idx: usize, set_idx: usize) -> Result>> { - self.handle_get_disks(pool_idx, set_idx).await - } - - #[instrument(skip(self))] - fn set_drive_counts(&self) -> Vec { - self.handle_set_drive_counts() - } } #[async_trait::async_trait] diff --git a/crates/ecstore/src/store/rebalance.rs b/crates/ecstore/src/store/rebalance.rs index 6c511dd98..65475e924 100644 --- a/crates/ecstore/src/store/rebalance.rs +++ b/crates/ecstore/src/store/rebalance.rs @@ -315,10 +315,8 @@ impl ECStore { return (idx, 0, Vec::new()); } - let disk_infos = match pool.get_disks_by_key(object).get_disks(0, 0).await { - Ok(disks) => get_disk_infos(&disks).await, - Err(_) => Vec::new(), - }; + let disks = pool.get_disks_by_key(object).disk_inventory().await; + let disk_infos = get_disk_infos(&disks).await; (idx, pool.set_count, disk_infos) })) diff --git a/crates/ecstore/src/store_api.rs b/crates/ecstore/src/store_api.rs index 921f2001f..b69a31017 100644 --- a/crates/ecstore/src/store_api.rs +++ b/crates/ecstore/src/store_api.rs @@ -15,7 +15,6 @@ use crate::bucket::metadata_sys::get_versioning_config; use crate::bucket::versioning::VersioningApi as _; use crate::config::storageclass; -use crate::disk::DiskStore; use crate::error::{Error, Result}; use crate::rio::{HashReader, LimitReader}; use crate::store_utils::clean_metadata; diff --git a/crates/ecstore/src/store_api/traits.rs b/crates/ecstore/src/store_api/traits.rs index facabeae3..e683e770c 100644 --- a/crates/ecstore/src/store_api/traits.rs +++ b/crates/ecstore/src/store_api/traits.rs @@ -182,11 +182,4 @@ pub trait StorageAPI: ObjectIO + BucketOperations + ObjectOperations + ListOperations + MultipartOperations + HealOperations + Debug { async fn new_ns_lock(&self, bucket: &str, object: &str) -> Result; - - async fn backend_info(&self) -> rustfs_madmin::BackendInfo; - async fn storage_info(&self) -> rustfs_madmin::StorageInfo; - async fn local_storage_info(&self) -> rustfs_madmin::StorageInfo; - - async fn get_disks(&self, pool_idx: usize, set_idx: usize) -> Result>>; - fn set_drive_counts(&self) -> Vec; } diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index 4d1a85250..c27d94436 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-storage-admin-remaining-readers` -- Baseline: `origin/main` at `f325b9f71ce4807488829e6f558383242bbcb6a2` -- PR type for this branch: `consumer-migration` +- Branch: `overtrue/arch-storage-api-next-contracts` +- Baseline: `origin/main` at `87968275a776362e9b3734eac5bd9e3233d6cbe9` +- PR type for this branch: `dependency-migration` - Runtime behavior changes: none. -- Rust code changes: route maintenance/background read-side storage inventory - consumers through the inventory-facing `StorageAdminApi` contract while - preserving the old `StorageAPI` compatibility surface. +- Rust code changes: remove duplicate admin-read methods from the old + `StorageAPI` trait after all admin inventory consumers have moved to + `StorageAdminApi`. - CI/script changes: none. -- Docs changes: record API-007 maintenance/background inventory-reader context, - verification evidence, and expert review outcomes. +- Docs changes: record API-007 completion and the current admin surface cleanup + context, verification evidence, and expert review outcomes. ## Phase 0 Tasks @@ -192,7 +192,7 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block `rustfs-storage-api`. - Verification: focused storage-api tests, dependency tree, migration guards, formatting, and diff hygiene. -- [~] `API-007` Dual-route `get_disks` consumers. +- [x] `API-007` Dual-route `get_disks` consumers. - Completed first slice: `rustfs/rustfs#3331` bound `ECStore` to `StorageAdminApi` while keeping all consumers unchanged. - Completed second slice: `rustfs/rustfs#3332` migrated the admin @@ -210,35 +210,33 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block - Completed sixth slice: `rustfs/rustfs#3336` migrated ECStore internal decommission space, local-storage-info, backend-info, drive-count, and disk-inventory admin handlers away from old `StorageAPI` method calls. - - Current branch slice: migrate maintenance/background read-side storage - inventory consumers in rebalance metadata initialization, heal resume disk - lookup, and scanner local disk scan lookup. - - Acceptance: these maintenance/background consumers no longer use old - `StorageAPI` calls for storage-info or disk-set inventory when the - inventory-facing `StorageAdminApi` contract already represents the same - read-only operation. - - Must preserve: old `StorageAPI` trait shape, `StorageAPI::get_disks` - behavior, rebalance metadata serialization/save/load, heal resume disk - selection, scanner local disk selection, object/rebalance object selection - paths, scanner data-cache persistence, heal object repair, object paths, - replication/config/tier persistence, and storage hot paths. - - Risk defense: change only trait call entry points to existing ECStore - `StorageAdminApi` handlers; do not migrate object APIs, config or - replication persistence, scanner cache writes, heal object repair, or - storage implementation hot paths in this PR. - - Verification: - - `cargo fmt --all && cargo fmt --all --check`. - - `cargo check -p rustfs-ecstore -p rustfs-heal -p rustfs-scanner`. - - `cargo test -p rustfs-ecstore rebalance --lib`. - - `cargo test -p rustfs-heal storage --lib`. - - `cargo test -p rustfs-scanner scanner_io --lib`. - - `./scripts/check_architecture_migration_rules.sh`. - - `./scripts/check_layer_dependencies.sh`. - - `./scripts/check_metrics_migration_refs.sh`. - - `./scripts/check_unsafe_code_allowances.sh`. - - `git diff --check`. - - Pre-push review: pending required quality/architecture, - migration-preservation, and testing/verification review. + - Completed seventh slice: `rustfs/rustfs#3337` migrated maintenance and + background read-side storage inventory consumers in rebalance metadata + initialization, heal resume disk lookup, and scanner local disk scan lookup. + - Completion acceptance: admin inventory consumers no longer use old + `StorageAPI` calls for backend info, storage info, local storage info, + drive-count, or disk-set inventory when the inventory-facing + `StorageAdminApi` contract represents the same read-only operation. + +- [~] `API-008` Remove duplicate old-path admin surfaces. + - Current branch slice: remove admin-read methods from the old `StorageAPI` + trait and its ECStore/Sets/SetDisks test implementations after API-007 + migrated their consumers. + - Acceptance: old `StorageAPI` keeps storage operation traits and + `new_ns_lock`, while admin inventory surfaces live only on + `StorageAdminApi`. + - Must preserve: object API trait bounds, namespace lock behavior, bucket, + object, list, multipart, and heal operations; config read/write byte shape; + dynamic storage-class lookup; object/rebalance object selection paths; + scanner cache persistence; replication/tier persistence; and storage hot + paths. + - Risk defense: do not remove `StorageAPI` itself, do not move object/config + persistence contracts, and only replace previous `SetDisks::get_disks(0, 0)` + trait calls with the same internal `disk_inventory()` data source that the + old impl returned. + - Verification: pending focused compile/tests, migration guards, Rust risk + scan, and required quality/architecture, migration-preservation, and + testing/verification review. ## Phase 8 Background Controller Tasks @@ -282,18 +280,21 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block | Expert | Status | Notes | |---|---|---| -| Quality/architecture | pass | Confirmed the diff is limited to maintenance/background inventory-reader entry-point migration plus accurate progress notes; dependency direction, naming, and scope are clean. | -| Migration preservation | pass | Confirmed old `StorageAPI` shape remains, ECStore old/new trait paths still delegate to the same storage-info and disk-inventory handlers, and rebalance/heal/scanner call sites only change the read-side entry point. | -| Testing/verification | pass | Confirmed the focused ECStore/heal/scanner checks, migration guards, diff hygiene, and added-line Rust quality scan are sufficient for this equivalent read-side call-path migration while skipping full pre-commit under the current instruction. | +| Quality/architecture | pass | Confirmed the old `StorageAPI` now keeps only operation traits and `new_ns_lock`, admin inventory surfaces remain on `StorageAdminApi`, dependency direction is clean, and scope/naming stay limited to API-008. | +| Migration preservation | pass | Confirmed object/bucket/list/multipart/heal contracts, namespace locks, config byte paths, dynamic storage-class behavior, rebalance selection, scanner cache, heal repair, replication/tier/config persistence, and storage hot paths are preserved. | +| Testing/verification | pass | Initially requested wider rebalance coverage; after `cargo test -p rustfs-ecstore rebalance --lib` passed, confirmed the focused matrix is sufficient for this larger dependency-migration slice while skipping full pre-commit per current instruction. | ## Verification Notes Passed: -- `cargo fmt --all && cargo fmt --all --check`. -- `cargo check -p rustfs-ecstore -p rustfs-heal -p rustfs-scanner`. +- `cargo fmt --all`. +- `cargo check -p rustfs-ecstore`. +- `cargo fmt --all --check`. +- `cargo check -p rustfs-storage-api -p rustfs-ecstore -p rustfs --lib`. +- `cargo test -p rustfs-ecstore config --lib`; 59 passed. +- `cargo test -p rustfs-ecstore store::rebalance --lib`; 19 passed. - `cargo test -p rustfs-ecstore rebalance --lib`; 198 passed. -- `cargo test -p rustfs-heal storage --lib`; 3 passed. -- `cargo test -p rustfs-scanner scanner_io --lib`; 18 passed. +- `cargo test -p rustfs-ecstore set_disk --lib`; 86 passed. - `./scripts/check_architecture_migration_rules.sh`. - `./scripts/check_layer_dependencies.sh`. - `./scripts/check_metrics_migration_refs.sh`. @@ -304,29 +305,29 @@ Passed: println/eprintln, and `Ordering::Relaxed`. Notes: -- Full pre-commit was intentionally skipped because the focused tests and guards - above passed, per the current migration instruction to increase PR granularity. -- The broad changed-file quality scan reports pre-existing test unwrap/expect - plus pre-existing casts and relaxed atomics in touched ECStore files; the +- Full pre-commit is intentionally skipped when the focused tests and guards + pass, per the current instruction to increase PR granularity. +- The broad changed-file quality scan reports pre-existing unwrap/cast matches + and `Result` business return values in touched ECStore files; the added-line scan found no new risky code patterns. -- Old `StorageAPI` trait shape and implementations remain in place; ECStore old - and new trait paths delegate to the same storage-info and disk-inventory - handlers. +- Old `StorageAPI` remains as the object/bucket/list/multipart/heal namespace + lock contract; only duplicate admin inventory methods are removed. +- Config dynamic storage-class lookup now reads drive counts through + `StorageAdminApi`. - Object/rebalance object selection paths, scanner cache persistence, heal object repair, object APIs, replication/config/tier persistence paths, and - storage hot paths are unchanged. + storage hot paths must remain unchanged. - No temporary compatibility shim was added. ## Handoff Notes -- Keep this API-007 slice as a maintenance/background inventory-reader - `consumer-migration` PR. -- The only scanner/heal scope in this PR is read-side disk lookup for scanner - local disk scan and heal resume; do not migrate scanner cache writes, heal - object repair, object APIs, replication, config/tier persistence, or storage - hot-path consumers in this PR. -- Do not remove `StorageAPI::get_disks` or route object/hot-path consumers - around it in this PR. -- Do not make the old `StorageAPI` trait inherit `StorageAdminApi` in this PR. +- Keep this API-008 slice as a `dependency-migration` PR that only removes + duplicate admin inventory methods from old `StorageAPI`. +- Do not remove `StorageAPI` itself, object operation traits, or + `new_ns_lock` in this PR. +- Do not migrate scanner cache writes, heal object repair, object APIs, + replication, config/tier persistence, or storage hot-path consumers in this + PR. +- Do not make the old `StorageAPI` trait inherit `StorageAdminApi`. - Do not add temporary compatibility code unless a matching `RUSTFS_COMPAT_TODO()` marker and cleanup-register entry are added.