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

Refresh project: Update dependencies, update node support, add CI, fix duplex streams #35

Merged
merged 22 commits into from
Feb 7, 2022

Conversation

jwhitlock
Copy link
Contributor

@jwhitlock jwhitlock commented Feb 1, 2022

Updating this code base for node v16, and to fix issue #34. This involved changing a lot of things while I experimented to get the tests working:

  • Use hijacking, a feature introduced in Docker engine v1.22, released with Docker 1.10 (February 2016). This allows the socket connection to better support stdin, stdout, and stderr streams, including closing a non-tty stdin stream (also see dockerode: Streams goodness). This is what fixed issue "docker exec wc -c" test fails with Node.js v16.13.2 #34, which blocked stdout in node v14 and earlier, and hung in node v16 and later.
  • When shutting down the server, use socket.terminate() on the WebSocket, which will call socket.destroy() on the underlying Node socket.
  • Fix the test setup / teardown, so that tests exit without mocha --exit (thanks to Stack Overflow commenter Martin P for suggesting why-is-node-running)
  • Update new Buffer(<array or size>) (deprecation warning) to Buffer.from(array) and Buffer.alloc(size).
  • Drop node v8, require v12. Tests continue to pass in node v10, so keeping it in the test matrix for now.
  • Set debug prefixes from docker-exec-server to docker-exec-websocket-server
  • Update to the latest versions of all dependencies. This includes:
  • Use community-tc for continuous integration testing. It tests node v10, v12, v14, and v16, as well as the "current" version. It requires a generic worker that is setup to run Docker. It uses docker-compose, which it downloads and installs.
  • Remove tests/runtests.sh, since the test setup creates its own ubuntu container for testing.
  • Remove TravisCI and Vagrant

@jwhitlock jwhitlock marked this pull request as draft February 1, 2022 15:53
@jwhitlock
Copy link
Contributor Author

No CI ran when opening this PR.

I'm removing two repo hooks:

@jwhitlock
Copy link
Contributor Author

Added the Community-TC Integration app to this repo (and -client) in the Taskcluster organization settings

`new Buffer(array or size)` was deprecated in v6, emits a warning in v10.
@community-tc-integration

This comment has been minimized.

@jwhitlock jwhitlock force-pushed the node-16 branch 15 times, most recently from 10e7b75 to 919f764 Compare February 2, 2022 17:22
@community-tc-integration
Copy link

Uh oh! Looks like an error! Details

bad indentation of a mapping entry (45:94)

42 | ...
43 | ...
44 | ...
45 | ... val: 'env.image.split(":")[1]'}
-----------------------------------------^
46 | ...

@jwhitlock jwhitlock force-pushed the node-16 branch 3 times, most recently from 46da9ce to f9a7eb6 Compare February 2, 2022 19:33
@community-tc-integration
Copy link

Uh oh! Looks like an error! Details

missed comma between flow collection entries (41:44)

38 | ... RunTime: 3600
39 | ... mand:
40 | ... [git, --version]
41 | ... [git, clone, --no-progress, ${repository}, repo]
-----------------------------------------^
42 | ... [cd, repo]
43 | ... [git, config, advice.detachedHead, false]

@jwhitlock jwhitlock force-pushed the node-16 branch 4 times, most recently from ca4df19 to 12e85b6 Compare February 2, 2022 19:49
@jwhitlock jwhitlock marked this pull request as ready for review February 4, 2022 22:44
@jwhitlock
Copy link
Contributor Author

I've fixed the failing test. The test wasn't actually working before either - the exit code of 0 was being read, but the output of wc -c was not. A stronger version of the test now passes.

I'll summarize the changes in the PR summary.

@jwhitlock jwhitlock requested a review from petemoore February 4, 2022 23:17
@jwhitlock
Copy link
Contributor Author

Because we're updating node versions, I think the next release is a major version bump to 3.0.0. I can include that version bump in this PR.

Copy link
Contributor

@djmitche djmitche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, @jwhitlock, I'm impressed! I looked through all of the commits here, in light of the PR comments, and it all checks out. Nice work!

@jwhitlock
Copy link
Contributor Author

Thanks! This is a week or so of effort, and changes more than I'd like in a PR. I'd be willing to break it up into smaller bits for review. I'd also like @petemoore to look at .taskcluster.yml and see if there are any bits he'd want changed first.

Copy link
Contributor

@lotas lotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

@jwhitlock jwhitlock changed the title Updates for node v16 Refresh project: Update dependencies, update node support, add CI, fix duplex streams Feb 5, 2022
Copy link

@matt-boris matt-boris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks John. Not sure on the .taskcluster.yml file config, but everything else all looks great!

Copy link
Member

@petemoore petemoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks truly amazing, thank you so much for this mammoth effort!

Thanks for migrating from Vagrant to docker-compose, that seems like a much more modern and maintainable solution. I see this uses v1 rather than v2 (which I think are incompatible?). Is there a reason for this choice, and if so, is it worth adding a note about why?

.taskcluster.yml Outdated
curl -sL "https://github.com/docker/compose/releases/download/1.29.2/docker-compose-$(uname -s)-$(uname -m)" -o ~/bin/docker-compose &&
chmod +x ~/bin/docker-compose &&
~/bin/docker-compose build --build-arg "NODE_VERSION=${env.node_image_tag}" &&
~/bin/docker-compose run --rm test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to lose all the && references, you can format it with -| and just have line returns, see e.g. this yaml extract.

.taskcluster.yml Outdated
git config advice.detachedHead "false" &&
git checkout --no-progress "${head_rev}" &&
mkdir -p ~/bin &&
curl -sL "https://github.com/docker/compose/releases/download/1.29.2/docker-compose-$(uname -s)-$(uname -m)" -o ~/bin/docker-compose &&
Copy link
Member

@petemoore petemoore Feb 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth having retries here to protect against network blips?

One option to do that would be to use the mounts feature which already uses an exponential backoff algorithm (that doesn't impact maxRunTime when there are delays), e.g. have the following inside the task payload (i.e. sibling of maxruntime, env, command):

          mounts:
            - file: bin/docker-compose
              content:
                url: https://github.com/docker/compose/releases/download/1.29.2/docker-compose-Linux-x86_64
                sha256: f3f10cf3dbb8107e9ba2ea5f23c1d2159ff7321d16f0a23051d68d8e2547b323

Note, the sha256 is optional, but I see that it is provided in the release, so might be a nice extra to have to protect against man-in-the-middle modifications. Another advantage (of using the mounts feature) is that the download also gets cached on the worker, so if another task comes in for the same worker that requires the same file, it would get reused.

@jwhitlock jwhitlock force-pushed the node-16 branch 2 times, most recently from c9cde34 to 7e80de3 Compare February 7, 2022 17:35
@community-tc-integration
Copy link

Uh oh! Looks like an error! Details

InterpreterError at template.tasks.payload.command[0][2]: unknown context value USER

@community-tc-integration
Copy link

Uh oh! Looks like an error! Details

InterpreterError at template.tasks.payload.command[0][2]: unknown context value USER

@community-tc-integration
Copy link

Uh oh! Looks like an error! Details

InterpreterError at template.tasks.payload.command[0][2]: unknown context value USER

@jwhitlock jwhitlock force-pushed the node-16 branch 3 times, most recently from 101c8a5 to 4558d0f Compare February 7, 2022 17:46
* Use a mount for docker-compose, with automatic retries
* Use docker-compose v2 (not yet integrated into Docker on Linux)
* Print the docker engine info
* Remove the && from commands (stopping on error is set with bash -e)
@jwhitlock
Copy link
Contributor Author

Thanks @petemoore! I applied the suggestions in the last commit:

  • Use mounts to download docker-compose. It took a few iterations to determine where it was downloaded and to make it executable, but the iterations are squashed
  • Switch to docker-compose v2.2.3
  • Change to a list of command rather than a chain of && commands.

@jwhitlock jwhitlock merged commit f80d7d4 into taskcluster:master Feb 7, 2022
@jwhitlock jwhitlock deleted the node-16 branch February 7, 2022 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants