Dockerize node #257

phklive · 2024-03-01T01:23:01Z

Docker will help us end-to-end test our code & enable easier sharing and installation of our code by our users.

Closes: #93

hackaugusto

Thanks! I left a few questions and suggestions.

Dockerfile

hackaugusto · 2024-03-04T19:27:06Z

Dockerfile

+RUN cd miden-node && make
+
+### Run Miden-Node
+FROM ubuntu:22.04


questions:

why ubuntu here instead of debian like the builder image? if we can, debian slim/alpine would be better

why 22.04 instead of 24.04?

This image should have LABELs. I suggest using open containers labels:

LABEL [email protected] \ org.opencontainers.image.url=https://0xpolygonmiden.github.io/ \ org.opencontainers.image.documentation=https://github.com/0xPolygonMiden/miden-node \ org.opencontainers.image.source=https://github.com/0xPolygonMiden/miden-node \ org.opencontainers.image.vendor=Polygon \ org.opencontainers.image.licenses=MIT ARG CREATED ARG VERSION ARG COMMIT LABEL org.opencontainers.image.created=$CREATED \ org.opencontainers.image.version=$VERSION \ org.opencontainers.image.revision=$COMMIT

We need to add a VOLUME to have the store's db.

To add a volume we would need to modify where the node reads and writes it's database files in order to persist them to the volume.

To add a volume we would need to modify where the node reads and writes it's database files in order to persist them to the volume.

The WORKDIR setting should handle that.

This is the last thing pending, have not yet been able to make it work correctly. Looking into it now.

Managed to fix this, now the node creates a db folder to put it's sqlite files, this volume is mounted from the host as a volume.

Dockerfile

scripts/start-miden-node.sh

Makefile

Dockerfile

polydez

I've left one nit comment inline, but overall it looks good!

polydez · 2024-03-12T12:59:33Z

node/Dockerfile

+
+ARG CREATED
+ARG VERSION
+ARG COMMIT


Can we move all args to the top (line 27 or even top of file) and group all labels into one single LABEL?

I think this is not a good idea. When I'm debugging the DockerFile, I will rebuild the same image multiple times using the same COMMIT, the same VERSION, and different CREATED timestamps.

The CREATED timestamps change will invalidate the cache, and force a rebuild of everything that follows it. Moving this to the top of the file means we never reuse the caching system when rebuilding images, and we spend tons of time with rebuilds.

Please move it down, as far as you can. Here is the docs

hackaugusto

Thanks! LGTM

Makefile.toml

hackaugusto · 2024-03-12T15:07:12Z

Dockerfile

+RUN cd miden-node && make
+
+### Run Miden-Node
+FROM ubuntu:22.04


To add a volume we would need to modify where the node reads and writes it's database files in order to persist them to the volume.

The WORKDIR setting should handle that.

hackaugusto · 2024-03-12T15:08:55Z

node/Dockerfile

+    libssl-dev \
+    libsqlite3-dev \


Do we need these dependencies? If so, can you please add some docs about it?

@hackaugusto IIRC, we need them for building under linux.

This is the runner image, no?

Yes it is indeed. Running the program failed when these were not installed on the machine.

That is interesting. Can you please add a comment above the deps. It seems a bit off that the headers are needed to run the compiled binary. Also, what was the error?

2024-03-12 16:55:51 miden-node: error while loading shared libraries: libsqlite3.so.0: cannot open shared object file: No such file or directory

Ah, for that you shouldn't need the headers, you only need the library. libsqlite3 should be enough, it is only marginally better though (to avoid confusion like I had here)

Hence what do you recommend to do?

Here is what I have been proposing:

keep dependencies to a minimum

document why they are needed

why am I proposing the above?

it is hard to maintain script files without this information, for example, the discussion of gcc and clang being needed is something that happened because of lack of documentation. I'm trying to prevent that

the above is specially a problem when upgrade the file down the line, and usually causes issues were one doesn't know why something is there, so we default to "better not touch it since it is working", which can compound over time

so here, since libsqlite3-dev is not needed, these are the headers, we would remove that dependency and have libsqlite3 instead. since that is the .so file that is needed to run the software, and add a comment # sqlite3: used by the store.

That way we have a minimum number of dependencies and some traceability on them, making it easier to maintain it.

edit: same thing for libssl-dev

hackaugusto

Thank you! I got the node started and played around with it a bit :) Super neat. We have some few things to fix though.

hackaugusto · 2024-03-14T11:52:31Z

node/Dockerfile

+RUN --mount=type=cache,target=/usr/local/cargo/registry \
+    cargo fetch --manifest-path node/Cargo.toml


This path doesn't seem right.

The build cache is ./target ref

The registry cache is $CARGO_HOME/registry, which is usually $HOME/.cargo/registry ref

Yeah, I just tested, the cache is not working, the VM is downloading everything fresh:

[1/2] STEP 10/12: RUN --mount=type=cache,target=/usr/local/cargo/registry cargo fetch --manifest-path node/Cargo.toml info: syncing channel updates for '1.75-aarch64-unknown-linux-gnu' info: latest update on 2023-12-28, rust version 1.75.0 (82e1608df 2023-12-21) info: downloading component 'cargo' info: downloading component 'clippy' info: downloading component 'rust-docs' info: downloading component 'rust-std' info: downloading component 'rustc' info: downloading component 'rustfmt' info: installing component 'cargo' info: installing component 'clippy' info: installing component 'rust-docs' info: installing component 'rust-std' info: installing component 'rustc' info: installing component 'rustfmt'

(also compiling)

Indeed for now the caching does not work. I am looking into doing it manually as we did or using cargo-chef: https://github.com/LukeMathWalker/cargo-chef

What do you think?

Makefile.toml

hackaugusto · 2024-03-14T12:09:31Z

.dockerignore

+**/secrets.dev.yaml
+**/values.dev.yaml
+/bin
+/target


This is not working for me. Maybe it is a different among podman and docker. But this is causing me to upload 11G of data to the docker VM, and at first sign it looked like podman wasn't working.

Can you change the paths from /bin and /target to ./bin and ./target? And also test on your machine?

I now wonder how the --mount is implemented on MacOs, because I may have given bad advice with the cargo cache below, if the data has to be copied to the VM.

hackaugusto · 2024-03-14T12:15:54Z

node/Dockerfile

+RUN miden-node make-genesis --inputs-path node/genesis.toml
+
+# Run Miden node
+FROM debian:bookworm-slim


Note: The final image has an okay size:

REPOSITORY TAG IMAGE ID CREATED SIZE localhost/miden-node-image latest 63b9223ac3cb About a minute ago 124 MB

Note: start/stop is working, the state is not lost

Note: The final image has an okay size:

REPOSITORY TAG IMAGE ID CREATED SIZE localhost/miden-node-image latest 63b9223ac3cb About a minute ago 124 MB

Do you have ideas in mind to improve on it?

Note: start/stop is working, the state is not lost

This is positive right?

Do you have ideas in mind to improve on it?

I think this size is good enough. I was just adding context to the PR.

Note: start/stop is working, the state is not lost

Yes. This was not a request. I was just adding context to the PR.

hackaugusto · 2024-03-14T12:29:23Z

node/Dockerfile

+
+ARG CREATED
+ARG VERSION
+ARG COMMIT


I think this is not a good idea. When I'm debugging the DockerFile, I will rebuild the same image multiple times using the same COMMIT, the same VERSION, and different CREATED timestamps.

The CREATED timestamps change will invalidate the cache, and force a rebuild of everything that follows it. Moving this to the top of the file means we never reuse the caching system when rebuilding images, and we spend tons of time with rebuilds.

Please move it down, as far as you can. Here is the docs

hackaugusto · 2024-03-14T12:31:05Z

Makefile.toml

+# docker
+[tasks.docker-build-node]
+command = "docker"
+args = ["build", "-f", "node/Dockerfile", "-t", "miden-node-image", "."]


This is not setting the ENV vars:

time="2024-03-14T13:29:04+01:00" level=warning msg="missing \"CREATED\" build argument. Try adding \"--build-arg CREATED=<VALUE>\" to the command line" time="2024-03-14T13:29:04+01:00" level=warning msg="missing \"VERSION\" build argument. Try adding \"--build-arg VERSION=<VALUE>\" to the command line" time="2024-03-14T13:29:04+01:00" level=warning msg="missing \"COMMIT\" build argument. Try adding \"--build-arg COMMIT=<VALUE>\" to the command line"

Indeed, I am curious from what data you would want to populate these fields ?

I added the version and created from the crate version and date shell command.

I am not sure how you would like to get the commit info.

The COMMIT should have the git commit.
The VERSION should have the node's software version.
The CREATED should have the created timestamp.

Where can i get the created timestamp and the git commit ?

hackaugusto · 2024-03-14T12:33:58Z

node/Dockerfile

+VOLUME db
+
+# Copy artifacts from the builder stage
+COPY --from=builder /app/node/miden-node.toml miden-node.toml


This means we can't change the configuration without having to rebuild all images. This is not good. This should be mounted from the host.

I enabled the manual mounting of the file using the -v flag.

ah, makes sense, so this file is the default, and the user can overwrite it with the flag 👍

node/Dockerfile

hackaugusto

LGTM, thanks!

Makefile.toml

phklive · 2024-03-14T15:12:58Z

Tested. Last thing we could do would be to improve caching to reduce build time of the image, created a issue so we can merge this PR and go forward: #277

bobbinth · 2024-03-14T17:14:01Z

miden-node.toml

Quick question: why do we need to duplicate this file here?

