Details

Reviewers

tomek
• max
varun
• jim
ashoat

Commits

rCOMM15911abd9dee: [services] Move as much content as possible to the parent base image

Summary

https://linear.app/comm/issue/ENG-804/fix-services-images

Moved as much as possible to the parent base image, bumped images' versions.

Depends on D3198

Test Plan

cd services
yarn build-backup-base
yarn build-tunnelbroker-base
yarn build-blob-base
yarn build-backup-base

yarn test-blob-service
yarn test-backup-service
yarn run-tunnelbroker-service

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Feb 23 2022, 8:47 AM

Herald added subscribers: atul, • adrian, tomek, ashoat. · View Herald TranscriptFeb 23 2022, 8:47 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 23 2022, 9:20 AM

Harbormaster failed remote builds in B6976: Diff 9840!

• karol edited the summary of this revision. (Show Details)Feb 24 2022, 2:44 AM

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: tomek, • max, varun, • jim.

• karol added a parent revision: D3198: [services] Move folly from Blob to Backup base.

remove failing COPY instructions

• karol added a child revision: D3200: [services] Backup - dev mode.Feb 24 2022, 2:56 AM

Harbormaster completed remote builds in B7011: Diff 9889.Feb 24 2022, 2:59 AM

• karol requested review of this revision.Feb 24 2022, 2:59 AM

tomek accepted this revision.Feb 25 2022, 3:59 AM

This revision is now accepted and ready to land.Feb 25 2022, 3:59 AM

Herald added a reviewer: ashoat. · View Herald TranscriptFeb 25 2022, 3:59 AM

Herald added a subscriber: • benschac. · View Herald Transcript

This revision now requires review to proceed.Feb 25 2022, 3:59 AM

Can you be more precise about what is being "fixed"? I thought that we had some strange situation where either blob depending on backup, or backup depended on blob... but looking at the "before" state here, I can't seem to find where that dependency is...

This revision is now accepted and ready to land.Feb 25 2022, 12:45 PM

rebase

• karol retitled this revision from [services] fix images to [services] Move as much content as possible to the parent base image.Mar 2 2022, 4:50 AM

In D3268#88515, @ashoat wrote:

Can you be more precise about what is being "fixed"? I thought that we had some strange situation where either blob depending on backup, or backup depended on blob... but looking at the "before" state here, I can't seem to find where that dependency is...

This is done in D3198, here we

Moved as much as possible to the parent base image

I edited the revision's title.

Harbormaster completed remote builds in B7102: Diff 10012.Mar 2 2022, 5:43 AM

Good idea. There are a couple other improvements possible now, in particular building gRPC with the -DgRPC_SSL_PROVIDER=package and -DgRPC_ZLIB_PROVIDER=package flags to avoid building BoringSSL and rebuilding zlib and instead use the system libs installed in the base image.

services/backup/docker-base/Dockerfile
7 ↗	(On Diff #10012)	Moving this into a separate RUN command defeats the whole point, which is to avoid having a layer with the whole apt cache. See https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#apt-get "In addition, when you clean up the apt cache by removing /var/lib/apt/lists it reduces the image size, since the apt cache is not stored in a layer."

This is done in D3198, here we

Huh? It's confusing to read this. In D3198 you clearly indicate that the confusing dependency is not addressed – in fact, in that diff you created a Linear task to track that issue (on my request).

But now you seem to be indicating that that diff solves the confusing dependency?

Honestly, I think the reason for my continued confusion is that you responded to my request to be "more precise" with a total of 12 words. Can you please try again, and this time explain in detail an answer to my question?

Here's the question again:

I thought that we had some strange situation where either blob depending on backup, or backup depended on blob... but looking at the "before" state here, I can't seem to find where that dependency is...

Where is the dependency? Does it still exist?

Requesting changes for @jimpo's comment. @karol-bisztyga, I'm pretty sure that the issues around build caching in Docker and the reasoning for merging these RUN commands has been discussed before in diffs that you have been on. See my comment here – it's concerning to me that you missed that whole discussion, and then made this change without doing the appropriate research

This revision now requires changes to proceed.Mar 2 2022, 10:46 PM

Here I said:

I'm going to push here some changes that get rid of it and an additional diff that follows the DRY rule.

And by "it" I meant this confusing dependency, this was said in D3198. So, once again:

in D3198 I get rid of the confusing dependency
here I get rid of repeated stuff in the Dockerfiles

I see now that the description in this task was probably misleading so I updated it.

I hope it's now all clear, I'm not sure what else I can say to put yet more light on this, it looks pretty straightforward to me.

Requesting changes for @jimpo's comment. @karol-bisztyga, I'm pretty sure that the issues around build caching in Docker and the reasoning for merging these RUN commands has been discussed before in diffs that you have been on. See my comment here – it's concerning to me that you missed that whole discussion, and then made this change without doing the appropriate research

Yes, this is an oversight on my end, sorry, I'm going to fix this. @jimpo thanks for clarifying and catching this!

In D3268#89281, @jimpo wrote:

Good idea. There are a couple other improvements possible now, in particular building gRPC with the -DgRPC_SSL_PROVIDER=package and -DgRPC_ZLIB_PROVIDER=package flags to avoid building BoringSSL and rebuilding zlib and instead use the system libs installed in the base image.

Is there a task for this?

update

Harbormaster completed remote builds in B7128: Diff 10039.Mar 3 2022, 3:49 AM

In D3268#89591, @karol-bisztyga wrote:

Is there a task for this?

Just made one https://linear.app/comm/issue/ENG-824/dont-build-redundant-dependencies-for-grpc

In D3268#89586, @karol-bisztyga wrote:

Here I said:

I'm going to push here some changes that get rid of it and an additional diff that follows the DRY rule.

And by "it" I meant this confusing dependency, this was said in D3198. So, once again:

in D3198 I get rid of the confusing dependency

here I get rid of repeated stuff in the Dockerfiles

I see now that the description in this task was probably misleading so I updated it.

I hope it's now all clear, I'm not sure what else I can say to put yet more light on this, it looks pretty straightforward to me.

Thanks for this explanation – I understand now!!

services/base-image/contents/install_grpc.sh
7–11	Let's make the semicolons match here too. Whenever a reviewer leaves a comment, it's always good to see if it applies anywhere else in your currently open diff(s)

This revision is now accepted and ready to land.Mar 3 2022, 10:16 PM

ashoat mentioned this in D3198: [services] Move folly from Blob to Backup base.Mar 3 2022, 10:20 PM

fix if then

In D3268#89659, @jimpo wrote:

In D3268#89591, @karol-bisztyga wrote:

Is there a task for this?

Just made one https://linear.app/comm/issue/ENG-824/dont-build-redundant-dependencies-for-grpc