Awareness health api rest and transport layer changes #5694

nishchay21 · 2023-01-04T07:08:10Z

Description

Adding rest and transport level changes for below approved PR #5231

Issues Resolved

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
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

github-actions · 2023-01-04T07:35:38Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/8782/
CommitID: b3843bf
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-01-04T07:49:07Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/8784/
CommitID: 1b62741

codecov-commenter · 2023-01-04T07:50:53Z

Codecov Report

Merging #5694 (eaede6b) into main (28e9b11) will increase coverage by 0.05%.
The diff coverage is 44.76%.

@@             Coverage Diff              @@
##               main    #5694      +/-   ##
============================================
+ Coverage     70.98%   71.04%   +0.05%     
- Complexity    58669    58769     +100     
============================================
  Files          4763     4771       +8     
  Lines        279945   280114     +169     
  Branches      40418    40425       +7     
============================================
+ Hits         198731   199014     +283     
+ Misses        64988    64842     -146     
- Partials      16226    16258      +32

Impacted Files	Coverage Δ
...awarenesshealth/ClusterAwarenessHealthRequest.java	`0.00% <0.00%> (ø)`
...sshealth/ClusterAwarenessHealthRequestBuilder.java	`0.00% <0.00%> (ø)`
.../src/main/java/org/opensearch/client/Requests.java	`46.42% <0.00%> (-0.85%)`	⬇️
.../org/opensearch/client/support/AbstractClient.java	`32.62% <0.00%> (+0.08%)`	⬆️
...earch/cluster/block/IndexCreateBlockException.java	`0.00% <0.00%> (ø)`
...rg/opensearch/common/settings/ClusterSettings.java	`91.89% <ø> (ø)`
...a/org/opensearch/test/OpenSearchIntegTestCase.java	`56.46% <0.00%> (-0.21%)`	⬇️
...action/admin/cluster/settings/SettingsUpdater.java	`91.17% <20.00%> (-5.65%)`	⬇️
...shealth/TransportClusterAwarenessHealthAction.java	`26.31% <26.31%> (ø)`
...dmin/cluster/RestClusterAwarenessHealthAction.java	`27.27% <27.27%> (ø)`
... and 513 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

imRishN · 2023-01-04T10:23:58Z

rest-api-spec/src/main/resources/rest-api-spec/api/cluster.awareness_attribute_health.json

