redash icon indicating copy to clipboard operation
redash copied to clipboard

added multiarch build

Open AndrewChubatiuk opened this issue 2 years ago • 10 comments

What type of PR is this?

Build multiarch docker images

  • [x] 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

AndrewChubatiuk avatar Nov 06 '23 11:11 AndrewChubatiuk

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?

guidopetri avatar Nov 06 '23 11:11 guidopetri

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

AndrewChubatiuk avatar Nov 06 '23 11:11 AndrewChubatiuk

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 avatar Nov 06 '23 12:11 guidopetri

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 avatar Nov 07 '23 18:11 AndrewChubatiuk

@guidopetri @eradman Are there any additional changes needed?

AndrewChubatiuk avatar Nov 09 '23 15:11 AndrewChubatiuk

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

eradman avatar Nov 09 '23 20:11 eradman

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           

codecov[bot] avatar Nov 09 '23 20:11 codecov[bot]

The code coverage report is not relavent.

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

eradman avatar Nov 09 '23 23:11 eradman

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

AndrewChubatiuk avatar Nov 23 '23 10:11 AndrewChubatiuk

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?

guidopetri avatar Nov 24 '23 22:11 guidopetri

merged alternative PR in https://github.com/getredash/redash/pull/6674

AndrewChubatiuk avatar Apr 10 '24 09:04 AndrewChubatiuk