Skip to content

Add max file size bootstrap check #25974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jul 31, 2017
Merged

Add max file size bootstrap check #25974

merged 4 commits into from
Jul 31, 2017

Conversation

@jasontedor
Copy link
Member

@jasontedor jasontedor commented Jul 31, 2017

This commit adds a bootstrap check for the maximum file size, and ensures the limit is set correctly when Elasticsearch is installed as a service on systemd-based systems.

@jasontedor
Add max file size bootstrap check
e17076d 
This commit adds a bootstrap check for the maximum file size, and
ensures the limit is set correctly when Elasticsearch is installed as a
service on systemd-based systems.
@jasontedor
Copy link
Member Author

jasontedor commented Jul 31, 2017

Otherwise this can happen:

java.io.IOException: File too large
	at sun.nio.ch.FileDispatcherImpl.write0(Native Method) ~[?:?]
	at sun.nio.ch.FileDispatcherImpl.write(FileDispatcherImpl.java:60) ~[?:?]
	at sun.nio.ch.IOUtil.writeFromNativeBuffer(IOUtil.java:93) ~[?:?]
	at sun.nio.ch.IOUtil.write(IOUtil.java:65) ~[?:?]
	at sun.nio.ch.FileChannelImpl.write(FileChannelImpl.java:211) ~[?:?]
	at java.nio.channels.Channels.writeFullyImpl(Channels.java:78) ~[?:1.8.0_141]
	at java.nio.channels.Channels.writeFully(Channels.java:101) ~[?:1.8.0_141]
	at java.nio.channels.Channels.access$000(Channels.java:61) ~[?:1.8.0_141]
	at java.nio.channels.Channels$1.write(Channels.java:174) ~[?:1.8.0_141]
	at java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java:82) ~[?:1.8.0_141]
	at java.io.BufferedOutputStream.flush(BufferedOutputStream.java:140) ~[?:1.8.0_141]
	at org.elasticsearch.index.translog.TranslogWriter$BufferedChannelOutputStream.flush(TranslogWriter.java:450) ~[elasticsearch-6.0.0-beta1-SNAPSHOT.jar:6.0.0-beta1-SNAPSHOT]

jpountz
jpountz approved these changes Jul 31, 2017
View reviewed changes
core/src/main/java/org/elasticsearch/bootstrap/JNACLibrary.java
public static final int ENOMEM = 12;
public static final int RLIMIT_MEMLOCK = Constants.MAC_OS_X ? 6 : 8;
public static final int RLIMIT_AS = Constants.MAC_OS_X ? 5 : 9;
public static final int RLIMIT_FSIZE = Constants.MAC_OS_X ? 1 : 1;
Copy link
Contributor

@jpountz jpountz Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ternary operator looks useless?

Copy link
Member Author

@jasontedor jasontedor Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer it this way because it's consistent with how the other RLIMIT constants are defined where there are differences between macOS and Linux.

docs/reference/setup/bootstrap-checks.asciidoc Outdated

The segment files that are the components of individual shards and the translog
generations that are components of the translog can get large (on the order of
hundreds of megabytes and even extending into gigabytes). On systems where the
Copy link
Contributor

@jpountz jpountz Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the phrasing ("even") suggests having files that are 1GB is uncommon, while I think it is actually common?

Copy link
Member Author

@jasontedor jasontedor Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, that was not intended, I pushed ee35808.

qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilJNANativesTests.java Outdated
public void testSetMaxFileSize() throws IOException {
if (Constants.LINUX) {
final List<String> lines = Files.readAllLines(PathUtils.get("/proc/self/limits"));
if (!lines.isEmpty()) {
Copy link
Contributor

@jpountz jpountz Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this if statement looks useless?

Copy link
Member Author

@jasontedor jasontedor Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside this block we make assertions and return from the method. Otherwise, there is a fail below this block that fails the test because we can not assert anything if for some reason we did not read any content from /proc/self/limits

Copy link
Contributor

@jpountz jpountz Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the for loop will prevent from reaching the assertions and returning if lines is empty?

Copy link
Member Author

@jasontedor jasontedor Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I pushed 34a82d5.

@jasontedor
Copy link
Member Author

jasontedor commented Jul 31, 2017

test this please

@jasontedor jasontedor merged commit 2ef0f8a into elastic:master Jul 31, 2017
jasontedor added a commit that referenced this pull request Jul 31, 2017
@jasontedor
Add max file size bootstrap check
6d6cd49 
This commit adds a bootstrap check for the maximum file size, and
ensures the limit is set correctly when Elasticsearch is installed as a
service on systemd-based systems.

Relates #25974
jasontedor added a commit that referenced this pull request Jul 31, 2017
@jasontedor
Add max file size bootstrap check
7b9934a 
This commit adds a bootstrap check for the maximum file size, and
ensures the limit is set correctly when Elasticsearch is installed as a
service on systemd-based systems.

Relates #25974
jasontedor added a commit that referenced this pull request Jul 31, 2017
@jasontedor
Add max file size bootstrap check
8c21797 
This commit adds a bootstrap check for the maximum file size, and
ensures the limit is set correctly when Elasticsearch is installed as a
service on systemd-based systems.

Relates #25974
@jasontedor
Copy link
Member Author

jasontedor commented Jul 31, 2017

Thank you for the review @jpountz.

@jasontedor jasontedor deleted the max-file-size-check branch July 31, 2017 12:03
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jpountz jpountz jpountz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Core/Infra/Core Core issues without another label >enhancement v5.6.0 v6.0.0-beta1 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jasontedor @jpountz @colings86 @jimczi @lcawl