Mongo | Remove mongo_pool code(1/2) #9149
Conversation
WalkthroughThis change set removes all support and logic related to "mongo pools" (internal MongoDB-backed storage pools) across the codebase. It eliminates configuration, schema definitions, API endpoints, error handling, and all code branches involving mongo pools. The system now exclusively supports "cloud pools" (filesystem/hosts-backed pools), simplifying pool management, agent handling, account and bucket operations, and related services. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant PoolServer
participant Agent
participant Storage
Client->>API: Create pool / agent / bucket
API->>PoolServer: Create pool (only HOSTS/cloud)
PoolServer->>Storage: Setup cloud_pool_info
API->>Agent: Create agent (cloud_pool_info only)
Agent->>Storage: Store agent info (cloud path)
API->>API: All internal mongo pool logic is removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (17)
💤 Files with no reviewable changes (7)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
naveenpaul1
force-pushed
the
remove-mongo-code
branch
5 times, most recently
from
697ec90 to
29fcf33
Compare
naveenpaul1
requested review from
dannyzaken,
shirady,
liranmauda,
jackyalbo and
tangledbytes
and removed request for
shirady
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (6)
src/server/node_services/nodes_monitor.js (2)
858-858: Remove remaining MongoDB node reference.This line still checks for
item.node.is_mongo_node, which should have been removed as part of the MongoDB elimination. Since MongoDB support is being completely removed, this condition should be updated.- if (!item.node_from_store && (item.node.is_mongo_node || item.node.is_cloud_node)) { + if (!item.node_from_store && item.node.is_cloud_node) {
1336-1340: Remove MongoDB N2N configuration block.This entire code block configures N2N settings for MongoDB nodes, which should be completely removed since MongoDB support has been eliminated from the codebase.
- if (item.node.is_mongo_node) { - n2n_config.tcp_permanent_passive = { - port: config.MONGO_AGENTS_N2N_PORT - }; - }src/agent/agent.js (3)
463-469: Fix typo in error messages: "cloud pool pool"The error messages on lines 465 and 475 contain a typo - "cloud pool pool" should be "cloud pool".
- dbg.error(`shouldn't be here. found duplicated node for cloud pool or mongo pool!!`); + dbg.error(`shouldn't be here. found duplicated node for cloud pool!!`);- dbg.error(`shouldn't be here. node not found for cloud pool pool!!`); + dbg.error(`shouldn't be here. node not found for cloud pool!!`);Also applies to: 474-477
794-796: Remove remaining MongoDB references.These lines still reference
mongo_infowhich should be removed as part of the MongoDB cleanup.- if (this.mongo_info && this.mongo_info.pool_name) { - reply.pool_name = this.mongo_info.pool_name; - }
1-1092: Remove remaining MongoDB references across the codebaseOur search found numerous MongoDB‐related entries that must be cleaned up to complete the removal:
• config.js
– MONGO_DEFAULTS, MONGO_AGENTS_N2N_PORT, CFG_DB_PATH, etc.
• src/util/
– mongo_client.js, mongo_utils.js
– postgres_client.js (mongo_query-to-postgres usage)
• src/server/system_services/
– system_server.js, stats_aggregator.js, pool_server.js, replication_store.js, cluster_server.js
• src/server/node_services/
– nodes_store.js, nodes_client.js, nodes_aggregator.js, nodes_monitor.js
• src/server/notifications/alerts_log_store.js
• src/object_services/ & bg_services/
– md_store.js, map_builder.js, mirror_writer.js, md_aggregator.js
• src/api/
– system_api.js, pool_api.js, node_api.js, common_api.js
• hosted_agents/hosted_agents.js
• sdk/bucketspace_fs.js
• cmd/manage_nsfs.js, tools/**/*.jsPlease remove or refactor all of these references so that MongoDB is fully eliminated from the codebase.
src/server/system_services/account_server.js (1)
379-383: Filter predicate is missing.The
_.filtercall on line 379 doesn't have a predicate function, which means it will return all pools. This appears to be incomplete - you likely need to filter out specific pools (perhaps the 'backingstores' pool?).- const pools = _.filter(req.system.pools_by_name); + const pools = _.filter(req.system.pools_by_name, pool => pool.name !== 'backingstores');
🧹 Nitpick comments (3)
src/server/system_services/account_server.js (1)
88-93: LGTM! Consider using optional chaining for cleaner code.The change correctly replaces the mongo pool with 'backingstores' as the default resource. However, the code could be simplified using optional chaining as suggested by static analysis.
- const resource = req.rpc_params.default_resource ? - req.system.pools_by_name[req.rpc_params.default_resource] || - (req.system.namespace_resources_by_name && req.system.namespace_resources_by_name[req.rpc_params.default_resource]) : - req.system.pools_by_name.backingstores; + const resource = req.rpc_params.default_resource ? + req.system.pools_by_name[req.rpc_params.default_resource] || + req.system.namespace_resources_by_name?.[req.rpc_params.default_resource] : + req.system.pools_by_name.backingstores;src/server/system_services/bucket_server.js (2)
330-334: LGTM! Consider using optional chaining.The function correctly removes mongo pool validation. Consider using optional chaining as suggested by static analysis for cleaner code.
- if (!(default_pool && default_pool._id)) throw new RpcError('SERVICE_UNAVAILABLE', 'Non existing pool'); + if (!default_pool?._id) throw new RpcError('SERVICE_UNAVAILABLE', 'Non existing pool');
1660-1660: Consider removing the unused metric entirely.Since internal storage is no longer supported, hardcoding
is_using_internal: falseis correct but not ideal. Consider removing this metric entirely from the return object if it's no longer relevant.return { - is_using_internal: false, has_any_pool_configured, has_enough_healthy_nodes_for_tiering, // ... rest of the metricsAlso update any code that references this metric, such as the
return_bucket_issues_modefunction on line 1732.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
config.js(1 hunks)src/agent/agent.js(2 hunks)src/agent/block_store_services/block_store_mongo.js(0 hunks)src/api/api.js(0 hunks)src/api/hosted_agents_api.js(0 hunks)src/api/object_api.js(0 hunks)src/api/pool_api.js(0 hunks)src/api/server_inter_process_api.js(0 hunks)src/deploy/NVA_build/common_funcs.sh(0 hunks)src/deploy/NVA_build/set_mongo_repo.sh(0 hunks)src/hosted_agents/hosted_agents.js(4 hunks)src/sdk/map_api_types.js(0 hunks)src/server/common_services/server_inter_process.js(0 hunks)src/server/mongo_services/mongo_monitor.js(0 hunks)src/server/node_services/node_allocator.js(0 hunks)src/server/node_services/nodes_monitor.js(2 hunks)src/server/object_services/map_db_types.js(0 hunks)src/server/object_services/mapper.js(2 hunks)src/server/server_rpc.js(0 hunks)src/server/system_services/account_server.js(3 hunks)src/server/system_services/bucket_server.js(4 hunks)src/server/system_services/pool_server.js(2 hunks)src/server/system_services/schemas/pool_schema.js(0 hunks)src/server/system_services/stats_aggregator.js(0 hunks)src/server/system_services/system_server.js(2 hunks)src/test/framework/report.js(0 hunks)src/tools/gridfs_stress.js(0 hunks)src/tools/mongo_md_stats.js(0 hunks)src/tools/mongo_profiler.js(0 hunks)src/tools/mongodb_blow.js(0 hunks)src/tools/mongodb_bucket_blow.js(0 hunks)src/util/mongo_utils.js(0 hunks)
🧠 Learnings (3)
src/hosted_agents/hosted_agents.js (1)
Learnt from: naveenpaul1
PR: #9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
src/server/system_services/account_server.js (1)
Learnt from: naveenpaul1
PR: #9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
src/server/system_services/bucket_server.js (1)
Learnt from: naveenpaul1
PR: #9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
🧬 Code Graph Analysis (1)
config.js (16)
src/server/node_services/nodes_monitor.js (1)
config(15-15)src/util/http_utils.js (1)
config(19-19)src/endpoint/endpoint.js (1)
config(18-18)src/manage_nsfs/nc_lifecycle.js (1)
config(10-10)src/util/notifications_util.js (1)
config(5-5)src/util/fork_utils.js (1)
config(11-11)src/cmd/manage_nsfs.js (1)
config(18-18)src/sdk/bucketspace_fs.js (1)
config(9-9)src/sdk/namespace_fs.js (1)
config(13-13)src/endpoint/s3/s3_rest.js (1)
config(15-15)src/util/native_fs_utils.js (1)
config(10-10)src/manage_nsfs/manage_nsfs_glacier.js (1)
config(6-6)src/cmd/nsfs.js (1)
config(20-20)src/agent/block_store_services/block_store_base.js (1)
config(9-9)src/util/debug_module.js (1)
config(24-24)src/server/bg_workers.js (1)
config(16-16)
🪛 Biome (1.9.4)
src/server/system_services/account_server.js
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/server/system_services/bucket_server.js
[error] 332-332: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
💤 Files with no reviewable changes (23)
- src/server/object_services/map_db_types.js
- src/api/pool_api.js
- src/sdk/map_api_types.js
- src/deploy/NVA_build/set_mongo_repo.sh
- src/server/common_services/server_inter_process.js
- src/api/hosted_agents_api.js
- src/server/server_rpc.js
- src/api/server_inter_process_api.js
- src/api/api.js
- src/server/system_services/schemas/pool_schema.js
- src/server/node_services/node_allocator.js
- src/test/framework/report.js
- src/server/system_services/stats_aggregator.js
- src/api/object_api.js
- src/tools/mongodb_bucket_blow.js
- src/server/mongo_services/mongo_monitor.js
- src/tools/mongo_profiler.js
- src/tools/mongo_md_stats.js
- src/agent/block_store_services/block_store_mongo.js
- src/deploy/NVA_build/common_funcs.sh
- src/util/mongo_utils.js
- src/tools/gridfs_stress.js
- src/tools/mongodb_blow.js
🧰 Additional context used
🧠 Learnings (3)
src/hosted_agents/hosted_agents.js (1)
Learnt from: naveenpaul1
PR: #9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
src/server/system_services/account_server.js (1)
Learnt from: naveenpaul1
PR: #9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
src/server/system_services/bucket_server.js (1)
Learnt from: naveenpaul1
PR: #9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
🧬 Code Graph Analysis (1)
config.js (16)
src/server/node_services/nodes_monitor.js (1)
config(15-15)src/util/http_utils.js (1)
config(19-19)src/endpoint/endpoint.js (1)
config(18-18)src/manage_nsfs/nc_lifecycle.js (1)
config(10-10)src/util/notifications_util.js (1)
config(5-5)src/util/fork_utils.js (1)
config(11-11)src/cmd/manage_nsfs.js (1)
config(18-18)src/sdk/bucketspace_fs.js (1)
config(9-9)src/sdk/namespace_fs.js (1)
config(13-13)src/endpoint/s3/s3_rest.js (1)
config(15-15)src/util/native_fs_utils.js (1)
config(10-10)src/manage_nsfs/manage_nsfs_glacier.js (1)
config(6-6)src/cmd/nsfs.js (1)
config(20-20)src/agent/block_store_services/block_store_base.js (1)
config(9-9)src/util/debug_module.js (1)
config(24-24)src/server/bg_workers.js (1)
config(16-16)
🪛 Biome (1.9.4)
src/server/system_services/account_server.js
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/server/system_services/bucket_server.js
[error] 332-332: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (14)
src/server/node_services/nodes_monitor.js (2)
615-615: LGTM - MongoDB pool exclusion correctly removed from fallback logic.The change properly removes
mongo_pool_infofiltering from the fallback pool selection, ensuring only cloud pools are excluded when determining the default pool for new nodes.
1725-1725: LGTM - Log message correctly updated to reflect MongoDB removal.The log message appropriately removes references to MongoDB nodes, maintaining only cloud node error logging which aligns with the codebase changes.
src/hosted_agents/hosted_agents.js (5)
65-65: LGTM: Correct filtering for cloud pools only.The filter change correctly removes support for pools with only
mongo_pool_infoand now only processes pools that havecloud_pool_infodefined, which aligns with the MongoDB removal objectives.
117-117: LGTM: Hardcoded path simplification.The hardcoded path to
'cloud_pool_info.agent_info.cloud_path'correctly removes the conditional logic that previously selected between mongo and cloud pool paths, focusing exclusively on cloud pools.
133-133: LGTM: Consistent cloud pool info usage.The change to always use
pool.cloud_pool_inforemoves the conditional branching between mongo and cloud pools, maintaining consistency with the MongoDB removal effort.
136-136: LGTM: Simplified pool agent path.Hardcoding
pool_agent_pathto'cloud_pool_info'removes the conditional logic and ensures all agent paths use cloud pool information exclusively.
377-377: LGTM: Consistent token path handling.The hardcoded
pool_property_pathto'cloud_pool_info.agent_info'ensures token operations always use cloud pool paths, completing the removal of mongo pool support from the helper function.src/server/system_services/system_server.js (2)
191-198: LGTM: Correct removal of INTERNAL pool type support.The change removes the INTERNAL pool type branch that created MongoDB-backed pools and now only supports HOSTS pool type with filesystem backing. This correctly implements the PR objective to remove MongoDB dependencies and use BLOCK_STORE_FS as the default.
The error handling for unsupported pool types is appropriate.
605-605: LGTM: Simplified pool filtering logic.The removal of the
mongo_pool_info.pending_deletecheck from the pool filtering logic correctly eliminates MongoDB-specific handling while maintaining the cloud pool filtering. This change is consistent with the broader MongoDB removal effort.src/server/system_services/pool_server.js (2)
750-750: LGTM! Correctly simplified to use cloud pool info only.The change from dynamically selecting the pending delete property to always using
cloud_pool_info.pending_deleteis appropriate since MongoDB pools are being removed.
1324-1334: No Mongo-specific Filter Found – Ignore This ConcernI searched the codebase for any references to “mongo” in the default_resource logic (e.g. is_mongo_pool, mongo_pool_info) and found none. It looks like the filter has always been merely
account.default_resource—there was never a Mongo-only check to remove.If you do need to limit this update to Mongo pools specifically, you’ll have to add that condition explicitly. Otherwise, no changes are required here.
Likely an incorrect or invalid review comment.
src/server/object_services/mapper.js (2)
62-68: Weight assignment logic reordered correctly.The reordering of weight assignments maintains the same priority (local=3, regular=2, redundant=1) while removing the special handling for mongo pools. The logic is cleaner and correct.
371-371: Simplified redundancy check is appropriate.The function now only considers cloud pools as having redundancy, which aligns with removing MongoDB pool support.
src/server/system_services/account_server.js (1)
1386-1388: LGTM!The change correctly updates the non-internal resources check to exclude the 'backingstores' pool instead of relying on mongo_pool_info.
|
@naveenpaul1 Any chance we can split this PR into 2 (maybe 2 commits is enough)? |
|
@jackyalbo Sure, I will push changes that are not related to mongo_pool to the next PR. |
naveenpaul1
changed the title
naveenpaul1
force-pushed
the
remove-mongo-code
branch
from
6fa6662 to
0b93d14
Compare
Signed-off-by: Naveen Paul <[email protected]>
naveenpaul1
force-pushed
the
remove-mongo-code
branch
from
0b93d14 to
775b8b6
Compare
Describe the Problem
Explain the Changes
BLOCK_STORE_FSbased default backingstoireIssues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation