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

added multiarch build #6577

Closed

Conversation

AndrewChubatiuk
Copy link
Contributor

@AndrewChubatiuk AndrewChubatiuk commented Nov 6, 2023

What type of PR is this?

Build multiarch docker images

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

Added ability to create multiarch images

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • [] Manually
  • N/A

@guidopetri
Copy link
Contributor

Hey @AndrewChubatiuk ! Thanks for the PR. We've tried a arm64 build on GHA recently and it runs super long - so long, in fact, that it hits some hidden 6 hour limit for GHA and does not finish. Would you mind testing this on your own repo and potentially making changes there before we consider merging this in?

@AndrewChubatiuk
Copy link
Contributor Author

this change will run arm64 and amd64 image builds on separate machines, so it should improve build speed. okay, will test it in my fork

@guidopetri
Copy link
Contributor

Here's an example action that we already ran: https://github.com/getredash/redash/actions/runs/6546702269

I'm not sure how that was running so maybe we just did it wrong. :)

@@ -1,7 +1,5 @@
FROM node:16.20.1-bookworm as frontend-builder

RUN npm install --global --force yarn@1.22.19
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain the changes you're making to the Dockerfile here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed frontend build from Dockerfile as there are to many places, where same node dependencies are installed. Also there's no sense to build same platform independent frontend in many places

Dockerfile Show resolved Hide resolved
client/.babelrc Outdated
@@ -4,7 +4,7 @@
"@babel/preset-env",
{
"exclude": ["@babel/plugin-transform-async-to-generator", "@babel/plugin-transform-arrow-functions"],
"corejs": "2",
"corejs": "3",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand any of the JS dependency related changes; @eradman would you mind taking a look? Are we able to change these deps to enable arm64 images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted frontend changes, as they are built separately now

Dockerfile Show resolved Hide resolved
@AndrewChubatiuk
Copy link
Contributor Author

AndrewChubatiuk commented Nov 7, 2023

Here's an example action that we already ran: https://github.com/getredash/redash/actions/runs/6546702269

I'm not sure how that was running so maybe we just did it wrong. :)

@guidopetri Finished doing my changes. you can check build time here
https://github.com/AndrewChubatiuk/redash/actions/runs/6786331640

There's lot's of work still, lot's of python packages which are either not supported or outdated, do not have arm64 wheels. Making them up to date can reduce build time as well

Also node dependencies for cypress can be optimized, cause it also takes several minutes to install them inside container

@AndrewChubatiuk
Copy link
Contributor Author

@guidopetri @eradman
Are there any additional changes needed?

@eradman
Copy link
Collaborator

eradman commented Nov 9, 2023

This change seems to check out for me. Triggering CI tests

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #6577 (1dda186) into master (8bfc574) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6577   +/-   ##
=======================================
  Coverage   61.89%   61.89%           
=======================================
  Files         158      158           
  Lines       12992    12992           
  Branches     1774     1774           
=======================================
  Hits         8042     8042           
  Misses       4673     4673           
  Partials      277      277           

@eradman
Copy link
Collaborator

eradman commented Nov 9, 2023

The code coverage report is not relavent.

@guidopetri can you re-test a build with these changes on arm64?

npm install --global --force yarn@1.22.19
yarn cache clean && yarn --frozen-lockfile --network-concurrency 1

name: frontend
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% clear where this frontend gets used in the build. Would you mind explaining? Does this also get pushed to Dockerhub as part of the preview image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guidopetri frontend is added to docker image during a build for each target arch and then first pushed to redash/preview and then copied from redash/preview to redash/redash

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we build this locally, then? docker build -t redash_test . seems to not build the frontend

Copy link
Contributor

@guidopetri guidopetri left a comment

Choose a reason for hiding this comment

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

I'm unclear on what these changes are doing, and I think we need better documentation around this. How do we build things locally? What exactly is happening in the GHA build process? Why do we need to split up the frontend build?

@AndrewChubatiuk
Copy link
Contributor Author

I'm unclear on what these changes are doing, and I think we need better documentation around this. How do we build things locally? What exactly is happening in the GHA build process? Why do we need to split up the frontend build?

  • github actions is using amd64 runners even for arm64 builds, so moving frontend out of docker images makes build faster
  • to build an image locally is a two step procedure, which requires to build frontend and then build a docker image (i can make a separate dockerfile for CI or add a make action to run a single action for a build)
  • during a build process frontend is built outside a docker images, cause it's platform independent and then docker copies it inside in image during a build of docker image for each platform

@guidopetri
Copy link
Contributor

Hmm, alright. I think we should separate this into two PRs then:

  • one enabling ARM64 builds (which may be slow if they're building the frontend);
  • one separating the frontend build out, and making it clear how to build this locally. We should be able to run a docker command for this, and not depend on e.g. yarn being installed locally.

Does this make sense to you too? Or how do you think we should proceed?

@AndrewChubatiuk
Copy link
Contributor Author

merged alternative PR in #6674

@AndrewChubatiuk AndrewChubatiuk deleted the multiarch-build branch April 10, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants