Cluster local health call to throw exception if node is decommissioned or weighed away #6198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anshu1106
changed the title
anshu1106
changed the title
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
anshu1106
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
adnapibar,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
20 tasks
anshu1106
force-pushed
the
healthcheck-weighed-in
branch
from
c66aba1
to
024ff6a
Compare
Gradle Check (Jenkins) Run Completed with:
|
Bukhtawar
reviewed
Feb 6, 2023
| @@ -135,7 +135,7 @@ public void writeTo(StreamOutput out) throws IOException { | |||
| out.writeEnum(level); | |||
| } | |||
| if (out.getVersion().onOrAfter(Version.V_2_6_0)) { | |||
| out.writeBoolean(ensureNodeCommissioned); | |||
| out.writeBoolean(ensureNodeWeighedIn); | |||
There was a problem hiding this comment.
Will this cause BWC failures? If yes we can retain both flags and deprecate the former
There was a problem hiding this comment.
Since 2.6.0 is not released yet, do you this BWC is required? I am looking out for test failures in gradle build, will that be sufficient?
Bukhtawar
reviewed
Feb 6, 2023
Bukhtawar
reviewed
Feb 6, 2023
| clusterHealthRequest.ensureNodeCommissioned(false); | ||
| clusterHealthRequest.ensureNodeWeighedIn(false); |
There was a problem hiding this comment.
Can we please also add an integ tests to cover all combinations of healthy/unhealthy/weighed in and decommissioned
There was a problem hiding this comment.
Added
Decommission test is captured here
There are validations in place and decommission fail in case weight is not zero https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/cluster/decommission/DecommissionService.java#L458 |
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #6198 +/- ##
============================================
- Coverage 70.90% 70.84% -0.06%
+ Complexity 58904 58899 -5
============================================
Files 4778 4780 +2
Lines 281149 281166 +17
Branches 40622 40625 +3
============================================
- Hits 199346 199195 -151
- Misses 65480 65686 +206
+ Partials 16323 16285 -38
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Bukhtawar
reviewed
Feb 7, 2023
Comment on lines
-277
to
+283
| listener.onResponse(getResponse(request, currentState, waitCount, TimeoutState.OK)); | ||
| ClusterHealthResponse clusterHealthResponse = getResponse(request, currentState, waitCount, TimeoutState.OK); | ||
| if (request.ensureNodeWeighedIn() && clusterHealthResponse.hasDiscoveredClusterManager()) { | ||
| DiscoveryNode localNode = clusterService.state().getNodes().getLocalNode(); |
There was a problem hiding this comment.
Can we assert that local node is true?
Gradle Check (Jenkins) Run Completed with:
|
Bukhtawar
reviewed
Feb 8, 2023
...r/src/main/java/org/opensearch/action/admin/cluster/health/TransportClusterHealthAction.java
Outdated
Show resolved
Hide resolved
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
anshu1106
force-pushed
the
healthcheck-weighed-in
branch
from
4abfd6e
to
3ee469b
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
…d or weighed away Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
anshu1106
force-pushed
the
healthcheck-weighed-in
branch
from
3ee469b
to
c6a7aa0
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
anshu1106
force-pushed
the
healthcheck-weighed-in
branch
from
555fd27
to
6300a4e
Compare
Gradle Check (Jenkins) Run Completed with:
|
Bukhtawar
approved these changes
Feb 8, 2023
imRishN
reviewed
Feb 11, 2023
| listener.onResponse(getResponse(request, currentState, waitCount, TimeoutState.OK)); | ||
| ClusterHealthResponse clusterHealthResponse = getResponse(request, currentState, waitCount, TimeoutState.OK); | ||
| if (request.ensureNodeWeighedIn() && clusterHealthResponse.hasDiscoveredClusterManager()) { | ||
| DiscoveryNode localNode = clusterService.state().getNodes().getLocalNode(); |
There was a problem hiding this comment.
Why are we picking up the new state of the cluster and not the one where the observer actually succeeded. IMO, this should be currentState.getNodes().getLocalNode(). Because, what if something has changed in the state and entire health action was ran on a previous state. This might give misleading results
There was a problem hiding this comment.
didn't understand this completely. Do you mean clusterService.state() may provide new uncommitted state? If that is case, I understand healthcheck wait for cluster state change events to complete and cluster service also has committed cluster state. Let me know if I am missing anything?
imRishN
reviewed
Feb 11, 2023
| * @return true if the node has attribute value with shard routing weight set to zero, else false | ||
| */ | ||
| public static boolean isWeighedAway(String nodeId, ClusterState clusterState) { | ||
| DiscoveryNode node = clusterState.nodes().get(nodeId); |
There was a problem hiding this comment.
What if the node has dropped during this check? Line 45 will throw NullPointerException
There was a problem hiding this comment.
makes sense to put a null check. I think in this case we need to consider node as weigh away since we can't explicitly check for this. What are your thoughts?
imRishN
reviewed
Feb 11, 2023
Comment on lines
+25
to
+30
| /** | ||
| * This function checks if the node is weighed away ie weighted routing weight is set to 0, | ||
| * | ||
| * @param nodeId the node | ||
| * @return true if the node has attribute value with shard routing weight set to zero, else false | ||
| */ |
There was a problem hiding this comment.
As I understand, we would need this check in the method below -
if (node.getRoles().contains(DiscoveryNodeRole.DATA_ROLE) == false) {
return false;
}
There was a problem hiding this comment.
Or rather -
if (node == null || node.getRoles().contains(DiscoveryNodeRole.DATA_ROLE) == false) {
return false;
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Anshu Agarwal anshukag@amazon.com
Description
This PR adds a param to cluster health local call to check if a node is commissioned or weighed in.
Example Request/Response-
Weights are set using PUT api-
For a node that is not weighed away ie weighting routing weight is not 0
For a node in zone-3 with weighted routing weight set to 0
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.