Handle negative free disk space in deciders#48392
Handle negative free disk space in deciders#48392DaveCTurner merged 5 commits intoelastic:masterfrom
Conversation
Today it is possible that the total size of all relocating shards exceeds the total amount of free disk space. For instance, this may be caused by another user of the same disk increasing their disk usage, or may be due to how Elasticsearch double-counts relocations that are nearly complete particularly if there are many concurrent relocations in progress. The `DiskThresholdDecider` treats negative free space similarly to zero free space, but it then fails when rendering the messages that explain its decision. This commit fixes its handling of negative free space. Fixes elastic#48380
|
Pinging @elastic/es-distributed (:Distributed/Allocation) |
ywelsch
left a comment
There was a problem hiding this comment.
I'm not sure whether we would like DiskUsage.freeBytes() to ever return a negative value. Its serialization as a VLong e.g. suggests that that value can never be negative. I know that we only expect this to be negative if we have locally create a DiskUsage instance that is not serialized, but that seems trappy either way. I wonder if we should instead add a new class that represents a DiskUsage object with relocations taken into account and use that object for the decision making and logging here. We can do this in a follow-up though.
|
I added a new class in 9ea918f. |
… arithmetic on it
Should we fix the tests then? I would like to have these assertions in DiskUsage checking that values >= 0? |
| long getFreeBytes() { | ||
| return diskUsage.getFreeBytes() - relocatingShardSize; | ||
| try { | ||
| return Math.subtractExact(diskUsage.getFreeBytes(), relocatingShardSize); |
There was a problem hiding this comment.
How could this overflow?
Isn't relocatingShardSize >= 0 and 0 <= diskUsage.getFreeBytes() <= Long.MAX_VALUE? Is this to capture the case where diskUsage.getFreeBytes() == 0 && relocatingShardSize == Long.MAX_VALUE?
There was a problem hiding this comment.
No, relocatingShardSize is the net size of all relocations, i.e. shards in minus shards out, so can indeed be negative :(
|
I opened #48413 to investigate other sources of negative sizes because it needs a deeper look than I can offer right now. |
Today it is possible that the total size of all relocating shards exceeds the total amount of free disk space. For instance, this may be caused by another user of the same disk increasing their disk usage, or may be due to how Elasticsearch double-counts relocations that are nearly complete particularly if there are many concurrent relocations in progress. The `DiskThresholdDecider` treats negative free space similarly to zero free space, but it then fails when rendering the messages that explain its decision. This commit fixes its handling of negative free space. Fixes #48380
Today it is possible that the total size of all relocating shards exceeds the total amount of free disk space. For instance, this may be caused by another user of the same disk increasing their disk usage, or may be due to how Elasticsearch double-counts relocations that are nearly complete particularly if there are many concurrent relocations in progress. The `DiskThresholdDecider` treats negative free space similarly to zero free space, but it then fails when rendering the messages that explain its decision. This commit fixes its handling of negative free space. Fixes #48380
Today it is possible that the total size of all relocating shards exceeds the total amount of free disk space. For instance, this may be caused by another user of the same disk increasing their disk usage, or may be due to how Elasticsearch double-counts relocations that are nearly complete particularly if there are many concurrent relocations in progress. The `DiskThresholdDecider` treats negative free space similarly to zero free space, but it then fails when rendering the messages that explain its decision. This commit fixes its handling of negative free space. Fixes #48380
In elastic#48392 we added a second computation of the sizes of the relocating shards in `canRemain()` but passed the wrong value for `subtractLeavingShards`. This fixes that. It also removes some unnecessary logging in a test case added in the same commit.
In #48392 we added a second computation of the sizes of the relocating shards in `canRemain()` but passed the wrong value for `subtractLeavingShards`. This fixes that. It also removes some unnecessary logging in a test case added in the same commit.
In #48392 we added a second computation of the sizes of the relocating shards in `canRemain()` but passed the wrong value for `subtractLeavingShards`. This fixes that. It also removes some unnecessary logging in a test case added in the same commit.
Today it is possible that the total size of all relocating shards exceeds the
total amount of free disk space. For instance, this may be caused by another
user of the same disk increasing their disk usage, or may be due to how
Elasticsearch double-counts relocations that are nearly complete particularly
if there are many concurrent relocations in progress.
The
DiskThresholdDecidertreats negative free space similarly to zero freespace, but it then fails when rendering the messages that explain its decision.
This commit fixes its handling of negative free space.
Fixes #48380