Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 42 additions & 5 deletions src/server/system_services/bucket_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -1319,12 +1319,34 @@ async function get_cloud_buckets(req) {
}
}


async function update_all_buckets_default_pool(req) {
// GAP - Internal mongo_pool no longer supported. This method needs to remove along with Noobaa operator reference.
dbg.warn('update_all_buckets_default_pool is deprecated and will be removed in the next release');
// No-op: bucket default pools are no longer supported
return { success: true };
const pool_name = req.rpc_params.pool_name;
const pool = req.system.pools_by_name[pool_name];
if (!pool) throw new RpcError('INVALID_POOL_NAME');
const internal_pool = pool_server.get_optimal_non_default_pool_id(pool.system);
if (!internal_pool || !internal_pool._id) return;
if (String(pool._id) === String(internal_pool._id)) return;
const buckets_with_internal_pool = _.filter(req.system.buckets_by_name, bucket =>
is_using_internal_storage(bucket, internal_pool));
if (!buckets_with_internal_pool.length) return;

// The loop pushes one update per bucket
const updates = _.uniqBy([], '_id');
for (const bucket of buckets_with_internal_pool) {
updates.push({
_id: bucket.tiering.tiers[0].tier._id,
mirrors: [{
_id: system_store.new_system_store_id(),
spread_pools: [pool._id]
}]
});
}
dbg.log0(`Updating ${buckets_with_internal_pool.length} buckets to use ${pool_name} as default resource`);
await system_store.make_changes({
update: {
tiers: updates
}
});
}

/**
Expand Down Expand Up @@ -1557,6 +1579,21 @@ function get_bucket_info({
return info;
}

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 _calc_metrics({
bucket,
nodes_aggregate_pool,
Expand Down
16 changes: 14 additions & 2 deletions src/server/system_services/pool_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -923,7 +923,7 @@ function get_associated_accounts(pool) {

function find_pool_by_name(req) {
const name = req.rpc_params.name;
const pool = req.system.pools_by_name[name];
const pool = req.system.pools_by_name?.[name];
if (!pool) {
throw new RpcError('NO_SUCH_POOL', 'No such pool: ' + name);
}
Expand Down Expand Up @@ -1301,9 +1301,20 @@ function _is_cloud_pool(pool) {
return Boolean(pool.cloud_pool_info);
}

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];
}
Comment on lines +1308 to +1310
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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));
}


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;
}
Comment on lines +1304 to +1317
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

const aggr_nodes = await nodes_client.instance().aggregate_nodes_by_pool([pool.name], pool.system._id);
const aggr_hosts = await nodes_client.instance().aggregate_hosts_by_pool([pool.name], pool.system._id);
const { mode = '' } = get_pool_info(pool, aggr_nodes, aggr_hosts);
Expand Down Expand Up @@ -1418,3 +1429,4 @@ exports.update_issues_report = update_issues_report;
exports.update_last_monitoring = update_last_monitoring;
exports.calc_namespace_resource_mode = calc_namespace_resource_mode;
exports.check_deletion_ownership = check_deletion_ownership;
exports.get_optimal_non_default_pool_id = get_optimal_non_default_pool_id;
32 changes: 32 additions & 0 deletions src/test/integration_tests/internal/test_upgrade_scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const http = require('http');
const system_store = require('../../../server/system_services/system_store').get_instance();
const upgrade_bucket_policy = require('../../../upgrade/upgrade_scripts/5.15.6/upgrade_bucket_policy');
const upgrade_bucket_cors = require('../../../upgrade/upgrade_scripts/5.19.0/upgrade_bucket_cors');
const remove_mongo_pool = require('../../../upgrade/upgrade_scripts/5.20.0/remove_mongo_pool');
const dbg = require('../../../util/debug_module')(__filename);
const assert = require('assert');
const mocha = require('mocha');
Expand Down Expand Up @@ -129,6 +130,37 @@ mocha.describe('test upgrade scripts', async function() {
assert.deepEqual(cors.CORSRules[0].ExposeHeaders, config.S3_CORS_EXPOSE_HEADERS);
});

mocha.it('test remove mongo_pool to version 5.20.0', async function() {
const system = system_store.data.systems[0];
const base = config.INTERNAL_STORAGE_POOL_NAME || config.DEFAULT_POOL_NAME || 'system-internal-storage-pool';
const internal_name = `${base}-${system._id}`;

// Seed an internal mongo pool entry
await system_store.make_changes({
insert: {
pools: [{
_id: system_store.new_system_store_id(),
system: system._id,
name: internal_name,
resource_type: 'INTERNAL',
owner_id: '6899822e9045e9dc216ef812',
}]
}
});

const before_names = system_store.data.pools.map(e => e.name);
dbg.info("Start : List all the pools in system: ", before_names);
await remove_mongo_pool.run({ dbg, system_store });
const afte_names = system_store.data.pools.map(e => e.name);
dbg.info("End : List all the pools in system: ", afte_names);

// Assert exact seeded name was removed, and no prefixed internal pools remain
const exact_removed = system_store.data.pools.find(pool => pool.name === internal_name);
const prefix_exists = system_store.data.pools.find(pool => pool.name.startsWith(base));
assert.strictEqual(exact_removed, undefined);
assert.strictEqual(prefix_exists, undefined);
});

mocha.after(async function() {
await s3.deleteBucket({ Bucket: BKT });
});
Expand Down
27 changes: 27 additions & 0 deletions src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/* Copyright (C) 2025 NooBaa */
"use strict";

const config = require('../../../../config.js');

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...');
}
Comment on lines +9 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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.

} catch (err) {
dbg.error('Got error while removing mongo pool:', err);
throw err;
}
}
Comment on lines +6 to +22
Copy link

@coderabbitai coderabbitai bot Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plan to do more tests for these scenarios, and the coming PRs will consider this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


module.exports = {
run,
description: 'Noobaa no longer support mongo_pool backingstore, Remove mongo pool',
};