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

Use multi-stage builds to unzip and copy jars #118

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

bradbown
Copy link
Contributor

@bradbown bradbown commented Sep 30, 2024

This PR implements issue(s) #134

Multi stage reduces size of image by 110MB.
Removes unnecessary libs depending on architecture.

Checklist

  • I wrote new tests for my new core changes.
  • I have successfully ran tests, style checker and build against my new changes locally.
  • I have informed the team of any breaking changes if there are any.

Copy link

github-actions bot commented Sep 30, 2024

Delta Summary - Kotlin Code Coverage

Generated on: 10/01/2024 - 12:55
Description Previous Current Delta
Coverage date: 10/01/2024 - 12:55 10/01/2024 - 12:55
Tag: 588_11125691323 588_11125691323
Line coverage: 27.3% 27.3% 0.0%
Covered lines: 25673 25673 0
Coverable lines: 93723 93723 0
Total lines: 132974 132974 0
Branch coverage: 10.4% 10.4% 0.0%
Covered branches: 5529 5529 0
Total branches: 52680 52680 0
Method coverage: Feature is only available for sponsors

@bradbown bradbown requested review from a team September 30, 2024 17:41
coordinator/Dockerfile Outdated Show resolved Hide resolved
@bradbown bradbown force-pushed the docker-coordinator-image-size-reduction branch 2 times, most recently from 39ebe00 to 509c849 Compare October 1, 2024 09:55
coordinator/Dockerfile Outdated Show resolved Hide resolved
coordinator/Dockerfile Outdated Show resolved Hide resolved
coordinator/Dockerfile Outdated Show resolved Hide resolved
coordinator/Dockerfile Outdated Show resolved Hide resolved
coordinator/Dockerfile Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.21%. Comparing base (c4abdd9) to head (eb11af2).

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #118      +/-   ##
============================================
- Coverage     70.32%   70.21%   -0.11%     
+ Complexity     1081     1080       -1     
============================================
  Files           304      304              
  Lines         12413    12413              
  Branches       1179     1179              
============================================
- Hits           8729     8716      -13     
- Misses         3190     3200      +10     
- Partials        494      497       +3     
Flag Coverage Δ *Carryforward flag
hardhat 98.69% <ø> (ø) Carriedforward from c4abdd9
kotlin 67.93% <ø> (-0.12%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

see 4 files with indirect coverage changes

---- 🚨 Try these New Features:

@bradbown bradbown force-pushed the docker-coordinator-image-size-reduction branch from d472ac4 to da905b6 Compare October 10, 2024 11:31

RUN case $(uname -m) in \
x86_64) \
rm -rf /libs/coordinator/lib/unpacked-blob-compressor/darwin-**; \
Copy link
Contributor

@jonesho jonesho Oct 14, 2024

Choose a reason for hiding this comment

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

Can this be moved out from the switch case here? Seems they would be removed regardless of the case, or can we use a local label to replace the the switch case to avoid repeated commands? sth like:

...
rm -rf /libs/coordinator/lib/unpacked-blob-compressor/linux-$MACHINE_TYPE/; \
...
rm -rf /libs/coordinator/lib/unpacked-blob-shnarf-calculator/linux-$MACHINE_TYPE/;

coordinator/Dockerfile Outdated Show resolved Hide resolved
RUN apt-get update \
&& apt-get install curl -y \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/* \
Copy link
Contributor

Choose a reason for hiding this comment

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

I dun think we even need these apt-get related commands here, right?

@jonesho jonesho force-pushed the docker-coordinator-image-size-reduction branch from a63e658 to 1939569 Compare November 6, 2024 08:41
@jonesho jonesho temporarily deployed to docker-build-and-e2e November 6, 2024 08:41 — with GitHub Actions Inactive
@jonesho jonesho temporarily deployed to docker-build-and-e2e November 6, 2024 10:39 — with GitHub Actions Inactive
@jonesho jonesho force-pushed the docker-coordinator-image-size-reduction branch from b2ef74c to c6d5449 Compare November 19, 2024 05:32
@jonesho jonesho temporarily deployed to docker-build-and-e2e November 19, 2024 05:32 — with GitHub Actions Inactive
@jonesho jonesho temporarily deployed to docker-build-and-e2e November 19, 2024 05:45 — with GitHub Actions Inactive
@jonesho jonesho force-pushed the docker-coordinator-image-size-reduction branch from 7efc0c9 to bf1b194 Compare November 20, 2024 07:18
@jonesho jonesho temporarily deployed to docker-build-and-e2e November 20, 2024 07:18 — with GitHub Actions Inactive
@jonesho jonesho force-pushed the docker-coordinator-image-size-reduction branch from 30df9bd to eb11af2 Compare November 25, 2024 06:25
@jonesho jonesho temporarily deployed to docker-build-and-e2e November 25, 2024 06:25 — with GitHub Actions Inactive
@@ -1,20 +1,38 @@
FROM openjdk:22-ea-17-slim-bookworm
# BUILDER image
FROM eclipse-temurin:21-jdk-noble AS builder
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jonesho IIRC from our last our meeting with @julien-marchand we agreed on not having a builder image because the step commands bellow in RUN can be executed in the host machine.?

What is the motivation and value of having a builder image?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jpnovais Yes, but as mentioned above, the jar command was not available in JRE image, and hence we need to use JDK image here to use the jar command to extract and recreate the JAR archive based on the machine type (from uname -m). I was thinking about moving this "extract and recreate JAR" step to pipeline job but then we'd need separate "extract + build" jobs for arm64 and amd64 image respectively, which would take up more pipeline resources and runtime imo, and complicate the build process. Welcome for any better approach :)

@jonesho jonesho requested review from jpnovais and removed request for jpnovais November 26, 2024 03:37
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