Fix total disk bytes returning negative value #23093
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dakrone
added
:Data Management/Stats
dakrone
commented
Feb 9, 2017
| fsPath.available = nodePath.fileStore.getUsableSpace(); | ||
| fsPath.total = adjustForHugeFilesystems(nodePath.fileStore.getTotalSpace()); | ||
| fsPath.free = adjustForHugeFilesystems(nodePath.fileStore.getUnallocatedSpace()); | ||
| fsPath.available = adjustForHugeFilesystems(nodePath.fileStore.getUsableSpace()); |
There was a problem hiding this comment.
So I am on the fence about these two, free and available, because I have seen filesystems where a negative "free" value is legal (I believe ZFS has returned negative free values before for disk space), so I'm not sure whether we should drop these.
I do think we should have the protection on getTotalSpace though, I don't think it's as likely for it to be negative and still a legal value.
There was a problem hiding this comment.
I think we should only make the change for total until we see evidence that adjustments need to be made for the others.
dakrone
force-pushed
the
workaround-for-openjdk-8162520-large-fs
branch
from
af51c93
to
f6a3d01
Compare
|
Can you please change the title of this PR to reflect the underlying problem (a JDK bug ID is too opaque), and rewrite the commit message to do the same? |
jasontedor
requested changes
Feb 12, 2017
| fsPath.available = nodePath.fileStore.getUsableSpace(); | ||
| fsPath.total = adjustForHugeFilesystems(nodePath.fileStore.getTotalSpace()); | ||
| fsPath.free = adjustForHugeFilesystems(nodePath.fileStore.getUnallocatedSpace()); | ||
| fsPath.available = adjustForHugeFilesystems(nodePath.fileStore.getUsableSpace()); |
There was a problem hiding this comment.
I think we should only make the change for total until we see evidence that adjustments need to be made for the others.
This adds a workaround for JDK-8162520 - https://bugs.openjdk.java.net/browse/JDK-8162520 Some filesystems can be so large that they return a negative value for their free/used/available disk bytes due to being larger than `Long.MAX_VALUE`. This adds protection for our `FsProbe` implementation and adds a test that it does the right thing.
dakrone
force-pushed
the
workaround-for-openjdk-8162520-large-fs
branch
from
f6a3d01
to
2cb898b
Compare
|
@jasontedor I pushed a new commit that has a different commit header (and removed the changes so it only affects |
dakrone
changed the title
jasontedor
approved these changes
Feb 13, 2017
| @@ -135,14 +135,22 @@ public FsInfo stats(FsInfo previous, @Nullable ClusterInfo clusterInfo) throws I | |||
| return Files.readAllLines(PathUtils.get("/proc/diskstats")); | |||
| } | |||
|
|
|||
| /** See: https://bugs.openjdk.java.net/browse/JDK-8162520 */ | |||
There was a problem hiding this comment.
Sorry to be a pain, but can you make this not a Javadoc, just a regular comment? 😦
|
retest this please |
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Feb 13, 2017
* master: Update to forbiddenapis 2.3 (improves Gradle configuration time) (elastic#23154) Make the version of the remote node accessible on a transport channel (elastic#23019) Fix total disk bytes returning negative value (elastic#23093) Fix communication with 5.3.0 nodes Update redirects.asciidoc (elastic#23148)
dakrone
added a commit
to dakrone/elasticsearch
that referenced
this pull request
Feb 22, 2017
From elastic#23093, we fixed the issue where a filesystem can be so large that it overflows and returns a negative number. However, there is another issue when adding a path as a sub-path to another `FsInfo.Path` object, when adding the totals the values can still overflow. This adds the same safety to return `Long.MAX_VALUE` instead of the negative number, as well as a test exercising the logic.
jasontedor
pushed a commit
that referenced
this pull request
Mar 19, 2017
* Fix total disk bytes returning negative value This adds a workaround for JDK-8162520 - https://bugs.openjdk.java.net/browse/JDK-8162520 Some filesystems can be so large that they return a negative value for their free/used/available disk bytes due to being larger than `Long.MAX_VALUE`. This adds protection for our `FsProbe` implementation and adds a test that it does the right thing.
jasontedor
pushed a commit
that referenced
this pull request
Mar 19, 2017
From #23093, we fixed the issue where a filesystem can be so large that it overflows and returns a negative number. However, there is another issue when adding a path as a sub-path to another `FsInfo.Path` object, when adding the totals the values can still overflow. This adds the same safety to return `Long.MAX_VALUE` instead of the negative number, as well as a test exercising the logic.
jasontedor
pushed a commit
that referenced
this pull request
Mar 19, 2017
* Fix total disk bytes returning negative value This adds a workaround for JDK-8162520 - https://bugs.openjdk.java.net/browse/JDK-8162520 Some filesystems can be so large that they return a negative value for their free/used/available disk bytes due to being larger than `Long.MAX_VALUE`. This adds protection for our `FsProbe` implementation and adds a test that it does the right thing.
jasontedor
pushed a commit
that referenced
this pull request
Mar 19, 2017
From #23093, we fixed the issue where a filesystem can be so large that it overflows and returns a negative number. However, there is another issue when adding a path as a sub-path to another `FsInfo.Path` object, when adding the totals the values can still overflow. This adds the same safety to return `Long.MAX_VALUE` instead of the negative number, as well as a test exercising the logic.
|
I backported this to 5.3 and 5.x. |
dakrone
added a commit
to dakrone/elasticsearch
that referenced
this pull request
May 26, 2017
In elastic#23093 we made a change so that total bytes for a filesystem would not be a negative value when the total bytes were > Long.MAX_VALUE. This fixes elastic#24453 which had a related issue where `available` and `free` bytes could also be so large that they were negative. These will now return `Long.MAX_VALUE` for the bytes if the JDK returns a negative value.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://bugs.openjdk.java.net/browse/JDK-8162520
Some filesystems can be so large that they return a negative value for their
free/used/available disk bytes due to being larger than
Long.MAX_VALUE.This adds protection for our
FsProbeimplementation and adds a test that itdoes the right thing.
You can see a failure from this here: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+nfs/4/console