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.

Sign up for GitHub

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

Jump to bottom

Implement optimized healthcheck for Dashboards #463

Merged
merged 5 commits into from
Jun 11, 2021
Merged

Implement optimized healthcheck for Dashboards #463

merged 5 commits into from
Jun 11, 2021

Conversation

boktorbb
Copy link
Contributor

@boktorbb boktorbb commented Jun 8, 2021 •
edited
Loading

Description

Ensures that Dashboards checks only the local OpenSearch node when
cluster_id node attribute is present and all nodes have some cluster_id
value; Otherwise, it uses default behavior

Testing

Tested on both older 7.10 and newer 1.0 versions:

  • using clean 7.10 engine and 7.10 in package.json for 7.10 version
  • using OpenSearch 1.0 engine and 1.0 in package.json for 1.0 version

Tested using local OpenSearch cluster. Configurable setting is read from opensearch_dashboards.yml config file and is able to move through separate code paths for the setting being enabled and disabled in the config file.

Screen Shot 2021-06-09 at 4 46 23 PM

Screen Shot 2021-06-09 at 4 46 55 PM

The getNodeId() function is called in the optimized path and correctly returns a promise

Screen Shot 2021-06-09 at 4 48 09 PM

Value used when cluster_id is set as node attr and all nodes have the same value:

Screen Shot 2021-06-09 at 5 03 57 PM

Signed-off-by: Bishoy Boktor [email protected]

Issues Resolved

Closes #330

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

Implement optimized healthcheck for Dashboards
3e04890 
Ensures that Dashboards checks only the local OpenSearch node when
cluster_id node attribute is present and all nodes have some cluster_id
value; Otherwise, it uses default behavior

Closes opensearch-project#330

Signed-off-by: Bishoy Boktor <[email protected]>
@boktorbb boktorbb added bug Something isn't working migration Any plans, changes, or enhancements needed for migration labels Jun 8, 2021
@boktorbb boktorbb added this to the 1.x release milestone Jun 8, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 3e04890

tmarkley
tmarkley reviewed Jun 9, 2021
View reviewed changes
config/opensearch_dashboards.yml Outdated Show resolved Hide resolved
src/core/server/opensearch/version_check/ensure_opensearch_version.test.ts Show resolved Hide resolved
src/core/server/opensearch/version_check/ensure_opensearch_version.test.ts Show resolved Hide resolved
src/core/server/opensearch/version_check/ensure_opensearch_version.ts Outdated Show resolved Hide resolved
src/core/server/opensearch/version_check/ensure_opensearch_version.ts Outdated Show resolved Hide resolved
src/core/server/opensearch/version_check/ensure_opensearch_version.ts Show resolved Hide resolved
src/core/server/opensearch/version_check/ensure_opensearch_version.ts Outdated Show resolved Hide resolved
src/core/server/opensearch/version_check/ensure_opensearch_version.ts Outdated Show resolved Hide resolved
ananzh
ananzh previously approved these changes Jun 9, 2021
View reviewed changes
Copy link
Member

@ananzh ananzh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to ask someone in AES to also check if this patch can work well for them?

@ananzh ananzh self-requested a review June 9, 2021 17:48
@ananzh ananzh dismissed their stale review June 9, 2021 17:58

I think this patch works well for AES. Just curious whether we need to simplify the logic.

@tianleh
Copy link
Member

tianleh commented Jun 9, 2021

Can you add some test results when "cluster_id" is set for "opensearch.optimizedHealthcheck"?

Update optimizedHealthcheck setting to be configurable
a7dfedb 
opensearch.optimizedHealthcheck is now {string|undefined} setting that
corresponds to the user's node attribute created in OpenSearch.
Healthcheck will now check the node attribute path ending in the value
of the setting.

Signed-off-by: Bishoy Boktor <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed a7dfedb

@boktorbb boktorbb requested a review from tmarkley June 10, 2021 00:11
Simplify getNodeId logic and update documentation
3278aaf 
Simplifies getNodeId code. Also, updates healthcheck param to
healthcheckAttributeName.

Signed-off-by: Bishoy Boktor <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 3278aaf

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 4644c4e

@boktorbb boktorbb requested review from mihirsoni and tmarkley June 10, 2021 19:50
mihirsoni
mihirsoni previously approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@mihirsoni mihirsoni left a comment

Choose a reason for hiding this comment

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

LGTM !! Thanks for the changes.

tmarkley
tmarkley previously approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the incremental improvements. 👍

@kavilla
Copy link
Member

kavilla commented Jun 10, 2021 •
edited
Loading

I don't see how this could impact backwards compatibility but we are confident on BWC passed on your comments?

@boktorbb
Copy link
Contributor Author

I don't see how this could impact backwards compatibility but we are confident on BWC passed on your comments?

Yes :)

@boktorbb boktorbb dismissed stale reviews from tmarkley and mihirsoni via 45e6ab9 June 11, 2021 00:09
@boktorbb boktorbb requested review from kavilla and tmarkley June 11, 2021 00:09
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 45e6ab9

ananzh
ananzh approved these changes Jun 11, 2021
View reviewed changes
Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

LGTM

@boktorbb boktorbb merged commit 03e6462 into opensearch-project:main Jun 11, 2021
boktorbb added a commit that referenced this pull request Jun 11, 2021
@boktorbb
Implement optimized healthcheck for Dashboards (#463)
1b5e178 
* Implement optimized healthcheck for Dashboards

Ensures that Dashboards checks only the local OpenSearch node when
cluster_id node attribute is present and all nodes have some cluster_id
value; Otherwise, it uses default behavior

Closes #330

Signed-off-by: Bishoy Boktor <[email protected]>

* Update optimizedHealthcheck setting to be configurable

opensearch.optimizedHealthcheck is now {string|undefined} setting that
corresponds to the user's node attribute created in OpenSearch.
Healthcheck will now check the node attribute path ending in the value
of the setting.

Signed-off-by: Bishoy Boktor <[email protected]>

* Simplify getNodeId logic and update documentation

Simplifies getNodeId code. Also, updates healthcheck param to
healthcheckAttributeName.

Signed-off-by: Bishoy Boktor <[email protected]>

* Update opensearch_dashboards.yml with setting example

Signed-off-by: Bishoy Boktor <[email protected]>

* Update healthcheck setting name to optimizedHealthcheckId

Signed-off-by: Bishoy Boktor <[email protected]>
kavilla pushed a commit that referenced this pull request Jun 21, 2021
@boktorbb @kavilla
Implement optimized healthcheck for Dashboards (#463)
cc18449 
* Implement optimized healthcheck for Dashboards

Ensures that Dashboards checks only the local OpenSearch node when
cluster_id node attribute is present and all nodes have some cluster_id
value; Otherwise, it uses default behavior

Closes #330

Signed-off-by: Bishoy Boktor <[email protected]>

* Update optimizedHealthcheck setting to be configurable

opensearch.optimizedHealthcheck is now {string|undefined} setting that
corresponds to the user's node attribute created in OpenSearch.
Healthcheck will now check the node attribute path ending in the value
of the setting.

Signed-off-by: Bishoy Boktor <[email protected]>

* Simplify getNodeId logic and update documentation

Simplifies getNodeId code. Also, updates healthcheck param to
healthcheckAttributeName.

Signed-off-by: Bishoy Boktor <[email protected]>

* Update opensearch_dashboards.yml with setting example

Signed-off-by: Bishoy Boktor <[email protected]>

* Update healthcheck setting name to optimizedHealthcheckId

Signed-off-by: Bishoy Boktor <[email protected]>
@ananzh ananzh mentioned this pull request Jun 24, 2021
Closed
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ananzh ananzh ananzh approved these changes

@kavilla kavilla kavilla approved these changes

@mihirsoni mihirsoni mihirsoni left review comments

@tmarkley tmarkley Awaiting requested review from tmarkley

Assignees
No one assigned
Labels
bug Something isn't working migration Any plans, changes, or enhancements needed for migration
Projects
None yet
Milestone
1.x release
Development

Successfully merging this pull request may close these issues.

[Bug] Ensure OpenSearch Dashboards stays available in large clusters
7 participants
@boktorbb @opensearch-ci-bot @tianleh @kavilla @mihirsoni @tmarkley @ananzh