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

[v8.x] Backport aix test fixes to v8.x #29599

Conversation

andrewhughes101
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

refs: #28600
refs: #28516
refs: #28469

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. v8.x labels Sep 18, 2019
sam-github and others added 3 commits September 18, 2019 16:47
These tests seem to trigger failures in the entire CI job (not just the
test) on AIX. Skip them to see if that helps alleviate spurious failures
in node-test-commit-aix (and the upstream PR and commit test jobs).

See:
- nodejs/build#1820 (comment)
- nodejs/build#1847 (comment)

PR-URL: nodejs#28469
Backport-PR-URL: nodejs#29599
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Add SKIP status for more tests in stringbytes-external-exceed-max that
are failing on AIX.

PR-URL: nodejs#28516
Backport-PR-URL: nodejs#29599
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Some pty tests persistently hung on the AIX CI buildbots. Fix that by
adding a helper script that properly sets up the pty before spawning
the script under test.

On investigation I discovered that the test runner hung when it tried
to close the slave pty's file descriptor, probably due to a bug in
AIX's pty implementation. I could reproduce it with a short C program.
The test runner also leaked file descriptors to the child process.

I couldn't convince python's `subprocess.Popen()` to do what I wanted
it to do so I opted to move the logic to a helper script that can do
fork/setsid/etc. without having to worry about stomping on state in
tools/test.py.

In the process I also uncovered some bugs in the pty module of the
python distro that ships with macOS 10.14, leading me to reimplement
a sizable chunk of the functionality of that module.

And last but not least, of course there are differences between ptys
on different platforms and the helper script has to paper over that.
Of course.

Really, this commit took me longer to put together than I care to admit.

Caveat emptor: this commit takes the hacky ^D feeding to the slave out
of tools/test.py and puts it in the *.in input files. You can also feed
other control characters to tests, like ^C or ^Z, simply by inserting
them into the corresponding input file. I think that's nice.

Fixes: nodejs/build#1820
Fixes: nodejs#28489

PR-URL: nodejs#28600
Backport-PR-URL: nodejs#29599
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

test/addons/addon.status Outdated Show resolved Hide resolved
test/message/message.status Show resolved Hide resolved
Assumption that if memory can be malloc()ed it can be used is not true
on AIX. Later access of the allocated pages can trigger SIGKILL if there
are insufficient VM pages.

Use psdanger() to better estimate available memory.

Fixes: nodejs/build#1849

More info:
- https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/generalprogramming/sys_mem_alloc.html
- https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/p_bostechref/psdanger.html

Related to:
- nodejs/build#1820 (comment)
- nodejs#28469
- nodejs#28516

PR-URL: nodejs#28857
Backport-PR-URL: nodejs#29599
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
One skipped test remains, it creates very large Buffer objects,
triggering the AIX OOM to kill node and its parent processes.

See: nodejs/build#1849 (comment)

PR-URL: nodejs#29054
Backport-PR-URL: nodejs#29599
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

@BethGriggs
Copy link
Member

Landed on v8.x-staging - thanks @andrewhughes101

BethGriggs pushed a commit that referenced this pull request Sep 19, 2019
These tests seem to trigger failures in the entire CI job (not just the
test) on AIX. Skip them to see if that helps alleviate spurious failures
in node-test-commit-aix (and the upstream PR and commit test jobs).

See:
- nodejs/build#1820 (comment)
- nodejs/build#1847 (comment)

PR-URL: #28469
Backport-PR-URL: #29599
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 19, 2019
Add SKIP status for more tests in stringbytes-external-exceed-max that
are failing on AIX.

PR-URL: #28516
Backport-PR-URL: #29599
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 19, 2019
Some pty tests persistently hung on the AIX CI buildbots. Fix that by
adding a helper script that properly sets up the pty before spawning
the script under test.

On investigation I discovered that the test runner hung when it tried
to close the slave pty's file descriptor, probably due to a bug in
AIX's pty implementation. I could reproduce it with a short C program.
The test runner also leaked file descriptors to the child process.

I couldn't convince python's `subprocess.Popen()` to do what I wanted
it to do so I opted to move the logic to a helper script that can do
fork/setsid/etc. without having to worry about stomping on state in
tools/test.py.

In the process I also uncovered some bugs in the pty module of the
python distro that ships with macOS 10.14, leading me to reimplement
a sizable chunk of the functionality of that module.

And last but not least, of course there are differences between ptys
on different platforms and the helper script has to paper over that.
Of course.

Really, this commit took me longer to put together than I care to admit.

Caveat emptor: this commit takes the hacky ^D feeding to the slave out
of tools/test.py and puts it in the *.in input files. You can also feed
other control characters to tests, like ^C or ^Z, simply by inserting
them into the corresponding input file. I think that's nice.

Fixes: nodejs/build#1820
Fixes: #28489

PR-URL: #28600
Backport-PR-URL: #29599
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 19, 2019
Assumption that if memory can be malloc()ed it can be used is not true
on AIX. Later access of the allocated pages can trigger SIGKILL if there
are insufficient VM pages.

Use psdanger() to better estimate available memory.

Fixes: nodejs/build#1849

More info:
- https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/generalprogramming/sys_mem_alloc.html
- https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/p_bostechref/psdanger.html

Related to:
- nodejs/build#1820 (comment)
- #28469
- #28516

PR-URL: #28857
Backport-PR-URL: #29599
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 19, 2019
One skipped test remains, it creates very large Buffer objects,
triggering the AIX OOM to kill node and its parent processes.

See: nodejs/build#1849 (comment)

PR-URL: #29054
Backport-PR-URL: #29599
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@BethGriggs BethGriggs closed this Sep 19, 2019
@andrewhughes101 andrewhughes101 deleted the backport-aix-to-v8.x branch September 26, 2019 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants