Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

romayalon
Copy link
Contributor

@romayalon romayalon commented Dec 18, 2024

Explain the changes

  1. Replaced the error coming from the condition for failing config dir upgrade if the --expected_hosts list does not match the hosts registered in system.json with a warning in the logs. This change is needed because NooBaa is not cluster aware and we can not take system.json as the source of truth of the current hosts status in the cluster, therefore we will rely on CES to send us the expected hosts list that should contain only hosts the upgraded their noobaa RPM.
  2. Note - we still verify that the hosts in the expected hosts list have upgraded noobaa version and that we have information in system.json about the NooBaa version of the host.
  3. Changed tests that tested this condition to expect a successful upgrade.

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

Automatic tests -
sudo jest --testRegex=jest_tests/test_cli_upgrade

Manual tests -

  1. Check that upgrade won't fail if system.json contains a host that doesn't exist in expected_hosts (Artificially)-
    a. sudo mkdir /etc/noobaa.conf.d/
    b. checkout to 5.16
    c. start noobaa - sudo node src/cmd/nsfs.js --debug 5, stop noobaa CTRL+C
    d. checkout to 5.18
    e. start noobaa - sudo node src/cmd/nsfs.js --debug 5, stop noobaa CTRL+C
    f. edit system.json and add another hostname object with key that doesn't exist, for example -
    vi cat /etc/noobaa.conf.d/system.json
{"existing_hostname":{"current_version":"5.18.0","upgrade_history":{"successful_upgrades":[{"timestamp":1734620619190,"from_version":"5.16.5","to_version":"5.18.0"}]}}
+ "non_existing_hostname":{"current_version":"5.18.0","upgrade_history":{"successful_upgrades":[{"timestamp":1734620619190,"from_version":"5.16.5","to_version":"5.18.0"}]}}
}

g. run config dir upgrade start with expected_hosts having non only the existing_host -
noobaa-cli upgrade start --expected_version 5.18.0 --expected_hosts existing_host_name
and expect a successful upgrade

{
  "response": {
    "code": "UpgradeSuccessful",
    ....

Inside the logs you can see a warning about it but no failure -

[WARN] core.upgrade.nc_upgrade_manager:: _verify_config_dir_upgrade - system.json contains one or more hosts info that are not specified in expected_hosts hosts_data...
  1. Check that upgrade still fails if expected_hosts contains a host that doesn't exist in system.json -
    a. sudo mkdir /etc/noobaa.conf.d/
    b. checkout to 5.16
    c. start noobaa - sudo node src/cmd/nsfs.js --debug 5, stop noobaa CTRL+C
    d. checkout to 5.18
    e. start noobaa - sudo node src/cmd/nsfs.js --debug 5, stop noobaa CTRL+C
    f. run config dir upgrade start with expected_hosts having non existing host -
    noobaa-cli upgrade start --expected_version 5.18.0 --expected_hosts existing_host_name,non_existing_hostname
    and expect a failure -
{
  "error": {
    "code": "UpgradeFailed",
    "message": "Upgrade request failed",
    "cause": "Error: config dir upgrade can not be started - expected_hosts contains one or more hosts that are not specified in system.json hosts_data=...."
  }
}
  • Doc added/updated
  • Tests added

@romayalon romayalon force-pushed the romy-online-upgrade-remove-nodes-verification branch from 309e38a to aa2322d Compare December 18, 2024 15:26
Copy link
Contributor

@shirady shirady left a comment

Choose a reason for hiding this comment

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

LGTM

@romayalon romayalon force-pushed the romy-online-upgrade-remove-nodes-verification branch from aa2322d to 61af36c Compare December 19, 2024 15:17
@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 19, 2024
@romayalon romayalon mentioned this pull request Dec 19, 2024
12 tasks
@romayalon romayalon requested a review from shirady December 19, 2024 15:31
@romayalon romayalon changed the title NC | Online Upgrade | Remove expected_hosts matching to system.json hosts condition NC | Online Upgrade | Replace failure if system.json contains hosts that do not exist in --expected_hosts with a warning Dec 19, 2024
@romayalon romayalon force-pushed the romy-online-upgrade-remove-nodes-verification branch 2 times, most recently from fc79c55 to 7794c82 Compare December 19, 2024 16:12
Copy link
Contributor

@shirady shirady left a comment

Choose a reason for hiding this comment

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

LGTM

src/test/unit_tests/jest_tests/test_cli_upgrade.test.js Outdated Show resolved Hide resolved
src/upgrade/nc_upgrade_manager.js Outdated Show resolved Hide resolved
src/upgrade/nc_upgrade_manager.js Outdated Show resolved Hide resolved
@romayalon romayalon force-pushed the romy-online-upgrade-remove-nodes-verification branch from 7794c82 to 08a7740 Compare December 19, 2024 16:53
Signed-off-by: Romy <35330373+romayalon@users.noreply.github.com>
@romayalon romayalon force-pushed the romy-online-upgrade-remove-nodes-verification branch from 08a7740 to 4aaf02e Compare December 23, 2024 07:04
@romayalon romayalon merged commit 7c70730 into noobaa:master Dec 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants