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 ADD to reduce docker image sizes #7537

Merged
merged 3 commits into from
Nov 2, 2021

Conversation

jrhizor
Copy link
Contributor

@jrhizor jrhizor commented Nov 1, 2021

Goes from:

airbyte/server                       0.30.23-alpha   fee3d39ea555   800MB
airbyte/worker                       0.30.23-alpha   9f0cac3aa409   1.2GB
airbyte/scheduler                    0.30.23-alpha   786de4d6c25a   759MB

to

airbyte/server                       dev             4d3c4904659d   607MB
airbyte/worker                       dev             524ab249c040   1.03GB
airbyte/scheduler                    dev             54f64ac7e087   586MB

by utilizing the tar extraction ADD feature.

see https://airbytehq.slack.com/archives/C019WEENQRM/p1635686411078800 for more context.

@github-actions github-actions bot added area/platform issues related to the platform area/scheduler area/server area/worker Related to worker labels Nov 1, 2021
@jrhizor jrhizor temporarily deployed to more-secrets November 1, 2021 20:30 Inactive
COPY build/distributions/${APPLICATION}-0*.tar ${APPLICATION}.tar

RUN tar xf ${APPLICATION}.tar --strip-components=1
ADD build/distributions/${APPLICATION}-0*.tar /app
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to your PR but why do we hav -0*. in the tar path. It will break when we will switch the major version to 1.

On another point could you ping me when this will get merge since I am working on those file at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benmoriceau PTAL. I'm thinking we can add this to each of the projects. If we add this to all of the projects, #1605 will be resolved as well.

Copy link
Contributor Author

@jrhizor jrhizor Nov 1, 2021

Choose a reason for hiding this comment

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

With #7542 and this PR we should be good to close out #1605

@jrhizor jrhizor temporarily deployed to more-secrets November 1, 2021 20:54 Inactive
@benmoriceau
Copy link
Contributor

Other apps might benefits from that (migration for example)

@jrhizor jrhizor temporarily deployed to more-secrets November 1, 2021 22:04 Inactive
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

nice!

COPY build/distributions/${APPLICATION}-0*.tar ${APPLICATION}.tar

RUN tar xf ${APPLICATION}.tar --strip-components=1
ADD build/distributions/${APPLICATION}-0.30.23-alpha.tar /app
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, would we get the same effect if we deleted the tar archive after running tar xf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe that combined with multistage builds, but just doing that directly still keeps the separate layers which are still part of the image and downloaded.

Copy link
Contributor

@davinchia davinchia Nov 2, 2021

Choose a reason for hiding this comment

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

double checking this doesn't make it harder to pull in dev changes.

my understanding is we always build the tar named at the current version, regardless of dev or not, so this doesn't affect our current problem where we run into gradle caching when trying to build dev, where older versions are pulled in as the build task is cached.

so this should be an okay change. is this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do change how that's done we may need to inject a VERSION environment variable here, but since it works for both dev and normal versions now, I'm inclined to leave it as-is until we get to that point. It should obviously fail on CI if this becomes in a problem so I feel like it's pretty safe.

@jrhizor jrhizor merged commit b1f5c23 into master Nov 2, 2021
@jrhizor jrhizor deleted the jrhizor/use-add-to-reduce-docker-size branch November 2, 2021 16:45
lmossman pushed a commit that referenced this pull request Nov 3, 2021
* use ADD to reduce docker image sizes

* switch to full paths
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
* use ADD to reduce docker image sizes

* switch to full paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/scheduler area/server area/worker Related to worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants