build: clear stalled jobs on POSIX CI hosts#11246

Trott · 2017-02-08T19:44:40Z

Sometimes, after a cluster or debug test fails, a fixture hangs around
and holds onto a needed port, causing subsequent CI runs to fail. This
adds a command I've been running manually when this occurs. The command
will clear the stalled jobs before a CI run.

Checklist

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

Affected core subsystem(s)

build

mscdex · 2017-02-08T19:56:16Z

Makefile

What about simply using pkill instead? Isn't that available on all *nix platforms in CI?

It's not on our AIX hosts.

Trott · 2017-02-08T21:17:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6296/

Trott · 2017-02-08T21:51:45Z

CI is green. Removing WIP from the title and removing the in progress label.

/cc @nodejs/build for reviews

mhdawson

LGTM, thanks for this, we've had to clean the AIX machines several times in the last few days so this is great to see.

Trott · 2017-02-09T00:04:22Z

I'm terrible at grok'ing Makefile stuff. I put this as an order-only prerequisite. If someone knowledgable could indicate whether that's the right thing to do or not in this case, that would be appreciated.

richardlau · 2017-02-09T00:54:45Z

The clear-stalled target should be phony and added to .PHONY at the end of the makefile since it does not refer to a file or directory called clear-stalled.

If the target is declared phony, I'm not sure that it matters if it is order-only or not as a prerequisite.

gibfahn · 2017-02-09T01:53:03Z

Related to nodejs/build#591

As discussed in that issue, I'd rather have something in the test runner that kept track of processes that hadn't been cleaned up at the end of each run, and failed the job if there were any. However I do think this is better than what we currently have (later jobs randomly failing and @nodejs/build members having to clean up machines manually), so I'd be +1 on this for now.

Currently I don't think this is logging what processes it is cleaning up though, which is information I do think we need, so I'd be -1 on this without that.

I'm also not sure under what circumstances processes get left behind, doesn't tools/test.py clean up once the TIMEOUT happens? I assumed it was for orphaned node subprocesses, but I'm not sure.

gibfahn · 2017-02-09T03:31:17Z

Makefile

I think you could just add the same line up to the awk above to list the processes that will be killed, i.e.:
ps awwwx | grep Release/node | grep -v grep ps awwwx | grep Release/node | grep -v grep | awk '{print $$1}'|$(XARGS)kill

Trott · 2017-02-09T04:40:41Z

I assumed it was for orphaned node subprocesses, but I'm not sure.

That seems right to me. Always seems to be subprocesses from cluster or debug tests.

Trott · 2017-02-09T04:52:14Z

Implemented changes per @richardlau and @gibfahn.

CI: https://ci.nodejs.org/job/node-test-pull-request/6306/

gibfahn

The code addition LGTM, no idea about the Makefile syntax

santigimeno · 2017-02-09T09:05:43Z

I'd rather have something in the test runner that kept track of processes that hadn't been cleaned up at the end of each run, and failed the job if there were any.

I agree on this. If a test is leaving processes behind I think it should fail.

Currently I don't think this is logging what processes it is cleaning up though, which is information I do think we need, so I'd be -1 on this without that.

I also agree on this.

gibfahn · 2017-02-09T09:59:47Z

@santigimeno So with the latest update this will now list the processes it's killing, so I'd say this is an improvement over what we currently do. Are you -1 on this landing as a step in the right direction?

If we could fix tools/test.py that would be great, but I'd rather have this in the meantime. Once the tools/test.py fix is in place, we could probably change this to make it fail the build if it finds any processes (as that means the tools/test.py cleanup failed).

santigimeno · 2017-02-09T10:19:58Z

So with the latest update this will now list the processes it's killing, so I'd say this is an improvement over what we currently do. Are you -1 on this landing as a step in the right direction?

Sorry, I had overlooked the cat command. I'd rather have this running at the end of the CI run than before so we know when it really happened, but I agree this is an improvement.

Trott · 2017-02-09T18:10:49Z

Updated Makefile per nit from @joshgav.

Re-running CI: https://ci.nodejs.org/job/node-test-pull-request/6317/

Trott · 2017-02-09T18:44:32Z

@santigimeno wrote:

I'd rather have this running at the end of the CI run than before so we know when it really happened, but I agree this is an improvement.

Queued up for a subsequent PR: Trott@48693fc

Sometimes, after a cluster or debug test fails, a fixture hangs around and holds onto a needed port, causing subsequent CI runs to fail. This adds a command I've been running manually when this occurs. The command will clear the stalled jobs before a CI run. PR-URL: nodejs#11246 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Josh Gavant <[email protected]>

Trott · 2017-02-10T21:24:02Z

Landed in 90ab68b

Sometimes, after a cluster or debug test fails, a fixture hangs around and holds onto a needed port, causing subsequent CI runs to fail. This adds a command I've been running manually when this occurs. The command will clear the stalled jobs before a CI run. PR-URL: #11246 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Josh Gavant <[email protected]>

Sometimes, after a cluster or debug test fails, a fixture hangs around and holds onto a needed port, causing subsequent CI runs to fail. This adds a command I've been running manually when this occurs. The command will clear the stalled jobs before a CI run. PR-URL: nodejs#11246 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Josh Gavant <[email protected]>

jasnell · 2017-03-07T18:46:14Z

would need backport PRs to land on v6 and v4

gibfahn · 2017-06-17T23:47:22Z

I think it's worth backporting this to v6.x-staging. @Trott would you be willing to backport this?

guide

#12158 is a follow-up to this.

Sometimes, after a cluster or debug test fails, a fixture hangs around and holds onto a needed port, causing subsequent CI runs to fail. This adds a command I've been running manually when this occurs. The command will clear the stalled jobs before a CI run. PR-URL: nodejs#11246 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Josh Gavant <[email protected]>

Trott · 2017-06-18T00:33:11Z

@gibfahn #13754

Sometimes, after a cluster or debug test fails, a fixture hangs around and holds onto a needed port, causing subsequent CI runs to fail. This adds a command I've been running manually when this occurs. The command will clear the stalled jobs before a CI run. PR-URL: #11246 Backport-PR-URL: #13754 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Josh Gavant <[email protected]>

Trott added build Issues and PRs related to build files or the CI. wip Issues and PRs that are still a work in progress. labels Feb 8, 2017

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 8, 2017

mscdex reviewed Feb 8, 2017
View reviewed changes

mscdex added the test Issues and PRs related to the tests. label Feb 8, 2017

Trott force-pushed the terminate-stalled branch 3 times, most recently from e537333 to f402ec6Compare February 8, 2017 20:26

jasnell approved these changes Feb 8, 2017
View reviewed changes

Trott force-pushed the terminate-stalled branch from f402ec6 to f4695f4Compare February 8, 2017 21:16

Trott removed the wip Issues and PRs that are still a work in progress. label Feb 8, 2017

Trott changed the title ~~WIP: build: clear stalled jobs on POSIX CI hosts~~build: clear stalled jobs on POSIX CI hostsFeb 8, 2017

Trott mentioned this pull request Feb 8, 2017
lib: deprecate node --debug at runtime #10970
Closed
3 tasks

mhdawson approved these changes Feb 8, 2017
View reviewed changes

richardlau mentioned this pull request Feb 9, 2017
Job to clean up old processes nodejs/build#591
Closed

gibfahn reviewed Feb 9, 2017
View reviewed changes

Trott force-pushed the terminate-stalled branch from f4695f4 to 08735d5Compare February 9, 2017 04:51

gibfahn approved these changes Feb 9, 2017
View reviewed changes

richardlau approved these changes Feb 9, 2017
View reviewed changes

santigimeno approved these changes Feb 9, 2017
View reviewed changes

Trott force-pushed the terminate-stalled branch from 08735d5 to 0140171Compare February 9, 2017 18:10

Trott mentioned this pull request Feb 9, 2017
build: fail on CI if leftover processes #11269
Closed
2 tasks

Trott closed this Feb 10, 2017

italoacasas mentioned this pull request Feb 13, 2017
Proposal - 7.6.0 (Current) #11185
Merged

jasnell added dont-land-on-v4.x labels Mar 7, 2017

gibfahn added backport-requested-v6.x and removed dont-land-on-v6.x labels Jun 17, 2017

gibfahn mentioned this pull request Jun 17, 2017
build: avoid passing kill empty input in Makefile #12158
Merged
2 tasks

Trott mentioned this pull request Jun 18, 2017
(v6.x backport) build: clear stalled jobs on POSIX CI hosts #13754
Closed
4 tasks

gibfahn added backported-to-v6.x and removed backport-requested-v6.x labels Jun 18, 2017

MylesBorins mentioned this pull request Jul 18, 2017
v6.11.2 proposal #14356
Merged

Trott deleted the terminate-stalled branch January 13, 2022 22:35

Uh oh!

build: clear stalled jobs on POSIX CI hosts#11246

build: clear stalled jobs on POSIX CI hosts #11246

Uh oh!

Conversation

Trott commented Feb 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mscdexFeb 8, 2017

Choose a reason for hiding this comment

Uh oh!

TrottFeb 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Feb 8, 2017

Uh oh!

Trott commented Feb 8, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Feb 9, 2017

Uh oh!

richardlau commented Feb 9, 2017

Uh oh!

gibfahn commented Feb 9, 2017

Uh oh!

gibfahnFeb 9, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Feb 9, 2017

Uh oh!

Trott commented Feb 9, 2017

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

santigimeno commented Feb 9, 2017

Uh oh!

gibfahn commented Feb 9, 2017

Uh oh!

santigimeno commented Feb 9, 2017

Uh oh!

Trott commented Feb 9, 2017

Uh oh!

Trott commented Feb 9, 2017

Uh oh!

Trott commented Feb 10, 2017

Uh oh!

jasnell commented Mar 7, 2017

Uh oh!

gibfahn commented Jun 17, 2017

Uh oh!

Trott commented Jun 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Trott commented Feb 8, 2017•
edited
Loading

TrottFeb 8, 2017•
edited
Loading