From 4aaf02e2e93a8dd539a2fe28da2ab47536a3408c Mon Sep 17 00:00:00 2001 From: Romy <35330373+romayalon@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:58:06 +0200 Subject: [PATCH] Remove expected_hosts matching to system.json hosts Signed-off-by: Romy <35330373+romayalon@users.noreply.github.com> --- .../jest_tests/test_cli_upgrade.test.js | 21 +++++++------- .../test_nc_upgrade_manager.test.js | 6 ++-- src/upgrade/nc_upgrade_manager.js | 28 ++++++++++--------- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/test/unit_tests/jest_tests/test_cli_upgrade.test.js b/src/test/unit_tests/jest_tests/test_cli_upgrade.test.js index 217e02e511..b2ab4bace4 100644 --- a/src/test/unit_tests/jest_tests/test_cli_upgrade.test.js +++ b/src/test/unit_tests/jest_tests/test_cli_upgrade.test.js @@ -282,23 +282,22 @@ describe('noobaa cli - upgrade', () => { const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, { config_root, expected_version: pkg.version, expected_hosts: `${hostname},bla1,bla2` }, 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 upgrade can not be started - expected_hosts missing one or more hosts specified in system.json hosts_data='); + expect(parsed_res.error.cause).toContain('config dir upgrade can not be started - expected_hosts contains one or more hosts that are not specified in system.json hosts_data='); }); - it('upgrade start - should fail on missing system.json hosts in expected_hosts', async () => { + it('upgrade start - should succeed although system.json contains extra hosts than specified in expected_hosts', async () => { await fs_utils.replace_file(config_fs.system_json_path, JSON.stringify(invalid_hostname_system_json)); const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, { config_root, expected_version: pkg.version, expected_hosts: `${hostname}` }, 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 upgrade can not be started - system.json missing one or more hosts info specified in expected_hosts hosts_data'); + const parsed_res = JSON.parse(res); + expect(parsed_res.response.code).toBe(ManageCLIResponse.UpgradeSuccessful.code); }); - it('upgrade start - should fail on missing system.json hosts in expected_hosts1', async () => { + it('upgrade start - should succeed although on missing system.json hosts in expected_hosts with comma', async () => { + // we set intentionally comma at the end so we will test we know how to parse it await fs_utils.replace_file(config_fs.system_json_path, JSON.stringify(invalid_hostname_system_json)); const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, { config_root, expected_version: pkg.version, expected_hosts: `${hostname},` }, 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 upgrade can not be started - system.json missing one or more hosts info specified in expected_hosts hosts_data'); + const parsed_res = JSON.parse(res); + expect(parsed_res.response.code).toBe(ManageCLIResponse.UpgradeSuccessful.code); }); it('upgrade start - should fail expected_version invalid', async () => { @@ -316,7 +315,7 @@ describe('noobaa cli - upgrade', () => { 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 upgrade can not be started until all nodes have the expected version'); + expect(parsed_res.error.cause).toContain('config dir upgrade can not be started until all expected hosts have the expected version'); }); it('upgrade start - should fail - RPM version is higher than source code version', async () => { @@ -326,7 +325,7 @@ describe('noobaa cli - upgrade', () => { const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, options, true); const parsed_res = JSON.parse(res.stdout); expect(parsed_res.error.code).toBe(ManageCLIError.UpgradeFailed.code); - expect(parsed_res.error.cause).toContain('config dir upgrade can not be started until all nodes have the expected'); + expect(parsed_res.error.cause).toContain('config dir upgrade can not be started until all expected hosts have the expected'); 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/jest_tests/test_nc_upgrade_manager.test.js b/src/test/unit_tests/jest_tests/test_nc_upgrade_manager.test.js index f1270bbc9a..b5c5f0c7b0 100644 --- a/src/test/unit_tests/jest_tests/test_nc_upgrade_manager.test.js +++ b/src/test/unit_tests/jest_tests/test_nc_upgrade_manager.test.js @@ -282,7 +282,7 @@ describe('nc upgrade manager - upgrade config directory', () => { it('_verify_config_dir_upgrade - empty host current_version', async () => { const system_data = { [hostname]: []}; - const expected_err_msg = `config dir upgrade can not be started until all nodes have the expected version=${pkg.version}, host=${hostname} host's current_version=undefined`; + 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); }); @@ -290,7 +290,7 @@ describe('nc upgrade manager - upgrade config directory', () => { 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 expected_err_msg = `config dir upgrade can not be started until all nodes have the expected version=${pkg.version}, host=${hostname} host's current_version=${old_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); }); @@ -298,7 +298,7 @@ describe('nc upgrade manager - upgrade config directory', () => { it('_verify_config_dir_upgrade - host current_version > new_version should upgrade RPM', async () => { const newer_version = pkg.version + '.1'; 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 nodes have the expected version=${pkg.version}, host=${hostname} host's current_version=${newer_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); }); diff --git a/src/upgrade/nc_upgrade_manager.js b/src/upgrade/nc_upgrade_manager.js index 37ff2313d8..d473bf861d 100644 --- a/src/upgrade/nc_upgrade_manager.js +++ b/src/upgrade/nc_upgrade_manager.js @@ -154,11 +154,11 @@ class NCUpgradeManager { * 2. The user's expected_version is the host's package version - * expected_version is the expected source code version that the user asks to upgrade to, it's an optional verification, * if expected_version was not provided we assume that the source code on this host is - * 3. TODO? - verifies a backup confirmation of the config directory received by the user/location exists - * updated and that the source code version and the schema version is the one we want to upgrade to - * QUESTION - - * 3. should we verify a backup, by stat of the backup location just have a confirmation of the user in the CLI - * 4. are there more verifications we should do? + * 3. The user's expected_hosts exist in system.json + * 4. If there are hosts in system.json that ere not provided in the expected_hosts we will print a warning but won't fail + * we do that because of hostname can be renamed, hosts that are on maintainance and we don't want to block the upgrade becuase it might take a lot of time, + * or because of hosts that used to be a part of the cluster and they were removed from the cluster, we don't get the updated info of the hosts on system.json + * therefore we can not treat the system.json as the source of truth of the hosts information * @param {Object} system_data * @param {String} expected_version * @param {[String]} expected_hosts @@ -176,20 +176,22 @@ class NCUpgradeManager { if (!err_message && !hostnames.length) { err_message = `config dir upgrade can not be started missing hosts_data hosts_data=${util.inspect(hosts_data)}`; } - const missing_expected_hosts = !(expected_hosts.every(item => hostnames.includes(item))); - const missing_hostnames = !(hostnames.every(item => expected_hosts.includes(item))); - if (!err_message && missing_expected_hosts) { - err_message = `config dir upgrade can not be started - expected_hosts missing one or more hosts specified in system.json hosts_data=${util.inspect(hosts_data)}`; + const all_hostnames_exist_in_expected_hosts = hostnames.every(item => expected_hosts.includes(item)); + if (!all_hostnames_exist_in_expected_hosts) { + dbg.warn(`_verify_config_dir_upgrade - system.json contains one or more hosts info that are not specified in expected_hosts: hosts_data=${util.inspect(hosts_data)} expected_hosts=${util.inspect(expected_hosts)}`); } - if (!err_message && missing_hostnames) { - err_message = `config dir upgrade can not be started - system.json missing one or more hosts info specified in expected_hosts hosts_data=${util.inspect(hosts_data)}`; + + const all_expected_hosts_exist_in_system_json = expected_hosts.every(item => hostnames.includes(item)); + if (!err_message && !all_expected_hosts_exist_in_system_json) { + err_message = `config dir upgrade can not be started - expected_hosts contains one or more hosts that are not specified in system.json hosts_data=${util.inspect(hosts_data)} expected_hosts=${util.inspect(expected_hosts)}`; } if (!err_message) { - for (const [host, host_data] of Object.entries(hosts_data)) { + for (const cur_hostname of expected_hosts) { + const host_data = hosts_data[cur_hostname]; if (!host_data.current_version || version_compare(host_data.current_version, new_version) !== 0) { - err_message = `config dir upgrade can not be started until all nodes have the expected version=${new_version}, host=${host} host's current_version=${host_data.current_version}`; + err_message = `config dir upgrade can not be started until all expected hosts have the expected version=${new_version}, host=${cur_hostname} host's current_version=${host_data.current_version}`; } } }