From 98c39533d7c3138c35a731c118472afe57aaaa55 Mon Sep 17 00:00:00 2001 From: Mike McCandless Date: Fri, 22 Jul 2016 15:02:31 -0400 Subject: [PATCH 1/4] Guard against negative result from FileStore.getUsableSpace when picking data path for a new shard --- .../elasticsearch/index/shard/ShardPath.java | 9 ++++--- .../index/shard/NewPathForShardTests.java | 27 +++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java b/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java index be0d51bd2b6..1b160d5fad5 100644 --- a/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java +++ b/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java @@ -173,7 +173,7 @@ public final class ShardPath { } else { long totFreeSpace = 0; for (NodeEnvironment.NodePath nodePath : env.nodePaths()) { - totFreeSpace += nodePath.fileStore.getUsableSpace(); + totFreeSpace += Math.max(0, nodePath.fileStore.getUsableSpace()); } // TODO: this is a hack!! We should instead keep track of incoming (relocated) shards since we know @@ -189,12 +189,15 @@ public final class ShardPath { long maxUsableBytes = Long.MIN_VALUE; for (NodeEnvironment.NodePath nodePath : paths) { FileStore fileStore = nodePath.fileStore; - long usableBytes = fileStore.getUsableSpace(); + + // Apparently, FileStore.getUsableSpace() can sometimes be negative, even possibly Long.MIN_VALUE, which can lead to NPE + // below since we would fail to set bestPath: + long usableBytes = Math.max(0, fileStore.getUsableSpace()); // Deduct estimated reserved bytes from usable space: Integer count = dataPathToShardCount.get(nodePath.path); if (count != null) { - usableBytes -= estShardSizeInBytes * count; + usableBytes = Math.subtractExact(usableBytes, Math.multiplyExact(estShardSizeInBytes, count)); } if (usableBytes > maxUsableBytes) { maxUsableBytes = usableBytes; diff --git a/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java b/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java index f1515cd559b..2698de2d840 100644 --- a/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java +++ b/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java @@ -211,4 +211,31 @@ public class NewPathForShardTests extends ESTestCase { nodeEnv.close(); } + + public void testNegativeUsableSpace() throws Exception { + Path path = PathUtils.get(createTempDir().toString()); + + // Use 1 data path: + String[] paths = new String[] {path.resolve("a").toString()}; + + Settings settings = Settings.builder() + .put(Environment.PATH_HOME_SETTING.getKey(), path) + .putArray(Environment.PATH_DATA_SETTING.getKey(), paths).build(); + NodeEnvironment nodeEnv = new NodeEnvironment(settings, new Environment(settings)); + + // Make sure all our mocking above actually worked: + NodePath[] nodePaths = nodeEnv.nodePaths(); + assertEquals(1, nodePaths.length); + + assertEquals("mocka", nodePaths[0].fileStore.name()); + + // Path a is confused about its free space: + aFileStore.usableSpace = -Long.MIN_VALUE; + + ShardId shardId = new ShardId("index", "_na_", 0); + ShardPath result = ShardPath.selectNewPathForShard(nodeEnv, shardId, INDEX_SETTINGS, 100, Collections.emptyMap()); + assertTrue(result.getDataPath().toString().contains(aPathPart)); + + nodeEnv.close(); + } } From ef15e1b91f93e0b3c825c1cf86e20ee37720de01 Mon Sep 17 00:00:00 2001 From: Mike McCandless Date: Thu, 28 Jul 2016 11:31:16 -0400 Subject: [PATCH 2/4] work around JDK bug: if FileStore.getXXXSpace APIs return negative value, change that to Long.MAX_VALUE instead --- .../org/elasticsearch/env/ESFileStore.java | 21 +++- .../elasticsearch/index/shard/ShardPath.java | 18 ++-- .../elasticsearch/env/ESFileStoreTests.java | 95 +++++++++++++++++++ .../index/shard/NewPathForShardTests.java | 27 ------ 4 files changed, 122 insertions(+), 39 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/env/ESFileStoreTests.java diff --git a/core/src/main/java/org/elasticsearch/env/ESFileStore.java b/core/src/main/java/org/elasticsearch/env/ESFileStore.java index 8051c522d03..8ac6cf8a02a 100644 --- a/core/src/main/java/org/elasticsearch/env/ESFileStore.java +++ b/core/src/main/java/org/elasticsearch/env/ESFileStore.java @@ -213,17 +213,32 @@ class ESFileStore extends FileStore { @Override public long getTotalSpace() throws IOException { - return in.getTotalSpace(); + long result = in.getTotalSpace(); + if (result < 0) { + // see https://bugs.openjdk.java.net/browse/JDK-8162520: + result = Long.MAX_VALUE; + } + return result; } @Override public long getUsableSpace() throws IOException { - return in.getUsableSpace(); + long result = in.getUsableSpace(); + if (result < 0) { + // see https://bugs.openjdk.java.net/browse/JDK-8162520: + result = Long.MAX_VALUE; + } + return result; } @Override public long getUnallocatedSpace() throws IOException { - return in.getUnallocatedSpace(); + long result = in.getUnallocatedSpace(); + if (result < 0) { + // see https://bugs.openjdk.java.net/browse/JDK-8162520: + result = Long.MAX_VALUE; + } + return result; } @Override diff --git a/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java b/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java index 1b160d5fad5..9ec28d6d777 100644 --- a/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java +++ b/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java @@ -26,6 +26,7 @@ import org.elasticsearch.env.ShardLock; import org.elasticsearch.index.IndexSettings; import java.io.IOException; +import java.math.BigInteger; import java.nio.file.FileStore; import java.nio.file.Files; import java.nio.file.Path; @@ -171,9 +172,9 @@ public final class ShardPath { dataPath = env.resolveCustomLocation(indexSettings, shardId); statePath = env.nodePaths()[0].resolve(shardId); } else { - long totFreeSpace = 0; + BigInteger totFreeSpace = BigInteger.ZERO; for (NodeEnvironment.NodePath nodePath : env.nodePaths()) { - totFreeSpace += Math.max(0, nodePath.fileStore.getUsableSpace()); + totFreeSpace = totFreeSpace.add(BigInteger.valueOf(nodePath.fileStore.getUsableSpace())); } // TODO: this is a hack!! We should instead keep track of incoming (relocated) shards since we know @@ -181,25 +182,24 @@ public final class ShardPath { // Very rough heuristic of how much disk space we expect the shard will use over its lifetime, the max of current average // shard size across the cluster and 5% of the total available free space on this node: - long estShardSizeInBytes = Math.max(avgShardSizeInBytes, (long) (totFreeSpace/20.0)); + BigInteger estShardSizeInBytes = BigInteger.valueOf(avgShardSizeInBytes).max(totFreeSpace.divide(BigInteger.valueOf(20))); // TODO - do we need something more extensible? Yet, this does the job for now... final NodeEnvironment.NodePath[] paths = env.nodePaths(); NodeEnvironment.NodePath bestPath = null; - long maxUsableBytes = Long.MIN_VALUE; + BigInteger maxUsableBytes = BigInteger.valueOf(-1); for (NodeEnvironment.NodePath nodePath : paths) { FileStore fileStore = nodePath.fileStore; - // Apparently, FileStore.getUsableSpace() can sometimes be negative, even possibly Long.MIN_VALUE, which can lead to NPE - // below since we would fail to set bestPath: - long usableBytes = Math.max(0, fileStore.getUsableSpace()); + BigInteger usableBytes = BigInteger.valueOf(fileStore.getUsableSpace()); + assert usableBytes.compareTo(BigInteger.ZERO) >= 0; // Deduct estimated reserved bytes from usable space: Integer count = dataPathToShardCount.get(nodePath.path); if (count != null) { - usableBytes = Math.subtractExact(usableBytes, Math.multiplyExact(estShardSizeInBytes, count)); + usableBytes = usableBytes.subtract(estShardSizeInBytes.multiply(BigInteger.valueOf(count))); } - if (usableBytes > maxUsableBytes) { + if (usableBytes.compareTo(maxUsableBytes) > 0) { maxUsableBytes = usableBytes; bestPath = nodePath; } diff --git a/core/src/test/java/org/elasticsearch/env/ESFileStoreTests.java b/core/src/test/java/org/elasticsearch/env/ESFileStoreTests.java new file mode 100644 index 00000000000..a0e33979a03 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/env/ESFileStoreTests.java @@ -0,0 +1,95 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.env; + +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; +import java.nio.file.FileStore; +import java.nio.file.attribute.FileAttributeView; +import java.nio.file.attribute.FileStoreAttributeView; + +public class ESFileStoreTests extends ESTestCase { + static class MockFileStore extends FileStore { + + public long usableSpace = -1; + + @Override + public String type() { + return "mock"; + } + + @Override + public String name() { + return "mock"; + } + + @Override + public String toString() { + return "mock"; + } + + @Override + public boolean isReadOnly() { + return false; + } + + @Override + public long getTotalSpace() throws IOException { + return usableSpace*3; + } + + @Override + public long getUsableSpace() throws IOException { + return usableSpace; + } + + @Override + public long getUnallocatedSpace() throws IOException { + return usableSpace*2; + } + + @Override + public boolean supportsFileAttributeView(Class type) { + return false; + } + + @Override + public boolean supportsFileAttributeView(String name) { + return false; + } + + @Override + public V getFileStoreAttributeView(Class type) { + return null; + } + + @Override + public Object getAttribute(String attribute) throws IOException { + return null; + } + } + + public void testNegativeSpace() throws Exception { + FileStore store = new ESFileStore(new MockFileStore()); + assertEquals(Long.MAX_VALUE, store.getUsableSpace()); + assertEquals(Long.MAX_VALUE, store.getTotalSpace()); + assertEquals(Long.MAX_VALUE, store.getUnallocatedSpace()); + } +} diff --git a/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java b/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java index 2698de2d840..f1515cd559b 100644 --- a/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java +++ b/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java @@ -211,31 +211,4 @@ public class NewPathForShardTests extends ESTestCase { nodeEnv.close(); } - - public void testNegativeUsableSpace() throws Exception { - Path path = PathUtils.get(createTempDir().toString()); - - // Use 1 data path: - String[] paths = new String[] {path.resolve("a").toString()}; - - Settings settings = Settings.builder() - .put(Environment.PATH_HOME_SETTING.getKey(), path) - .putArray(Environment.PATH_DATA_SETTING.getKey(), paths).build(); - NodeEnvironment nodeEnv = new NodeEnvironment(settings, new Environment(settings)); - - // Make sure all our mocking above actually worked: - NodePath[] nodePaths = nodeEnv.nodePaths(); - assertEquals(1, nodePaths.length); - - assertEquals("mocka", nodePaths[0].fileStore.name()); - - // Path a is confused about its free space: - aFileStore.usableSpace = -Long.MIN_VALUE; - - ShardId shardId = new ShardId("index", "_na_", 0); - ShardPath result = ShardPath.selectNewPathForShard(nodeEnv, shardId, INDEX_SETTINGS, 100, Collections.emptyMap()); - assertTrue(result.getDataPath().toString().contains(aPathPart)); - - nodeEnv.close(); - } } From 37e0e63a655947901a41fd317df36e4177671969 Mon Sep 17 00:00:00 2001 From: Mike McCandless Date: Fri, 29 Jul 2016 11:51:33 -0400 Subject: [PATCH 3/4] add defense to selectNewPathForShard --- .../main/java/org/elasticsearch/index/shard/ShardPath.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java b/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java index 9ec28d6d777..154619951f6 100644 --- a/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java +++ b/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java @@ -187,7 +187,7 @@ public final class ShardPath { // TODO - do we need something more extensible? Yet, this does the job for now... final NodeEnvironment.NodePath[] paths = env.nodePaths(); NodeEnvironment.NodePath bestPath = null; - BigInteger maxUsableBytes = BigInteger.valueOf(-1); + BigInteger maxUsableBytes = BigInteger.valueOf(Long.MIN_VALUE); for (NodeEnvironment.NodePath nodePath : paths) { FileStore fileStore = nodePath.fileStore; @@ -199,7 +199,7 @@ public final class ShardPath { if (count != null) { usableBytes = usableBytes.subtract(estShardSizeInBytes.multiply(BigInteger.valueOf(count))); } - if (usableBytes.compareTo(maxUsableBytes) > 0) { + if (bestPath == null || usableBytes.compareTo(maxUsableBytes) > 0) { maxUsableBytes = usableBytes; bestPath = nodePath; } From 59181c8a6604bfce6b2fe155f43751352c077d5b Mon Sep 17 00:00:00 2001 From: Mike McCandless Date: Fri, 29 Jul 2016 17:13:01 -0400 Subject: [PATCH 4/4] use mockito instead --- .../elasticsearch/env/ESFileStoreTests.java | 70 +++---------------- 1 file changed, 9 insertions(+), 61 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/env/ESFileStoreTests.java b/core/src/test/java/org/elasticsearch/env/ESFileStoreTests.java index a0e33979a03..7ce278708a0 100644 --- a/core/src/test/java/org/elasticsearch/env/ESFileStoreTests.java +++ b/core/src/test/java/org/elasticsearch/env/ESFileStoreTests.java @@ -25,69 +25,17 @@ import java.nio.file.FileStore; import java.nio.file.attribute.FileAttributeView; import java.nio.file.attribute.FileStoreAttributeView; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + public class ESFileStoreTests extends ESTestCase { - static class MockFileStore extends FileStore { - - public long usableSpace = -1; - - @Override - public String type() { - return "mock"; - } - - @Override - public String name() { - return "mock"; - } - - @Override - public String toString() { - return "mock"; - } - - @Override - public boolean isReadOnly() { - return false; - } - - @Override - public long getTotalSpace() throws IOException { - return usableSpace*3; - } - - @Override - public long getUsableSpace() throws IOException { - return usableSpace; - } - - @Override - public long getUnallocatedSpace() throws IOException { - return usableSpace*2; - } - - @Override - public boolean supportsFileAttributeView(Class type) { - return false; - } - - @Override - public boolean supportsFileAttributeView(String name) { - return false; - } - - @Override - public V getFileStoreAttributeView(Class type) { - return null; - } - - @Override - public Object getAttribute(String attribute) throws IOException { - return null; - } - } - public void testNegativeSpace() throws Exception { - FileStore store = new ESFileStore(new MockFileStore()); + FileStore mocked = mock(FileStore.class); + when(mocked.getUsableSpace()).thenReturn(-1L); + when(mocked.getTotalSpace()).thenReturn(-1L); + when(mocked.getUnallocatedSpace()).thenReturn(-1L); + assertEquals(-1, mocked.getUsableSpace()); + FileStore store = new ESFileStore(mocked); assertEquals(Long.MAX_VALUE, store.getUsableSpace()); assertEquals(Long.MAX_VALUE, store.getTotalSpace()); assertEquals(Long.MAX_VALUE, store.getUnallocatedSpace());