Remove node from cluster when node locks broken #61400

amoghRZP · 2020-08-21T05:27:35Z

In #52680 we introduced a mechanism that will allow nodes to remove
themselves from the cluster if they locally determine themselves to be
unhealthy. The only check today is that their data paths are all
empirically writeable. This commit extends this check to consider a
failure of NodeEnvironment#assertEnvIsLocked() to be an indication of
unhealthiness.

Closes #58373

DaveCTurner

Thanks @amoghRZP, this is good work. I requested a few small changes but nothing fundamental.

DaveCTurner · 2020-08-21T07:02:23Z

server/src/main/java/org/elasticsearch/monitor/fs/FsHealthService.java

-                    }
-                    currentUnhealthyPaths.add(path);
                }
+            } catch (IllegalStateException e) {


I'd prefer the try{} block only to contain the call to nodeEnv.nodeDataPaths(), would you reduce its scope? That way you don't need a local lockAssertionFailed, you can set brokenLock and exit immediately.

DaveCTurner · 2020-08-21T07:06:59Z

server/src/main/java/org/elasticsearch/monitor/fs/FsHealthService.java

        if (enabled == false) {
            statusInfo = new StatusInfo(HEALTHY, "health check disabled");
+        } else if (brokenLock == true) {
+            statusInfo = new StatusInfo(UNHEALTHY, "health check failed on node due to broken locks");


Minor wording nit: specify which lock was broken (and remove redundant on node), suggest this:

Suggested change

statusInfo = new StatusInfo(UNHEALTHY, "health check failed on node due to broken locks");

statusInfo = new StatusInfo(UNHEALTHY, "health check failed due to broken node lock");

DaveCTurner · 2020-08-21T07:10:27Z

server/src/test/java/org/elasticsearch/monitor/fs/FsHealthServiceTests.java

                assert pathPrefix != null : "must set pathPrefix before starting disruptions";
-                if (path.toString().startsWith(pathPrefix) && path.toString().endsWith(".es_temp_file")) {
+                if (path.toString().startsWith(pathPrefix) && path.toString().
+                    endsWith(FsHealthService.FsHealthMonitor.TEMP_FILE_NAME)) {


DaveCTurner · 2020-08-21T07:10:31Z

server/src/test/java/org/elasticsearch/monitor/fs/FsHealthServiceTests.java

                        assert pathPrefix != null : "must set pathPrefix before starting disruptions";
-                        if (path.toString().startsWith(pathPrefix) && path.toString().endsWith(".es_temp_file")) {
+                        if (path.toString().startsWith(pathPrefix) && path.toString().
+                            endsWith(FsHealthService.FsHealthMonitor.TEMP_FILE_NAME)) {


DaveCTurner · 2020-08-21T07:15:09Z

server/src/test/java/org/elasticsearch/monitor/fs/FsHealthServiceTests.java

        }
    }

+    public void testFailsHealthOnMissingLockFile() throws IOException {


Thorough tests 😄 However they're not really testing anything in the FsHealthService so much as testing the details of the implementation of the NativeFSLock. Let's just have one of these here, and maybe consider filling in any gaps in Lucene's TestNativeFSLockFactory separately.

Thanks, i am thinking to keep two of them where one throws an IOException and another for AlreadyClosedException.

No, one is all we need here.

NodeEnvironmentTests would be the right place to verify that NodeEnvironment#assertEnvIsLocked throws an IllegalStateException in both of those cases. I think we don't do that today, but again that's a question for a separate PR.

ok, Got it.

amoghRZP · 2020-08-24T11:47:49Z

Thanks @amoghRZP, this is good work. I requested a few small changes but nothing fundamental.

Thanks @DaveCTurner, I have made changes as suggested.

DaveCTurner

One issue remains (and another minor wording change)

DaveCTurner · 2020-08-27T10:19:40Z

server/src/main/java/org/elasticsearch/monitor/fs/FsHealthService.java

        private void monitorFSHealth() {
            Set<Path> currentUnhealthyPaths = null;
-            for (Path path : nodeEnv.nodeDataPaths()) {
+            brokenLock = false;


Clearing this flag here may mean the node reports itself as healthy even though it hasn't actually passed this health check yet. I think we should clear this flag only after setting unhealthyPaths at the very bottom of this method.

sure, done.

DaveCTurner · 2020-08-27T10:20:56Z

server/src/main/java/org/elasticsearch/monitor/fs/FsHealthService.java

+            try {
+                paths = nodeEnv.nodeDataPaths();
+            } catch (IllegalStateException e) {
+                logger.error("Lock assertions failed due to", e);


Minor wording nit:

Suggested change

logger.error("Lock assertions failed due to", e);

logger.error("health check failed", e);

changed 👍

…nd of the method.

amoghRZP · 2020-09-01T04:03:36Z

@DaveCTurner i have made changes as suggested.

amoghRZP · 2020-09-08T04:04:43Z

@DaveCTurner let me know if any change etc is required, if you got chance to look at it.

DaveCTurner · 2020-09-16T10:08:37Z

@elasticmachine update branch

DaveCTurner · 2020-09-16T10:08:52Z

@elasticmachine ok to test

DaveCTurner

LGTM

elasticmachine · 2020-09-22T09:07:05Z

Pinging @elastic/es-core-infra (:Core/Infra/Resiliency)

In #52680 we introduced a mechanism that will allow nodes to remove themselves from the cluster if they locally determine themselves to be unhealthy. The only check today is that their data paths are all empirically writeable. This commit extends this check to consider a failure of `NodeEnvironment#assertEnvIsLocked()` to be an indication of unhealthiness. Closes #58373

Remove node from cluster when node locks are broken.

9478e2b

DaveCTurner requested changes Aug 21, 2020

View reviewed changes

amoghRZP added 3 commits August 24, 2020 15:51

Merge remote-tracking branch 'origin/master' into broken_nl_handling

3ce8839

changes as per suggested in review comments

eeb51ba

Keeping one test for broken node locks in FsHealthServiceTests

c43b6fb

DaveCTurner reviewed Aug 27, 2020

View reviewed changes

amoghRZP added 2 commits August 28, 2020 10:41

Merge branch 'master' into broken_nl_handling

841a8b1

Changes error message being logged and clearing boolean flag at the e…

4a207fb

…nd of the method.

Merge branch 'master' into broken_nl_handling

9caac47

DaveCTurner approved these changes Sep 16, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into broken_nl_handling

fd9d292

DaveCTurner changed the title ~~Remove node from cluster when node locks are broken.~~ Remove node from cluster when node locks are broken Sep 22, 2020

DaveCTurner changed the title ~~Remove node from cluster when node locks are broken~~ Remove node from cluster when node locks broken Sep 22, 2020

DaveCTurner added :Core/Infra/Resiliency Keep running when everything is ok. Die quickly if things go horribly wrong. v7.10.0 v8.0.0 labels Sep 22, 2020

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Sep 22, 2020

DaveCTurner added the >enhancement label Sep 22, 2020

DaveCTurner merged commit 71d0958 into elastic:master Sep 22, 2020

amoghRZP deleted the broken_nl_handling branch September 22, 2020 09:28

occidere mentioned this pull request Jan 25, 2021

진정한 오픈소스 Elasticsearch 를 위한 발돋움 (Stepping up for a truly open source Elasticsearch) occidere/TIL#161

Open

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	statusInfo = new StatusInfo(UNHEALTHY, "health check failed on node due to broken locks");
	statusInfo = new StatusInfo(UNHEALTHY, "health check failed due to broken node lock");

	logger.error("Lock assertions failed due to", e);
	logger.error("health check failed", e);

Remove node from cluster when node locks broken #61400

Remove node from cluster when node locks broken #61400

Uh oh!

Conversation

amoghRZP commented Aug 21, 2020 • edited by DaveCTurner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amoghRZP Aug 24, 2020 • 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!

amoghRZP commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner 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!

Choose a reason for hiding this comment

Uh oh!

amoghRZP commented Sep 1, 2020

Uh oh!

amoghRZP commented Sep 8, 2020

Uh oh!

DaveCTurner commented Sep 16, 2020

Uh oh!

DaveCTurner commented Sep 16, 2020

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amoghRZP commented Aug 21, 2020 •

edited by DaveCTurner

Loading

amoghRZP Aug 24, 2020 •

edited

Loading

amoghRZP commented Aug 24, 2020 •

edited

Loading