Mongo | Fix mongo_pool issues #9182
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds logic to migrate buckets that use the internal/mongo backing pool to a specified pool, introduces helpers in pool_server to identify/skip the internal pool, adds an upgrade script to remove the internal mongo pool, and an integration test exercising that upgrade script. (≈30 words) Changes
Sequence Diagram(s)sequenceDiagram
participant API as Admin/API
participant BucketServer as bucket_server.js
participant PoolServer as pool_server.js
participant SystemStore as system_store
API->>BucketServer: update_all_buckets_default_pool(req)
BucketServer->>PoolServer: find_pool_by_name(req.rpc_params.pool_name)
PoolServer-->>BucketServer: pool (or error)
BucketServer->>PoolServer: get_optimal_non_default_pool_id(system)
PoolServer-->>BucketServer: internal_pool
BucketServer->>SystemStore: read buckets
loop per bucket
BucketServer->>BucketServer: is_using_internal_storage(bucket, internal_pool)?
alt uses internal
BucketServer->>SystemStore: make_changes(update tier mirror -> target pool)
end
end
BucketServer-->>API: result (number updated)
sequenceDiagram
participant Test as integration test
participant UpgradeScript as remove_mongo_pool.js
participant SystemStore as system_store
participant Debug as dbg
Test->>SystemStore: seed INTERNAL pool with internal name
Test->>UpgradeScript: run({ dbg, system_store })
UpgradeScript->>SystemStore: find pool by internal name
alt pool found
UpgradeScript->>SystemStore: make_changes(remove pool)
SystemStore-->>UpgradeScript: confirmation
UpgradeScript->>Debug: log removal
else not found
UpgradeScript->>Debug: log not found
end
Test->>SystemStore: assert pool name absent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ 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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
naveenpaul1
force-pushed
the
mongo_pool_fix
branch
from
c2e5ba6 to
75e6a13
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
src/server/system_services/pool_server.js (1)
926-931: Defensive access for pools_by_name is good; consider reusing it elsewhereGuarding
req.system.pools_by_nameavoids undefined deref. Consider routing all pool lookups (e.g., in assign_pool_to_region) throughfind_pool_by_name(req)to centralize validation.src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js (2)
8-8: Typo in log message"monogo" -> "mongo".
- dbg.log0(`Starting monogo pool delete...`); + dbg.log0('Starting mongo pool delete...');
24-27: Description grammarMinor clarity and grammar improvement.
- description: 'Noobaa no longer support mongo_pool backingstore, Remove mongo pool', + description: 'NooBaa no longer supports mongo_pool backingstore. Remove internal mongo pool.',src/server/system_services/bucket_server.js (1)
1582-1595:is_using_internal_storage()can short-circuit earlier and is easier to read with optional-chainingMinor readability nit:
-function is_using_internal_storage(bucket, internal_pool) { - if (!internal_pool || !internal_pool._id) return false; - const tiers = bucket.tiering && bucket.tiering.tiers; - if (!tiers || tiers.length !== 1) return false; - const mirrors = tiers[0].tier.mirrors; - if (mirrors.length !== 1) return false; - const spread_pools = mirrors[0].spread_pools; - if (spread_pools.length !== 1) return false; - return String(spread_pools[0]._id) === String(internal_pool._id); -} +function is_using_internal_storage(bucket, internal_pool) { + if (!internal_pool?.-_id) return false; // fast-fail + const spread_pools = bucket.tiering?.tiers?.[0]?.tier?.mirrors?.[0]?.spread_pools; + return spread_pools?.length === 1 && + String(spread_pools[0]._id) === String(internal_pool._id); +}No functional change, just terser.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/server/system_services/bucket_server.js(2 hunks)src/server/system_services/pool_server.js(3 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit Configuration File
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/integration_tests/internal/test_upgrade_scripts.js
⏰ 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). (2)
- GitHub Check: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
🔇 Additional comments (3)
src/server/system_services/pool_server.js (1)
1432-1433: Export looks fine; ensure callers tolerate undefinedIf no internal pool is present,
get_internal_backingstoremay return undefined. Ensure callers null-check.src/test/integration_tests/internal/test_upgrade_scripts.js (1)
14-15: Import of upgrade script: LGTMImport path and name are correct.
src/server/system_services/bucket_server.js (1)
1337-1342: Mirror object may miss required optional fieldsThroughout the codebase (e.g.
tier_server.new_tier_defaults) mirrors usually carry additional keys (weight,storage_class, …).
Overwritingmirrorswith a minimal object risks breaking assumptions elsewhere that those keys exist.Verify the expected schema for the
mirrorsarray inTierdocuments and supply defaults if necessary.
Comment on lines
+6
to
+22
| async function run({ dbg, system_store }) { | ||
| try { | ||
| dbg.log0(`Starting monogo pool delete...`); | ||
| const internal_mongo_pool = `${config.INTERNAL_STORAGE_POOL_NAME}-${system_store.data.systems[0]._id}`; | ||
| dbg.log0(`Internal mongo pool id is : ${internal_mongo_pool}`); | ||
| const pool_ids = system_store.data.pools.filter(pool => pool.name === internal_mongo_pool); | ||
| if (pool_ids.length > 0) { | ||
| dbg.log0(`Removing default mongo pool: ${pool_ids[0]._id}`); | ||
| await system_store.make_changes({ remove: { pools: [pool_ids[0]._id] }}); | ||
| } else { | ||
| dbg.log0('Removing mongo pool: Could not find the mongo pool...'); | ||
| } | ||
| } catch (err) { | ||
| dbg.error('Got error while removing mongo pool:', err); | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Order of operations: ensure buckets/tiers are migrated before removal
Directly removing pools can leave dangling references (tiers/buckets default resource). Confirm that buckets using the internal pool are migrated first (e.g., via update_all_buckets_default_pool) in the upgrade flow; otherwise removal may break invariants.
Run to verify the migration step exists and is invoked by an upgrade script:
🏁 Script executed:
#!/bin/bash
# Find the implementation and any invocations of update_all_buckets_default_pool
rg -n "update_all_buckets_default_pool" -A 3
# Inspect upgrade scripts that might call it
fd upgrade_scripts | xargs -I{} rg -n "update_all_buckets_default_pool" {}Length of output: 1179
Ensure buckets are migrated before removing the internal pool
We didn’t find any call to update_all_buckets_default_pool in the upgrade scripts. To avoid leaving buckets with a missing default pool, invoke the migration step before you delete it.
• File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js
• Before calling system_store.make_changes({ remove: { pools: […] } }), add a call to migrate all buckets off the old pool. For example:
async function run({ dbg, system_store }) {
try {
dbg.log0(`Starting monogo pool delete...`);
const internal_mongo_pool = `${config.INTERNAL_STORAGE_POOL_NAME}-${system_store.data.systems[0]._id}`;
+ // Migrate every bucket with the old default pool to a new one
+ dbg.log0(`Migrating buckets off pool: ${internal_mongo_pool}`);
+ await system_store.call('update_all_buckets_default_pool', {
+ pool_name: internal_mongo_pool,
+ // TODO: replace with the target default pool name
+ new_pool_name: config.DEFAULT_STORAGE_POOL_NAME
+ });
dbg.log0(`Internal mongo pool id is : ${internal_mongo_pool}`);
const pool_ids = system_store.data.pools.filter(pool => pool.name === internal_mongo_pool);
if (pool_ids.length > 0) {
dbg.log0(`Removing default mongo pool: ${pool_ids[0]._id}`);
await system_store.make_changes({ remove: { pools: [pool_ids[0]._id] }});
} else {
dbg.log0('Removing mongo pool: Could not find the mongo pool...');
}
} catch (err) {
dbg.error('Got error while removing mongo pool:', err);
throw err;
}
}This ensures no dangling bucket references remain after pool deletion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function run({ dbg, system_store }) { | |
| try { | |
| dbg.log0(`Starting monogo pool delete...`); | |
| const internal_mongo_pool = `${config.INTERNAL_STORAGE_POOL_NAME}-${system_store.data.systems[0]._id}`; | |
| dbg.log0(`Internal mongo pool id is : ${internal_mongo_pool}`); | |
| const pool_ids = system_store.data.pools.filter(pool => pool.name === internal_mongo_pool); | |
| if (pool_ids.length > 0) { | |
| dbg.log0(`Removing default mongo pool: ${pool_ids[0]._id}`); | |
| await system_store.make_changes({ remove: { pools: [pool_ids[0]._id] }}); | |
| } else { | |
| dbg.log0('Removing mongo pool: Could not find the mongo pool...'); | |
| } | |
| } catch (err) { | |
| dbg.error('Got error while removing mongo pool:', err); | |
| throw err; | |
| } | |
| } | |
| async function run({ dbg, system_store }) { | |
| try { | |
| dbg.log0(`Starting monogo pool delete...`); | |
| const internal_mongo_pool = `${config.INTERNAL_STORAGE_POOL_NAME}-${system_store.data.systems[0]._id}`; | |
| // Migrate every bucket with the old default pool to a new one | |
| dbg.log0(`Migrating buckets off pool: ${internal_mongo_pool}`); | |
| await system_store.call('update_all_buckets_default_pool', { | |
| pool_name: internal_mongo_pool, | |
| // TODO: replace with the target default pool name | |
| new_pool_name: config.DEFAULT_STORAGE_POOL_NAME | |
| }); | |
| dbg.log0(`Internal mongo pool id is : ${internal_mongo_pool}`); | |
| const pool_ids = system_store.data.pools.filter(pool => pool.name === internal_mongo_pool); | |
| if (pool_ids.length > 0) { | |
| dbg.log0(`Removing default mongo pool: ${pool_ids[0]._id}`); | |
| await system_store.make_changes({ remove: { pools: [pool_ids[0]._id] }}); | |
| } else { | |
| dbg.log0('Removing mongo pool: Could not find the mongo pool...'); | |
| } | |
| } catch (err) { | |
| dbg.error('Got error while removing mongo pool:', err); | |
| throw err; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js around lines 6 to 22,
before calling system_store.make_changes to remove the internal mongo pool, add
a call to update_all_buckets_default_pool to migrate all buckets off the old
pool. This prevents buckets from referencing a deleted pool and avoids dangling
references after pool deletion.
There was a problem hiding this comment.
Plan to do more tests for these scenarios, and the coming PRs will consider this.
There was a problem hiding this comment.
@naveenpaul1, understood! That's a sensible approach to handle the bucket migration separately with proper testing.
Would you like me to create a GitHub issue to track the bucket migration work for the upcoming PRs? This would help ensure the migration logic is implemented before pools are removed in production deployments.
✏️ Learnings added
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.304Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.718Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.099Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.168Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
jackyalbo
reviewed
Aug 10, 2025
naveenpaul1
force-pushed
the
mongo_pool_fix
branch
from
75e6a13 to
ae17027
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js (1)
9-17: Use structural detection instead of relying on removed config constant.Based on the codebase learnings,
config.INTERNAL_STORAGE_POOL_NAMEhas been removed from the system. The script should use structural detection with name-prefix fallback and handle multi-system stores.Apply this approach based on the retrieved learnings:
- const internal_mongo_pool = `${config.INTERNAL_STORAGE_POOL_NAME}-${system_store.data.systems[0]._id}`; - dbg.log0(`Internal mongo pool id is : ${internal_mongo_pool}`); - const pool_ids = system_store.data.pools.filter(pool => pool.name === internal_mongo_pool); - if (pool_ids.length > 0) { - dbg.log0(`Removing default mongo pool: ${pool_ids[0]._id}`); - await system_store.make_changes({ remove: { pools: [pool_ids[0]._id] }}); - } else { - dbg.log0('Removing mongo pool: Could not find the mongo pool...'); - } + const systems = Array.isArray(system_store.data.systems) ? system_store.data.systems : []; + const pools = Array.isArray(system_store.data.pools) ? system_store.data.pools : []; + + // Use structural detection with name-prefix fallback + const pools_to_remove = pools.filter(pool => { + // Prefer structural markers first + if (pool?.mongo_info) return true; + if (pool?.resource_type === 'INTERNAL') return true; + // Fallback to name-based detection for legacy pools + if (pool?.name) { + const name = pool.name; + const prefixes = ['system-internal-storage-pool', 'mongo-pool']; + return prefixes.some(prefix => name.startsWith(prefix)); + } + return false; + }); + + if (pools_to_remove.length > 0) { + dbg.log0(`Removing internal mongo pools: ${pools_to_remove.map(p => String(p._id)).join(', ')}`); + await system_store.make_changes({ + remove: { pools: pools_to_remove.map(p => p._id) } + }); + } else { + dbg.log0('No internal mongo pools found to remove'); + }
🧹 Nitpick comments (1)
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js (1)
8-8: Fix typo in log message.There's a typo in the log message: "monogo" should be "mongo".
Apply this diff to fix the typo:
- dbg.log0(`Starting monogo pool delete...`); + dbg.log0(`Starting mongo pool delete...`);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/server/system_services/bucket_server.js(2 hunks)src/server/system_services/pool_server.js(3 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/integration_tests/internal/test_upgrade_scripts.js
- src/server/system_services/bucket_server.js
- src/server/system_services/pool_server.js
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.304Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.718Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.099Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.168Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1308-1311
Timestamp: 2025-08-08T13:10:57.562Z
Learning: In noobaa-core, config.INTERNAL_STORAGE_POOL_NAME has been removed. Use config.DEFAULT_POOL_NAME for internal/backingstore pool identification. Prefer exact match first, with prefix fallback to handle potential system-id suffixes in pool names.
📚 Learning: 2025-08-08T13:12:46.718Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.718Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js
📚 Learning: 2025-08-11T06:12:12.304Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.304Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js
📚 Learning: 2025-08-08T13:10:36.099Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.099Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js
📚 Learning: 2025-08-08T13:13:42.168Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.168Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js
📚 Learning: 2025-08-08T13:10:57.562Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1308-1311
Timestamp: 2025-08-08T13:10:57.562Z
Learning: In noobaa-core, config.INTERNAL_STORAGE_POOL_NAME has been removed. Use config.DEFAULT_POOL_NAME for internal/backingstore pool identification. Prefer exact match first, with prefix fallback to handle potential system-id suffixes in pool names.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js
📚 Learning: 2025-08-08T13:08:38.312Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/bucket_server.js:1324-1327
Timestamp: 2025-08-08T13:08:38.312Z
Learning: In src/server/system_services/bucket_server.js, the update_all_buckets_default_pool(req) handler expects req.rpc_params.pool_name to be a plain string (not a SensitiveString wrapper), so calling .unwrap() is not needed there.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js
🧬 Code Graph Analysis (1)
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js (3)
src/test/integration_tests/internal/test_upgrade_scripts.js (4)
config(18-18)require(8-8)require(9-9)system_store(11-11)src/server/system_services/pool_server.js (18)
config(12-12)require(13-13)require(25-25)system_store(19-19)pool(146-146)pool(175-175)pool(221-221)pool(422-422)pool(452-452)pool(506-506)pool(519-519)pool(577-577)pool(617-617)pool(667-667)pool(780-780)pool(926-926)pool(935-935)pool(1246-1246)src/server/system_services/bucket_server.js (6)
config(14-14)require(19-19)require(40-40)system_store(28-28)pool(1324-1324)pool(1528-1528)
⏰ 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)
- GitHub Check: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
🔇 Additional comments (2)
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js (2)
4-4: LGTM!The config import is appropriate for accessing system configuration constants.
24-27: LGTM!The module exports follow the standard upgrade script pattern with both the run function and descriptive text.
naveenpaul1
force-pushed
the
mongo_pool_fix
branch
from
ae17027 to
35066a2
Compare
Signed-off-by: Naveen Paul <[email protected]>
naveenpaul1
force-pushed
the
mongo_pool_fix
branch
from
35066a2 to
7f235ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js (2)
6-22: Bucket migration acknowledgmentRemoving pools without migrating buckets can leave dangling references. You noted migration will be handled in follow-ups with tests—SGTM. Please ensure it’s tracked.
I can open a tracking issue with acceptance criteria if helpful.
11-14: Variable naming and single-removal
- pool_ids holds pool objects, not IDs.
- Only removes the first match; remove all matches.
See the previous diff which renames to pools_to_remove and deletes all matching pools.
🧹 Nitpick comments (4)
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js (3)
8-8: Typo in log message"monogo" → "mongo".
- dbg.log0(`Starting monogo pool delete...`); + dbg.log0('Starting mongo pool delete...');
10-10: Misleading log text: logs name, says “id”You’re logging the pool’s name, not its id. If you keep the current approach, fix the message.
- dbg.log0(`Internal mongo pool id is : ${internal_mongo_pool}`); + dbg.log0(`Internal mongo pool name is: ${internal_mongo_pool}`);
26-26: Description grammar and brandingMinor grammar/branding fix.
- description: 'Noobaa no longer support mongo_pool backingstore, Remove mongo pool', + description: 'NooBaa no longer supports mongo_pool backingstore. Remove mongo pool.',src/server/system_services/pool_server.js (1)
1308-1310: Naming/semantics mismatch: function name ends with “_id” but returns a pool objectEither return the pool._id or rename to reflect it returns the pool object (e.g., get_default_pool). At minimum, add JSDoc clarifying the return type to avoid misuse.
If you want, I can scan for call sites that expect an ID vs object to propose a safe rename plan.
Also applies to: 1432-1432
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/server/system_services/bucket_server.js(2 hunks)src/server/system_services/pool_server.js(3 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/integration_tests/internal/test_upgrade_scripts.js
- src/server/system_services/bucket_server.js
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.304Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.718Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.099Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.168Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
📚 Learning: 2025-08-08T13:12:46.718Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.718Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.jssrc/server/system_services/pool_server.js
📚 Learning: 2025-08-11T06:12:12.304Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.304Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.jssrc/server/system_services/pool_server.js
📚 Learning: 2025-08-08T13:10:36.099Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.099Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.jssrc/server/system_services/pool_server.js
📚 Learning: 2025-08-08T13:13:42.168Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.168Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.jssrc/server/system_services/pool_server.js
📚 Learning: 2025-08-08T13:10:57.562Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1308-1311
Timestamp: 2025-08-08T13:10:57.562Z
Learning: In noobaa-core, config.INTERNAL_STORAGE_POOL_NAME has been removed. Use config.DEFAULT_POOL_NAME for internal/backingstore pool identification. Prefer exact match first, with prefix fallback to handle potential system-id suffixes in pool names.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.jssrc/server/system_services/pool_server.js
📚 Learning: 2025-08-08T13:08:38.312Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/bucket_server.js:1324-1327
Timestamp: 2025-08-08T13:08:38.312Z
Learning: In src/server/system_services/bucket_server.js, the update_all_buckets_default_pool(req) handler expects req.rpc_params.pool_name to be a plain string (not a SensitiveString wrapper), so calling .unwrap() is not needed there.
Applied to files:
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.jssrc/server/system_services/pool_server.js
🧬 Code Graph Analysis (2)
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js (3)
src/test/integration_tests/internal/test_upgrade_scripts.js (5)
config(18-18)require(8-8)require(9-9)dbg(15-15)system_store(11-11)src/server/system_services/pool_server.js (19)
config(12-12)require(13-13)require(25-25)dbg(11-11)system_store(19-19)pool(146-146)pool(175-175)pool(221-221)pool(422-422)pool(452-452)pool(506-506)pool(519-519)pool(577-577)pool(617-617)pool(667-667)pool(780-780)pool(926-926)pool(935-935)pool(1246-1246)src/server/system_services/bucket_server.js (7)
config(14-14)require(19-19)require(40-40)dbg(13-13)system_store(28-28)pool(1324-1324)pool(1529-1529)
src/server/system_services/pool_server.js (4)
src/server/system_services/bucket_server.js (8)
pool(1324-1324)pool(1529-1529)req(1116-1116)req(1149-1149)req(2013-2013)config(14-14)system(689-689)system_store(28-28)src/hosted_agents/hosted_agents.js (5)
pool(327-327)pool(375-375)req(302-302)config(18-18)system_store(10-10)src/server/system_services/stats_aggregator.js (7)
pool(309-309)system(598-598)system(1162-1162)system(1175-1175)system(1199-1199)system(1232-1232)system_store(16-16)src/server/system_services/system_server.js (18)
req(287-287)req(334-339)req(815-815)req(883-883)req(958-958)req(1050-1050)req(1231-1231)req(1312-1312)req(1359-1359)config(18-18)system(75-75)system(99-99)system(125-173)system(178-178)system(482-482)system(860-860)system_store(32-32)system_store(1188-1188)
⏰ 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). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
src/server/system_services/pool_server.js (1)
926-926: Safe lookup with optional chaining — LGTM
Comment on lines
+1304
to
+1317
| function _is_optimal_non_default_pool_id(pool) { | ||
| return Boolean(pool.name === config.DEFAULT_POOL_NAME); | ||
| } | ||
|
|
||
| function get_optimal_non_default_pool_id(system) { | ||
| return system.pools_by_name[config.DEFAULT_POOL_NAME]; | ||
| } | ||
|
|
||
| async function get_optimal_non_mongo_pool_id() { | ||
| for (const pool of system_store.data.pools) { | ||
|
|
||
| // skip backingstore_pool. | ||
| if (_is_optimal_non_default_pool_id(pool)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Skip internal/backingstore pools robustly (structural markers + prefix); fix helper naming
Name equality against DEFAULT_POOL_NAME is fragile; prefer explicit backingstore flags and legacy markers, with a DEFAULT_POOL_NAME prefix fallback. Also, the helper name implies an “id” but returns a boolean. Replace the helper and its usage:
-function _is_optimal_non_default_pool_id(pool) {
- return Boolean(pool.name === config.DEFAULT_POOL_NAME);
-}
+function _is_backingstore_pool(pool) {
+ if (!pool) return false;
+ // Prefer explicit markers
+ if (pool.hosts_pool_info?.backingstore || pool.cloud_pool_info?.backingstore) return true;
+ // Legacy internal/mongo markers
+ if (pool.resource_type === 'INTERNAL' || pool.mongo_info) return true;
+ // Fallback by prefix (covers suffixed names)
+ const base = config.DEFAULT_POOL_NAME;
+ return !!(base && pool.name?.startsWith(base));
+}
@@
- // skip backingstore_pool.
- if (_is_optimal_non_default_pool_id(pool)) {
+ // skip backingstore/internal/mongo pools.
+ if (_is_backingstore_pool(pool)) {
continue;
}🤖 Prompt for AI Agents
In src/server/system_services/pool_server.js around lines 1304-1317, the helper
_is_optimal_non_default_pool_id is misnamed (it returns a boolean but implies an
id) and uses fragile name-equality to detect internal/backingstore pools;
replace it with a clearly named predicate (e.g.,
is_backingstore_or_internal_pool) that first checks structural flags on the pool
object (e.g., pool.backingstore === true or pool.is_internal === true or
pool.legacy_backingstore === true), and only as a fallback checks
pool.name.startsWith(config.DEFAULT_POOL_NAME); update all call sites (like
get_optimal_non_mongo_pool_id) to use the new predicate name, remove the old
helper, and ensure functions that should return ids actually return pool.id (or
the intended id) rather than boolean or the whole pool object.
Comment on lines
+1308
to
+1310
| function get_optimal_non_default_pool_id(system) { | ||
| return system.pools_by_name[config.DEFAULT_POOL_NAME]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
get_optimal_non_default_pool_id may return undefined; add prefix fallback and null-safety
If the default pool name is suffixed, direct lookup fails. Add a safe fallback:
-function get_optimal_non_default_pool_id(system) {
- return system.pools_by_name[config.DEFAULT_POOL_NAME];
-}
+function get_optimal_non_default_pool_id(system) {
+ const pools_by_name = system?.pools_by_name || {};
+ const base = config.DEFAULT_POOL_NAME;
+ if (!base) return undefined;
+ if (pools_by_name[base]) return pools_by_name[base];
+ return Object.values(pools_by_name).find(p => p?.name?.startsWith(base));
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function get_optimal_non_default_pool_id(system) { | |
| return system.pools_by_name[config.DEFAULT_POOL_NAME]; | |
| } | |
| function get_optimal_non_default_pool_id(system) { | |
| const pools_by_name = system?.pools_by_name || {}; | |
| const base = config.DEFAULT_POOL_NAME; | |
| if (!base) return undefined; | |
| if (pools_by_name[base]) return pools_by_name[base]; | |
| return Object.values(pools_by_name).find(p => p?.name?.startsWith(base)); | |
| } |
Comment on lines
+9
to
+17
| const internal_mongo_pool = `${config.INTERNAL_STORAGE_POOL_NAME}-${system_store.data.systems[0]._id}`; | ||
| dbg.log0(`Internal mongo pool id is : ${internal_mongo_pool}`); | ||
| const pool_ids = system_store.data.pools.filter(pool => pool.name === internal_mongo_pool); | ||
| if (pool_ids.length > 0) { | ||
| dbg.log0(`Removing default mongo pool: ${pool_ids[0]._id}`); | ||
| await system_store.make_changes({ remove: { pools: [pool_ids[0]._id] }}); | ||
| } else { | ||
| dbg.log0('Removing mongo pool: Could not find the mongo pool...'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make removal robust: avoid INTERNAL_STORAGE_POOL_NAME, handle multi-system, detect structurally, remove all matches
Relying on config.INTERNAL_STORAGE_POOL_NAME (removed elsewhere), exact name equality, and only the first system is brittle. Detect internal/mongo pools structurally (mongo_info or resource_type === 'INTERNAL') with a safe name-prefix fallback (DEFAULT_POOL_NAME and legacy literal), and remove all matches across systems.
- const internal_mongo_pool = `${config.INTERNAL_STORAGE_POOL_NAME}-${system_store.data.systems[0]._id}`;
- dbg.log0(`Internal mongo pool id is : ${internal_mongo_pool}`);
- const pool_ids = system_store.data.pools.filter(pool => pool.name === internal_mongo_pool);
- if (pool_ids.length > 0) {
- dbg.log0(`Removing default mongo pool: ${pool_ids[0]._id}`);
- await system_store.make_changes({ remove: { pools: [pool_ids[0]._id] }});
- } else {
- dbg.log0('Removing mongo pool: Could not find the mongo pool...');
- }
+ const systems = Array.isArray(system_store.data.systems) ? system_store.data.systems : [];
+ const sysIds = new Set(systems.map(s => String(s?._id)).filter(Boolean));
+
+ // Build candidate base names from current config and known legacy prefix
+ const bases = [config?.DEFAULT_POOL_NAME, 'system-internal-storage-pool']
+ .filter(x => typeof x === 'string' && x.length > 0);
+
+ const hasNameMatch = name => {
+ if (!name) return false;
+ if (bases.some(base => name.startsWith(base))) return true;
+ for (const base of bases) for (const id of sysIds) {
+ if (name === `${base}-${id}`) return true;
+ }
+ return false;
+ };
+
+ const pools = Array.isArray(system_store.data.pools) ? system_store.data.pools : [];
+ const pools_to_remove = pools.filter(p =>
+ p?.mongo_info || p?.resource_type === 'INTERNAL' || hasNameMatch(p?.name)
+ );
+ if (pools_to_remove.length > 0) {
+ dbg.log0(`Removing internal mongo pools: ${pools_to_remove.map(p => String(p._id)).join(', ')}`);
+ await system_store.make_changes({
+ remove: { pools: pools_to_remove.map(p => p._id) }
+ });
+ } else {
+ dbg.log0('Removing mongo pool: no internal mongo pools found');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const internal_mongo_pool = `${config.INTERNAL_STORAGE_POOL_NAME}-${system_store.data.systems[0]._id}`; | |
| dbg.log0(`Internal mongo pool id is : ${internal_mongo_pool}`); | |
| const pool_ids = system_store.data.pools.filter(pool => pool.name === internal_mongo_pool); | |
| if (pool_ids.length > 0) { | |
| dbg.log0(`Removing default mongo pool: ${pool_ids[0]._id}`); | |
| await system_store.make_changes({ remove: { pools: [pool_ids[0]._id] }}); | |
| } else { | |
| dbg.log0('Removing mongo pool: Could not find the mongo pool...'); | |
| } | |
| const systems = Array.isArray(system_store.data.systems) ? system_store.data.systems : []; | |
| const sysIds = new Set(systems.map(s => String(s?._id)).filter(Boolean)); | |
| // Build candidate base names from current config and known legacy prefix | |
| const bases = [config?.DEFAULT_POOL_NAME, 'system-internal-storage-pool'] | |
| .filter(x => typeof x === 'string' && x.length > 0); | |
| const hasNameMatch = name => { | |
| if (!name) return false; | |
| if (bases.some(base => name.startsWith(base))) return true; | |
| for (const base of bases) { | |
| for (const id of sysIds) { | |
| if (name === `${base}-${id}`) return true; | |
| } | |
| } | |
| return false; | |
| }; | |
| const pools = Array.isArray(system_store.data.pools) ? system_store.data.pools : []; | |
| const pools_to_remove = pools.filter(p => | |
| p?.mongo_info || p?.resource_type === 'INTERNAL' || hasNameMatch(p?.name) | |
| ); | |
| if (pools_to_remove.length > 0) { | |
| dbg.log0(`Removing internal mongo pools: ${pools_to_remove.map(p => String(p._id)).join(', ')}`); | |
| await system_store.make_changes({ | |
| remove: { pools: pools_to_remove.map(p => p._id) } | |
| }); | |
| } else { | |
| dbg.log0('Removing mongo pool: no internal mongo pools found'); | |
| } |
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js around lines 9 to 17,
the current code relies on config.INTERNAL_STORAGE_POOL_NAME, exact name
equality and only the first system which is brittle; update the logic to iterate
all systems in system_store.data.systems, scan every pool in each system and
identify internal/mongo pools structurally (pool.mongo_info present or
pool.resource_type === 'INTERNAL'), and also treat name-prefixed fallbacks
(DEFAULT_POOL_NAME and the legacy literal) as matches; collect all matching pool
IDs across all systems (deduplicated), and call system_store.make_changes once
to remove all of them, logging what is removed or that none were found.
jackyalbo
approved these changes
Aug 11, 2025
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe the Problem
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Improvements
Upgrade
Tests