Skip to content
This repository was archived by the owner on Dec 20, 2023. It is now read-only.

Dockerize server. #9

Merged
merged 3 commits into from
Sep 5, 2023
Merged

Dockerize server. #9

merged 3 commits into from
Sep 5, 2023

Conversation

yuroitaki
Copy link
Member

@yuroitaki yuroitaki commented Aug 23, 2023

#8

  • Dockerization
  • Minor fixes on wording, pinning github version
  • Minor config/fixture folder restructuring to facilitate docker configuration

@themighty1
Copy link
Member

I tried to run docker build . -t notary-server:local but I got an error:

Updating git repository https://github.com/tlsnotary/tlsn
warning: spurious network error (3 tries remaining): failed to resolve address for github.com: > Temporary failure in name resolution; class=Net (12)

This is not a problem of this docker image but generally a problem of docker for those who are on a VPN.
I had the same problem before. I had to temporarily turn off my VPN to allow the docker image to build.

Im wondering is this is just me and if this can be worked around to avoid potential friction during the hackathon.

heeckhau
heeckhau previously approved these changes Aug 24, 2023
Copy link
Member

@heeckhau heeckhau left a comment

Choose a reason for hiding this comment

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

Looks good. Great work!

sinui0
sinui0 previously approved these changes Aug 25, 2023
Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

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

gw 👍

@yuroitaki
Copy link
Member Author

I tried to run docker build . -t notary-server:local but I got an error:

Updating git repository https://github.com/tlsnotary/tlsn
warning: spurious network error (3 tries remaining): failed to resolve address for github.com: > Temporary failure in name resolution; class=Net (12)

This is not a problem of this docker image but generally a problem of docker for those who are on a VPN. I had the same problem before. I had to temporarily turn off my VPN to allow the docker image to build.

Im wondering is this is just me and if this can be worked around to avoid potential friction during the hackathon.

Thanks for reporting the issue, let me take a look

th4s
th4s previously approved these changes Aug 25, 2023
Copy link
Member

@th4s th4s left a comment

Choose a reason for hiding this comment

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

gw 🚀

@yuroitaki
Copy link
Member Author

warning: spurious network error (3 tries remaining): failed to resolve address for github.com: > Temporary failure in name resolution; class=Net (12)

Can you try if this solution works for you? rust-lang/cargo#6513 (comment)
Sorry I don't have any VPN setup here to test quickly

@themighty1
Copy link
Member

Thanks, I will look into the network problem.

Another unrelated problem: if there are already some artefacts from a previous cargo build present, they will be copied into the docker image:

Sending build context to Docker daemon 2.648GB

this can be solved if we explicitely copy only the files that we need

@themighty1
Copy link
Member

Can you try if this solution works for you? rust-lang/cargo#6513 (comment)
Sorry I don't have any VPN setup here to test quickly

Sadly, I got the same error even after I modified the first part of the Dockerfile to:

FROM rust:bookworm AS builder
RUN mkdir /root/.cargo
RUN echo "[net]\ngit-fetch-with-cli = true" > /root/.cargo/config.toml
RUN cat /root/.cargo/config.toml
WORKDIR /usr/src/notary-server
COPY . .
RUN cargo install --path .

@yuroitaki
Copy link
Member Author

yuroitaki commented Aug 25, 2023

Thanks, I will look into the network problem.

Another unrelated problem: if there are already some artefacts from a previous cargo build present, they will be copied into the docker image:

Sending build context to Docker daemon 2.648GB

this can be solved if we explicitely copy only the files that we need

Ah in the Dockerfile in this PR, we are already only copying into the final docker image the files we need, i.e. default config files and the compiled binary, and nothing else — am using the "builder runner" pattern.

The image generated in my machine has been in the order of 100-150MB, is your final image size much bigger in subsequent runs?

EDITED: Oh I think you're referring to the COPY . . step in the building process, then previous artefact inside /target folder shouldn't be copied across because /target is specified in .dockerignore — is your artefact being copied across?

@yuroitaki
Copy link
Member Author

Can you try if this solution works for you? rust-lang/cargo#6513 (comment)
Sorry I don't have any VPN setup here to test quickly

Sadly, I got the same error even after I modified the first part of the Dockerfile to:

FROM rust:bookworm AS builder
RUN mkdir /root/.cargo
RUN echo "[net]\ngit-fetch-with-cli = true" > /root/.cargo/config.toml
RUN cat /root/.cargo/config.toml
WORKDIR /usr/src/notary-server
COPY . .
RUN cargo install --path .

I tried with this modified Dockerfile, and in my machine I think cargo somehow is not reading from /root/.cargo/config.toml, but if I change the location of config.toml to be in WORKDIR, then cargo's reading from it (I tested with a verbose flag in the config file), perhaps can you try with the following Dockerfile?

FROM rust:bookworm AS builder
WORKDIR /usr/src/notary-server
COPY . .
RUN mkdir .cargo
RUN echo "[net]\ngit-fetch-with-cli  = true\n" > .cargo/config.toml
RUN cargo install --path .

@themighty1
Copy link
Member

Tried with the update you suggested, I still get an error. It is more descriptive this time

  Installing notary-server v0.1.0 (/usr/src/notary-server)
    Updating crates.io index
    Updating git repository `https://github.com/tlsnotary/tlsn`
fatal: unable to access 'https://github.com/tlsnotary/tlsn/': Could not resolve host: github.com
error: failed to compile `notary-server v0.1.0 (/usr/src/notary-server)`, intermediate artifacts can be found at `/usr/src/notary-server/target`

@themighty1
Copy link
Member

is specified in .dockerignore — is your artefact being copied across?

Sorry for the noise, I mistakenly assumed those files were being copied. The culprit was another unrelated folder inside notary-server with 2GB+ which was indexed by docker. And yes, the target folder is correctly getting ignored.

@yuroitaki yuroitaki dismissed stale reviews from th4s, sinui0, and heeckhau via 8cec530 August 25, 2023 12:10
@yuroitaki yuroitaki requested review from heeckhau, th4s and sinui0 August 28, 2023 09:13
th4s
th4s previously approved these changes Aug 28, 2023
Copy link
Member

@th4s th4s left a comment

Choose a reason for hiding this comment

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

ack 👍

heeckhau
heeckhau previously approved these changes Aug 28, 2023
Copy link
Member

@heeckhau heeckhau left a comment

Choose a reason for hiding this comment

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

Awesome!

sinui0
sinui0 previously approved these changes Aug 28, 2023
Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

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

1 comment on CI, otherwise lgtm 👍

@yuroitaki yuroitaki dismissed stale reviews from sinui0, heeckhau, and th4s via 2886f84 August 30, 2023 03:25
@yuroitaki yuroitaki changed the title Ns-3_dockerize Dockerize server. Aug 30, 2023
@yuroitaki yuroitaki requested review from heeckhau, th4s and sinui0 August 30, 2023 08:27
th4s
th4s previously approved these changes Aug 31, 2023
heeckhau
heeckhau previously approved these changes Aug 31, 2023
@yuroitaki yuroitaki dismissed stale reviews from heeckhau and th4s via b455e0c September 4, 2023 08:14
@yuroitaki
Copy link
Member Author

yuroitaki commented Sep 4, 2023

Tried with the update you suggested, I still get an error. It is more descriptive this time

  Installing notary-server v0.1.0 (/usr/src/notary-server)
    Updating crates.io index
    Updating git repository `https://github.com/tlsnotary/tlsn`
fatal: unable to access 'https://github.com/tlsnotary/tlsn/': Could not resolve host: github.com
error: failed to compile `notary-server v0.1.0 (/usr/src/notary-server)`, intermediate artifacts can be found at `/usr/src/notary-server/target`

For the record this has been solved by doing what's mentioned here

@sinui0 sinui0 self-requested a review September 4, 2023 19:01
@yuroitaki yuroitaki merged commit 46acc04 into main Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants