Adding impacts block to the health info API response by masseyke · Pull Request #84899 · elastic/elasticsearch

masseyke · 2022-03-10T23:37:41Z

This change introduces the notion of an impact to a health API indicator, and adds impacts to all currently-existing
indicators.
Closes #84773

elasticmachine · 2022-03-10T23:37:44Z

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

elasticsearchmachine · 2022-03-15T13:56:14Z

Hi @masseyke, I've created a changelog YAML for you.

andreidan

Thanks for working on this Keith.

I've left a couple of minor questions. Can you please post an example of the output in the description?

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

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java

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

…e/elasticsearch into feature/health-api-impacts-block

masseyke · 2022-03-16T22:06:50Z

Here's an example response. You can see empty impact blocks for green items, and a filled out impact block for the shards_availability indicator.

{
    "cluster_name": "elasticsearch",
    "components": {
        "cluster_coordination": {
            "indicators": {
                "instance_has_master": {
                    "details": {
                        "coordinating_node": {
                            "name": "keiths-mbp.lan",
                            "node_id": "R3iN7tj1TP6dfAQCNFzFmA"
                        },
                        "master_node": {
                            "name": "keiths-mbp.lan",
                            "node_id": "R3iN7tj1TP6dfAQCNFzFmA"
                        }
                    },
                    "impact": {},
                    "status": "green",
                    "summary": "Health coordinating instance has a master node."
                }
            },
            "status": "green"
        },
        "data": {
            "indicators": {
                "ilm": {
                    "details": {
                        "ilm_status": "RUNNING",
                        "policies": 15
                    },
                    "impact": {},
                    "status": "green",
                    "summary": "ILM is running"
                },
                "shards_availability": {
                    "details": {
                        "creating_primaries": 0,
                        "initializing_primaries": 0,
                        "initializing_replicas": 0,
                        "restarting_primaries": 0,
                        "restarting_replicas": 0,
                        "started_primaries": 12,
                        "started_replicas": 0,
                        "unassigned_primaries": 0,
                        "unassigned_replicas": 18
                    },
                    "impact": {
                        "impact_description": "Redundancy for 3 indices [my-index-000001, my-index-000003, my-index-000002] is currently disrupted. Fault tolerance and search scalability are reduced.",
                        "severity": 3
                    },
                    "status": "yellow",
                    "summary": "This cluster has 18 unavailable replicas."
                }
            },
            "status": "yellow"
        },
        "snapshot": {
            "indicators": {
                "repository_integrity": {
                    "details": {},
                    "impact": {},
                    "status": "green",
                    "summary": "No repositories configured."
                },
                "slm": {
                    "details": {
                        "policies": 0,
                        "slm_status": "RUNNING"
                    },
                    "impact": {},
                    "status": "green",
                    "summary": "No policies configured"
                }
            },
            "status": "green"
        }
    },
    "status": "yellow"
}

Here's the impact if the primaries and replicas are unavailable:

                    "impact": {
                        "impact_description": "Indices [my-index-000001, my-index-000003, my-index-000002] are unavailable. You are at risk of losing the data in these indices.",
                        "severity": 1
                    },

And here is the impact if primaries are unavailable but replicas are:

                    "impact": {
                        "impact_description": "Cannot add data to 1 index [my-index-000001]. Searches might return incomplete results.",
                        "severity": 2
                    },

In all of the above, it will show at most 10 indices. If there are more, it will include ... at the end. Also note that the impact can be populated even if the status is green. For example if a user has disabled replication there will be zero replicas. The status will be green (because no replicas are unavailable). But we still populate the impact to say that redundancy and fault tolerance are impacted.
Here is the impact block if there is no master:

                    "impact": {
                        "impact_description": "The cluster cannot create, delete, or rebalance indices, and is likely to be very unstable",
                        "severity": 1
                    },

And if there are corrupted repositories:

                    "impact": {
                        "impact_description": "Snapshots in corrupted repositories cannot be restored. Data loss is possible.",
                        "severity": 2
                    },

If ILM is expected to be running but is not:

                    "impact": {
                        "impact_description": "Indices are not being rolled over, which could lead to future instability.",
                        "severity": 3
                    },

And if SLM is expected to be running but is not:

                    "impact": {
                        "impact_description": "Scheduled snapshots are not happening, which could lead to future data loss.",
                        "severity": 3
                    },

masseyke · 2022-03-16T22:13:44Z

@elasticmachine update branch

andreidan

Thanks Keith.

I just had a couple of comments.

In the interest of keeping scope to a minimum can you please remove the impacts for all indicators except the shards_availability one? I think it'd be easier to have separate PRs for the other impacts and merge them independently The cluster coordination one for e.g. needs a lot of work to be able to safely indicate the master is unstable, the snapshot one might want to indicate which repository is corrupted etc
What do you think?

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java

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

masseyke · 2022-03-17T16:36:39Z

Thanks Keith.

I just had a couple of comments.

In the interest of keeping scope to a minimum can you please remove the impacts for all indicators except the shards_availability one? I think it'd be easier to have separate PRs for the other impacts and merge them independently The cluster coordination one for e.g. needs a lot of work to be able to safely indicate the master is unstable, the snapshot one might want to indicate which repository is corrupted etc What do you think?

I'll change them to "TODO". With this change, we have to have some impact so I figured I'd at least put a placeholder in for all of them. The snapshot health indicator already reports the repository name in the summary so I was avoiding duplicating that in the impact.

andreidan

Thanks for iterating on this Keith.

On the TODO aspect, I think this would make it harder for the UI team to work out if they should display the impact or not.
How about we don't include the impact section at all for the indicators for which we don't have the impacts yet? (so for this PR only the shards_availability indicator will report an impact section)

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java

masseyke · 2022-03-18T13:18:29Z

Thanks for iterating on this Keith.

On the TODO aspect, I think this would make it harder for the UI team to work out if they should display the impact or not. How about we don't include the impact section at all for the indicators for which we don't have the impacts yet? (so for this PR only the shards_availability indicator will report an impact section)

I assumed the UI team was just showing whatever string we gave them, and that it would be better to see a placeholder than nothing. I'll change it to an empty impact (there's no such thing as no impact any more).

andreidan · 2022-03-18T14:56:43Z

I assumed the UI team was just showing whatever string we gave them, and that it would be better to see a placeholder than nothing. I'll change it to an empty impact (there's no such thing as no impact any more).

Thanks for iterating on this Keith.

Whilst they're testing the first iterations we might not have impacts for all indicators. I think our APIs generally don't report empty blocks e.g. https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplanation.java#L171 , https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/IndexLifecycleExplainResponse.java#L471

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

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java

andreidan

Thanks for iterating on this Keith. I think this is nearly done.

I've left one question.

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java

andreidan · 2022-03-21T13:18:58Z

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java

+            if (indicesWithUnavailablePrimariesAndReplicas.isEmpty() == false) {
+                String impactDescription = String.format(
+                    Locale.ROOT,
+                    "%s [%s] %s unavailable. You are at risk of losing the data in %s.",


The current impacts are:

Primaries available, replicas unassigned:

"impact": { "description": "Redundancy for 3 indices [my-index-000001, my-index-000003, my-index-000002] is currently disrupted. Fault tolerance and search scalability are reduced.", "severity": 3 }

edit: update category to "Primaries unassigned"
Primaries unassigned:

"impact": { "description": "Cannot add data to 1 index [my-index-000001]. Searches might return incomplete results.", "severity": 2 }

Both primaries and replicas unavailable:

"impact": { "description": "Indices [my-index-000001, my-index-000003, my-index-000002] are unavailable. You are at risk of losing the data in these indices.", "severity": 1 }

@cristina-eleonora @shubhaat @Leaf-Lin Can you please provide some feedback on these messages?

Thanks for the ping @andreidan !

Primaries available, replicas unassigned:

"impact": { "description": "Redundancy for 3 indices [my-index-000001, my-index-000003, my-index-000002] is currently disrupted. Fault tolerance and search scalability are reduced.", "severity": 3 }

Could we rephrase the first half to something like: Data redundancy is affected on 3 indices [my-index-000001, my-index-000003, my-index-000002]. ? Are there any impacts on search/ingest? And could we phrase Fault tolerance and search scalability are reduced. in a way that a non technical / minimally technical person would understand? Perhaps some of the writers could help with this.

Primaries unassigned, but replicas available:

"impact": { "description": "Cannot add data to 1 index [my-index-000001]. Searches might return incomplete results.", "severity": 2 }

Looks great 👍🏻

Both primaries and replicas unavailable:

"impact": { "description": "Indices [my-index-000001, my-index-000003, my-index-000002] are unavailable. You are at risk of losing the data in these indices.", "severity": 1 }

Does this mean that those indices cannot be accessed anymore? Or that the data on those indices cannot be accessed? How will that impact search/ingest?

I thought Primaries unassigned, but replicas available would not happen because replica will be promoted?

Based on @Leaf-Lin comment above we decided to simplify the impacts into the following categories:

primaries unavailable for indices:

Cannot add data to 1 index [my-index-000001]. Searches will return incomplete results.

replicas unavailable for indices:

Data redundancy is affected on 3 indices [my-index-000001, my-index-000003, my-index-000002]. Fault tolerance and search scalability are reduced.

Thanks for your thoughts on the messages @cristina-eleonora
I think with the simplified cases we have the Fault tolerance and search scalability are reduced. bit to rephrase somehow.
When talking about replicas in our docs we say:

Replicas provide redundant copies of your data to protect against hardware failure and increase capacity to serve read requests like searching or retrieving a document

Would impact number 2 make more sense if it was: (or something along those lines?)

Data redundancy is affected on 3 indices [my-index-000001, my-index-000003, my-index-000002]. The capacity to serve search requests is reduced and you could be susceptible to data loss in case of hardware failure.

OK I have the wording in from Andre's last comment. I can easily change it if we need to.

For missing replicas, how about just:
Searches might return slower than expected. Fewer redundant copies of the data exist than expected on 3 indices [my-index-000001, my-index-000003, my-index-000002].
Or maybe the slightly scarier:
Searches might return slower than expected, and you are at higher risk of the cluster becoming unstable in the future. Fewer redundant copies of the data exist than expected on 3 indices [my-index-000001, my-index-000003, my-index-000002].
Or the even scarier:
Searches might return slower than expected, and you are at higher risk of losing data. Fewer redundant copies of the data exist than expected on 3 indices [my-index-000001, my-index-000003, my-index-000002].

I personally like the 1st and the 3rd 🙂 The third is more explicit on the importance of replicas, but may scare the users. If they should be scared, let's keep it, if not, perhaps use the 1st.

Pinging @alaudazzi , our UX writer.

I like 1 too, with the only mentions that "than expected ..." is repeated and in the 2nd sentence it should come earlier (?)
I suggest:

Searches might return slower than usual. Fewer than expected redundant copies of the data exist on 3 indices [my-index-000001, my-index-000003, my-index-000002].

Or maybe even drop "than expected" in the 2nd sentence altogether (it reads very strange to me)

Searches might return slower than usual. Fewer redundant copies of the data exist on 3 indices [my-index-000001, my-index-000003, my-index-000002].

I went with Andre's last wording, but can update at any point later if we work out something better.

andreidan

LGTM, thanks for iterating on this Keith

Left 2 very minor comments, but I think we can merge this soon as the messaging seems to be getting some consensus.

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

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java

masseyke · 2022-03-24T16:58:42Z

@elasticmachine run CLA

The ability to add impacts to indicators was added in #84899, but impacts for all indicators other than shards availability were left empty. This commit adds potential impacts for the other indicators.

masseyke added 5 commits March 10, 2022 08:58

Initial health API impact work

7b9e9ca

more work in progress

eb31b6d

Fixing impact messages

6187d25

unit test

e93001e

Adding impacts for all indicators

99380e4

masseyke added :Distributed/Health Issues for the health report API v8.2.0 labels Mar 10, 2022

elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Mar 10, 2022

andreidan added the >feature label Mar 15, 2022

Update docs/changelog/84899.yaml

ea47508

andreidan reviewed Mar 15, 2022

View reviewed changes

masseyke added 2 commits March 16, 2022 10:31

Code review feedback

d46fc87

Merge branch 'feature/health-api-impacts-block' of github.com:masseyk…

97d9a80

…e/elasticsearch into feature/health-api-impacts-block

masseyke requested a review from andreidan March 16, 2022 15:36

masseyke added 4 commits March 16, 2022 11:25

Code review feedback

c4751aa

Code review feedback

5dad115

Making the number of indices shown higher

398d240

Updating unit tests

ae93e5c

formatting

b083e40

elasticmachine and others added 2 commits March 16, 2022 17:13

Merge branch 'master' into feature/health-api-impacts-block

76bdecc

fixing a merge

a15f50c

andreidan requested changes Mar 17, 2022

View reviewed changes

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/health/HealthIndicatorImpact.java Outdated Show resolved Hide resolved

masseyke added 2 commits March 17, 2022 11:51

Marking non-disk-related impacts as TODO

8e24c9a

Removing impact when replication is 0

bae3f60

andreidan reviewed Mar 18, 2022

View reviewed changes

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java Show resolved Hide resolved

removing placeholder impacts

1050190

andreidan reviewed Mar 18, 2022

View reviewed changes

server/src/main/java/org/elasticsearch/health/SimpleHealthIndicatorImpact.java Outdated Show resolved Hide resolved

andreidan reviewed Mar 18, 2022

View reviewed changes

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java Outdated Show resolved Hide resolved

Returning multiple impacts for a single indicator

93d213a

andreidan reviewed Mar 21, 2022

View reviewed changes

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java Outdated Show resolved Hide resolved

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java Outdated Show resolved Hide resolved

andreidan reviewed Mar 21, 2022

View reviewed changes

masseyke added 4 commits March 22, 2022 12:56

putting index names into an ordinary HashSet

7486b77

Merge branch 'master' into feature/health-api-impacts-block

b47f8b4

Updating impact wording

7db2a57

spotlessApply

210a454

masseyke mentioned this pull request Mar 23, 2022

Extract actions for the shard availability health indicator #84797

Closed

masseyke added 3 commits March 23, 2022 10:05

Sorting impacts by severity, and limiting to 3 per indicator

ddca38b

spotlessApply

a06bb45

simplifying code

20d8940

andreidan approved these changes Mar 24, 2022

View reviewed changes

server/src/main/java/org/elasticsearch/health/HealthIndicatorResult.java Outdated Show resolved Hide resolved

...a/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java Show resolved Hide resolved

masseyke added 4 commits March 24, 2022 08:37

code review feedback

11d05a6

checkstyle violation

cb1dfc4

Updating impact message

f302f09

checkstyle violation

e4ca584

masseyke merged commit 99fe8dc into elastic:master Mar 24, 2022

masseyke deleted the feature/health-api-impacts-block branch March 24, 2022 17:01

masseyke mentioned this pull request Mar 24, 2022

Sorting impact index names by index priority #85347

Merged

masseyke mentioned this pull request Apr 26, 2022

Adding potential impacts to remaining health indicators #86197

Merged

Conversation

masseyke commented Mar 10, 2022

Uh oh!

elasticmachine commented Mar 10, 2022

Uh oh!

elasticsearchmachine commented Mar 15, 2022

Uh oh!

andreidan left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

masseyke commented Mar 16, 2022

Uh oh!

masseyke commented Mar 16, 2022

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

masseyke commented Mar 17, 2022

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

masseyke commented Mar 18, 2022

Uh oh!

andreidan commented Mar 18, 2022

Uh oh!

Uh oh!

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andreidan Mar 21, 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!

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 Mar 24, 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!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

masseyke commented Mar 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

andreidan left a comment •

edited

Loading

andreidan Mar 21, 2022 •

edited

Loading

andreidan Mar 24, 2022 •

edited

Loading