+{
+  "cluster.awareness_attribute_health": {
+    "documentation": {
+      "url": "https://opensearch.org/docs/latest/opensearch/rest-api/awareness_attribute_health/",


We need to open a documentation issue for this

Already done opensearch-project/documentation-website#2281

imRishN · 2023-01-04T10:26:17Z

rest-api-spec/src/main/resources/rest-api-spec/api/cluster.awareness_attribute_health.json

+      "url": "https://opensearch.org/docs/latest/opensearch/rest-api/awareness_attribute_health/",
+      "description": "Returns basic information about the health of the cluster aggregated per awareness attribute."
+    },
+    "stability": "experimental",


Why is it experimental? I don't see API is behind feature flag. Is there any more work pending on this action?

This is discussed with gaurav as well . We will have this experimental and then GA later

Lets have a issue open to mark it stable later or the meta issues we are tracking?

imRishN · 2023-01-04T11:11:26Z

...java/org/opensearch/action/admin/cluster/awarenesshealth/ClusterAwarenessHealthResponse.java

+    @Override
+    public RestStatus status() {
+        return RestStatus.OK;
+    }


This is the default behaviour anyways, right?

That is correct

Then lets remove this override?

It is interface that is getting implemented we need to override the method

ClusterHealthResponse also has functionality for timeouts -

@Override public RestStatus status() { return isTimedOut() ? RestStatus.REQUEST_TIMEOUT : RestStatus.OK; }

Do we need something similar?

We can add that later if required. For now not a necessary parameter for API usage

imRishN · 2023-01-04T11:12:13Z

...g/opensearch/action/admin/cluster/awarenesshealth/TransportClusterAwarenessHealthAction.java

+            ClusterAwarenessHealthRequest::new,
+            indexNameExpressionResolver
+        );
+


nit: extra line

imRishN · 2023-01-04T11:13:10Z

...g/opensearch/action/admin/cluster/awarenesshealth/TransportClusterAwarenessHealthAction.java

+        ClusterState state,
+        ActionListener<ClusterAwarenessHealthResponse> listener
+    ) {
+        ClusterState currentState = clusterService.state();


state is already present in the method param

imRishN · 2023-01-04T11:13:44Z

...g/opensearch/action/admin/cluster/awarenesshealth/TransportClusterAwarenessHealthAction.java

+        ActionListener<ClusterAwarenessHealthResponse> listener
+    ) {
+        ClusterState currentState = clusterService.state();
+        ClusterSettings currentSettings = clusterService.getClusterSettings();


Can we inject it?

cluster settings are required from cluster service which is already injected

imRishN · 2023-01-04T12:39:03Z

.../java/org/opensearch/action/admin/cluster/awarenesshealth/ClusterAwarenessHealthRequest.java

+        if (awarenessAttributeName == null || awarenessAttributeName.isBlank()) {
+            return addValidationError("awareness attribute value seems to be missing", null);
+        }


Should we also check if the requested attribute is actually set in the cluster settings for awareness attribute? I believe it might be already handled in the service layer, but we can fail fast here

Yes that is correct that is handled in the service layer fail fast wont be necessary as other output which is cluster level health will be seen.

~~We should throw an exception from here. This should be a mandatory param .~~ Ignore, since we are already doing that.

imRishN · 2023-01-04T12:40:06Z

...java/org/opensearch/action/admin/cluster/awarenesshealth/ClusterAwarenessHealthResponse.java

+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject();
+        builder.field(CLUSTER_NAME, getClusterName());
+        builder.field(STATUS, getStatus().name().toLowerCase(Locale.ROOT));
+        builder.field(NUMBER_OF_NODES, getNumberOfNodes());
+        builder.field(NUMBER_OF_DATA_NODES, getNumberOfDataNodes());
+        builder.field(DISCOVERED_CLUSTER_MANAGER, hasDiscoveredClusterManager());
+        builder.field(ACTIVE_PRIMARY_SHARDS, getActivePrimaryShards());
+        builder.field(ACTIVE_SHARDS, getActiveShards());
+        builder.field(RELOCATING_SHARDS, getRelocatingShards());
+        builder.field(INITIALIZING_SHARDS, getInitializingShards());
+        builder.field(UNASSIGNED_SHARDS, getUnassignedShards());
+        builder.field(DELAYED_UNASSIGNED_SHARDS, getDelayedUnassignedShards());
+        builder.field(NUMBER_OF_PENDING_TASKS, getNumberOfPendingTasks());
+        builder.field(NUMBER_OF_IN_FLIGHT_FETCH, getNumberOfInflightFetches());
+        builder.humanReadableField(TASK_MAX_WAIT_TIME_IN_QUEUE_IN_MILLIS, TASK_MAX_WAIT_TIME_IN_QUEUE, getMaxWaitTime());
+        builder.percentageField(ACTIVE_SHARDS_PERCENT_AS_NUMBER, ACTIVE_SHARDS_PERCENT, getActiveShardsPercent());
+        getAwarenessAttributeHealth().toXContent(builder, params);
+        builder.endObject();
+        return builder;
+    }


Corresponding fromXContent?

Majority is tested on service layer itself.

imRishN

Thanks for the changes. Some minor comments, rest lgtm.

gbbafna · 2023-01-05T10:55:49Z

.../java/org/opensearch/action/admin/cluster/awarenesshealth/ClusterAwarenessHealthRequest.java

+        if (awarenessAttributeName == null || awarenessAttributeName.isBlank()) {
+            return addValidationError("awareness attribute value seems to be missing", null);
+        }


~~We should throw an exception from here. This should be a mandatory param .~~ Ignore, since we are already doing that.

gbbafna · 2023-01-05T10:57:13Z

...rg/opensearch/action/admin/cluster/awarenesshealth/ClusterAwarenessHealthRequestBuilder.java

+ *
+ * @opensearch.internal
+ */
+public class ClusterAwarenessHealthRequestBuilder extends ClusterManagerNodeReadOperationRequestBuilder<


this is not used anywhere ?

For now not but have kept for test cases . Do you want this to be removed completely?

#5694 (comment) Also on this it will throw a validation error

gbbafna · 2023-01-05T11:04:06Z

...g/opensearch/action/admin/cluster/awarenesshealth/TransportClusterAwarenessHealthAction.java

+        ClusterSettings currentSettings = clusterService.getClusterSettings();
+        String awarenessAttributeName = request.getAttributeName();
+        int numberOfPendingTasks = clusterService.getClusterManagerService().numberOfPendingTasks();
+        TimeValue maxWeightTime = clusterService.getClusterManagerService().getMaxTaskWaitTime();


typo : maxWaitTime instead of maxWeightTime .

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Bukhtawar · 2023-01-10T06:52:36Z

server/src/main/java/org/opensearch/action/admin/cluster/health/ClusterHealthRequest.java

+        if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
+            awarenessAttribute = in.readOptionalString();
+            level = in.readEnum(Level.class);


3.0 will require modifying this to 2.5 when you backport and then again modifying 2.5 to main. You can start with 2.5 and then directly backport, will be faster

Bukhtawar · 2023-01-10T06:54:04Z

server/src/main/java/org/opensearch/action/admin/cluster/health/ClusterHealthRequest.java

+            case "shards":
+                this.level = ClusterHealthRequest.Level.SHARDS;
+                break;
+            case "awareness_attribute":


awareness_attribute(s)

Bukhtawar · 2023-01-10T06:54:20Z

server/src/main/java/org/opensearch/action/admin/cluster/health/ClusterHealthRequest.java

+        if (level.equals(Level.AWARENESS_ATTRIBUTE) && indices.length > 0) {
+            return addValidationError("awareness_attribute is not a supported parameter with index health", null);
+        } else if (!level.equals(Level.AWARENESS_ATTRIBUTE) && awarenessAttribute != null) {
+            return addValidationError("level=awareness_attribute is required with awareness_attribute parameter", null);


awareness_attribute(s)

Bukhtawar · 2023-01-10T06:54:34Z

server/src/main/java/org/opensearch/action/admin/cluster/health/ClusterHealthRequest.java

@@ -290,6 +332,7 @@ public ActionRequestValidationException validate() {
    public enum Level {
        CLUSTER,
        INDICES,
-        SHARDS
+        SHARDS,
+        AWARENESS_ATTRIBUTE


AWARENESS_ATTRIBUTE(S)

Bukhtawar · 2023-01-10T06:57:22Z

server/src/main/java/org/opensearch/action/admin/cluster/health/ClusterHealthResponse.java

+        this.clusterName = clusterName;
+        this.numberOfPendingTasks = numberOfPendingTasks;
+        this.numberOfInFlightFetch = numberOfInFlightFetch;
+        this.delayedUnassignedShards = delayedUnassignedShards;
+        this.taskMaxWaitingTime = taskMaxWaitingTime;
+        this.clusterStateHealth = new ClusterStateHealth(clusterState);
+        this.clusterAwarenessHealth = new ClusterAwarenessHealth(clusterState, clusterSettings, awarenessAttributeName);
+        this.clusterHealthStatus = clusterStateHealth.getStatus();


change to

this(.....); this.clusterAwarenessHealth = new ClusterAwarenessHealth(clusterState, clusterSettings, awarenessAttributeName);

github-actions · 2023-01-10T07:45:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9137/
CommitID: b35667f
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

github-actions · 2023-01-10T08:21:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9148/
CommitID: a6d128e
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-01-10T08:27:09Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9147/
CommitID: 352c47f
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-01-10T08:44:48Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9149/
CommitID: d62dfc8
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Bukhtawar · 2023-01-10T09:03:18Z

rest-api-spec/src/main/resources/rest-api-spec/api/cluster.health.json

@@ -45,7 +45,8 @@
        "options":[
          "cluster",
          "indices",
-          "shards"
+          "shards",
+          "awareness_attribute"


awareness_attribute(s) here and at all other places

Bukhtawar · 2023-01-10T09:03:27Z

rest-api-spec/src/main/resources/rest-api-spec/api/cluster.health.json

@@ -106,6 +107,10 @@
          "red"
        ],
        "description":"Wait until cluster is in a specific state"
+      },
+      "awareness_attribute":{


Bukhtawar

Please add an integ tests in ClusterHealthIT class and BWC tests

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

github-actions · 2023-01-10T10:44:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9162/
CommitID: 156bc0e
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

opensearch-trigger-bot · 2023-01-10T11:28:08Z

The backport to 2.5 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.5 2.5
# Navigate to the new working tree
pushd ../.worktrees/backport-2.5
# Create a new branch
git switch --create backport/backport-5694-to-2.5
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 386ce9b2fa64a959860cabaff478024dc8879ee4
# Push it to GitHub
git push --set-upstream origin backport/backport-5694-to-2.5
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.5

Then, create a pull request where the base branch is 2.5 and the compare/head branch is backport/backport-5694-to-2.5.

opensearch-trigger-bot · 2023-01-10T11:28:16Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5694-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 386ce9b2fa64a959860cabaff478024dc8879ee4
# Push it to GitHub
git push --set-upstream origin backport/backport-5694-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5694-to-2.x.

github-actions · 2023-01-10T11:39:27Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9166/
CommitID: 156bc0e
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

nishchay21 requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, adnapibar, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners January 4, 2023 07:08

gbbafna requested a review from imRishN January 4, 2023 09:39

imRishN reviewed Jan 4, 2023

View reviewed changes

imRishN approved these changes Jan 4, 2023

View reviewed changes

gbbafna reviewed Jan 5, 2023

View reviewed changes

Adding awareness health api rest and transport layer changes

7439a0c

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

nishchay21 force-pushed the awareness-health-rest branch from ab55875 to 7439a0c Compare January 5, 2023 11:59

Bukhtawar reviewed Jan 10, 2023

View reviewed changes

gbbafna mentioned this pull request Jan 10, 2023

Add API to check per awareness attribute value health. #5230

Closed

nishchay21 added 3 commits January 10, 2023 13:34

Added changes as per comments

352c47f

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Added changes as per comments

a6d128e

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Added changes as per comments

d62dfc8

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

gbbafna approved these changes Jan 10, 2023

View reviewed changes

Bukhtawar reviewed Jan 10, 2023

View reviewed changes

Added integ tests

156bc0e

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Bukhtawar approved these changes Jan 10, 2023

View reviewed changes

gbbafna merged commit 386ce9b into opensearch-project:main Jan 10, 2023

gbbafna added backport 2.x Backport to 2.x branch backport 2.5 labels Jan 10, 2023

Awareness health api rest and transport layer changes #5694

Awareness health api rest and transport layer changes #5694

Conversation

nishchay21 commented Jan 4, 2023

Description

Issues Resolved

Check List

github-actions bot commented Jan 4, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 4, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Jan 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imRishN Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishchay21 Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

gbbafna Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishchay21 Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

imRishN left a comment

Choose a reason for hiding this comment

gbbafna Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukhtawar Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 10, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 10, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 10, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukhtawar left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2023

Gradle Check (Jenkins) Run Completed with:

opensearch-trigger-bot bot commented Jan 10, 2023

opensearch-trigger-bot bot commented Jan 10, 2023

github-actions bot commented Jan 10, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Jan 4, 2023 •

edited

Loading

imRishN Jan 4, 2023 •

edited

Loading

nishchay21 Jan 4, 2023 •

edited

Loading

gbbafna Jan 5, 2023 •

edited

Loading

nishchay21 Jan 4, 2023 •

edited

Loading

gbbafna Jan 5, 2023 •

edited

Loading

Bukhtawar Jan 10, 2023 •

edited

Loading

Bukhtawar left a comment •

edited

Loading