From 537769c225c9bfe883cb0d8fd7f3b706581115f2 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Tue, 13 Jan 2015 22:53:25 +0100 Subject: [PATCH] Relax restrictions on filesystem size reporting Apparently some filesystems such as ZFS and occasionally NTFS can report filesystem usages that are negative, or above the maximum total size of the filesystem. This relaxes the constraints on `DiskUsage` so that an exception is not thrown. If 0 is passed as the totalBytes, `.getFreeDiskAsPercentage()` will always return 100.0% free (to ensure the disk threshold decider fails open) Fixes #9249 Relates to #9260 --- .../org/elasticsearch/cluster/DiskUsage.java | 26 ++++++++++---- .../decider/DiskThresholdDecider.java | 7 +++- .../elasticsearch/cluster/DiskUsageTests.java | 34 ++++++++++++++----- 3 files changed, 51 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/elasticsearch/cluster/DiskUsage.java b/src/main/java/org/elasticsearch/cluster/DiskUsage.java index e24fa89fd19c8..4ff5fd53c50dc 100644 --- a/src/main/java/org/elasticsearch/cluster/DiskUsage.java +++ b/src/main/java/org/elasticsearch/cluster/DiskUsage.java @@ -31,20 +31,32 @@ public class DiskUsage { final long totalBytes; final long freeBytes; + /** + * Create a new DiskUsage, if {@code totalBytes} is 0, {@get getFreeDiskAsPercentage} + * will always return 100.0% free + */ public DiskUsage(String nodeId, String nodeName, long totalBytes, long freeBytes) { - if ((totalBytes < freeBytes) || (totalBytes < 0)) { - throw new IllegalStateException("Free bytes [" + freeBytes + - "] cannot be less than 0 or greater than total bytes [" + totalBytes + "]"); - } this.nodeId = nodeId; this.nodeName = nodeName; - this.totalBytes = totalBytes; this.freeBytes = freeBytes; + this.totalBytes = totalBytes; + } + + public String getNodeId() { + return nodeId; + } + + public String getNodeName() { + return nodeName; } public double getFreeDiskAsPercentage() { - double freePct = 100.0 * ((double)freeBytes / totalBytes); - return freePct; + // We return 100.0% in order to fail "open", in that if we have invalid + // numbers for the total bytes, it's as if we don't know disk usage. + if (totalBytes == 0) { + return 100.0; + } + return 100.0 * ((double)freeBytes / totalBytes); } public long getFreeBytes() { diff --git a/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/DiskThresholdDecider.java b/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/DiskThresholdDecider.java index 0c8049da8703c..f6cd4f4c93fae 100644 --- a/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/DiskThresholdDecider.java +++ b/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/DiskThresholdDecider.java @@ -519,6 +519,9 @@ public Decision canRemain(ShardRouting shardRouting, RoutingNode node, RoutingAl * @return DiskUsage representing given node using the average disk usage */ public DiskUsage averageUsage(RoutingNode node, Map usages) { + if (usages.size() == 0) { + return new DiskUsage(node.nodeId(), node.node().name(), 0, 0); + } long totalBytes = 0; long freeBytes = 0; for (DiskUsage du : usages.values()) { @@ -537,7 +540,9 @@ public DiskUsage averageUsage(RoutingNode node, Map usages) { */ public double freeDiskPercentageAfterShardAssigned(DiskUsage usage, Long shardSize) { shardSize = (shardSize == null) ? 0 : shardSize; - return 100.0 - (((double)(usage.getUsedBytes() + shardSize) / usage.getTotalBytes()) * 100.0); + DiskUsage newUsage = new DiskUsage(usage.getNodeId(), usage.getNodeName(), + usage.getTotalBytes(), usage.getFreeBytes() - shardSize); + return newUsage.getFreeDiskAsPercentage(); } /** diff --git a/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java b/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java index be90831a95026..62196446de8c5 100644 --- a/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java +++ b/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java @@ -34,6 +34,25 @@ public void diskUsageCalcTest() { assertThat(du.getUsedBytes(), equalTo(60L)); assertThat(du.getTotalBytes(), equalTo(100L)); + // Test that DiskUsage handles invalid numbers, as reported by some + // filesystems (ZFS & NTFS) + DiskUsage du2 = new DiskUsage("node1", "n1", 100, 101); + assertThat(du2.getFreeDiskAsPercentage(), equalTo(101.0)); + assertThat(du2.getFreeBytes(), equalTo(101L)); + assertThat(du2.getUsedBytes(), equalTo(-1L)); + assertThat(du2.getTotalBytes(), equalTo(100L)); + + DiskUsage du3 = new DiskUsage("node1", "n1", -1, -1); + assertThat(du3.getFreeDiskAsPercentage(), equalTo(100.0)); + assertThat(du3.getFreeBytes(), equalTo(-1L)); + assertThat(du3.getUsedBytes(), equalTo(0L)); + assertThat(du3.getTotalBytes(), equalTo(-1L)); + + DiskUsage du4 = new DiskUsage("node1", "n1", 0, 0); + assertThat(du4.getFreeDiskAsPercentage(), equalTo(100.0)); + assertThat(du4.getFreeBytes(), equalTo(0L)); + assertThat(du4.getUsedBytes(), equalTo(0L)); + assertThat(du4.getTotalBytes(), equalTo(0L)); } @Test @@ -42,18 +61,17 @@ public void randomDiskUsageTest() { for (int i = 1; i < iters; i++) { long total = between(Integer.MIN_VALUE, Integer.MAX_VALUE); long free = between(Integer.MIN_VALUE, Integer.MAX_VALUE); - if (free > total || total <= 0) { - try { - new DiskUsage("random", "random", total, free); - fail("should never reach this"); - } catch (IllegalStateException e) { - } + DiskUsage du = new DiskUsage("random", "random", total, free); + if (total == 0) { + assertThat(du.getFreeBytes(), equalTo(free)); + assertThat(du.getTotalBytes(), equalTo(0L)); + assertThat(du.getUsedBytes(), equalTo(-free)); + assertThat(du.getFreeDiskAsPercentage(), equalTo(100.0)); } else { - DiskUsage du = new DiskUsage("random", "random", total, free); assertThat(du.getFreeBytes(), equalTo(free)); assertThat(du.getTotalBytes(), equalTo(total)); assertThat(du.getUsedBytes(), equalTo(total - free)); - assertThat(du.getFreeDiskAsPercentage(), equalTo(100.0 * ((double)free / total))); + assertThat(du.getFreeDiskAsPercentage(), equalTo(100.0 * ((double) free / total))); } } }