`bootstrap.mlockall` for Windows (VirtualLock) by gmarz · Pull Request #10887 · elastic/elasticsearch

gmarz · 2015-04-29T18:32:19Z

This PR supersedes #9186. The original PR became a bit stale and was also working off my master branch.

I've incorporated all the feedback from #9186 as best as I could. The biggest difference from the original PR are the changes that had to be made in order to work properly with the refactoring that was done in #9923.

I believe this is now good to go, but would love to get some final eyes/testing before merging it in.

/cc @clintongormley @pickypg @ppf2 @Mpdreamz @tlrx @kimchy

Closes #8480

kimchy · 2015-04-29T18:36:52Z

src/main/java/org/elasticsearch/common/jna/Natives.java

do we want to use Xmx here? getHeapInit is Xms, I mean, typically, this will be the same, but by default, they are different in our config

If they are different then mlockall will not really work on unix either. That is because it may map additional stuff later!

Actually, the comment is wrong...using Xms/getHeapInit here is intentional. The assumption is that they're the same if mlockall is enabled. I think I chose Xms to be on the safe side in case they aren't.

"the comment" as being the one in code, not yours @kimchy :)

@rmuir @kimchy come to think of it, probably makes more sense to log an error and abort locking if Xmx != Xms

I think its fine if we only lock HeapInit, thats why I was confused, either by the comment in the code, or the code :). We should assume that HeapInit==HeapMax in prod deployment since we ask people to set the heap which automatically set both of them.

Can we just add max(HeapInit, HeapMax) feels like a quick win here.

@Mpdreamz That feels a little wrong to me since it implies that it's okay for them to be different.

@gmarz @kimchy but it is okay for them to be different simply because we allow it :)

The worse case is that we kill starting with MinHeap (growing to max immediately) instead of otherwise the worse case being mlockall'ing too little.

kimchy · 2015-04-29T18:38:39Z

left one comment, hard to tell if it works on my end :), I assume we tested it on windows, so I am good

pickypg · 2015-04-29T19:50:10Z

src/main/java/org/elasticsearch/common/jna/Kernel32Library.java

com.sun.glass.ui.Size appears to be unused.

Removed, thanks @pickypg

pickypg · 2015-04-29T22:12:06Z

Really minor stuff. Excited to see this get in! LGTM

Mpdreamz · 2015-05-01T15:50:19Z

I'm being bit by the following:

[2015-05-01 17:42:00,360][INFO ][node                     ] [Troll] version[2.0.0-SNAPSHOT], pid[5676], build[e0560cf/2015-05-01T15:25:35Z]
[2015-05-01 17:42:00,361][INFO ][node                     ] [Troll] initializing ...
[2015-05-01 17:42:00,367][INFO ][plugins                  ] [Troll] loaded [], sites []
[2015-05-01 17:42:00,442][INFO ][env                      ] [Troll] using [1] data paths, mounts [[(c:)]], net usable_space [31.7gb], net total_space [80.2gb], spins? [unknown], types [NTFS]
[2015-05-01 17:42:03,192][INFO ][node                     ] [Troll] initialized
{2.0.0-SNAPSHOT}: Initialization Failed ...
- ExceptionInInitializerError
        AccessControlException[access denied ("java.io.FilePermission" "C:\Users\mpdreamz\AppData\Local\Temp\jna-280264762" "read")]

Both whether mlockall is enabled or not and whether I run as Admin or not.

dakrone · 2015-05-01T15:52:19Z

@Mpdreamz that's the security manager, if you are running from an IDE you will need to add -Des.security.manager.enabled=false to the arguments

rmuir · 2015-05-01T16:04:04Z

@Mpdreamz can you confirm whether this happens when you run bin/elasticsearch? Our policy file grants access to java.io.tmpdir (which it looks like is where you hit the issue), so if it happens then we need to open an issue, something is not right on windows maybe.

Mpdreamz · 2015-05-01T16:05:47Z

It happens on the command line (bin/elasticsearch) not tested inside IntelliJ

-Des.security.manager.enabled=false does fix it now wondering wether its something in this PR that causing this or current master.

rmuir · 2015-05-01T16:10:46Z

I suspect its caused by this issue, because @rjernst tested security manager changes on a windows VM before they went in. So we need to do some investigation.

Mpdreamz · 2015-05-01T16:21:12Z

Yeah current HEAD in master runs without a problem, seems like something introduced in this branch.

gmarz · 2015-05-01T16:25:33Z

