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

Add tini to the images #165

Merged
merged 8 commits into from
Aug 30, 2021
Merged

Add tini to the images #165

merged 8 commits into from
Aug 30, 2021

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Aug 25, 2021

Closes #161

@felipecrs
Copy link
Contributor Author

References: krallin/tini-images#9

@felipecrs
Copy link
Contributor Author

felipecrs commented Aug 25, 2021

@wperron any word about the distroless and centos approach? It's the only place where we pin the tini version.

@wperron wperron self-requested a review August 26, 2021 13:21
@wperron
Copy link
Contributor

wperron commented Aug 26, 2021

@wperron any word about the distroless and centos approach? It's the only place where we pin the tini version.

We should probably pin the other's version to the same version as well then, I know this is possible with apt, pretty sure it's also possible with apk for Alpine. if the versions on those package managers is lower than the latest GitHub release then let's just revert to the oldest so that they all use the same version.

Otherwise looks good to me 👍 Inspecting the images locally, this change barely adds around 1.7Mb to the images, I think that's fine given the benefits we get from it.

@felipecrs
Copy link
Contributor Author

this change barely adds around 1.7Mb to the images, I think that's fine given the benefits we get from it.

Well, this is way more than I expected... as the size of tini binary (non-statically compiled) is 23.5KB. The static one is still less than 1MB.

@wperron
Copy link
Contributor

wperron commented Aug 26, 2021

this change barely adds around 1.7Mb to the images, I think that's fine given the benefits we get from it.

Well, this is way more than I expected... as the size of tini binary (non-statically compiled) is 23.5KB. The static one is still less than 1MB.

I'm guessing there's also some dynamic dependencies or other files being installed on the filesystem that are required for it to work. I'll dive deeper later today, but as I said, it's nothing that would prevent a merge imo

@felipecrs
Copy link
Contributor Author

felipecrs commented Aug 26, 2021

@wperron now all uses the same tini binary (not the statically compiled one), and the final size of the image was not even noticeable affected:

container-diff diff daemon://deno:debian daemon://deno:debian-tini-small --type=file --type=size

-----File-----

These entries have been added to deno:debian:
FILE         SIZE
/tini        23.5K

These entries have been deleted from deno:debian: None

These entries have been changed between deno:debian and deno:debian-tini-small: None

-----Size-----

Image size difference between deno:debian and deno:debian-tini-small:
SIZE1         SIZE2
163.7M        163.7M

It turned out that the tini provided by the distribution was installing both variants of tini, which I think is unnecessary.

The advice is: https://github.com/krallin/tini#statically-linked-version

As it did not fail to start, I guess we are good to go.

What surprised me is that it worked even for the distroless. :)

@felipecrs felipecrs changed the title Add tini Include tini in the container Aug 26, 2021
@felipecrs felipecrs changed the title Include tini in the container Add tini to the images Aug 26, 2021
Copy link
Contributor

@wperron wperron left a comment

Choose a reason for hiding this comment

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

LGTM with a minor nitpick; can we get the tini version in an arg like we do for the deno version?

@felipecrs
Copy link
Contributor Author

@wperron
Copy link
Contributor

wperron commented Aug 27, 2021

LGTM with a minor nitpick; can we get the tini version in an arg like we do for the deno version?

Yeah sorry, nevermind me

Copy link
Contributor

@wperron wperron left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@wperron wperron merged commit 619a4b7 into denoland:main Aug 30, 2021
@felipecrs felipecrs deleted the tini branch August 30, 2021 15:25
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.

Add tini to container
3 participants