Merge pull request #9189 from jackyalbo/jacky-nc-fix

jackyalbo · web-flow · commit 97296e360957 · 2025-08-10T19:16:38.000+03:00
NC | Remove error on No upgrade required
diff --git a/src/manage_nsfs/manage_nsfs_cli_responses.js b/src/manage_nsfs/manage_nsfs_cli_responses.js
@@ -154,6 +154,12 @@ ManageCLIResponse.UpgradeSuccessful = Object.freeze({
     status: {}
 });
 
+ManageCLIResponse.NoUpgradeRequired = Object.freeze({
+    code: 'NoUpgradeRequired',
+    message: 'Config directory upgrade not required',
+    status: {}
+});
+
 ManageCLIResponse.UpgradeStatus = Object.freeze({
     code: 'UpgradeStatus',
     message: 'Config directory upgrade status retrieved successfully',
diff --git a/src/manage_nsfs/upgrade.js b/src/manage_nsfs/upgrade.js
@@ -55,7 +55,10 @@ async function start_config_dir_upgrade(user_input, config_fs) {
         const nc_upgrade_manager = new NCUpgradeManager(config_fs, { custom_upgrade_scripts_dir });
         const upgrade_res = await nc_upgrade_manager.upgrade_config_dir(expected_version, expected_hosts, { skip_verification });
         if (!upgrade_res) throw new Error('Upgrade config directory failed', { cause: upgrade_res });
-        write_stdout_response(ManageCLIResponse.UpgradeSuccessful, upgrade_res, { expected_version, expected_hosts });
+        write_stdout_response(
+            upgrade_res.code || ManageCLIResponse.UpgradeSuccessful,
+            upgrade_res.upgrade_info, { expected_version, expected_hosts }
+        );
     } catch (err) {
         if (err instanceof ManageCLIError) {
             // those are errors that the admin made in the CLI, the upgrade didn't start
diff --git a/src/test/integration_tests/nc/cli/test_cli_upgrade.test.js b/src/test/integration_tests/nc/cli/test_cli_upgrade.test.js
@@ -76,15 +76,14 @@ const old_expected_system_json2 = {
         'upgrade_package_version': '5.18.0',
         'phase': CONFIG_DIR_PHASES.CONFIG_DIR_UNLOCKED,
         'upgrade_history': {
-            'successful_upgrades': [
-                {
-                    'timestamp': 1724687496424,
-                    'running_host': hostname,
-                    'package_from_version': '5.17.0',
-                    'package_to_version': '5.18.0',
-                    'config_dir_from_version': '0.0.0',
-                    'config_dir_to_version': '1.0.0'
-                }]
+            'successful_upgrades': [{
+                'timestamp': 1724687496424,
+                'running_host': hostname,
+                'package_from_version': '5.17.0',
+                'package_to_version': '5.18.0',
+                'config_dir_from_version': '0.0.0',
+                'config_dir_to_version': '1.0.0'
+            }]
         }
     }
 };
@@ -105,15 +104,14 @@ const old_expected_system_json5 = {
         'upgrade_package_version': '5.17.0',
         'phase': CONFIG_DIR_PHASES.CONFIG_DIR_UNLOCKED,
         'upgrade_history': {
-            'successful_upgrades': [
-                {
-                    'timestamp': 1724687496424,
-                    'running_host': hostname,
-                    'package_from_version': '5.16.0',
-                    'package_to_version': '5.17.0',
-                    'config_dir_from_version': '-1.0.0',
-                    'config_dir_to_version': '0.0.0'
-                }]
+            'successful_upgrades': [{
+                'timestamp': 1724687496424,
+                'running_host': hostname,
+                'package_from_version': '5.16.0',
+                'package_to_version': '5.17.0',
+                'config_dir_from_version': '-1.0.0',
+                'config_dir_to_version': '0.0.0'
+            }]
         }
     }
 };
@@ -133,8 +131,7 @@ const new_expected_system_json = {
         'config_dir_version': '1.0.0',
         'phase': CONFIG_DIR_PHASES.CONFIG_DIR_UNLOCKED,
         'upgrade_history': {
-            'successful_upgrades': [
-                {
+            'successful_upgrades': [{
                     'timestamp': 1724687496424,
                     'running_host': hostname,
                     'package_from_version': '5.17.0',
@@ -143,11 +140,12 @@ const new_expected_system_json = {
                     'config_dir_to_version': '1.0.0'
                 },
                 {
-                'timestamp': 1724687496424,
-                'completed_scripts': [],
-                'from_version': '5.16.0',
-                'to_version': '5.17.0'
-            }]
+                    'timestamp': 1724687496424,
+                    'completed_scripts': [],
+                    'from_version': '5.16.0',
+                    'to_version': '5.17.0'
+                }
+            ]
         }
     }
 };
@@ -368,9 +366,9 @@ describe('noobaa cli - upgrade', () => {
         const system_data_before_upgrade = await config_fs.get_system_config_file();
         const options = { config_root, expected_version: pkg.version, expected_hosts };
         const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, options, true);
-        const parsed_res = JSON.parse(res.stdout);
-        expect(parsed_res.error.message).toBe(ManageCLIError.UpgradeFailed.message);
-        expect(parsed_res.error.cause).toContain(`config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade`);
+        const parsed_res = JSON.parse(res);
+        expect(parsed_res.response.code).toBe(ManageCLIResponse.NoUpgradeRequired.code);
+        expect(parsed_res.response.reply.detail).toContain(`config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade`);
         const system_data_after_upgrade = await config_fs.get_system_config_file();
         // check that in the hostname section nothing changed
         expect(system_data_before_upgrade[hostname]).toStrictEqual(system_data_after_upgrade[hostname]);
diff --git a/src/test/unit_tests/nc/configuration/test_nc_upgrade_manager.test.js b/src/test/unit_tests/nc/configuration/test_nc_upgrade_manager.test.js
@@ -13,19 +13,29 @@ const path = require('path');
 const fs_utils = require('../../../../util/fs_utils');
 const config = require('../../../../../config');
 const pkg = require('../../../../../package.json');
-const { NCUpgradeManager, DEFAULT_NC_UPGRADE_SCRIPTS_DIR, OLD_DEFAULT_PACKAGE_VERSION,
-    OLD_DEFAULT_CONFIG_DIR_VERSION } = require('../../../../upgrade/nc_upgrade_manager');
+const {
+    NCUpgradeManager,
+    DEFAULT_NC_UPGRADE_SCRIPTS_DIR,
+    OLD_DEFAULT_PACKAGE_VERSION,
+    OLD_DEFAULT_CONFIG_DIR_VERSION
+} = require('../../../../upgrade/nc_upgrade_manager');
 const { ConfigFS, CONFIG_DIR_PHASES } = require('../../../../sdk/config_fs');
-const { TMP_PATH, create_redirect_file, create_config_dir,
-    fail_test_if_default_config_dir_exists, clean_config_dir, TEST_TIMEOUT } = require('../../../system_tests/test_utils');
+const {
+    TMP_PATH,
+    create_redirect_file,
+    create_config_dir,
+    fail_test_if_default_config_dir_exists,
+    clean_config_dir,
+    TEST_TIMEOUT
+} = require('../../../system_tests/test_utils');
 
 const config_root = path.join(TMP_PATH, 'config_root_nc_upgrade_manager_test');
 const config_fs = new ConfigFS(config_root);
 const hostname = os.hostname();
 const mock_higher_version_than_pkg_version = pkg.version + '.1';
 
 const dummy_upgrade_script_1 =
-`/* Copyright (C) 2024 NooBaa */
+    `/* Copyright (C) 2024 NooBaa */
 'use strict';
 async function run() {
     console.log('script number 1');
@@ -36,7 +46,7 @@ module.exports = {
 };
 `;
 const dummy_upgrade_script_2 =
-`/* Copyright (C) 2024 NooBaa */
+    `/* Copyright (C) 2024 NooBaa */
 'use strict';
 async function run() {
     console.log('script number 2');
@@ -47,7 +57,7 @@ module.exports = {
 };`;
 
 const dummy_failing_upgrade_script_3 =
-`/* Copyright (C) 2024 NooBaa */
+    `/* Copyright (C) 2024 NooBaa */
 'use strict';
 async function run() {
     console.log('script number 3');
@@ -353,23 +363,31 @@ describe('nc upgrade manager - upgrade config directory', () => {
         });
 
         it('_verify_config_dir_upgrade - empty host current_version', async () => {
-            const system_data = { [hostname]: []};
+            const system_data = {
+                [hostname]: []
+            };
             const expected_err_msg = `config dir upgrade can not be started until all expected hosts have the expected version=${pkg.version}, host=${hostname} host's current_version=undefined`;
             await expect(nc_upgrade_manager._verify_config_dir_upgrade(system_data, pkg.version, [hostname]))
                 .rejects.toThrow(expected_err_msg);
         });
 
         it('_verify_config_dir_upgrade - host current_version < new_version should upgrade RPM', async () => {
             const old_version = '5.16.0';
-            const system_data = { [hostname]: { current_version: old_version }, other_hostname: { current_version: pkg.version } };
+            const system_data = {
+                [hostname]: { current_version: old_version },
+                other_hostname: { current_version: pkg.version }
+            };
             const expected_err_msg = `config dir upgrade can not be started until all expected hosts have the expected version=${pkg.version}, host=${hostname} host's current_version=${old_version}`;
             await expect(nc_upgrade_manager._verify_config_dir_upgrade(system_data, pkg.version, [hostname, 'other_hostname']))
                 .rejects.toThrow(expected_err_msg);
         });
 
         it('_verify_config_dir_upgrade - host current_version > new_version should upgrade RPM', async () => {
             const newer_version = mock_higher_version_than_pkg_version;
-            const system_data = { [hostname]: { current_version: newer_version }, other_hostname: { current_version: pkg.version } };
+            const system_data = {
+                [hostname]: { current_version: newer_version },
+                other_hostname: { current_version: pkg.version }
+            };
             const expected_err_msg = `config dir upgrade can not be started until all expected hosts have the expected version=${pkg.version}, host=${hostname} host's current_version=${newer_version}`;
             await expect(nc_upgrade_manager._verify_config_dir_upgrade(system_data, pkg.version, [hostname, 'other_hostname']))
                 .rejects.toThrow(expected_err_msg);
@@ -385,7 +403,9 @@ describe('nc upgrade manager - upgrade config directory', () => {
 
         it('_verify_config_dir_upgrade - fail on mismatch expected_version', async () => {
             const expected_version = mock_higher_version_than_pkg_version;
-            const system_data = { [hostname]: { current_version: pkg.version, other_hostname: { current_version: pkg.version } }};
+            const system_data = {
+                [hostname]: { current_version: pkg.version, other_hostname: { current_version: pkg.version } }
+            };
             const nc_upgrade_manager_higher_version = new NCUpgradeManager(config_fs);
             const expected_err_msg = `config dir upgrade can not be started - the host's package version=${pkg.version} does not match the user's expected version=${expected_version}`;
             await expect(nc_upgrade_manager_higher_version._verify_config_dir_upgrade(system_data, expected_version, [hostname]))
@@ -413,7 +433,8 @@ describe('nc upgrade manager - upgrade config directory', () => {
         const successful_upgrade_scripts_obj = { dummy_upgrade_script_1, dummy_upgrade_script_2 };
         const failing_upgrade_scripts_obj = { ...successful_upgrade_scripts_obj, dummy_failing_upgrade_script_3 };
         const default_this_upgrade = Object.freeze({
-            config_dir_from_version: from_version, config_dir_to_version: to_version,
+            config_dir_from_version: from_version,
+            config_dir_to_version: to_version,
             completed_scripts: []
         });
         const default_upgrade_script_paths = Object.keys(successful_upgrade_scripts_obj).map(script_name =>
@@ -546,7 +567,7 @@ describe('nc upgrade manager - upgrade config directory', () => {
             system_data.config_directory.in_progress_upgrade = {
                 completed_scripts: [],
                 running_host: hostname,
-                    ...options
+                ...options
             };
             assert_upgrade_start_data(upgrade_start_data, system_data);
             const system_json_data = await config_fs.get_system_config_file();
@@ -773,7 +794,7 @@ describe('nc upgrade manager - upgrade config directory', () => {
                     config_dir_version: config_fs.config_dir_version,
                     upgrade_package_version: expected_version,
                     upgrade_history: {
-                        successful_upgrades: [res]
+                        successful_upgrades: [res.upgrade_info]
                     }
                 }
             };
@@ -785,9 +806,9 @@ describe('nc upgrade manager - upgrade config directory', () => {
             const expected_version = pkg.version;
             const hosts_list = [hostname];
             await config_fs.create_system_config_file(JSON.stringify(system_data));
-            const expected_err_msg = 'config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade';
-            await expect(nc_upgrade_manager.upgrade_config_dir(expected_version, hosts_list))
-                .rejects.toThrow(expected_err_msg);
+            const expected_msg = 'config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade';
+            const res = await nc_upgrade_manager.upgrade_config_dir(expected_version, hosts_list);
+            expect(res.upgrade_info.detail).toStrictEqual(expected_msg);
         });
 
         it('upgrade_config_dir - config_dir_version in system.json is newer than the hosts current config_dir_version', async () => {
@@ -825,7 +846,7 @@ describe('nc upgrade manager - upgrade config directory', () => {
                     upgrade_package_version: expected_version,
                     upgrade_history: {
                         successful_upgrades: [
-                            res,
+                            res.upgrade_info,
                             ...failed_expected_system_json_has_config_directory.config_directory.upgrade_history.successful_upgrades,
                         ]
                     }
@@ -845,7 +866,7 @@ describe('nc upgrade manager - upgrade config directory', () => {
  */
 function assert_upgrade_start_data(actual_upgrade_start, expected_system_data) {
     const { config_dir_version, upgrade_package_version, phase, upgrade_history, in_progress_upgrade, current_version } =
-        actual_upgrade_start.config_directory;
+    actual_upgrade_start.config_directory;
     const expected_in_progress_upgrade = expected_system_data.config_directory?.in_progress_upgrade;
 
     expect(phase).toBe(CONFIG_DIR_PHASES.CONFIG_DIR_LOCKED);
@@ -871,8 +892,14 @@ function assert_upgrade_start_data(actual_upgrade_start, expected_system_data) {
  * @param {Object} expected_system_data 
  */
 function assert_upgrade_finish_or_fail_data(actual_system_data, expected_system_data) {
-    const { config_dir_version, upgrade_package_version, phase, upgrade_history, in_progress_upgrade, current_version } =
-        actual_system_data.config_directory;
+    const {
+        config_dir_version,
+        upgrade_package_version,
+        phase,
+        upgrade_history,
+        in_progress_upgrade,
+        current_version
+    } = actual_system_data.config_directory;
 
     expect(phase).toBe(expected_system_data.config_directory?.phase);
     expect(config_dir_version).toBe(expected_system_data.config_directory?.config_dir_version);
diff --git a/src/upgrade/nc_upgrade_manager.js b/src/upgrade/nc_upgrade_manager.js
@@ -9,6 +9,7 @@ const dbg = require('../util/debug_module')(__filename);
 const { CONFIG_DIR_PHASES } = require('../sdk/config_fs');
 const { should_upgrade, run_upgrade_scripts } = require('./upgrade_utils');
 const { version_compare } = require('../util/versions_utils');
+const { ManageCLIResponse } = require('../manage_nsfs/manage_nsfs_cli_responses');
 
 const hostname = os.hostname();
 // prior to 5.18.0 - there is no config dir version, the config dir version to be used on the first upgrade is 0.0.0 (5.17.0 -> 5.18.0)
@@ -110,9 +111,14 @@ class NCUpgradeManager {
         const this_upgrade_versions = { config_dir_from_version, config_dir_to_version, package_from_version, package_to_version };
 
         if (!should_upgrade(config_dir_from_version, config_dir_to_version)) {
-            const err_message = 'config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade';
-            dbg.error(`upgrade_config_dir: ${err_message}`);
-            throw new Error(err_message);
+            const message = 'config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade';
+            dbg.warn(`upgrade_config_dir: ${message}`);
+            return {
+                code: ManageCLIResponse.NoUpgradeRequired,
+                upgrade_info: {
+                    detail: message
+                }
+            };
         }
 
         if (!skip_verification) await this._verify_config_dir_upgrade(system_data, expected_version, hosts_list);
@@ -128,7 +134,9 @@ class NCUpgradeManager {
         }
 
         await this._update_config_dir_upgrade_finish(system_data, this_upgrade);
-        return this_upgrade;
+        return {
+            upgrade_info: this_upgrade
+        };
     }
 
     //////////////////////////////
@@ -230,7 +238,7 @@ class NCUpgradeManager {
      * @param {Object} system_data
      * @param {{ config_dir_from_version: string; config_dir_to_version: string; package_from_version: string; package_to_version: string; }} options
      * @returns {Promise<Object>}
-    */
+     */
     async _update_config_dir_upgrade_start(system_data, options) {
         const updated_config_directory = {
             ...system_data.config_directory,