My fork isn't up to date with master and is missing f599c23. I wonder if that could be the issue?

Strangely though, I'm not running into this issue running from the cmd line, and same snapshot build as @Mpdreamz.

rmuir · 2015-05-01T16:40:35Z

It makes some sense: as described in that commit, mlockall tests don't run with security manager enabled, so that is why we mlockall first (so we know it works, since we dont test it).

What is maybe not explained is why you dont have access to what appears to be a temp dir. To satisfy curiosity, can you run ES with an additional VM argument, the windows equivalent of this?:

JAVA_OPTS=-Djava.security.debug=access:failure bin/elasticsearch

This guarantees full stacktrace on failure, which we don't have above.

Mpdreamz · 2015-05-01T16:59:23Z

Output listed here: https://gist.github.com/anonymous/be30448ed6b0e0fa58e8

rmuir · 2015-05-01T17:08:14Z

Thank you, so its as i feared, something is wrong with temporary directory permissions in your case. Can you run the same way with current master, I want to see if we are still unable to delete our own temporary file there too (ES will not fail here, but something is not right)

Mpdreamz · 2015-05-01T17:36:00Z

https://gist.github.com/Mpdreamz/9da3e21ab98bb413a54d

Does go into started state:

[2015-05-01 19:09:13,288][INFO ][node ] [Taurus] started

But still getting an access error:

access: access denied ("java.io.FilePermission" "C:\Users\mpdreamz\AppData\Local\Temp\5508702577511658466.tmp" "delete") java.lang.Exception: Stack trace

rmuir · 2015-05-01T17:45:18Z

Thanks @Mpdreamz I created #10925 to debug this, its important but unrelated to this issue, so we can figure it out over there.

Mpdreamz · 2015-05-01T18:16:36Z

LGTM 👍 sInce a rebased build does not throw the exception and goes into started mode where it locks the memory as expected

@rmuir moved my issue over to #10925

clintongormley · 2015-05-08T08:45:01Z

@gmarz is this ready to go in?

Closes elastic#8480

gmarz · 2015-05-08T13:46:00Z

@clintongormley Yes :) Merging shortly.

mlockall for Windows (VirtualLock)

gmarz mentioned this pull request Apr 29, 2015

Bootstrap: Implement VirtualLock on Windows #9186

Closed

kimchy reviewed Apr 29, 2015
View reviewed changes

pickypg reviewed Apr 29, 2015
View reviewed changes

gmarz added >enhancement v2.0.0-beta1 v1.6.0 labels Apr 30, 2015

rmuir mentioned this pull request May 1, 2015

securitymanager issue with windows temp dir: or at least for @Mpdreamz #10925

Closed

gmarz force-pushed the feature/virtuallock branch from e0560cf to 99b2410 Compare May 1, 2015 18:06

VirtualLock implementation for Windows (mlockall equivalent)

6f99f5d

Closes elastic#8480

gmarz force-pushed the feature/virtuallock branch from 99b2410 to 6f99f5d Compare May 8, 2015 13:50

gmarz added a commit that referenced this pull request May 8, 2015

Merge pull request #10887 from gmarz/feature/virtuallock

dbba761

mlockall for Windows (VirtualLock)

gmarz merged commit dbba761 into elastic:master May 8, 2015

clintongormley changed the title ~~mlockall for Windows (VirtualLock)~~ bootstrap.mlockall for Windows (VirtualLock) May 29, 2015

clintongormley added :Core/Infra/Settings Settings infrastructure and APIs >feature and removed >enhancement labels May 29, 2015

Conversation

gmarz commented Apr 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimchy commented Apr 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pickypg commented Apr 29, 2015

Uh oh!

Mpdreamz commented May 1, 2015

Uh oh!

dakrone commented May 1, 2015

Uh oh!

rmuir commented May 1, 2015

Uh oh!

Mpdreamz commented May 1, 2015

Uh oh!

rmuir commented May 1, 2015

Uh oh!

Mpdreamz commented May 1, 2015

Uh oh!

gmarz commented May 1, 2015

Uh oh!

rmuir commented May 1, 2015

Uh oh!

Mpdreamz commented May 1, 2015

Uh oh!

rmuir commented May 1, 2015

Uh oh!

Mpdreamz commented May 1, 2015

Uh oh!

rmuir commented May 1, 2015

Uh oh!

Mpdreamz commented May 1, 2015

Uh oh!

clintongormley commented May 8, 2015

Uh oh!

gmarz commented May 8, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants