Use nodeenv to manage node version in Jenkins builds - #13911
Conversation
|
@e0d @edx/testeng This is a WIP but I'd appreciate your feedback, esp in case there's anything about this overall approach that sticks out |
7dd80d7 to
8415ca6
Compare
|
@maxrothman as well I'm pulling this out of 'wip' and into 'for real'. Please consider; open to feedback. |
|
Looks good to me. 👍 |
|
This will indeed be very helpful when we upgrade node in platform. Strictly a feature request/Q: does anybody know what happens if we pass a non-specific version to nodeenv? E.g., 👍 |
|
@bjacobel The short answer is: a 404. For posterity, two outputs: \1. When specifying a non-existent node version: \2. A generic version number: |
cf636fc to
e9ba241
Compare
|
got the cb never called error. jenkins run python |
| # Ensure we are starting with a clean node env directory | ||
| rm -rf $NODE_ENV_DIR | ||
| WAIT_COUNT=0 | ||
| until timeout doCreateNodeEnv || [ $WAIT_COUNT -eq 3 ]; do |
There was a problem hiding this comment.
if doCreateNodeEnv itself exits with nonzero status, this loop would assume a timeout. May need to restructure the loop to accomodate for that detail. timeout(1) will exit with status 127 124 on timeout, which is what you're really looking for.
There was a problem hiding this comment.
refresh page just in case, i made an important typo fix in the above comment :)
|
cb() never called error; flaky JS test jenkins run js |
|
jenkins run bokchoy |
|
@pwnage101 could you suggest an implementation that would trap the exit code in the case of a timeout vs not-a-timeout? Depending on how easy/hard that is to accomplish, we could consider shipping this as-is or refactoring. |
|
Quoting Ben Patterson (2016-11-09 21:24:46)
Something like this should work: EXIT_STATUS = 0 Troy |
|
oops, bugs and typos on so many levels. Let me try again... TIMEOUT_STATUS = 124 # timed-out status, see `man 1 timeout`
EXIT_STATUS = $TIMEOUT_STATUS
WAIT_COUNT = 0
WAIT_LIMIT = 3
# two conditions for breaking out of the retry loop: (1) we didn't timeout, or
# (2) we reached the retry limit.
until [ $EXIT_STATUS -neq $TIMEOUT_STATUS -o $WAIT_COUNT -eq $WAIT_LIMIT ]; do
timeout 30s $NODE_INSTALL_COMMAND
EXIT_STATUS = $?
WAIT_COUNT=$(( WAIT_COUNT++ ))
sleep 5
done |
494c367 to
dda4424
Compare
There was a problem hiding this comment.
This condition shouldn't be removed, I don't think. Without it, nothing gets done on max retries.
|
@pwnage101 Thanks for the help, but it never quite behaved as intended. I think because the exit status was captured in the pipe, it wasn't registering correctly (i.e., because it was technically still happening). I did a fair amount of testing on a Jenkins box but in the end wasn't able to make it that smart. So I'm leaving the older implementation...wherein it'll exit 1 whether it's a timeout or an actual command failure. |
|
Oh also I'm intentionally leaving a unique echo statement in there so we can hit splunk and see if this is catching an intermittent hang. |
|
The less fun implementation will likely work for our use case, but keep in mind that a command failure will result in retries which hopefully are not bad to perform. |
|
@pwnage101 you are right. I figure retrying a failed command is better than risking occasional hangs. I was about to merge this yesterday when I ran into a hang. It was the first I'd seen out of a couple or few dozen builds, but that could amount to another flaky condition and I wanted to (attempt to) nip it in the bud. I gave a smarter implementation a go, but after a couple of hours working on it, I'm going to let it go for now. Not sure what the emoticon is for that...I'll say 🐂 as a hard working, albeit slow-moving animal. (er, that's meant to reflect the implementation, not me!) |
|
makes sense. 👍 provided that you add a comment like "command will also be retried on failure, even if it didn't timeout" |
|
Done. I'll let this run, then squash and run again, and merge at that point unless we discover something else. |
|
github failures... jenkins run lettuce |
This change: * gives us the flexibility of managing node from pull-request to pull-request. It'll be a huge lift to folks that want to test out node upgrades like @bjacobel or @andy-armstrong * Sets us up for a port to Xenial, which will be accomplished in part by using nodeenv for managing the node environment. See a companion PR on the configuration repo for edxapp: edx/configuration#3444 This pull request does not: * Attempt to solve any stability issues in builds related to node. * Make node installs any faster * Create any caching infrastructure to speed up node installs. They will be approximately as fast as they were before. Downloading the node bits build-to-build adds about 3 seconds, which is worth it for the added flexibility.
87f811c to
b3a3d72
Compare
|
jenkins run bokchoy |
This pull request
This pull request does not: