From c26593fa7a4b55849e98832cde657c6d4b167262 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Tue, 16 Jun 2026 06:35:32 +0800 Subject: [PATCH] refactor: clean config storage boundaries (#3489) --- crates/ecstore/src/client/api_remove.rs | 2 +- crates/ecstore/src/config/com.rs | 280 +----------------- crates/ecstore/src/store_list_objects.rs | 4 +- .../ecstore-config-consumer-inventory.md | 10 +- docs/architecture/migration-progress.md | 48 +-- 5 files changed, 33 insertions(+), 311 deletions(-) diff --git a/crates/ecstore/src/client/api_remove.rs b/crates/ecstore/src/client/api_remove.rs index 27e453c9b..3f8ad7770 100644 --- a/crates/ecstore/src/client/api_remove.rs +++ b/crates/ecstore/src/client/api_remove.rs @@ -44,7 +44,7 @@ use crate::client::{ }; use crate::{ disk::DiskAPI, - store_api::{GetObjectReader, ObjectInfo, StorageAPI}, + store_api::{GetObjectReader, ObjectInfo}, }; use rustfs_utils::hash::EMPTY_STRING_SHA256_HASH; diff --git a/crates/ecstore/src/config/com.rs b/crates/ecstore/src/config/com.rs index 55c8539cd..670fdf022 100644 --- a/crates/ecstore/src/config/com.rs +++ b/crates/ecstore/src/config/com.rs @@ -1217,12 +1217,7 @@ mod tests { use crate::error::{Error, Result}; use crate::global::{is_dist_erasure, is_erasure, is_erasure_sd, update_erasure_type}; use crate::set_disk::SetDisks; - use crate::store_api::{ - BucketInfo, BucketOperations, BucketOptions, CompletePart, DeleteBucketOptions, DeletedObject, GetObjectReader, - HTTPRangeSpec, HealOperations, ListMultipartsInfo, ListObjectVersionsInfo, ListObjectsV2Info, ListOperations, - MakeBucketOptions, MultipartInfo, MultipartOperations, MultipartUploadResult, NamespaceLocking, ObjectIO, ObjectInfo, - ObjectOperations, ObjectOptions, ObjectToDelete, PartInfo, PutObjReader, StorageAPI, WalkOptions, - }; + use crate::store_api::{GetObjectReader, HTTPRangeSpec, NamespaceLocking, ObjectIO, ObjectInfo, ObjectOptions, PutObjReader}; use http::HeaderMap; use rustfs_config::audit::{AUDIT_AMQP_SUB_SYS, AUDIT_KAFKA_SUB_SYS, AUDIT_MQTT_SUB_SYS, AUDIT_WEBHOOK_SUB_SYS}; use rustfs_config::notify::{ @@ -1233,7 +1228,6 @@ mod tests { use rustfs_config::{ DEFAULT_DELIMITER, ENABLE_KEY, EnableState, MYSQL_DSN_STRING, MYSQL_MAX_OPEN_CONNECTIONS, MYSQL_QUEUE_DIR, MYSQL_TABLE, }; - use rustfs_filemeta::FileInfo; use rustfs_lock::client::LockClient; use rustfs_lock::client::local::LocalClient; use rustfs_lock::{LockError, LockInfo, LockResponse, LockStats}; @@ -1249,7 +1243,6 @@ mod tests { use time::OffsetDateTime; use tokio::io::{AsyncRead, ReadBuf}; use tokio::sync::RwLock; - use tokio_util::sync::CancellationToken; #[derive(Debug, Default)] struct FailingClient; @@ -1457,277 +1450,6 @@ mod tests { } } - #[async_trait::async_trait] - impl BucketOperations for LockingConfigStorage { - async fn make_bucket(&self, _bucket: &str, _opts: &MakeBucketOptions) -> Result<()> { - panic!("unused in test") - } - - async fn get_bucket_info(&self, _bucket: &str, _opts: &BucketOptions) -> Result { - panic!("unused in test") - } - - async fn list_bucket(&self, _opts: &BucketOptions) -> Result> { - panic!("unused in test") - } - - async fn delete_bucket(&self, _bucket: &str, _opts: &DeleteBucketOptions) -> Result<()> { - panic!("unused in test") - } - } - - #[async_trait::async_trait] - impl ObjectOperations for LockingConfigStorage { - async fn get_object_info(&self, _bucket: &str, _object: &str, _opts: &ObjectOptions) -> Result { - panic!("unused in test") - } - - async fn verify_object_integrity(&self, _bucket: &str, _object: &str, _opts: &ObjectOptions) -> Result<()> { - panic!("unused in test") - } - - async fn copy_object( - &self, - _src_bucket: &str, - _src_object: &str, - _dst_bucket: &str, - _dst_object: &str, - _src_info: &mut ObjectInfo, - _src_opts: &ObjectOptions, - _dst_opts: &ObjectOptions, - ) -> Result { - panic!("unused in test") - } - - async fn delete_object_version( - &self, - _bucket: &str, - _object: &str, - _fi: &FileInfo, - _force_del_marker: bool, - ) -> Result<()> { - panic!("unused in test") - } - - async fn delete_object(&self, _bucket: &str, _object: &str, _opts: ObjectOptions) -> Result { - panic!("unused in test") - } - - async fn delete_objects( - &self, - _bucket: &str, - _objects: Vec, - _opts: ObjectOptions, - ) -> (Vec, Vec>) { - panic!("unused in test") - } - - async fn put_object_metadata(&self, _bucket: &str, _object: &str, _opts: &ObjectOptions) -> Result { - panic!("unused in test") - } - - async fn get_object_tags(&self, _bucket: &str, _object: &str, _opts: &ObjectOptions) -> Result { - panic!("unused in test") - } - - async fn put_object_tags(&self, _bucket: &str, _object: &str, _tags: &str, _opts: &ObjectOptions) -> Result { - panic!("unused in test") - } - - async fn delete_object_tags(&self, _bucket: &str, _object: &str, _opts: &ObjectOptions) -> Result { - panic!("unused in test") - } - - async fn add_partial(&self, _bucket: &str, _object: &str, _version_id: &str) -> Result<()> { - panic!("unused in test") - } - - async fn transition_object(&self, _bucket: &str, _object: &str, _opts: &ObjectOptions) -> Result<()> { - panic!("unused in test") - } - - async fn restore_transitioned_object(self: Arc, _bucket: &str, _object: &str, _opts: &ObjectOptions) -> Result<()> { - panic!("unused in test") - } - } - - #[async_trait::async_trait] - impl ListOperations for LockingConfigStorage { - async fn list_objects_v2( - self: Arc, - _bucket: &str, - _prefix: &str, - _continuation_token: Option, - _delimiter: Option, - _max_keys: i32, - _fetch_owner: bool, - _start_after: Option, - _incl_deleted: bool, - ) -> Result { - panic!("unused in test") - } - - async fn list_object_versions( - self: Arc, - _bucket: &str, - _prefix: &str, - _marker: Option, - _version_marker: Option, - _delimiter: Option, - _max_keys: i32, - ) -> Result { - panic!("unused in test") - } - - async fn walk( - self: Arc, - _rx: CancellationToken, - _bucket: &str, - _prefix: &str, - _result: tokio::sync::mpsc::Sender, - _opts: WalkOptions, - ) -> Result<()> { - panic!("unused in test") - } - } - - #[async_trait::async_trait] - impl MultipartOperations for LockingConfigStorage { - async fn list_multipart_uploads( - &self, - _bucket: &str, - _prefix: &str, - _key_marker: Option, - _upload_id_marker: Option, - _delimiter: Option, - _max_uploads: usize, - ) -> Result { - panic!("unused in test") - } - - async fn new_multipart_upload( - &self, - _bucket: &str, - _object: &str, - _opts: &ObjectOptions, - ) -> Result { - panic!("unused in test") - } - - async fn copy_object_part( - &self, - _src_bucket: &str, - _src_object: &str, - _dst_bucket: &str, - _dst_object: &str, - _upload_id: &str, - _part_id: usize, - _start_offset: i64, - _length: i64, - _src_info: &ObjectInfo, - _src_opts: &ObjectOptions, - _dst_opts: &ObjectOptions, - ) -> Result<()> { - panic!("unused in test") - } - - async fn put_object_part( - &self, - _bucket: &str, - _object: &str, - _upload_id: &str, - _part_id: usize, - _data: &mut PutObjReader, - _opts: &ObjectOptions, - ) -> Result { - panic!("unused in test") - } - - async fn get_multipart_info( - &self, - _bucket: &str, - _object: &str, - _upload_id: &str, - _opts: &ObjectOptions, - ) -> Result { - panic!("unused in test") - } - - async fn list_object_parts( - &self, - _bucket: &str, - _object: &str, - _upload_id: &str, - _part_number_marker: Option, - _max_parts: usize, - _opts: &ObjectOptions, - ) -> Result { - panic!("unused in test") - } - - async fn abort_multipart_upload( - &self, - _bucket: &str, - _object: &str, - _upload_id: &str, - _opts: &ObjectOptions, - ) -> Result<()> { - panic!("unused in test") - } - - async fn complete_multipart_upload( - self: Arc, - _bucket: &str, - _object: &str, - _upload_id: &str, - _uploaded_parts: Vec, - _opts: &ObjectOptions, - ) -> Result { - panic!("unused in test") - } - } - - #[async_trait::async_trait] - impl HealOperations for LockingConfigStorage { - async fn heal_format(&self, _dry_run: bool) -> Result<(rustfs_madmin::heal_commands::HealResultItem, Option)> { - panic!("unused in test") - } - - async fn heal_bucket( - &self, - _bucket: &str, - _opts: &rustfs_common::heal_channel::HealOpts, - ) -> Result { - panic!("unused in test") - } - - async fn heal_object( - &self, - _bucket: &str, - _object: &str, - _version_id: &str, - _opts: &rustfs_common::heal_channel::HealOpts, - ) -> Result<(rustfs_madmin::heal_commands::HealResultItem, Option)> { - panic!("unused in test") - } - - async fn get_pool_and_set(&self, _id: &str) -> Result<(Option, Option, Option)> { - panic!("unused in test") - } - - async fn check_abandoned_parts( - &self, - _bucket: &str, - _object: &str, - _opts: &rustfs_common::heal_channel::HealOpts, - ) -> Result<()> { - panic!("unused in test") - } - } - - #[async_trait::async_trait] - impl StorageAPI for LockingConfigStorage {} - #[async_trait::async_trait] impl NamespaceLocking for LockingConfigStorage { async fn new_ns_lock(&self, bucket: &str, object: &str) -> Result { diff --git a/crates/ecstore/src/store_list_objects.rs b/crates/ecstore/src/store_list_objects.rs index b95374dd1..a35e7495c 100644 --- a/crates/ecstore/src/store_list_objects.rs +++ b/crates/ecstore/src/store_list_objects.rs @@ -1552,8 +1552,8 @@ fn get_quorum_disk_infos(disks: &[DiskStore], infos: &[DiskInfo], read_quorum: u for (i, info) in infos.iter().enumerate() { let mutations = info.metrics.total_deletes + info.metrics.total_writes; if mutations >= common_mutations { - new_disks.push(disks[i].clone()); // Assuming StorageAPI derives Clone - new_infos.push(infos[i].clone()); // Assuming DiskInfo derives Clone + new_disks.push(disks[i].clone()); + new_infos.push(infos[i].clone()); } } diff --git a/docs/architecture/ecstore-config-consumer-inventory.md b/docs/architecture/ecstore-config-consumer-inventory.md index c6f8e570e..c7b7e0cd3 100644 --- a/docs/architecture/ecstore-config-consumer-inventory.md +++ b/docs/architecture/ecstore-config-consumer-inventory.md @@ -80,7 +80,7 @@ behind narrower contracts. | `DEFAULT_KVS` | `crates/ecstore/src/config/mod.rs` | Registry for defaults across storage class, scanner, notify, audit, and OIDC. | Move defaults only after an explicit registration contract exists. | | `GLOBAL_SERVER_CONFIG` | `crates/ecstore/src/config/mod.rs` | Process-wide mutable server config snapshot. | Migrate readers behind `AppContext` or a server-config provider before changing storage. | | `ConfigSys::init` | `crates/ecstore/src/config/mod.rs` | Reads persisted config, looks up derived config, and stores the global snapshot. | Startup order must remain unchanged until the lifecycle contract owns this dependency. | -| `read_config_without_migrate` | `crates/ecstore/src/config/com.rs` | Loads persisted server config through `StorageAPI`. | Persistence stays in `ecstore` until pure model and persistence are separated. | +| `read_config_without_migrate` | `crates/ecstore/src/config/com.rs` | Loads persisted server config through ECStore-owned object I/O and storage-admin contracts. | Persistence stays in `ecstore` until pure model and persistence are separated. | | `save_server_config` | `crates/ecstore/src/config/com.rs` | Persists the canonical server config object. | Preserve external object shape and config-history behavior. | | `get_global_server_config` / `set_global_server_config` | `crates/ecstore/src/config/mod.rs` | Clone/read and replace the global server-config snapshot. | Do not remove until all runtime readers have an injected provider path. | @@ -91,7 +91,7 @@ behind narrower contracts. | Files | Current usage | |---|---| | `crates/ecstore/src/config/mod.rs` | Defines `KV`, `KVS`, `Config`, defaults, global snapshot, initialization, and tests. | -| `crates/ecstore/src/config/com.rs` | Encodes, decodes, reads, writes, creates, and normalizes server config objects through `StorageAPI`. | +| `crates/ecstore/src/config/com.rs` | Encodes, decodes, reads, writes, creates, and normalizes server config objects through ECStore-local persistence helpers. | | `crates/ecstore/src/config/{notify,audit,oidc,scanner,storageclass}.rs` | Register default `KVS` values and subsystem-specific parsing helpers. | | `crates/ecstore/src/store.rs` | Exposes store-level server-config accessors that delegate to the global config snapshot. | @@ -162,7 +162,7 @@ behind narrower contracts. | `KVS` is the effective target config carrier | Notify, audit, and target factories consume `KVS` after file/env merge. | Keep `KVS` API stable until target descriptor and runtime crates are behind a shared contract. | | `DEFAULT_KVS` registration is global | Defaults are initialized centrally and used by admin validation/rendering. | Add a registration contract before changing initialization order. | | Global snapshot readers still exist | Server, admin, IAM, scanner, and site-replication paths can still read global config. | Migrate readers through `AppContext`/provider paths in small steps after the model contract is stable. | -| Persistence helpers depend on `StorageAPI` | Moving them with the pure model would pull storage implementation dependencies upward. | Keep read/write helpers in `ecstore` until a storage-facing persistence contract is explicit. | +| Persistence helpers depend on ECStore storage contracts | Moving them with the pure model would pull storage implementation dependencies upward. | Keep read/write helpers in `ecstore` until a storage-facing persistence contract is explicit. | ## Recommended Migration Order @@ -176,8 +176,8 @@ behind narrower contracts. behavior, and representative admin config rendering paths. 5. Migrate global `Config` readers behind `ServerConfigInterface` or a narrower provider in small PRs. -6. Move persistence helpers only after `StorageAPI` dependencies can stay below - the model contract. +6. Move persistence helpers only after object-I/O and storage-admin dependencies + can stay below the model contract. 7. Evaluate crate split only after consumers no longer need old paths except explicit `RUSTFS_COMPAT_TODO()` compatibility shims. diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index 321c10336..fb4819a72 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-namespace-lock-large-cleanup` -- Baseline: `origin/main` at `49c0f131205035d125271bb5b87db5b0f5bc2a6d` +- Branch: `overtrue/arch-config-storage-boundary-cleanup` +- Baseline: `origin/main` at `fbab160c2b09075f5e2503a669d82917ef82d40e` - PR type for this branch: `consumer-migration` - Runtime behavior changes: no external behavior change expected. -- Rust code changes: narrow replication pool, resync, delete replication, and - object replication storage bounds away from full `StorageAPI` to their actual - object I/O, object operation, list, and namespace-lock capabilities. +- Rust code changes: remove stale full `StorageAPI` coupling from config + persistence tests, an unused S3 remove-client import, and an obsolete storage + list comment. - CI/script changes: none. -- Docs changes: record the current `API-012` consumer cleanup slice and its +- Docs changes: refresh the ECStore config persistence inventory and current verification state. ## Phase 0 Tasks @@ -419,6 +419,9 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block full `StorageAPI` when the helper only needs `ObjectIO`, `ObjectOperations`, `BucketOperations`, `ListOperations`, or `StorageAdminApi`. + - Cleanup slice: remove stale full `StorageAPI` dependencies from config + persistence test support after the server-config persistence helpers moved + to their actual object I/O and storage-admin bounds. - Acceptance: metadata helper contracts express the actual operation group they need, while callers and persistence behavior remain unchanged. @@ -463,10 +466,10 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block catalog backend and rebalance metadata helper consumers away from full `StorageAPI` where they only need object I/O, object operations, list operations, and namespace locking. - - Current cleanup slice: narrow replication pool, resync leader-lock, delete - replication, object replication, and multipart replication helpers away - from full `StorageAPI` where they only need object I/O, object operations, - list operations, and namespace locking. + - Completed follow-up slice: `rustfs/rustfs#3485` narrowed replication pool, + resync leader-lock, delete replication, object replication, and multipart + replication helpers away from full `StorageAPI` where they only need object + I/O, object operations, list operations, and namespace locking. - Acceptance: table catalog object backend contracts express the actual object read/write, metadata/delete, list, and namespace-lock capabilities they need; namespace-lock consumers depend on `NamespaceLocking` instead of @@ -759,18 +762,16 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block | Expert | Status | Notes | |---|---|---| -| Quality/architecture | passed | Replication consumers now depend on a local replication storage capability boundary plus object-I/O-only helper bounds, without changing replication method bodies. | -| Migration preservation | passed | Resync leader locks, per-object replication locks, delete replication, object reader flow, multipart upload flow, MRF recovery, and replication scheduling semantics are unchanged. | -| Testing/verification | passed | Focused replication tests, storage API compatibility test, compile checks, migration/layer guards, formatting, diff hygiene, Rust risk scan, and full `make pre-commit` passed. | +| Quality/architecture | passed | Config persistence tests now express their actual object I/O, namespace-lock, and storage-admin requirements instead of a full storage facade. | +| Migration preservation | passed | Config object encoding/decoding, metadata reads, namespace-lock behavior, and S3 remove-client behavior are unchanged. | +| Testing/verification | passed | Focused config tests, compile checks, migration/layer guards, formatting, diff hygiene, Rust risk scan, and full `make pre-commit` passed. | ## Verification Notes -Passed on `49c0f131205035d125271bb5b87db5b0f5bc2a6d`: +Passed on `fbab160c2b09075f5e2503a669d82917ef82d40e`: - `cargo check -p rustfs-ecstore`: passed. -- `cargo test -p rustfs-ecstore bucket::replication --no-fail-fast`: passed. -- `cargo test -p rustfs-ecstore --test storage_api_compat_test --no-fail-fast`: - passed. +- `cargo test -p rustfs-ecstore config::com --no-fail-fast`: passed. - `cargo check -p rustfs -p rustfs-ecstore`: passed. - `./scripts/check_architecture_migration_rules.sh`: passed. - `./scripts/check_layer_dependencies.sh`: passed. @@ -783,17 +784,16 @@ Passed on `49c0f131205035d125271bb5b87db5b0f5bc2a6d`: Notes: -- This slice keeps the existing replication method bodies unchanged while - narrowing the generic storage capabilities they require. -- Replication storage still depends on object I/O, object metadata/delete - operations, bucket walking, and namespace locking; multipart replication - helpers only need object reader access to stream source ranges. +- This slice removes test-only full facade scaffolding after config persistence + helpers already moved to narrower object I/O and storage-admin contracts. +- The S3 remove client had a stale full facade import only; behavior remains + unchanged. - The slice does not remove the full storage facade or move traits across crate boundaries. ## Handoff Notes -- API-012 replication storage cleanup is locally verified and current with +- Config storage boundary cleanup is locally verified and current with `origin/main`. -- Remaining namespace-lock cleanup can continue by migrating other consumers +- Remaining storage-facade cleanup can continue by migrating other consumers that no longer need the full storage facade.