Health API - Monitoring local disk health by gmarouli · Pull Request #88390 · elastic/elasticsearch

gmarouli · 2022-07-08T15:47:31Z

This PR introduces the local health monitoring functionality needed for #84811 . The monitor uses the NodeService to get the disk usage stats and determines the node's disk health.

When a change in the disk's is detected or when the health node changes, this class would be responsible to send the node's health to the health node. Currently this is simulated with a method that just logs the current health.

The monitor keeps the last reported health, this way, if something fails on the next check it will try to resend the new health state.

gmarouli · 2022-07-25T10:08:57Z

@elasticmachine update branch

gmarouli · 2022-07-28T10:10:38Z

@elasticmachine update branch

elasticsearchmachine · 2022-07-28T14:28:06Z

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

andreidan

Thanks for working on this Mary

I left a first set of comments (and a suggestion to break this PR up)

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

andreidan · 2022-07-29T10:06:18Z

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

+            if (healthMetadataInitialized == false) {
+                healthMetadataInitialized = HealthMetadata.getFromClusterState(event.state()) != null;
+                if (healthMetadataInitialized) {
+                    scheduleNextRunIfEnabled(TimeValue.timeValueMillis(1));


I think this should schedule based the monitoring based on the normal interval

hm, you mean that if it gets rescheduled it should have the initial delay?

I have to admit that when I see something executed at an interval I do not expect to have an initial delay. I expect that it's going to be executed asap and then wait for the interval before the next execution. For example let's say that the interval is 10 minutes. I would think it's wrong to wait for 10 minutes before running it for the first time.

What do you think?

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

gmarouli · 2022-07-29T12:46:19Z

Update:

The sending is removed and left for the next PR.
We use scheduleUnlessShuttingDown and used an AtomicBoolean to ensure the single execution of the monitoring. I like this approach more. I think this reads better and it doesn't do extra cancelling and scheduling. If there are multiple runs scheduled at the same time only one will run, the other ones will just do nothing. What do you think?

gmarouli · 2022-07-29T13:04:03Z

Investigating the test failure. I can reproduce the failure locally too with this seed:

./gradlew ':server:test' --tests "org.elasticsearch.health.node.LocalHealthMonitorTests.testEnablingAndDisabling" -Dtests.seed=C307AA9DA1F7BA82 -Dtests.locale=es-CO -Dtests.timezone=Etc/Greenwich -Druntime.java=17

gmarouli · 2022-07-29T13:10:53Z

Test fixed. The issue was that we removed the scheduled field that was used both to cancel a run but also to signal not allow setting changes to schedule runs. Fixed by using healthMetadataInitialized for the same purpose.

gmarouli · 2022-07-29T14:47:13Z

@elasticmachine update branch

gmarouli · 2022-07-29T20:28:39Z

@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample

andreidan

Thanks for iterating on this Mary

Left a few more suggestions

server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java

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

andreidan · 2022-07-29T15:19:02Z

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

+        if (inProgress.compareAndSet(false, true)) {
+            ClusterState clusterState = clusterService.state();
+            HealthMetadata healthMetadata = HealthMetadata.getFromClusterState(clusterState);
+            assert healthMetadata != null : "health metadata should have been initialized.";


is this assertion fair?

The master node might've not initialised the health metadata but the monitor might be running because the service is enabled ?

Before we schedule any monitoring task we check the flag healthMetadataInitialized so theoretically this should hold true. The only case I can think of, is if someone removes the health metadata. But I think we do not give that option programmatically. Would you prefer to have an if here?

Ah, right. The HealthMetadataService is controlled by a different setting - enabled/disabled via health.node.enabled so that made me think things can diverge.
I prefer the explicit checks, but it's a personal preference so feel free to ignore

They are all controlled by the same flag health.node.enabled. The reason I have done this, is that if the health node is disabled, there is no point to monitor the disk health locally, or to publish health metadata right?

On a second thought, depending on the use case @jbaiera is working on we might want to decouple this and have the health metadata always be published. They might be useful for the coordinator node too.

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

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

andreidan · 2022-08-01T09:01:52Z

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

+        // Wait until every node in the cluster is upgraded to 8.4.0 or later
+        if (event.state().nodesIfRecovered().getMinNodeVersion().onOrAfter(Version.V_8_4_0)) {
+            // Wait until the health metadata is available in the cluster state
+            if (healthMetadataInitialized == false) {


This flag is a little misleading as it waits for a bit more than just the metadata to be initialised (ie. nodes versions). Could we rename it to reflect that?

Also, if the service is enabled,disabled, and enabled again we might end up with 2 scheduled monitoring tracks? (since setEnabled doesn't check if we're already running?)

About the scheduling two monitoring tasks. That's true, but then one will see that the inProgress flag is not false and it will just exit without doing anything or scheduling anything. I thought it was good trade-off compared to locking. What do you think?

Yes ! That's a good trade-off 🚀 Can you please document it?

Ah yes! Good point.

Done, let me know if it's clear.

Also renamed healthMetadataInitialized to something more inclusive.

server/src/main/java/org/elasticsearch/health/node/selection/HealthNode.java

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

andreidan

Thanks for iterating on this.

Left a could more comments

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

andreidan · 2022-08-02T09:47:26Z

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

+    // monitoring tasks scheduled, one of them will be no-op.
+    private final AtomicBoolean inProgress = new AtomicBoolean();
+    // Keeps the latest health state that was successfully reported.
+    private DiskHealthInfo lastReportedDiskHealthInfo = null;


should this be volatile to ensure it's written to main memory? (being written from another thread, without volatile it might not be immediately visible as it'll not be flushed to main memory ie. getLastReportedDiskHealthInfo might not see the "last reported" info)

Yeap, I probably removed it accidentally at some point.

andreidan · 2022-08-02T09:50:54Z

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

+        if (prerequisitesFulfilled == false) {
+            prerequisitesFulfilled = event.state().nodesIfRecovered().getMinNodeVersion().onOrAfter(Version.V_8_5_0)
+                && HealthMetadata.getFromClusterState(event.state()) != null;
+            scheduleNowIfEnabled();
+        }


this seems to schedule the polling even if the prerequisites were not fulfilled?

could the version check be still 8_4_0 as we have the metadata already? or is there a reason we should wait for 8.5?

Hm, I think I need to rename this method. We check the prerequisites in the method scheduleNowIfEnabled. This was a choice I made to ensure the prerequisites are always checked before any scheduling. For this reason it felt redundant to check it again here. What do you think about renaming to maybeScheduleNow() indicating that it is not a given that it will schedule it?

About 8.5.0 you are right the healthMetadata do exists in 8.4.0 but this local monitor will be pushing changes to the HealthNode and that endpoint is not implemented in 8.4.0. That's why I thought that the minimum required version should be 8.5.0. Does this make sense?

Ah ++ fair enough

andreidan

LGTM thanks for iterating on this Mary 🚀

gmarouli · 2022-08-03T07:06:56Z

@elasticmachine update branch

gmarouli · 2022-08-03T07:34:29Z

@elasticmachine run elasticsearch-ci/part-1

gmarouli · 2022-08-03T07:49:47Z

@andreidan thanks for the feedback!

elasticsearchmachine added the v8.4.0 label Jul 8, 2022

gmarouli mentioned this pull request Jul 8, 2022

Disk Usage health indicator #84811

Closed

9 tasks

gmarouli changed the title ~~Health disk monitor health~~ Health API - Monitoring local disk health Jul 8, 2022

gmarouli added 2 commits July 21, 2022 10:40

Calculate free disk space

8cb9032

Monitor disk health

d636893

gmarouli force-pushed the health-disk-monitor-health branch from 5013389 to d636893 Compare July 21, 2022 08:56

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:05

Small improvements

bd37694

elasticmachine and others added 2 commits July 25, 2022 19:38

Merge branch 'main' into health-disk-monitor-health

e0302b6

Move package

ff99009

mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022

Merge branch 'main' into health-disk-monitor-health

9d9ac33

elasticmachine and others added 4 commits July 28, 2022 19:40

Merge branch 'main' into health-disk-monitor-health

0849005

Implement send data when new health node detected

dba5f22

Rephrase the javadoc of LocalHealthMonitor

60ca036

Ensure sending health is race free

a6d5b70

gmarouli requested a review from andreidan July 28, 2022 12:56

gmarouli added >non-issue :Distributed/Health Issues for the health report API labels Jul 28, 2022

gmarouli added 4 commits July 28, 2022 16:00

fix formatting

ac6b24b

Change export order

6b334c7

polish

69f2fe4

fix formatting

216d7f3

gmarouli marked this pull request as ready for review July 28, 2022 14:27

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

andreidan reviewed Jul 29, 2022

View reviewed changes

gmarouli added 2 commits July 29, 2022 15:18

Remove sending data simulation

321414c

Refactor the synchronization of monitoring

5c187a9

gmarouli requested a review from andreidan July 29, 2022 12:46

No runs before metadata is initialized

eaba96d

Merge branch 'main' into health-disk-monitor-health

8908c9f

andreidan reviewed Aug 1, 2022

View reviewed changes

gmarouli added 3 commits August 1, 2022 15:10

Merge branch 'main' into health-disk-monitor-health

d36b2bd

Polishing after review

c738639

Remove IndividualNodeHealth.java

7dc7bba

gmarouli mentioned this pull request Aug 1, 2022

Extract least/most available paths to DiskUsage #88996

Merged

gmarouli added 4 commits August 1, 2022 21:35

Merge branch 'main' into health-disk-monitor-health

8f84751

Document synchronizing with inProgress flag.

e091273

Fix version

3490b19

Rename healthMetadataInitialized to be inclusive

e16431a

gmarouli requested a review from andreidan August 1, 2022 19:42

andreidan reviewed Aug 2, 2022

View reviewed changes

Clarify scheduling, make lastReported volatile

241cb2c

andreidan approved these changes Aug 2, 2022

View reviewed changes

Merge branch 'main' into health-disk-monitor-health

a1d1f08

gmarouli added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 3, 2022

elasticsearchmachine merged commit d828c2a into elastic:main Aug 3, 2022

gmarouli deleted the health-disk-monitor-health branch August 3, 2022 08:10

Conversation

gmarouli commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmarouli commented Jul 25, 2022

Uh oh!

gmarouli commented Jul 28, 2022

Uh oh!

elasticsearchmachine commented Jul 28, 2022

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gmarouli commented Jul 29, 2022

Uh oh!

gmarouli commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmarouli commented Jul 29, 2022

Uh oh!

gmarouli commented Jul 29, 2022

Uh oh!

gmarouli commented Jul 29, 2022

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

gmarouli Aug 1, 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!

Uh oh!

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

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!

gmarouli commented Jul 8, 2022 •

edited

Loading

gmarouli commented Jul 29, 2022 •

edited

Loading

gmarouli Aug 1, 2022 •

edited

Loading