Skip to content

docker: General purpose docker container.#4816

Merged
onetechnical merged 1 commit intoalgorand:masterfrom
winder:will/docker
Dec 8, 2022
Merged

docker: General purpose docker container.#4816
onetechnical merged 1 commit intoalgorand:masterfrom
winder:will/docker

Conversation

@winder
Copy link
Copy Markdown
Contributor

@winder winder commented Nov 18, 2022

Summary

Port of the docker container here: https://github.com/winder/docker

Some of the decisions made in this container might not make sense with the Dockerfile bundled with the source. The most glaring is that the container downloads source from S3 (if the CHANNEL build-arg is used), or from GitHub (if the URL/BRANCH/SHA build-args are used).

Background

This container incorporates features and suggestions from many places, the biggest influences are:

Resources

Test Plan

From the project root, build:

docker build \
    -t algod_test
    --build-arg CHANNEL=stable \
    --no-cache \
    .

Then run:

# connect to mainnet
docker run --rm -it \
    -p 4190:8080 \
    -e NETWORK=mainnet \
    -e TOKEN=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
    algod_test

# connect to mainnet and use fast catchup
docker run --rm -it \
    -p 4190:8080 \
    -e NETWORK=mainnet \
    -e FAST_CATCHUP=1 \
    -e TOKEN=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
    algod_test

# start a private network
docker run --rm -it \
    -p 4190:8080 \
    -e TOKEN=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
    algod_test

Comment thread Dockerfile Outdated
Comment on lines 21 to 24
Copy link
Copy Markdown
Contributor Author

@winder winder Nov 18, 2022

Choose a reason for hiding this comment

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

This is the main change compared to the winder/docker repo. The files were moved from the project root to the ./docker/files directory, and files which already exist in this repo (genesis files, update.sh, config.json) are used directly instead of copied.

@winder winder marked this pull request as ready for review November 18, 2022 21:51
Comment thread docker/files/run/run.sh
mainnet) ID="<network>.algorand.network";;
testnet) ID="<network>.algorand.network";;
betanet) ID="<network>.algodev.network";;
alphanet) ID="<network>.algodev.network";;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I also added alphanet support.

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 18, 2022

Codecov Report

Merging #4816 (523c6f3) into master (8c7fb86) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4816      +/-   ##
==========================================
- Coverage   54.66%   54.63%   -0.03%     
==========================================
  Files         417      417              
  Lines       53735    53735              
==========================================
- Hits        29373    29357      -16     
- Misses      21930    21942      +12     
- Partials     2432     2436       +4     
Impacted Files Coverage Δ
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
ledger/blockqueue.go 85.63% <0.00%> (-2.88%) ⬇️
ledger/tracker.go 77.87% <0.00%> (-1.71%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
data/transactions/verify/txn.go 75.11% <0.00%> (-0.93%) ⬇️
cmd/tealdbg/debugger.go 72.69% <0.00%> (-0.81%) ⬇️
catchup/service.go 68.96% <0.00%> (-0.74%) ⬇️
ledger/acctonline.go 78.12% <0.00%> (-0.53%) ⬇️
ledger/accountdb.go 72.35% <0.00%> (-0.16%) ⬇️
network/wsNetwork.go 67.17% <0.00%> (+0.17%) ⬆️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cce
Copy link
Copy Markdown
Contributor

cce commented Nov 18, 2022

This is for developer use only, right? Otherwise we should ensure people running dockerized mainnet nodes use the official binaries from the DEB or RPM packages we distribute with every release.

@winder
Copy link
Copy Markdown
Contributor Author

winder commented Nov 18, 2022

This is for developer use only, right? Otherwise we should ensure people running dockerized mainnet nodes use the official binaries from the DEB or RPM packages we distribute with every release.

It's designed as a general purpose container.

@joe-p
Copy link
Copy Markdown
Contributor

joe-p commented Nov 21, 2022

One of the things that I think would be beneficial is having is having private network creation being a part of the build process and not runtime. As discussed on Slack, there are pros and cons of this approach.

Pros

  1. End-users don't have to wait for partkey generation every time they reset the container
  2. For a given release, the environment will be the exact same for every user. This is really helpful in the context of educational content, hackathons, workshops, etc.

Cons

  1. Docker containers generally produce binaries during build with configuration at runtime. Creating the network during build blurs that line
  2. Users wanting a custom private network will need to build their own image

For con 1, I personally view the local network as a "deliverable" that makes sense to include in the build process. It's a key functionality that is essential to the use case of developers, most of which will be using the same exact configuration (see sandbox).

I think con 2 is very reasonable. Improving the experience for most users (devs wanting the standard sandnet configuration) while having more complex configuration require a re-build is a reasonable compromise in my opinion.

Edit: Another option is to keep this image as is, but create a image specifically for private networks that builds from this image, but having one image would be more simple in my opinion.

@joe-p
Copy link
Copy Markdown
Contributor

joe-p commented Nov 21, 2022

What is the rationale for using the update script? Simply building from source would simplify the build-process and allow for proper cache busting. If the reasoning is to use the released binaries, why is it so important to use the binaries? ETH for example does not have reproducible builds and their docker image simply builds from source. Is having official docker images not good enough?

Edit: I realized we could use an API to get the latest channel release for the sake of cache-busting, but the point of simplicity still stands.

Comment thread docker/README.md

Private networks work a little bit differently. They are configured with, potentially, several data directories. The default topology supplied with this container is installed to `/algod/`, and has a single node named `data`. This means the private network has a data directory at `/algod/data`, matching the production configuration.

Because the root directory contains some metadata, if persistence of the private network is required, you should mount the volume `/algod/` instead of `/algod/data`. This will ensure the extra metadata is included when changing images.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what if the recommendation was to mount /algod for both production and private networks? to remove the caveat.

Copy link
Copy Markdown
Contributor

@algolucky algolucky left a comment

Choose a reason for hiding this comment

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

good starting point. we can iterate from here if necessary.

Copy link
Copy Markdown
Contributor

@algobarb algobarb left a comment

Choose a reason for hiding this comment

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

From what my team has validated, I'm good to approve for merge. We plan to use these files as a starting base to add changes on top of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants