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

container: support kmd #4984

Merged
merged 19 commits into from
Feb 1, 2023
Merged

Conversation

algolucky
Copy link
Contributor

@algolucky algolucky commented Jan 9, 2023

merge after #4982

Summary

based on #4972

  • adds support for starting kmd if desired by setting the START_KMD=1 environment variable at runtime.
  • consolidates apply_configuration function into configure_data_dir in the entrypoint script.
  • some whitespace and formatting changes.
  • adds the update-repo-description job to update the DockerHub Repositories description (README). only executes on master.
  • adds a configurable KMD_PORT with a default of 7833.
  • puts kmd_config.json in $ALGORAND_DATA
  • updates certain environment variables to explicitly require the value of 1 compared to just checking for existence.
  • adds DEBUG environment variable to make set -x in the entrypoint conditional. it's too verbose to be the default.

Test Plan

  • ci
  • internal package testing

example test scecnario

docker run -d -it --rm -v algod-betanet-data:/algod/data -e NETWORK=betanet -e FAST_CATCHUP=1 -e START_KMD=1 -p 18080:8080 -p 17833:7833 --name algod-betanet algolucky/algod:container-support-kmd
sleep 5
curl localhost:17833/swagger.json
curl localhost:17833/versions
docker stop algod-betanet

@algolucky algolucky self-assigned this Jan 9, 2023
@algolucky algolucky force-pushed the container/support-kmd branch 3 times, most recently from dfa9f33 to 3cc090f Compare January 9, 2023 16:07
@algolucky algolucky requested review from winder, a team, excalq, algobarb and algojack January 9, 2023 16:29
@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Merging #4984 (e0bbf8a) into master (5c17ad6) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4984      +/-   ##
==========================================
+ Coverage   53.64%   53.68%   +0.03%     
==========================================
  Files         432      432              
  Lines       54068    54068              
==========================================
+ Hits        29003    29024      +21     
+ Misses      22814    22794      -20     
+ Partials     2251     2250       -1     
Impacted Files Coverage Δ
network/wsNetwork.go 64.92% <0.00%> (ø)
ledger/acctupdates.go 69.36% <0.00%> (+0.12%) ⬆️
catchup/service.go 70.04% <0.00%> (+0.24%) ⬆️
ledger/testing/randomAccounts.go 56.88% <0.00%> (+0.61%) ⬆️
ledger/tracker.go 75.10% <0.00%> (+0.84%) ⬆️
network/wsPeer.go 68.32% <0.00%> (+1.89%) ⬆️
ledger/blockqueue.go 88.50% <0.00%> (+4.02%) ⬆️

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

@algolucky algolucky force-pushed the container/support-kmd branch 7 times, most recently from 2978c3c to f6db622 Compare January 10, 2023 14:25
@algolucky algolucky marked this pull request as ready for review January 10, 2023 14:29
@algolucky algolucky marked this pull request as draft January 10, 2023 14:32
@algolucky algolucky force-pushed the container/support-kmd branch from f6db622 to acbf1c8 Compare January 10, 2023 18:50
docker/README.md Outdated Show resolved Hide resolved
docker/files/run/run.sh Outdated Show resolved Hide resolved
docker/files/run/run.sh Outdated Show resolved Hide resolved
@algolucky algolucky force-pushed the container/support-kmd branch 2 times, most recently from 79a0e6d to 72f1646 Compare January 11, 2023 22:50
docker/README.md Outdated Show resolved Hide resolved
docker/files/run/run.sh Outdated Show resolved Hide resolved
@algolucky algolucky force-pushed the container/support-kmd branch from 13a288b to a49b91a Compare January 26, 2023 14:45
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Some initial feedback. I haven't tested yet.

docker/README.md Outdated Show resolved Hide resolved
docker/files/run/run.sh Outdated Show resolved Hide resolved
docker/files/run/run.sh Outdated Show resolved Hide resolved
docker/files/run/run.sh Outdated Show resolved Hide resolved
@winder
Copy link
Contributor

winder commented Jan 26, 2023

Is kmd_config.json.example unused? Looks like it could be deleted.

algolucky and others added 6 commits January 26, 2023 09:18
Co-authored-by: Will Winder <wwinder.unh@gmail.com>
Co-authored-by: Will Winder <wwinder.unh@gmail.com>
Co-authored-by: Will Winder <wwinder.unh@gmail.com>
@winder winder self-requested a review January 26, 2023 16:31
@algolucky algolucky requested a review from excalq January 26, 2023 21:58
algojack
algojack previously approved these changes Jan 27, 2023
Co-authored-by: Will Winder <wwinder.unh@gmail.com>
winder
winder previously approved these changes Jan 27, 2023
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

I'm still seeing that Warning: Error loading config file from '.' error in some cases. We'll have to monitor for that and see if it's only an issue with local builds.

Aside from that, seems to work nicely.

Tested with the following:

docker build \
            -t algod_test \
            --build-arg CHANNEL=nightly \
            --build-arg TARGETARCH=amd64 --no-cache \
            .
docker run --rm -it -p 4190:8080 -e TOKEN=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa -e START_KMD=1 -p 4191:7833 -e KMD_TOKEN=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa algod_test

The service responds:
curl -H "Authorization: bearer aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" "http://localhost:4191/swagger.json"

I see it running in docker top:

UID                 PID                 PPID                C                   STIME               TTY                 TIME                CMD
systemd+            3192916             3192896             0                   14:04               pts/0               00:00:00            bash /node/run/run.sh
systemd+            3193140             3192916             0                   14:04               pts/0               00:00:00            /node/bin/kmd -d /algod/data/kmd-v0.5 -t 0
systemd+            3193162             3192916             30                  14:04               pts/0               00:00:01            /node/bin/algod -d /algod/data
systemd+            3193174             3192916             0                   14:04               pts/0               00:00:00            tail -f /algod/data/node.log

@algolucky
Copy link
Contributor Author

I was getting a permission denied error when starting KMD. so I went ahead and made sure KMD_DIR has mode 0700 in e0bbf8a

@algolucky algolucky requested review from winder and algojack January 27, 2023 21:01
Copy link
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.

It technically looks like this will work and it enables KMD api and port. It seems we are explicitly setting 1 for turning on all the flags as well now.

For some reason it looked a little odd to me when I look at the docker call in the README (I'm still not sure why I feel this way), but it's good that we have consistency and explicitly turning on the flags.

Copy link
Contributor

@jsgranados jsgranados left a comment

Choose a reason for hiding this comment

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

This looks good to me!
Benefits we get here are:
Per commit builds, ability to have KMD start through environmental variable on our algod container, and simplified documentation.
Thanks @algolucky for all of the work here and @algobarb @excalq and @winder for reviewing!

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.

7 participants