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

feat: dockerize iroh services #494

Merged
merged 7 commits into from
Nov 15, 2022
Merged

feat: dockerize iroh services #494

merged 7 commits into from
Nov 15, 2022

Conversation

b5
Copy link
Member

@b5 b5 commented Nov 14, 2022

building this in arm64 first because compiling amd64 on an M1 mac is slow to the point of not working. This'll take a while to get into a usable place, but I'd welcome any feedback / input / docker tips along the way

Update: I think this is in a pretty good spot for review. Seems to work well on my machine, going to try these same Dockerfiles on an an x86 machine tonight.

As with all docker nonsense, this is more searching the web & waiting on compilation than actually doing anything. My main article was How to Create Small Docker Images For Rust. I steered away from alpine after reading this article on the slowness of the MUSL allocator, which alpine relies upon. I haven't tested if those numbers are real.

So far the distroless project has been nice to work with.

@b5 b5 self-assigned this Nov 14, 2022
@b5 b5 changed the title feat: initial arm64 dockerfile work feat: dockerize iroh services Nov 14, 2022
@b5 b5 marked this pull request as ready for review November 14, 2022 21:42
@b5 b5 requested a review from Arqu November 14, 2022 21:47
.args([
"build",
"-t",
format!("{}:distroless", image).as_str(),
Copy link
Member Author

Choose a reason for hiding this comment

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

this tags images in the form iroh-one:distroless... that's probably wrong

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, for starters let's do :commit_hash and then builders can also tag with latest


COPY ../ .

RUN cargo build --bin iroh-gateway --release
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we build with cargo build --profile ci --bin iroh-gateway. This uses the release profile we want to maintain for binaries (additional perf optimizations & slimmer binaries) and also makes future changes simple .toml changes instead of needing to touch all scripts.

Copy link
Member Author

Choose a reason for hiding this comment

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

how about we do one-better & create a docker profile that matches ci so they can change independently?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sgtm, I can push that update then

Copy link
Member Author

Choose a reason for hiding this comment

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

pretty sure I can handle in this PR, it's just a change to the workspace Cargo.toml, right?


# Copy our build, changing owndership to distroless-provided "nonroot" user,
# (65532:65532)
COPY --from=builder --chown=65532:65532 /iroh/target/release/iroh-gateway ./
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would love to validate the file descriptor limits get raised/set correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe these are set at runtime, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but docker is weird. It sets up to max ~65k at runtime but if not root the "hard limit" specified on the system level might be low (usually 1024). Since it's a non-root user, we might hit that easily. The proper thing should be setting the FD limit on the image directly as the system level config. Then the runtime can self manage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

NVM just confirmed, this should be handled by the host system running docker, either by configuring the docker runner directly or supplying --ulimit=12321312 when running stuff. Either way think this is a solid point for docs.


COPY ../ .

RUN cargo build --bin iroh-one --release
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

USER nonroot

# expose gateway port
EXPOSE 9050
Copy link
Collaborator

@Arqu Arqu Nov 14, 2022

Choose a reason for hiding this comment

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

Unsure what the current story on iroh-one <> rpc api is but might want to open 4400-4403 too


COPY ../ .

RUN cargo build --bin iroh-p2p --release
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same

USER nonroot

# expose the default RPC port
EXPOSE 4401
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just the RPC port, we also have 4444 for all the p2p traffic as a default.

Arqu
Arqu previously approved these changes Nov 15, 2022
Copy link
Collaborator

@Arqu Arqu left a comment

Choose a reason for hiding this comment

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

LGTM, just left a comment to document the fd limit stuff.

@b5 b5 merged commit f8af48a into main Nov 15, 2022
@b5 b5 deleted the b5/dockerize branch November 15, 2022 19:35
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.

2 participants