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

Debug: VRT docker image #9533

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Debug: VRT docker image #9533

wants to merge 13 commits into from

Conversation

benbowler
Copy link
Collaborator

@benbowler benbowler commented Oct 17, 2024

Summary

Addresses issue:

Relevant technical choices

WIP switch to Ubuntu/Debian as Chromium isn't officially supported on Alpine. I'm going to run the VRTs job over and over on this branch to see if it fails or not across different times of day.

Also based on a suggestion from @techanvil I lock the version of Chromium installed by apt so that it's consistent over time.

Notes below on updates and continued testing...

PR Author Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 5.2 and PHP 7.4.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

  • Run the code.
  • Ensure the acceptance criteria are satisfied.
  • Reassess the implementation with the IB.
  • Ensure no unrelated changes are included.
  • Ensure CI checks pass.
  • Check Storybook where applicable.
  • Ensure there is a QA Brief.
  • Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

  • Ensure the PR has the correct target branch.
  • Double-check that the PR is okay to be merged.
  • Ensure the corresponding issue has a ZenHub release assigned.
  • Add a changelog message to the issue.

@benbowler benbowler changed the title Switch VRT docker image to Ubuntu as Alpine is not officially supported by Chromium. Switch VRT docker image to Ubuntu/Debian as Alpine is not officially supported by Chromium + lock Chromium and Node versions Oct 18, 2024
@benbowler
Copy link
Collaborator Author

Update 18 October 2024:

Using the ubuntu image created issues with installing a reliable node version, so I changed tack and used the official node Debian image then lock the version of chromium here. One thing to note is the Debian image is ~1.6GB vs ~600MB for the old Alpine image. This adds overhead for the job and local developers the first time they run the VRTs.

I'll continue to run the job now and see if it can complete reliably.

@benbowler
Copy link
Collaborator Author

Update 18 October 2024, I switched back to Alpine on a locked chromium and node version from a suggestion by @techanvil, however the node alpine image for node 14.20 includes a much older version of chromium and I has multiple issues with differences in failures between similar runs so I reverted to Debian and will test that image some more.

Comment on lines 24 to 26
ca-certificates \
nodejs \
npm
Copy link
Collaborator

Choose a reason for hiding this comment

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

@benbowler it's worth noting that if we are using a base Docker image with Node preinstalled we shouldn't need to install these dependencies:

Suggested change
ca-certificates \
nodejs \
npm
ca-certificates

That aside, thinking about the switch from Alpine to Debian, I still do have some reservations on that front if it's going to slow down the setup process and take up more space for the image.

I wonder, if someone has an existing image which was built when things had recently stabilised, we could take a look at the versions of the unpinned dependencies installed on that and compare it to a freshly built image to see what if anything has changed. That might allow us to simply specify some versions in the current Dockerfile for a fairly minimal change... I also wonder if the Alpine patch version could have changed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Of course, that's assuming things had stabilised due to the recently applied fix, it's a bit hard to be sure although it certainly looked like it...

Copy link
Collaborator

@techanvil techanvil Oct 18, 2024

Choose a reason for hiding this comment

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

One more thing, I don't think we need to specify Node 14 when building the Backstop image, as we're not running our own scripts on it, it just needs Node for running Backstop in the container...

It's interesting to take a look the Dockerfile on the Backstop repo. The repo doesn't have a tag for 6.1.1 which is the version of Backstop we currently install but we can see the base image for 6.1.3 is node:16.14.0 while the current master is using node:20-bullseye...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point here. I've successfully run the Debian version of this today 6 times without issues.

@benbowler benbowler changed the title Switch VRT docker image to Ubuntu/Debian as Alpine is not officially supported by Chromium + lock Chromium and Node versions Debug: VRT docker image to Ubuntu/Debian Oct 18, 2024
@benbowler benbowler changed the title Debug: VRT docker image to Ubuntu/Debian Debug: VRT docker image Oct 18, 2024
@benbowler
Copy link
Collaborator Author

@techanvil there are so many ways to slice things, and different base images to use, be it the OS base images then add our own node and chromium, node images then add chromium, I've also tried the puppeteer/puppeteer image this afternoon. Update so far:

  • alpine:3.17 base image (original image that appeared to be fixed but failed to launch but re-occurred).
  • node:alpine has been very flakey.
  • node:debian has been stable with 7 test runs successful across the day (although this adds to the build time and adds 1.2GB to the total docker image size).
  • ghcr.io/puppeteer/puppeteer however we're on puppeteer 10 and the oldest version supported by this image is 16 which is already 2 years old.

I feel like working and updating packages could help here to get us to a more recent node, jest, storybook and puppeteer package versions, this is an ongoing project prime for hackathons as well as the tickets we have such as #9408.

@techanvil
Copy link
Collaborator

Thanks @benbowler, interesting to hear the progress to date and it's a good point that updating the various package versions across the board is the preferred long time solution, it's something we need to address on a holistic level across the whole project really.

That said, in the short term we do just want to get a quick fix in to address the pain point of our VRTs failing so regularly in CI. I had imagined trying to figure out what if anything had changed in our current Alpine based image might be the most efficient way to tackle this. But, if it's easier to update the image to use Debian and that gives good results then we can certainly take that route for now, even if it adds a bit of time (I think ~30 seconds?) to creating the image and the image is larger, if it stabilises the test run it's a price worth paying for the overall time it will save us, and we can raise a followup issue to look at finessing this further in whatever way makes the most sense.

I do remain of the opinion we should probably revert to Alpine, but there are of course other options we can look at for speeding things up with a Debian or other image base.

Btw, please don't forget the point I made on my other comment about the Node version, we don't need to pin it to 14 for the Backstop image :)

@benbowler
Copy link
Collaborator Author

Thanks @techanvil, I've updated and tested the node version to node:16.14.2-bullseye and confirm this still appears stable.

I've written up the current state to an IB to put through to IBR officially.

@techanvil
Copy link
Collaborator

Thanks @techanvil, I've updated and tested the node version to node:16.14.2-bullseye and confirm this still appears stable.

I've written up the current state to an IB to put through to IBR officially.

Thanks @benbowler! I've unassigned myself from the issue in IBR so someone else can review it if they have capacity, although I will pick it up myself when I get a moment if it's still there to review.

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.

2 participants