Skip to content

NC | Online Upgrade CLI | Validate expected_version on upgrade start #8867

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

Merged
merged 1 commit into from
Apr 1, 2025

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Mar 12, 2025

Describe the Problem

Currently, we do not validate the value of the argument of expected_version in noobaa-cli when using upgrade start command.

Explain the Changes

  1. Add the function of validate_expected_version that would check that we have the flag of expected_version and it is from the supported versions we allow. Also, add specific errors to the CLI (MissingExpectedVersionFlag and InvalidArgumentType) so that eventually we would not report it as UpgradeFailed.
  2. Move functions that are related to versions to versions_utils, and added a helper function is_valid_sematic_version to check the structure of the string.
  3. Throw an error instead of the message "config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade" and change the tests accordingly.
  4. Add the skip_verification to BOOLEAN_STRING_OPTIONS and use it to in validate_expected_version.

Issues: Fixed #8852, Fixed #8851

  1. In the described issues, the example is of using upgrade start --expected_version <expected_version> when expected_version is not supported: either lower than expected, or at the same version.

Testing Instructions:

Automatic Testing

Please run:

  • sudo rm -rf /etc/noobaa.conf.d/; sudo npx jest test_cli_upgrade.test (only in a test environment).
  • sudo npx jest test_nc_upgrade_manager.test.js

Manual Testing

You can recreate the failing flag options, for example:

  • sudo node src/cmd/manage_nsfs upgrade start #invalid, missing --expected_version

  • sudo node src/cmd/manage_nsfs upgrade start --expected_version 9 #invalid (number and not string)

  • sudo node src/cmd/manage_nsfs upgrade start --expected_version bla #invalid not semver structure

  • sudo node src/cmd/manage_nsfs upgrade start --expected_version '6.18.0' #invalid version (higher)

  • sudo node src/cmd/manage_nsfs upgrade start --expected_version '4.18.0' #invalid version (lower)

  • sudo node src/cmd/manage_nsfs upgrade start --expected_version '5.19.0' #nothing to upgrade, for the following case, we need to have system.json created, therefore before running it please create an account and start the NSFS server: (1) Create an account with the CLI: sudo node src/cmd/manage_nsfs account add --name <account-name> --new_buckets_path /Users/buckets/ --access_key <access-key> --secret_key <secret-key> --uid <uid> --gid <gid> - before creating the account need to give permission to the new_buckets_path: chmod 777 /Users/buckets/
    (2) Start the NSFS server with: sudo node src/cmd/nsfs --debug 5

  • Doc added/updated

  • Tests added

@shirady shirady force-pushed the nc-upgrade-cli branch 3 times, most recently from 4f243a9 to 92cd6e2 Compare March 13, 2025 12:34
@shirady shirady self-assigned this Mar 13, 2025
@romayalon
Copy link
Contributor

romayalon commented Mar 16, 2025

@shirady
That's a nice change but I don't see how this answers the bug, the reporter got confused by the error code "SuccessfulUpgrade" although there was no upgrade because 1.0.0 is the current config dir version, this is the message they got - "message": "config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade" - this is an error of the user but not NooBaa's, we need to decide if we want to report it as an error or as a response saying nothing to upgrade.

@shirady
Copy link
Contributor Author

shirady commented Mar 17, 2025

@shirady That's a nice change but I don't see how this answers the bug, the reporter got confused by the error code "SuccessfulUpgrade" although there was no upgrade because 5.19.0 is the current version, this is the message they got - "message": "config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade" - this is an error of the user but not NooBaa's, we need to decide if we want to report it as an error or as a response saying nothing to upgrade.

@romayalon I added a case for throwing an error on the message "config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade".

In their case in the issue they used RPM noobaa-core-5.18.0-20250222.el9.x86_66 in it the pkg.version is 5.18.0 and in the suggested code change they would get UpgradeFailed when trying to upgrade using the CLI.

@shirady shirady force-pushed the nc-upgrade-cli branch 2 times, most recently from 9c81e8a to e5d1d38 Compare March 19, 2025 06:47
1. Add the function of validate_expected_version that would check that we have the flag of expected_version and it is from the supported versions we allow. Also, add specific errors to the CLI (MissingExpectedVersionFlag and InvalidArgumentType) so that eventually we would not report it as UpgradeFailed.
2. Move functions that are related to versions to versions_utils, and added a helper function is_valid_sematic_version to check the structure of the string.
3. Throw an error instead of the message "config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade" and change the tests accordingly.
4. Add the skip_verification to BOOLEAN_STRING_OPTIONS and use it to in validate_expected_version.

Signed-off-by: shirady <[email protected]>
@shirady shirady merged commit af65596 into noobaa:master Apr 1, 2025
11 checks passed
@shirady shirady deleted the nc-upgrade-cli branch April 1, 2025 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants