Skip to content
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

Access for jBarz and @gireeshpunathil to investigate https://github.com/nodejs/node/pull/15362 #918

Closed
mhdawson opened this issue Oct 16, 2017 · 12 comments

Comments

@mhdawson
Copy link
Member

John Barboza and Gireesh Punathil work2 at IBM with me and need access to an AIX machines in order to investigate failures listed in:

nodejs/node#15362

I think jBarz meets the requirements for temporary access in: https://github.com/nodejs/build/blob/master/doc/process/special_access_to_build_resources.md based on this subset of the criteria:

Length and consistency of involvement with Node.js working groups and/or community.
He has contributed the z/OS port for libuv over the last year. Gireesh is a collaborator and has contributed to Node.js for a number of years.

Consequences to the individual in case of mis-bahaviour. For example, would they potentially lose their job if they were reported as mis-behaving to their employer ? Would being banned from involvement in the Node.js community negatively affect them personally in some other way
He works at IBM in the Node.js Runtime technologies group. Being banned would
definitely have an impact
Are there collaborators who work with the individual and can vouch for them.
I'll vouch for him

@nodejs/build can I get an approver ?

@mhdawson
Copy link
Member Author

Approvals were given here: nodejs/TSC#381

@mhdawson
Copy link
Member Author

Got key from John and created user account for him.

@addaleax
Copy link
Member

Isn’t the AIX problem already solved by nodejs/node#16219? I kind of considered @gireeshpunathil’s approval on the PR a general “yea we are aware & this does fix it” statement, maybe I was mistaken about that?

@gireeshpunathil
Copy link
Member

@addaleax - the awareness I had is that the test failure has a reasonable explanation from your insights. We still would like to see if there are platform specific issues due to:

  • the exact error detail was not captured in the test failure (assert on the type masked off the real error).
  • the issue was never reproducible in dev systems.
    Hope this clarifies.

@addaleax
Copy link
Member

@gireeshpunathil Okay, sure – I don’t want to stop anybody from investigating, just want to avoid duplicate work :)

But generally, I think I have at least some idea of what’s going on; the string size limit was raised in V8 6.2, therefore the file that the test reads is now 1 GB instead of 256 MB in size, therefore the test consumes a lot more memory trying to read it in one pass, which fails because the CI machine has limited memory available… Does that sound plausible to you?

@gibfahn
Copy link
Member

gibfahn commented Oct 16, 2017

which fails because the CI machine has limited memory available…

The AIX boxes have 32GB RAM each, of which 5G is used for the Ramdisk. So if the available memory is being limited for the test, then we should be able to fix that.

@mhdawson
Copy link
Member Author

mhdawson commented Oct 16, 2017

It does seem strange that it would be failing due to memory. These are the ulimits:

# ulimit -a
time(seconds)        unlimited
file(blocks)         2097151
data(kbytes)         131072
stack(kbytes)        32768
memory(kbytes)       32768
coredump(blocks)     2097151
nofiles(descriptors) 2000
threads(per process) unlimited
processes(per user)  unlimited

for memory I see this in the AIX docs:

-m | Specifies the size of physical memory (resident set size), in number of K bytes. This limit is not enforced by the system.

@jBarz
Copy link
Contributor

jBarz commented Oct 18, 2017

It seems like the file size limit might be the issue.
This is a comment i posted on nodejs/node#15362 (comment)

The maximum file size on the aix machine (512 MB) is smaller than the kStringMaxLength.
The Maximum string length was increased here v8/v8@e8c9649 from ~256 MB to ~1 GB.
So now, the maximum string size exceeds the maximum file size.
I think that's why the error never gets triggered.

fs.readFile(file, 'utf8', common.mustCall(function(err, buf) {
  assert.ok(err instanceof Error);
  assert.strictEqual('"toString()" failed', err.message);
  assert.strictEqual(buf, undefined);
}));

Can we try to increase the file size limit to at least 2GB?

ulimit -f
1048575

To set it to 2GB

ulimit -f 4194304

@jBarz
Copy link
Contributor

jBarz commented Oct 18, 2017

I also submitted a PR to skip the test in case the file size ulimit wasn't sufficient.

nodejs/node#16273

@maclover7
Copy link
Contributor

Since nodejs/node#16273 has landed, is access still needed?

@jBarz
Copy link
Contributor

jBarz commented Nov 5, 2017

Not needed

@mhdawson
Copy link
Member Author

Ok removed access, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants