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

Docker multiarch image #703

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Docker multiarch image #703

wants to merge 14 commits into from

Conversation

eternal-flame-AD
Copy link
Member

@eternal-flame-AD eternal-flame-AD commented Oct 16, 2024

fixes #257 .
fixes #350

Simply make build-docker-multiarch and then docker run should work. I implemented building JS from docker as well but opt-in, just for people who want's to build a usable local container without having to do anything outside of docker.

Current blockers: (nevermind I had a nap and figured out how to do it)

The last step of installing runtime dependency need qemu, it's not easy to get around that unfortunately

Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
@eternal-flame-AD eternal-flame-AD requested a review from a team as a code owner October 16, 2024 00:42
@eternal-flame-AD eternal-flame-AD changed the title Docker multiarch Docker multiarch image Oct 16, 2024
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.42%. Comparing base (58084c8) to head (0bbf37a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #703      +/-   ##
==========================================
- Coverage   80.46%   79.42%   -1.05%     
==========================================
  Files          56       56              
  Lines        2191     2639     +448     
==========================================
+ Hits         1763     2096     +333     
- Misses        337      452     +115     
  Partials       91       91              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
@eternal-flame-AD
Copy link
Member Author

eternal-flame-AD commented Oct 16, 2024

Here's the Docker scout cloud analysis, you should be able to reproduce it with docker scout CLI.

Sorry about the force pushes, messed up a rebase :(

image

image

@eternal-flame-AD eternal-flame-AD force-pushed the docker-multiarch branch 2 times, most recently from 9caea83 to 377ef3c Compare October 16, 2024 05:23
Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

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

Nicely done, some remarks in subcomments.

docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
docker/Dockerfile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
docker/Dockerfile Show resolved Hide resolved
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
jmattheis
jmattheis previously approved these changes Oct 20, 2024
Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

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

👍

.github/workflows/build.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,8 @@
vendor/
Copy link
Member

Choose a reason for hiding this comment

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

We can add /ui/node_modules here too right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking you can save some useless download by pre installing dependencies at the cost of some needless cache invalidation, but I think the probability of actual hitting the problem is low in real development is lower in the latter case.

In the current solution if you want to build the container locally while testing the BUILD_JS you can pre install the dependencies to make it much quicker.

In the other way around I think theoretically you can have a situation where you have a large amount of arch-specific node modules but that's probably not currently true

Copy link
Member

@jmattheis jmattheis Oct 20, 2024

Choose a reason for hiding this comment

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

For me the copying of the node_modules folder takes ~50s. Which is basically the same as installing the deps for me.

With node_modules

 => [internal] load build definition from Dockerfile                     ... 0.0s
 => => transferring dockerfile: 2.07kB                                   ... 0.0s
 => [internal] load metadata for docker.io/library/debian:sid-slim       ... 1.0s
 => [internal] load metadata for docker.io/library/node:23               ... 1.0s
 => [internal] load metadata for docker.io/gotify/build:1.22.4-linux-amd6... 0.0s
 => [auth] library/node:pull token for registry-1.docker.io              ... 0.0s
 => [auth] library/debian:pull token for registry-1.docker.io            ... 0.0s
 => [internal] load .dockerignore                                        ... 0.0s
 => => transferring context: 105B                                        ... 0.0s
 => [internal] load build context                                        ... 9.0s
 => => transferring context: 801.08MB                                    ... 8.8s
 => [js-builder 1/4] FROM docker.io/library/node:23@sha256:9d09fa506f5b84... 0.0s
 => [stage-2 1/4] FROM docker.io/library/debian:sid-slim@sha256:7bee69f27... 0.0s
 => CACHED [builder 1/7] FROM docker.io/gotify/build:1.22.4-linux-amd64  ... 0.0s
 => [builder 2/7] RUN apt-get update && apt-get install -yq --no-install-... 5.9s
 => [builder 3/7] RUN ls -lah                                            ... 0.3s
 => [builder 4/7] COPY . /src/gotify                                     ...47.9s
 => CACHED [js-builder 2/4] COPY ./Makefile /src/gotify/Makefile         ... 0.0s
 => [js-builder 3/4] COPY ./ui /src/gotify/ui                            ...47.8s
 => [js-builder 4/4] RUN if [ "0" = "1" ]; then     (cd /src/gotify/ui &&... 0.4s
 => [builder 5/7] COPY --from=js-builder /src/gotify/ui/build /ui-build  ... 0.3s
 => CANCELED [builder 6/7] RUN if [ "0" = "1" ]; then     cp -r --update ... 0.5s

With ignored node_modules

 => [internal] load build definition from Dockerfile                     ...0.1s
 => => transferring dockerfile: 2.07kB                                   ...0.0s
 => [internal] load metadata for docker.io/library/debian:sid-slim       ...0.5s
 => [internal] load metadata for docker.io/library/node:23               ...0.5s
 => [internal] load metadata for docker.io/gotify/build:1.22.4-linux-amd6...0.0s
 => [internal] load .dockerignore                                        ...0.1s
 => => transferring context: 122B                                        ...0.0s
 => [stage-2 1/4] FROM docker.io/library/debian:sid-slim@sha256:7bee69f27...0.0s
 => [internal] load build context                                        ...1.7s
 => => transferring context: 20.23kB                                     ...1.6s
 => [js-builder 1/4] FROM docker.io/library/node:23@sha256:9d09fa506f5b84...0.0s
 => [builder 1/7] FROM docker.io/gotify/build:1.22.4-linux-amd64         ...0.0s
 => CACHED [builder 2/7] RUN apt-get update && apt-get install -yq --no-i...0.0s
 => CACHED [builder 3/7] RUN ls -lah                                     ...0.0s
 => CACHED [js-builder 2/4] COPY ./Makefile /src/gotify/Makefile         ...0.0s
 => CACHED [js-builder 3/4] COPY ./ui /src/gotify/ui                     ...0.0s
 => CACHED [js-builder 4/4] RUN if [ "0" = "1" ]; then     (cd /src/gotif...0.0s
 => [builder 4/7] COPY . /src/gotify                                     ...2.3s
 => [builder 5/7] COPY --from=js-builder /src/gotify/ui/build /ui-build  ...0.2s
 => [builder 6/7] RUN if [ "0" = "1" ]; then     cp -r --update /ui-build...0.3s
 => CANCELED [builder 7/7] RUN cd /src/gotify &&     if [ "0" = "1" ] && ...4.2s

See [builder 4/7] COPY . /src/gotify

Copy link
Member

@jmattheis jmattheis Oct 20, 2024

Choose a reason for hiding this comment

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

okay, maybe nvm, my storage seems pretty slow. Have you tried building the multiarch image via github actions? Does this run fast there?

Copy link
Member Author

@eternal-flame-AD eternal-flame-AD Oct 20, 2024

Choose a reason for hiding this comment

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

I ran on act (local GitHub actions clone) it wasn't too bad (10 seconds around if I recall). But it heavily depends on file system optimizations, I have an SSD with XFS, if you have an HDD with something like ext4 that may be very slow.

I will push an update later and make this more stable, if you have this issue other people may have it to

I can open a branch for dry-run releases and see how it performs, I am busy for Monday so maybe Tuesday

Copy link
Member

Choose a reason for hiding this comment

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

Okay, yeah I've an SSD with ext4, need to debug this. Maybe it's some docker specific thing

$ dd if=/dev/zero of=foo conv=fdatasync bs=8k count=50k

419430400 bytes (419 MB, 400 MiB) copied, 4.52201 s, 92.8 MB/s
$ hdparm -Tt /dev/sda1
$ sudo hdparm -Tt /dev/sda1
/dev/sda1:
 Timing cached reads:   22502 MB in  2.00 seconds = 11278.21 MB/sec
 Timing buffered disk reads: 500 MB in  0.93 seconds = 538.35 MB/sec

Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add curl package on ARM Dockerfile Multi-Arch Docker Image
2 participants