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 | Health CLI update config directory and upgrade checks #8532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

romayalon
Copy link
Contributor

@romayalon romayalon commented Nov 14, 2024

Explain the changes

  1. Added a check in the Health CLI that shows the config directory status if exists.
  2. Added a check in the Health CLI that asserts if the upgrade of the config directory has issues, this situation can be discovered by phase of the config directory is locked and there is no ongoing upgrade.
  3. Added unit tests (and did some refactoring).
  4. Fixed a bug that sets the process name to be Upgrade, it's showing in the logs of the endpoint, CLI etc.

Issues: Fixed #xxx / Gap #xxx

  1. Another idea for a health check - check per host if config directory is locked.

Testing Instructions:

  1. sudo node ./node_modules/mocha/bin/mocha src/test/unit_tests/test_nc_health.js
  • Doc added/updated
  • Tests added

phase: config_dir_data.phase,
config_dir_version: config_dir_data.config_dir_version,
upgrade_package_version: config_dir_data.upgrade_package_version,
in_progress_upgrade: config_dir_data.in_progress_upgrade
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add error type for config and upgrad status?

@@ -136,6 +138,8 @@ class NSFSHealth {
endpoint_state,
error_type: health_errors_tyes.TEMPORARY,
},
config_directory_status: this._get_config_dir_status(system_data),
upgrade_status: this._get_upgrade_status(system_data),
Copy link
Contributor

Choose a reason for hiding this comment

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

update health doc for newly added properties.


/**
* get_upgrade_status returns the status of an ongoing upgrade, if valid it returns an object with upgrade details
* if upgrade is not ongoing but config dir is locked in one of the hosts, the error details will return
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we return the "an object with upgrade details" I don't see it?

@@ -45,6 +56,10 @@ mocha.describe('nsfs nc health', function() {
});

mocha.describe('nsfs nc health cli validations', function() {
mocha.afterEach(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It was at the beginning of each it, right? why not use beforeEach?

});

mocha.it('Health all condition is success', async function() {
const get_service_state = sinon.stub(Health, "get_service_state");
mocha.it('Health all condition is success1', async function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this "1" intentional?

Suggested change
mocha.it('Health all condition is success1', async function() {
mocha.it('Health all condition is success', async function() {

get_endpoint_response.onFirstCall().returns(Promise.resolve({response: {response_code: 'RUNNING', total_fork_count: 0}}));
const health_status = await Health.nc_nsfs_health();
assert.strictEqual(health_status.checks.accounts_status.valid_accounts.length, 1);
assert.strictEqual(health_status.checks.accounts_status.invalid_accounts.length, 1);
await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.DELETE, { config_root, name: account_invalid.name });
});

mocha.it('Health all condition - invalid config directory upgrade', async function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you add a comment - what makes it invalid?

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.

3 participants