Merge pull request #8612 from romayalon/romy-online-upgrade-remove-nodes-verification

NC | Online Upgrade | Replace failure if system.json contains hosts that do not exist in --expected_hosts with a warning

Author: romayalon

Diff for: 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 for: src/test/unit_tests/jest_tests/test_nc_upgrade_manager.test.js

@@ -282,23 +282,23 @@ 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);
         });
 
         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);
         });
 
         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 for: 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}`;
                 }
             }
         }