Details

Reviewers

• max
• karol
varun
ashoat

Commits

rCOMM370381a0bf4c: tunnelbroker: Merge tunnelbroker-base and tunnelbroker-server images

Summary

Discussed with karol-bisztyga in chat that the original reason for splitting the images was for layer caching. This resolves the layer caching problem and merges the images.

This also removes the base image from docker-compose.yml, requiring that scripts/build_base_image.sh be run explicitly before docker-compose commands or implicitly using the run and test scripts.

I merged/flattened the directory structures in a way that I like, but this is open to discussion.

Only does this for one service, tunnelbroker, as it is a proof-of-concept in a series of diffs.

Test Plan

Run tunnelbroker service launch script

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jim created this revision.Feb 16 2022, 12:13 PM

Herald added subscribers: • benschac, atul, • adrian, tomek. · View Herald TranscriptFeb 16 2022, 12:13 PM

tunnelbroker: Merge tunnelbroker-base and tunnelbroker-server images
services: Remove base image from docker-compose services
tunnelbroker: Reorganize code structure

Harbormaster completed remote builds in B6900: Diff 9751.Feb 16 2022, 12:18 PM

• jim edited the summary of this revision. (Show Details)Feb 16 2022, 12:18 PM

• jim edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B6901: Diff 9753.Feb 16 2022, 12:20 PM

• jim requested review of this revision.Feb 16 2022, 12:20 PM

• jim added a parent revision: D3233: services: Use .env file for docker-compose.Feb 16 2022, 12:20 PM

• jim added a child revision: D3236: tunnelbroker: Generate gRPC/protobuf code with CMake.Feb 16 2022, 12:28 PM

I think it would be good to introduce this change after we fully release the services. We don't even have tests for tunnelbroker... It would be good to first have a code that works well and only then refactor the structure. But that's just my opinion about this.

Given that this is just a change to the environment, I think testing that the image builds and the container can be launched with the service running is a good enough test. Though this diff in the stack in particular is more of a proposal and maybe should be delayed until a similar change is ready for the other services so that they all have the same code structure.

I think it would be good to introduce this change after we fully release the services. We don't even have tests for tunnelbroker... It would be good to first have a code that works well and only then refactor the structure. But that's just my opinion about this.

Given the amount of work it probably took to put this up, and given we want to make this change sooner or later, I think it makes sense to land now.

@karol-bisztyga and @geekbrother, can you each give it a full review? The two of you have the most context on this code.

I'm unable to patch this diff, can you rebase?

This revision now requires changes to proceed.Feb 25 2022, 12:28 AM

Fix blob service base to not depend on tunnelbroker

Harbormaster completed remote builds in B7053: Diff 9942.Feb 27 2022, 8:57 AM

@karol-bisztyga Rebased, you should be able to patch now

• jim mentioned this in D3236: tunnelbroker: Generate gRPC/protobuf code with CMake.Feb 28 2022, 11:35 AM

• karol requested changes to this revision.Mar 2 2022, 8:46 AM

• karol added inline comments.

services/blob/docker-base/Dockerfile
3 ↗	(On Diff #9942)	I don't think we should add changes for other services when this is only for tunnelbroker.
services/package.json
7 ↗	(On Diff #9942)	why is this removed?
services/scripts/build_base_image.sh
6 ↗	(On Diff #9942)	why are you using `>&2`?
services/tunnelbroker/Dockerfile
14–27 ↗	(On Diff #9942)	I think it's better to copy the entire folder at once. What's the reason for separating these?
34–36 ↗	(On Diff #9942)	Any reason these `COPY` instructions are here, separated, explicitly declaring every single file? Also, in the end, you copy the entire folder anyway, so what's the point of all prior copies?

This revision now requires changes to proceed.Mar 2 2022, 8:46 AM

• jim requested review of this revision.Mar 2 2022, 9:05 AM

• jim added inline comments.

services/blob/docker-base/Dockerfile
3 ↗	(On Diff #9942)	This seems like a clear typo to me which broke the blob service build after the tunnelbroker source files were reorganized. I say it's a typo because there's an install_folly.sh script in blob contents which is unused. And the blob service shouldn't depend on tunnelbroker source anyway.
services/package.json
7 ↗	(On Diff #9942)	`build_base_image` is changed to only build the base image. And to build all, you can just do `docker-compose build`. If you want I can put back in an entry like `"build-all": "./scripts/build_base_image.sh && docker-compose build"`.
services/scripts/build_base_image.sh
6 ↗	(On Diff #9942)	I generally print to stderr before exiting with an error code. I'm fine removing this though, doesn't make much of a difference.
services/tunnelbroker/Dockerfile
14–27 ↗	(On Diff #9942)	To preserve the build cache. See https://linear.app/comm/issue/ENG-771/tunnelbroker-base-image-cache-inefficiency
34–36 ↗	(On Diff #9942)	I wanted to rename the `docker` directory to `scripts` inside the container. Just makes more sense to me since it's more descriptive within the context of the Docker container. COPYs are cheap anyway.

Is there a task for refactoring all other services?

Back to your queue.

services/package.json
7 ↗	(On Diff #9942)	The idea is to let developers perform any actions using these yarn scripts. Ideally, they won't have to do anything manually(`docker ...`, `docker-compose ...`, etc). We just have to assume the developer who wants to work on services has zero knowledge of docker, even when it comes to invoking a simple command, so they can just use the API from these scripts. I now see we miss an alias for `docker-compose down` in these scripts.
services/scripts/build_base_image.sh
6 ↗	(On Diff #9942)	I think we should either use it everywhere or nowhere. For now, it seems it's just in this one place, that makes the code inconsistent with the rest of the codebase.
services/tunnelbroker/Dockerfile
14–27 ↗	(On Diff #9942)	I see, nice, I think we should apply this to the rest of the services as well.
34–36 ↗	(On Diff #9942)	Why is it even called "docker" on the host machine? I understand it contains scripts, so why not call this just "scripts"? The name "docker" does not make sense to me in the first place.

This revision now requires changes to proceed.Mar 4 2022, 3:22 AM

Address review comments

Harbormaster failed remote builds in B7164: Diff 10090!Mar 4 2022, 12:42 PM

Rebased and addressed comments

In D3234#89832, @karol-bisztyga wrote:

Is there a task for refactoring all other services?

https://linear.app/comm/issue/ENG-771/service-base-image-cache-inefficiency and https://linear.app/comm/issue/ENG-777/generate-grpcproto-code-as-cmake-target for the next one in the stack apply to all services (I edited the issue name on the first one).

services/tunnelbroker/Dockerfile
34–36 ↗	(On Diff #9942)	I called it "docker" because these are scripts that are only meant to be run in the Docker container and are directly referenced and used in the Dockerfile. A directory called "scripts" generally indicates to me that they are tools that can be run on the host. For example, the `services/scripts` directory. I don't have a strong preference here though and would rename to "scripts" if you'd like.

Harbormaster completed remote builds in B7164: Diff 10090.Mar 4 2022, 1:54 PM

• karol accepted this revision.Mar 8 2022, 2:53 AM

• karol added inline comments.

services/tunnelbroker/Dockerfile
34–36 ↗	(On Diff #9942)	Yes, I understand. I'm deferring the final decision about naming to you, I still prefer something like "scripts" but it's ok.