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

Update docker images and provide examples #1532

Merged
merged 11 commits into from
Mar 11, 2024
Merged

Conversation

erubboli
Copy link
Member

@erubboli erubboli commented Feb 3, 2024

This PR adds more images for api-blockchain-scanner-daemon, api-web-server, dns_server, wallet-rpc-daemon and removes the image for node-gui. In addition runs the commands as normal user (mintlayer) instead of the default root user.

TODO:

  • Ensure all ports are correctly exposed (default to mainnet).
  • Add/fix documentation (will be done separately)
  • Add an example to run a whole block explorer in a docker-compose file. (it's better to do this in the block explorer's own repository)
  • Fix binding host on api-web-server and, if necessary, the other tools (e.g., --rpc-address node-daemon:3030 should work).
  • In Docker, it is very common to pass configuration details using environment variables so that this can work:
api-web-server:
  image: mintlayer/api-web-server:0.3.0
  environment: 
    - RPC_ADDRESS=node-daemon:3030 
    - POSTGRES_HOST=postgres 
    - POSTGRES_PASSWORD=use-strong-password 
    - POSTGRES_DATABASE=mintlayer_chain

@TheQuantumPhysicist TheQuantumPhysicist marked this pull request as draft February 5, 2024 10:48
@TheQuantumPhysicist TheQuantumPhysicist changed the title [draft] update docker images and provide examples Update docker images and provide examples Feb 5, 2024
@ImplOfAnImpl ImplOfAnImpl changed the base branch from master to fix/locked-utxo-conflict February 21, 2024 16:48
@ImplOfAnImpl
Copy link
Contributor

So, I've taken over this PR and pushed some commits. The PR is curently based on the "fix/locked-utxo-conflict" branch, because that branch contains some fixes required for api scanner daemon to work properly.

  1. Block explorer example hasn't been and will not be added here. Though technically it's not hard, Sam is against it. The main reason is that the block explorer is not core devs' responsibility; we don't want to have to support it. The better way should be to put the corresponding example into the block explorer's own repository.
  2. All services are put into the same docker compose file; users are supposed to copy the directory and comment out the services they don't need.
    The exception is dns-server, which has its own separate compose project.
    (This is a Sam's suggestion as well; initially I put unrelated services into diferent yml files and it worked, but it was kind of inconvenient to use).
  3. Instead of using a named volume, I just map the home directories of containers to a directory on the host machine; the latter is always a subdirectory of the current directory (i.e. the one that contains the docker compose files). This way, the user will have a predictable location for their data (wallet files in particular). Again, this approach assumes that the user will copy the docker compose project to a separate location.
  4. The host system's directory where containers' home directory is mounted must be writable by the containers. It'll be the responsibility of the user to ensure it is. To make it possible, dockerfiles create 'user' and 'group' with the explicit uid/gid of 1000. Chances are that host system's 'user' will have the ids of 1000 as well, so nothing will have to be done in this case. But even if not, at least the user will know which 'user' should be granted access to the mount point.
  5. I also removed the EXPOSE lines from docker files, because they don't do anything except for serving as documentation and IMO they are more misleading than helpful.

Regarding the question whether we should have the ability to run wallet-cli via docker compose.

  1. I don't see why not. It's useful to have.
  2. Running interactive shells via docker compose is possible and people do it (in the context of crypto wallets too).
    There are basically 2 ways to do it - via docker compose run, which starts a new container based on a service definition, and docker compose exec, which runs a command in an already running container.
    The "run" approach was the first I tried. I had to have an additional yml file for the wallet-cli service in this case, which then would have to be run like this:
    docker compose -f docker-compose.wallet-cli.yml -f docker-compose.yml run --rm wallet-cli
    But in the end I decided to put the wallet-cli binary into the same image as node-daemon, so now it's possible to run it via "exec":
    docker compose exec node-daemon wallet-cli
    And the separate docker-compose.wallet-cli.yml is not needed anymore.
    Also, because the separate docker image for wallet-cli has become redundant, I removed it.

@ImplOfAnImpl ImplOfAnImpl marked this pull request as ready for review February 21, 2024 17:13
Base automatically changed from fix/locked-utxo-conflict to master February 22, 2024 15:45
Copy link
Contributor

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

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

Besides the comments, this looks fine. I want Enrico to review it though to see if this is what he expects.

@ImplOfAnImpl
Copy link
Contributor

I've revamped dockerfiles once more. Now they don't use the USER directive; instead, there is the entrypoint.sh script, which is run as root initially and which then creates the user, chown's the mounted mintlayer-data directory and runs the passed executable as the user.
Basically, the main reason for this script to exist is the abovementioned chown call (because there is no other place to do this except as a part of CMD or ENTRYPOINT directive). But it also adds the possibility to specify the user id in the environment instead of hard-coding it in the docker file.

Copy link
Contributor

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

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

Besides the p2p port issue, this is looking OK at this point.

build-tools/docker/build.py Outdated Show resolved Hide resolved
erubboli and others added 5 commits March 11, 2024 20:06
…active) services; put dns-server in a separate project; mount container's home dir to host dir instead of a volume.
… docker compose project can now run wallet-cli via "exec".
…non-root user and chown the mounted host dir.

Reduce code duplication in dockerfiles.
Minor improvements.
improve comments;
use localhost as the bind address for rpc calls and api servers' postgres db;
unify argument passing via env vars for wallet and wallet rpc daemon;
a bit more logging
@ImplOfAnImpl ImplOfAnImpl merged commit 0bb41ae into master Mar 11, 2024
17 checks passed
@ImplOfAnImpl ImplOfAnImpl deleted the update_docker_images branch March 11, 2024 20:27
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.

4 participants