Merge pull request #19554 from mikemccand/negative_usable_space

Guard against negative result from FileStore.getUsableSpace when picking data path for a new shard

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

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 += 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,22 +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(Long.MIN_VALUE);
             for (NodeEnvironment.NodePath nodePath : paths) {
                 FileStore fileStore = nodePath.fileStore;
-                long usableBytes = 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 -= estShardSizeInBytes * count;
+                    usableBytes = usableBytes.subtract(estShardSizeInBytes.multiply(BigInteger.valueOf(count)));
                 }
-                if (usableBytes > maxUsableBytes) {
+                if (bestPath == null || usableBytes.compareTo(maxUsableBytes) > 0) {
                     maxUsableBytes = usableBytes;
                     bestPath = nodePath;
                 }

core/src/test/java/org/elasticsearch/env/ESFileStoreTests.java

@@ -0,0 +1,43 @@
+/*
+ * 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;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class ESFileStoreTests extends ESTestCase {
+    public void testNegativeSpace() throws Exception {
+        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());
+    }
+}