We do not need to, fixed it in the next PR.

* Working initial docker * fmt * added script * Removed docker script + updated integration * script works * Need to install grpcurl * Docker works * Working Dockerfile * ci: turn doc warnings into errors (#259) * Removed makefile, removing start.sh file moving in Dockerfile * Added bookworm + alpine + removed gcc + added caching * Moved Dockerfile to node and added Makefile.toml * cargo make works + builds dockerfile for node * remove start script * added comment regarding PID1 * Set labels at top of Dockerfile + added comments * Added correct dependencies and explanation * Volume works persisting db files between runs * Added documentation * Moved labels + enable mounting of miden-node.toml * Added docker-run-node command + added build arguments * Add git commit as arg to docker container * Added spacing --------- Co-authored-by: Augusto Hack <[email protected]>

phklive added 9 commits February 15, 2024 17:43

Working initial docker

e1c2e9b

Merge branch 'main' into phklive-dockerize-node

8f28a4d

fmt

80c60b3

added script

042ad7f

Removed docker script + updated integration

ac7ebe7

script works

2d01181

Need to install grpcurl

47a76e9

Docker works

20b4262

Working Dockerfile

3e77d00

phklive requested review from bobbinth and hackaugusto March 4, 2024 19:16

phklive marked this pull request as ready for review March 4, 2024 19:16

hackaugusto reviewed Mar 4, 2024

View reviewed changes

hackaugusto and others added 5 commits March 5, 2024 11:32

ci: turn doc warnings into errors (#259)

96f4a8b

Removed makefile, removing start.sh file moving in Dockerfile

8298f8f

Merge branch 'main' into phklive-dockerize-node

59e461d

Added bookworm + alpine + removed gcc + added caching

891f77e

Merge branch 'main' into phklive-dockerize-node

04a21d4

bobbinth reviewed Mar 8, 2024

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

phklive added 2 commits March 9, 2024 11:28

Moved Dockerfile to node and added Makefile.toml

21ee551

cargo make works + builds dockerfile for node

29f38c5

phklive changed the base branch from main to next March 11, 2024 17:51

bobbinth force-pushed the next branch from 96f4a8b to bfc63a1 Compare March 11, 2024 22:14

phklive added 4 commits March 12, 2024 10:11

Pull

7b97987

merge next + add docker build and doc in ci

53d9c96

remove start script

494bb55

added comment regarding PID1

c9ed28f

phklive requested review from hackaugusto and polydez March 12, 2024 09:55

polydez approved these changes Mar 12, 2024

View reviewed changes

hackaugusto approved these changes Mar 12, 2024

View reviewed changes

phklive added 2 commits March 13, 2024 15:16

Set labels at top of Dockerfile + added comments

5fb9679

Added correct dependencies and explanation

89883af

phklive mentioned this pull request Mar 14, 2024

Add End-to-End tests for the Miden Node #222

Open

phklive added 4 commits March 14, 2024 11:52

Volume works persisting db files between runs

203f6b1

Merge branch 'next' of github.com:0xPolygonMiden/miden-node into next

ddc3c88

Merge branch 'next' into phklive-dockerize-node

eac99cd

Added documentation

8600cf5

phklive requested a review from hackaugusto March 14, 2024 11:06

hackaugusto suggested changes Mar 14, 2024

View reviewed changes

hackaugusto reviewed Mar 14, 2024

View reviewed changes

node/Dockerfile Outdated Show resolved Hide resolved

phklive added 2 commits March 14, 2024 14:52

Moved labels + enable mounting of miden-node.toml

d04e71e

Added docker-run-node command + added build arguments

d09f6f9

hackaugusto approved these changes Mar 14, 2024

View reviewed changes

Makefile.toml Outdated Show resolved Hide resolved

phklive added 2 commits March 14, 2024 15:46

Add git commit as arg to docker container

8d138ec

Added spacing

1732666

phklive merged commit 2e66d45 into next Mar 14, 2024
5 checks passed

phklive deleted the phklive-dockerize-node branch March 14, 2024 15:23

bobbinth reviewed Mar 14, 2024

View reviewed changes

		RUN --mount=type=cache,target=/usr/local/cargo/registry \
		cargo fetch --manifest-path node/Cargo.toml

Dockerize node #257

Dockerize node #257

Conversation

phklive commented Mar 1, 2024

hackaugusto left a comment

Choose a reason for hiding this comment

hackaugusto Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phklive Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

hackaugusto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

hackaugusto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto left a comment

Choose a reason for hiding this comment

phklive commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto Mar 4, 2024 •

edited

Loading

phklive Mar 13, 2024 •

edited

Loading

hackaugusto Mar 14, 2024 •

edited

Loading

hackaugusto Mar 12, 2024 •

edited

Loading

hackaugusto Mar 14, 2024 •

edited

Loading