From 686068d8bd2b3f9e19852b0d887cbf8bb1052572 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 12:04:02 +0800 Subject: [PATCH] refactor(storage): narrow metadata bounds (#3343) --- crates/ecstore/src/bucket/migration.rs | 25 +++--- crates/ecstore/src/config/com.rs | 35 ++++---- crates/ecstore/src/rebalance.rs | 8 +- crates/ecstore/src/tier/tier.rs | 20 +++-- docs/architecture/migration-progress.md | 114 ++++++++++++------------ 5 files changed, 108 insertions(+), 94 deletions(-) diff --git a/crates/ecstore/src/bucket/migration.rs b/crates/ecstore/src/bucket/migration.rs index 1a91c7967..3249b0d17 100644 --- a/crates/ecstore/src/bucket/migration.rs +++ b/crates/ecstore/src/bucket/migration.rs @@ -17,7 +17,9 @@ use crate::bucket::metadata::BUCKET_METADATA_FILE; use crate::bucket::replication::{decode_resync_file, encode_resync_file}; use crate::disk::{BUCKET_META_PREFIX, MIGRATING_META_BUCKET, RUSTFS_META_BUCKET}; -use crate::store_api::{BucketOptions, ObjectOptions, PutObjReader, StorageAPI}; +use crate::store_api::{ + BucketOperations, BucketOptions, ListOperations, ObjectIO, ObjectOperations, ObjectOptions, PutObjReader, +}; use http::HeaderMap; use rustfs_policy::auth::UserIdentity; use rustfs_policy::policy::PolicyDoc; @@ -176,7 +178,10 @@ fn normalize_bucket_meta_blob(path: &str, data: &[u8]) -> std::result::Result(store: Arc) { +pub async fn try_migrate_bucket_metadata(store: Arc) +where + S: BucketOperations + ObjectIO + ObjectOperations, +{ let buckets_list = match store .list_bucket(&BucketOptions { no_metadata: true, @@ -218,13 +223,10 @@ pub async fn try_migrate_bucket_metadata(store: Arc) { } } -async fn migrate_one_if_missing( - store: Arc, - opts: &ObjectOptions, - headers: &HeaderMap, - path: &str, - label: &str, -) { +async fn migrate_one_if_missing(store: Arc, opts: &ObjectOptions, headers: &HeaderMap, path: &str, label: &str) +where + S: ObjectIO + ObjectOperations, +{ if store .get_object_info(RUSTFS_META_BUCKET, path, &ObjectOptions::default()) .await @@ -275,7 +277,10 @@ async fn migrate_one_if_missing( /// Lists all objects under the IAM prefix in the source, copies each to the target if not present. /// Skips objects that already exist in RustFS (idempotent). /// If list_objects_v2 on the legacy bucket fails (e.g. format differs), migration is skipped. -pub async fn try_migrate_iam_config(store: Arc) { +pub async fn try_migrate_iam_config(store: Arc) +where + S: ListOperations + ObjectIO + ObjectOperations, +{ let opts = ObjectOptions { max_parity: true, no_lock: true, diff --git a/crates/ecstore/src/config/com.rs b/crates/ecstore/src/config/com.rs index b2267b61f..19e343aef 100644 --- a/crates/ecstore/src/config/com.rs +++ b/crates/ecstore/src/config/com.rs @@ -16,7 +16,7 @@ use crate::config::{Config, KVS, audit, notify, oidc, set_global_storage_class, use crate::disk::{MIGRATING_META_BUCKET, RUSTFS_META_BUCKET}; use crate::error::{Error, Result}; use crate::global::is_first_cluster_node_local; -use crate::store_api::{ObjectInfo, ObjectOptions, PutObjReader, StorageAPI}; +use crate::store_api::{ObjectIO, ObjectInfo, ObjectOperations, ObjectOptions, PutObjReader}; use http::HeaderMap; use rustfs_config::audit::{ AUDIT_AMQP_KEYS, AUDIT_AMQP_SUB_SYS, AUDIT_KAFKA_KEYS, AUDIT_KAFKA_SUB_SYS, AUDIT_MQTT_KEYS, AUDIT_MQTT_SUB_SYS, @@ -181,12 +181,12 @@ fn audit_target_descriptors() -> [TargetConfigDescriptor; 9] { } #[instrument(skip(api))] -pub async fn read_config(api: Arc, file: &str) -> Result> { +pub async fn read_config(api: Arc, file: &str) -> Result> { let (data, _obj) = read_config_with_metadata(api, file, &ObjectOptions::default()).await?; Ok(data) } -pub async fn read_config_no_lock(api: Arc, file: &str) -> Result> { +pub async fn read_config_no_lock(api: Arc, file: &str) -> Result> { let (data, _obj) = read_config_with_metadata( api, file, @@ -199,7 +199,7 @@ pub async fn read_config_no_lock(api: Arc, file: &str) -> Resu Ok(data) } -pub async fn read_config_with_metadata( +pub async fn read_config_with_metadata( api: Arc, file: &str, opts: &ObjectOptions, @@ -227,7 +227,7 @@ pub async fn read_config_with_metadata( } #[instrument(skip(api, data))] -pub async fn save_config(api: Arc, file: &str, data: Vec) -> Result<()> { +pub async fn save_config(api: Arc, file: &str, data: Vec) -> Result<()> { save_config_with_opts( api, file, @@ -241,7 +241,7 @@ pub async fn save_config(api: Arc, file: &str, data: Vec) } #[instrument(skip(api))] -pub async fn delete_config(api: Arc, file: &str) -> Result<()> { +pub async fn delete_config(api: Arc, file: &str) -> Result<()> { match api .delete_object( RUSTFS_META_BUCKET, @@ -265,7 +265,7 @@ pub async fn delete_config(api: Arc, file: &str) -> Result<()> } } -pub async fn save_config_with_opts(api: Arc, file: &str, data: Vec, opts: &ObjectOptions) -> Result<()> { +pub async fn save_config_with_opts(api: Arc, file: &str, data: Vec, opts: &ObjectOptions) -> Result<()> { let mut put_data = PutObjReader::from_vec(data); if let Err(err) = api.put_object(RUSTFS_META_BUCKET, file, &mut put_data, opts).await { error!("save_config_with_opts: err: {:?}, file: {}", err, file); @@ -280,7 +280,7 @@ fn new_server_config() -> Config { async fn new_and_save_server_config(api: Arc) -> Result where - S: StorageAPI + StorageAdminApi, + S: ObjectIO + StorageAdminApi, { let mut cfg = new_server_config(); lookup_configs(&mut cfg, api.clone()).await; @@ -982,7 +982,10 @@ fn is_object_not_found(err: &Error) -> bool { *err == Error::FileNotFound || matches!(err, Error::ObjectNotFound(_, _) | Error::BucketNotFound(_)) } -pub async fn try_migrate_server_config(api: Arc) { +pub async fn try_migrate_server_config(api: Arc) +where + S: ObjectIO + ObjectOperations, +{ let config_file = get_config_file(); match api .get_object_info( @@ -1065,7 +1068,7 @@ 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 where - S: StorageAPI + StorageAdminApi, + S: ObjectIO + StorageAdminApi, { warn!("Configuration not found ({}): Start initializing new configuration", context); let cfg = if is_first_cluster_node_local().await { @@ -1087,7 +1090,7 @@ fn handle_config_read_error(err: Error, file_path: &str) -> Result { pub async fn read_config_without_migrate(api: Arc) -> Result where - S: StorageAPI + StorageAdminApi, + S: ObjectIO + StorageAdminApi, { let config_file = get_config_file(); @@ -1101,7 +1104,7 @@ where async fn read_server_config(api: Arc, data: &[u8]) -> Result where - S: StorageAPI + StorageAdminApi, + S: ObjectIO + StorageAdminApi, { // If the provided data is empty, try to read from the file again if data.is_empty() { @@ -1125,7 +1128,7 @@ where Ok(cfg.merge()) } -pub async fn save_server_config(api: Arc, cfg: &Config) -> Result<()> { +pub async fn save_server_config(api: Arc, cfg: &Config) -> Result<()> { let config_file = get_config_file(); let existing = match read_config(api.clone(), &config_file).await { Ok(v) => Some(v), @@ -1156,7 +1159,7 @@ pub async fn save_server_config(api: Arc, cfg: &Config) -> Res pub async fn lookup_configs(cfg: &mut Config, api: Arc) where - S: StorageAPI + StorageAdminApi, + S: StorageAdminApi, { // TODO: from etcd if let Err(err) = apply_dynamic_config(cfg, api).await { @@ -1166,7 +1169,7 @@ where async fn apply_dynamic_config(cfg: &mut Config, api: Arc) -> Result<()> where - S: StorageAPI + StorageAdminApi, + S: StorageAdminApi, { for key in SUB_SYSTEMS_DYNAMIC.iter() { apply_dynamic_config_for_sub_sys(cfg, api.clone(), key).await?; @@ -1177,7 +1180,7 @@ where async fn apply_dynamic_config_for_sub_sys(cfg: &mut Config, api: Arc, subsys: &str) -> Result<()> where - S: StorageAPI + StorageAdminApi, + S: StorageAdminApi, { let set_drive_counts = StorageAdminApi::set_drive_counts(api.as_ref()); if subsys == STORAGE_CLASS_SUB_SYS { diff --git a/crates/ecstore/src/rebalance.rs b/crates/ecstore/src/rebalance.rs index 9f7d32068..14c94f1b6 100644 --- a/crates/ecstore/src/rebalance.rs +++ b/crates/ecstore/src/rebalance.rs @@ -566,11 +566,11 @@ impl RebalanceMeta { pub fn new() -> Self { Self::default() } - pub async fn load(&mut self, store: Arc) -> Result<()> { + pub async fn load(&mut self, store: Arc) -> Result<()> { self.load_with_opts(store, ObjectOptions::default()).await } - pub async fn load_with_opts(&mut self, store: Arc, opts: ObjectOptions) -> Result<()> { + pub async fn load_with_opts(&mut self, store: Arc, opts: ObjectOptions) -> Result<()> { let (data, _) = read_config_with_metadata(store, REBAL_META_NAME, &opts).await?; if data.is_empty() { info!("rebalanceMeta load_with_opts: no data"); @@ -599,11 +599,11 @@ impl RebalanceMeta { Ok(()) } - pub async fn save(&self, store: Arc) -> Result<()> { + pub async fn save(&self, store: Arc) -> Result<()> { self.save_with_opts(store, ObjectOptions::default()).await } - pub async fn save_with_opts(&self, store: Arc, opts: ObjectOptions) -> Result<()> { + pub async fn save_with_opts(&self, store: Arc, opts: ObjectOptions) -> Result<()> { if self.pool_stats.is_empty() { info!("rebalanceMeta save_with_opts: no pool stats"); return Ok(()); diff --git a/crates/ecstore/src/tier/tier.rs b/crates/ecstore/src/tier/tier.rs index 660f0f5d6..e5c2b2674 100644 --- a/crates/ecstore/src/tier/tier.rs +++ b/crates/ecstore/src/tier/tier.rs @@ -46,12 +46,11 @@ use crate::tier::{ warm_backend::{check_warm_backend, new_warm_backend}, }; use crate::{ - StorageAPI, config::com::{CONFIG_PREFIX, read_config}, disk::{MIGRATING_META_BUCKET, RUSTFS_META_BUCKET}, global::is_first_cluster_node_local, store::ECStore, - store_api::{ObjectIO as _, ObjectOptions, PutObjReader}, + store_api::{ObjectIO, ObjectOperations, ObjectOptions, PutObjReader}, }; use rustfs_rio::HashReader; use rustfs_utils::path::{SLASH_SEPARATOR, path_join}; @@ -1033,14 +1032,14 @@ impl TierConfigMgr { self.save_tiering_config(api).await } - pub async fn save_tiering_config(&self, api: Arc) -> std::result::Result<(), std::io::Error> { + pub async fn save_tiering_config(&self, api: Arc) -> std::result::Result<(), std::io::Error> { let data = encode_external_tiering_config_blob(self)?; let config_file = tier_config_path(TIER_CONFIG_FILE); self.save_config(api, &config_file, data).await } - pub async fn save_config( + pub async fn save_config( &self, api: Arc, file: &str, @@ -1058,7 +1057,7 @@ impl TierConfigMgr { .await } - pub async fn save_config_with_opts( + pub async fn save_config_with_opts( &self, api: Arc, file: &str, @@ -1100,7 +1099,7 @@ impl TierConfigMgr { } } -async fn new_and_save_tiering_config(api: Arc) -> Result { +async fn new_and_save_tiering_config(api: Arc) -> Result { let mut cfg = TierConfigMgr { driver_cache: HashMap::new(), tiers: HashMap::new(), @@ -1159,7 +1158,7 @@ async fn load_tier_config(api: Arc) -> std::result::Result( +async fn read_tier_config_from_bucket( api: Arc, bucket: &str, path: &str, @@ -1177,7 +1176,7 @@ async fn read_tier_config_from_bucket( Ok(Some(data)) } -async fn write_tier_config_to_rustfs(api: Arc, path: &str, data: Bytes) -> io::Result<()> { +async fn write_tier_config_to_rustfs(api: Arc, path: &str, data: Bytes) -> io::Result<()> { let mut put_data = PutObjReader::from_vec(data.to_vec()); api.put_object( RUSTFS_META_BUCKET, @@ -1193,7 +1192,10 @@ async fn write_tier_config_to_rustfs(api: Arc, path: &str, dat Ok(()) } -pub async fn try_migrate_tiering_config(api: Arc) { +pub async fn try_migrate_tiering_config(api: Arc) +where + S: ObjectIO + ObjectOperations, +{ let target_path = tier_config_path(TIER_CONFIG_FILE); if api .get_object_info( diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index c27d94436..47199ba55 100644 --- a/docs/architecture/migration-progress.md +++ b/docs/architecture/migration-progress.md @@ -5,15 +5,15 @@ 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-api-next-contracts` -- Baseline: `origin/main` at `87968275a776362e9b3734eac5bd9e3233d6cbe9` +- Branch: `overtrue/arch-storage-api-bound-narrowing` +- Baseline: `origin/main` at `6b86e3875c4912f3a7c27915b0bbd43399402847` - PR type for this branch: `dependency-migration` - Runtime behavior changes: none. -- Rust code changes: remove duplicate admin-read methods from the old - `StorageAPI` trait after all admin inventory consumers have moved to - `StorageAdminApi`. +- Rust code changes: narrow metadata helper generic bounds away from full + `StorageAPI` where helpers only need object I/O, object metadata/delete, + bucket listing, object listing, or `StorageAdminApi`. - CI/script changes: none. -- Docs changes: record API-007 completion and the current admin surface cleanup +- Docs changes: record API-008 completion and the current bound-narrowing context, verification evidence, and expert review outcomes. ## Phase 0 Tasks @@ -218,25 +218,33 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block 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. +- [x] `API-008` Remove duplicate old-path admin surfaces. + - Completed slice: `rustfs/rustfs#3340` removed duplicate 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. + +- [~] `API-009` Narrow metadata helper storage bounds. + - Current branch slice: narrow server config, tier config, rebalance + metadata, and startup metadata migration helper bounds away from full + `StorageAPI` when the helper only needs `ObjectIO`, `ObjectOperations`, + `BucketOperations`, `ListOperations`, or `StorageAdminApi`. + - Acceptance: metadata helper contracts express the actual operation group + they need, while callers and persistence behavior remain unchanged. + - Must preserve: config/tier/rebalance serialized byte shapes, read/write + object names, legacy bucket/IAM migration normalization and skip behavior, + lock behavior around rebalance metadata merge, dynamic storage-class lookup, + object APIs, scanner cache persistence, heal repair, replication + persistence, and storage hot paths. + - Risk defense: do not move traits to `rustfs-storage-api`, do not remove + `StorageAPI`, do not alter helper bodies or storage options, and keep + `save_rebalance_meta_with_merge` on full `StorageAPI` because it still needs + `new_ns_lock`. + - Verification: focused compile/tests, migration guards, Rust risk scan, and + required quality/architecture, migration-preservation, and + testing/verification review passed before push. ## Phase 8 Background Controller Tasks @@ -261,28 +269,26 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Next PRs -1. `consumer-migration`: migrate the remaining readiness/admin/capacity - consumers to the inventory-facing admin contract one group at a time. -2. `dependency-migration`: remove duplicate old-path admin surfaces only after - consumer migration proves equivalent behavior. -3. `api-extraction`: move only the pure server-config model into +1. `dependency-migration`: narrow metadata/helper bounds away from full + `StorageAPI` where the helper only needs a specific operation trait. +2. `api-extraction`: move only the pure server-config model into rustfs-config as CFG-003. -4. `api-extraction`: keep the old rustfs_ecstore::config::* path with +3. `api-extraction`: keep the old rustfs_ecstore::config::* path with RUSTFS_COMPAT_TODO(CFG-004) and cleanup-register coverage. -5. `consumer-migration`: migrate external consumers one group at a time only +4. `consumer-migration`: migrate external consumers one group at a time only after the model path and compatibility shim are stable. -6. `security-change`: make Local KMS unsafe defaults explicit development +5. `security-change`: make Local KMS unsafe defaults explicit development opt-ins or production failures in KMSD-002. -7. `security-change`: make Vault unsafe defaults explicit development opt-ins +6. `security-change`: make Vault unsafe defaults explicit development opt-ins or production failures in KMSD-003. ## Pre-Push Review Log | Expert | Status | Notes | |---|---|---| -| 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. | +| Quality/architecture | pass | Confirmed the Rust diff only changes imports/generic bounds, the new operation-group bounds match actual calls, `StorageAPI` and `new_ns_lock` remain where needed, and no over-abstraction, naming regression, dependency reversal, or hot-path intrusion was found. | +| Migration preservation | pass | Confirmed config/tier/rebalance byte shapes, object names, buckets, options, bucket/IAM normalization and skip behavior, dynamic storage-class lookup, and rebalance metadata merge locking remain unchanged. | +| Testing/verification | pass | Confirmed the focused compile, config/tier/rebalance/bucket migration tests, migration guards, diff hygiene, and added-line Rust risk scan are sufficient; full pre-commit can be skipped under the current larger-granularity instruction. | ## Verification Notes @@ -292,42 +298,40 @@ Passed: - `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 tier --lib`; 27 passed. - `cargo test -p rustfs-ecstore rebalance --lib`; 198 passed. -- `cargo test -p rustfs-ecstore set_disk --lib`; 86 passed. +- `cargo test -p rustfs-ecstore bucket::migration --lib`; 2 passed. - `./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`. -- Rust code-quality scan on changed `.rs` files, plus added-line scan for - unwrap/expect, numeric casts, `Result<_, String>`, `Box`, - println/eprintln, and `Ordering::Relaxed`. +- Rust code-quality scan on changed `.rs` files: broad full-file matches are + pre-existing test/business-code patterns; added-line scan found no new + unwrap/expect, numeric cast, `Result<_, String>`, `Box`, + println/eprintln, or `Ordering::Relaxed`. Notes: - 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` 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 must remain unchanged. +- This slice changes generic bounds and imports only; helper bodies, storage + options, object names, and metadata normalization logic are unchanged. +- `save_rebalance_meta_with_merge` intentionally still requires full + `StorageAPI` because it uses `new_ns_lock`. - No temporary compatibility shim was added. ## Handoff Notes -- Keep this API-008 slice as a `dependency-migration` PR that only removes - duplicate admin inventory methods from old `StorageAPI`. +- Keep this API-009 slice as a `dependency-migration` PR that only narrows + metadata/helper generic bounds for config, tier, rebalance, and startup + metadata migration helpers. - 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 move traits into `rustfs-storage-api` or introduce new compatibility + shims in this PR. +- Do not alter config/tier/rebalance serialized byte shapes, legacy bucket/IAM + migration normalization, object names, storage options, scanner cache writes, + heal object repair, object APIs, replication persistence, or storage hot-path + consumers in this PR. - Do not add temporary compatibility code unless a matching `RUSTFS_COMPAT_TODO()` marker and cleanup-register entry are added.