-
-
Notifications
You must be signed in to change notification settings - Fork 101
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.
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
ensure macOS has enough mapped shared memory #881
Conversation
Why the updated umask? I don't see that documented in 840 and unless there's a good reason to relax the permissions we shouldn't be doing that |
@sxa555 I've change the link, it's required in eclipse-openj9/openj9#4375 |
Is |
I'll check with the OpenJ9 team whether that is a hard requirement on the machine setup as my initial feel is that anything testing that functionality could override it itself |
Default permissions on the file for OpenJ9 systems appears to be; There is also a Not sure if the test setting umask is an option or not. @hangshao0 or @Mesbah-Alam would be in a better position to answer that question. The comments for each change should be indicative of the actual change. |
A bit background info: It is testing a command line option to create a shared cache file that is readable and writable to other users within this group. The umask is preventing such permission to be set.
|
@hangshao0 Makes sense (although presumably since the tests are running as a single user it's only verifying the permissions afterwards as opposed to utilising group write?) Do you know if there is scope for the test to set the umask itself before verifying that functionality since that would assist third parties trying to run our tests? |
The JVM that creates the shared cache file will verify if the required access is successfully set or not (right after the file is created). I guess we need the modified umask only when we are running the command that creates the shared cache file. |
I think we can set @Mesbah-Alam could you help to set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rejecting on the basis that the test is to be modified to cover the umask
part of it
I'll back the umask bit of out of it |
@sxa555 updates PTAL |
@sxa555, is this another example where tests need a specific value, then perhaps the tests should set them and fail if they can't increase it? |
Even with from 8.WL4.stderr:
It seems the machine is running low on memory. Internal OSX machines were updated to the following values
@sxa555, @gdams, what are the values set for |
@Mesbah-Alam It's already got these values set as I've run this change on all the test macs: test-macincloud-macos1013-x64-1:~ admin$ cat /etc/sysctl.conf
# BEGIN ANSIBLE MANAGED BLOCK
kern.sysv.shmmax=314572800
kern.sysv.shmall=76800 Do the macs need a reboot after making the change I can make the numbers the same as the ones you've got as well if needed? |
based on https://serverfault.com/a/383990 it suggests that they do need a reboot, I'll add that to the playbook |
re-running grinder with change on that machine: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/1875/console |
grinder passed so I assume this fixes the issue. @sxa555 can you please land and I'll deploy to the rest of the macs |
Test updated to set |
as requested in #840
I've already run this on all the test mac's but they will need to be disconnected and reconnected in order to have the new umask.