[HealthAPI] Diagnosis: report typed affected resources by andreidan · Pull Request #90653 · elastic/elasticsearch

andreidan · 2022-10-04T17:21:18Z

The health API reports the affected resources in case of an unhealthy deployment. Until now all indicators reported one type of resource per diagnosis (index, ILM policy, snapshot repository)

With the introduction of the disk indicator we now have an indicator that reports multiple types of resources under the same diagnosis (ie. nodes and indices).

This changes the structure of the affected_resources field to accommodate multiple types of resources:

"affected_resources": {
  "nodes": [
    {
      "id": "e1af6F5rTcmgpExkdOMzCg",
      "name": "hot"
    },
    {
      "id": "u_wBVl4ZRne4uZq_ziLsuw",
      "name": "warm"
    }
  ],
  "indices": [
    ".geoip_databases",
    "test_index"
  ]
}

Fixes #90219

The health API reports the affected resources in case of an unhealthy deployment. Until now all indicators reported one type of resource per diagnosis (index, ILM policy, snapshot repository) With the introduction of the disk indicator we now have an indicator that reports multiple types of resources under the same diagnosis (ie. nodes and indices). This changes the structure of the `affected_resources` field to accommodate multiple types of resources: ``` "affected_resources": { "nodes": [ { "id": "e1af6F5rTcmgpExkdOMzCg", "name": "hot" }, { "id": "u_wBVl4ZRne4uZq_ziLsuw", "name": "warm" } ], "indices": [ ".geoip_databases", "test_index" ] } ```

elasticsearchmachine · 2022-10-04T17:21:42Z

Pinging @elastic/es-data-management (Team:Data Management)

andreidan · 2022-10-04T17:52:33Z

server/src/main/java/org/elasticsearch/health/Diagnosis.java

+            return false;
+        }
+        Diagnosis diagnosis = (Diagnosis) o;
+        return definition.equals(diagnosis.definition) && Arrays.equals(affectedResources, diagnosis.affectedResources);


This Diagnosis equals/hashcode as otherwise records will be unique (unless an explicit array is passed in the constructor).
Not sure varargs are worth all this though?

Removed them with 279ed2a

andreidan · 2022-10-04T18:55:55Z

@elasticmachine update branch

andreidan · 2022-10-05T07:57:51Z

Failure seems transient

org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':test:fixtures:s3-fixture:composeDown'.

andreidan · 2022-10-05T07:58:03Z

@elasticmachine run elasticsearch-ci/part-2

gmarouli

LGTM, thank you for picking this up! Some minor comments only :)

gmarouli · 2022-10-05T08:23:13Z

server/src/main/java/org/elasticsearch/health/Diagnosis.java

+                for (DiscoveryNode node : nodes) {
+                    builder.startObject();
+                    builder.field(ID_FIELD, node.getId());
+                    builder.field(NAME_FIELD, node.getName());


Should we check here if name is null to omit it?

Totally, great catch

gmarouli · 2022-10-05T08:37:03Z

server/src/main/java/org/elasticsearch/health/node/DiskHealthIndicatorService.java

            }
            if (masterNodes.containsKey(HealthStatus.YELLOW)) {
-                diagnosisList.add(createNonDataNodeDiagnosis(HealthStatus.YELLOW, masterNodes.get(HealthStatus.YELLOW), true));
+                List<DiscoveryNode> yellowMasterNodes = masterNodes.get(HealthStatus.YELLOW)


I am wondering if we should change the Set<DiscoveryNode> to a List<DiscoveryNode> and sort it in the constructor. I do not think the set is giving us anything now that I am thinking about it because we only encounter each node once in that for loop. What do you think?

Ah yes, this is a great idea. Coming up

gmarouli · 2022-10-05T08:54:25Z

.../elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorServiceTests.java

+                    List.of(
+                        new Diagnosis(
+                            DIAGNOSIS_WAIT_FOR_OR_FIX_DELAYED_SHARDS,
+                            List.of(new Diagnosis.Resource(INDEX, List.of("restarting" + "-index")))


I am guessing this wasn't intentional :).

Hm, what exactly? The formatting? I ran spotless

gmarouli · 2022-10-05T08:54:31Z

.../elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorServiceTests.java

+                    List.of(
+                        new Diagnosis(
+                            DIAGNOSIS_WAIT_FOR_OR_FIX_DELAYED_SHARDS,
+                            List.of(new Diagnosis.Resource(INDEX, List.of("restarting" + "-index")))


andreidan · 2022-10-05T11:58:28Z

Test failure is #90668

andreidan · 2022-10-05T11:58:49Z

@elasticmachine run elasticsearch-ci/part-1

andreidan · 2022-10-05T12:12:00Z

Test failure is transient Could not download ml-cpp-8.5.0-SNAPSHOT-deps.zip

andreidan · 2022-10-05T12:12:09Z

@elasticmachine update branch

elasticsearchmachine · 2022-10-05T13:11:59Z

💚 Backport successful

Status	Branch	Result
✅	8.5

The health API reports the affected resources in case of an unhealthy deployment. Until now all indicators reported one type of resource per diagnosis (index, ILM policy, snapshot repository) With the introduction of the disk indicator we now have an indicator that reports multiple types of resources under the same diagnosis (ie. nodes and indices). This changes the structure of the `affected_resources` field to accommodate multiple types of resources: ``` "affected_resources": { "nodes": [ { "id": "e1af6F5rTcmgpExkdOMzCg", "name": "hot" }, { "id": "u_wBVl4ZRne4uZq_ziLsuw", "name": "warm" } ], "indices": [ ".geoip_databases", "test_index" ] } ```

…#90678) The health API reports the affected resources in case of an unhealthy deployment. Until now all indicators reported one type of resource per diagnosis (index, ILM policy, snapshot repository) With the introduction of the disk indicator we now have an indicator that reports multiple types of resources under the same diagnosis (ie. nodes and indices). This changes the structure of the `affected_resources` field to accommodate multiple types of resources: ``` "affected_resources": { "nodes": [ { "id": "e1af6F5rTcmgpExkdOMzCg", "name": "hot" }, { "id": "u_wBVl4ZRne4uZq_ziLsuw", "name": "warm" } ], "indices": [ ".geoip_databases", "test_index" ] } ```

…lastic#90653) (elastic#90678)" This reverts commit 7b4f2d2.

andreidan added >bug :Distributed/Health Issues for the health report API v8.5.1 v8.6.0 labels Oct 4, 2022

andreidan requested a review from gmarouli October 4, 2022 17:21

elasticsearchmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Oct 4, 2022

andreidan added 3 commits October 4, 2022 18:31

Spotless

0af826e

Compilation

bdf861a

Fix record equals/hashcode to work with varargs

87b268d

andreidan commented Oct 4, 2022

View reviewed changes

andreidan added 3 commits October 4, 2022 19:16

Remove varargs

279ed2a

Fix yaml test

4e61930

Compile

6aa83a7

elasticmachine and others added 2 commits October 5, 2022 05:25

Merge branch 'main' into typed_affected_resources

6fe2b41

Update diagnosis version

838f980

gmarouli approved these changes Oct 5, 2022

View reviewed changes

Code review feedback

cdbc1ae

Merge branch 'main' into typed_affected_resources

f73d1b7

andreidan added the auto-backport Automatically create backport pull requests when merged label Oct 5, 2022

andreidan merged commit 5d97f0e into elastic:main Oct 5, 2022

andreidan mentioned this pull request Oct 5, 2022

[8.5] [HealthAPI] Diagnosis: report typed affected resources (#90653) #90678

Merged

andreidan added a commit to andreidan/elasticsearch that referenced this pull request Oct 6, 2022

Revert "[8.5] [HealthAPI] Diagnosis: report typed affected resources (e…

35d8449

…lastic#90653) (elastic#90678)" This reverts commit 7b4f2d2.

andreidan mentioned this pull request Oct 6, 2022

Revert "[8.5] [HealthAPI] Diagnosis: report typed affected resources … #90722

Merged

csoulios added v8.5.0 and removed v8.5.1 labels Nov 8, 2022

Conversation

andreidan commented Oct 4, 2022

Uh oh!

elasticsearchmachine commented Oct 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreidan commented Oct 4, 2022

Uh oh!

andreidan commented Oct 5, 2022

Uh oh!

andreidan commented Oct 5, 2022

Uh oh!

gmarouli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreidan Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreidan commented Oct 5, 2022

Uh oh!

andreidan commented Oct 5, 2022

Uh oh!

andreidan commented Oct 5, 2022

Uh oh!

andreidan commented Oct 5, 2022

Uh oh!

elasticsearchmachine commented Oct 5, 2022

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andreidan Oct 5, 2022 •

edited

Loading