Updating rosetta dockerfile #563

wileyj · 2021-04-21T19:13:08Z

Refactoring to use runit vs custom script
clone the latest releases vs static versions

Description

This change refactors the rosetta Dockerfile to use runit as a scheduler vs a customized shell script. This change was motivated by discussions with another team trying to use this Dockerfile, and realizing the static version was very out of date. Further work was added to standardize the Dockerfile, so there weren't things like custom users/scripts (particularly in the case of postgresql).

The builder images are also refactored to more closely resemble the default Dockerfiles, as well as cloning the latest release for each of stacks-blockchain-api and stacks-blockchain repos.

Other changes are more use of ARG and ENV variables to create the scripts, i.e.

printf '#!/bin/sh
exec svlogd -tt %s/postgresql' ${STACKS_LOG_DIR} > ${STACKS_SVC_DIR}/postgresql/log/run

Type of Change

New feature
Bug fix
API reference/documentation update
Other

- Refactoring to use runit vs custom script - clone the latest releases vs static versions

codecov · 2021-04-21T19:13:16Z

Codecov Report

Merging #563 (41f0ffe) into master (2c16f70) will increase coverage by 1.75%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
+ Coverage   65.14%   66.89%   +1.75%     
==========================================
  Files          59       76      +17     
  Lines        4963     8407    +3444     
  Branches      856     1494     +638     
==========================================
+ Hits         3233     5624    +2391     
- Misses       1729     2781    +1052     
- Partials        1        2       +1

Impacted Files	Coverage Δ
src/api/routes/rosetta/construction.ts	`77.27% <83.33%> (+2.00%)`	⬆️
src/api/rosetta-constants.ts	`94.87% <100.00%> (-1.36%)`	⬇️
src/api/routes/debug.ts	`23.25% <100.00%> (ø)`
src/api/routes/faucets.ts	`38.62% <100.00%> (ø)`
src/core-rpc/client.ts	`75.34% <100.00%> (+1.05%)`	⬆️
src/rosetta-helpers.ts	`66.66% <100.00%> (-5.75%)`	⬇️
src/api/routes/rosetta/account.ts	`58.51% <0.00%> (-10.73%)`	⬇️
src/datastore/common.ts	`76.34% <0.00%> (-1.44%)`	⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e625623...41f0ffe. Read the comment docs.

wileyj · 2021-04-23T15:56:56Z

ping @zone117x

CharlieC3 · 2021-04-23T21:01:50Z

stx-rosetta.Dockerfile

-
+ENV MAINNET_STACKS_CHAIN_ID=0x00000001
+ENV TESTNET_STACKS_CHAIN_ID=0x80000000
+ENV V2_POX_MIN_AMOUNT_USTX=90000000260


I think this is handled automatically now. @zone117x can you confirm?

Yes, this env var is no longer used anywhere.

CharlieC3 · 2021-04-23T21:04:20Z

stx-rosetta.Dockerfile

+## Build the stacks-blockchain-api
+FROM node:lts-buster as stacks-blockchain-api-build
+ARG STACKS_API_REPO=blockstack/stacks-blockchain-api
+ENV STACKS_API_REPO=${STACKS_API_REPO}


I think this ENV var can be removed. ${STACKS_API_REPO} will just use the ARG like it's an ENV var. The ENV var directive should be used when the env variable needs to be available in the resulting image.

unless the repo name changes, in which case this is a single change or a build-arg vs multiple changes in the file

I'm saying you can delete just this ENV var and the image will work the same as it did before. There's no benefit to declaring an ARG then setting an ENV var to it if the env var won't be used in the final image because it's redundant. If the repo name changes, you'd update the ARG default value.

CharlieC3 · 2021-04-23T21:08:08Z

stx-rosetta.Dockerfile

+ENV PG_DATA=${PG_DATA}
+ENV PG_VERSION=${PG_VERSION}
+ENV STACKS_SVC_DIR=${STACKS_SVC_DIR}
+ENV STACKS_BLOCKCHAIN_DIR=${STACKS_BLOCKCHAIN_DIR}
+ENV STACKS_BLOCKCHAIN_API_DIR=${STACKS_BLOCKCHAIN_API_DIR}
+ENV STACKS_NETWORK=${STACKS_NETWORK}
+ENV STACKS_LOG_DIR=${STACKS_LOG_DIR}
 ENV STACKS_CORE_EVENT_PORT=3700
 ENV STACKS_CORE_EVENT_HOST=127.0.0.1
-ENV STACKS_NETWORK=$STACKS_NETWORK
-
 ENV STACKS_EVENT_OBSERVER=127.0.0.1:3700
-
 ENV STACKS_BLOCKCHAIN_API_PORT=3999
 ENV STACKS_BLOCKCHAIN_API_HOST=0.0.0.0
-
 ENV STACKS_CORE_RPC_HOST=127.0.0.1
 ENV STACKS_CORE_RPC_PORT=20443
-
-### Startup script & coordinator
-RUN printf '#!/bin/bash\n\
-trap "exit" INT TERM\n\
-trap "kill 0" EXIT\n\
-echo Your container args are: "$@"\n\
-tail --retry -F stacks-api.log stacks-node.log 2>&1 &\n\
-while true\n\
-do\n\
-  pg_start\n\
-  stacks_api &> stacks-api.log &\n\
-  stacks_api_pid=$!\n\
-  if [ $STACKS_NETWORK = "mocknet" -o $STACKS_NETWORK = "dev" ]; then\n\
-    stacks-node start --config=/data/stacky/Stacks-${STACKS_NETWORK}.toml &> stacks-node.log &\n\
-  elif [ $STACKS_NETWORK = "testnet"]; then \n\
-    stacks-node start --config=/data/stacky/Stacks-mocknet.toml &> stacks-node.log &\n\
-  else\n\
-    stacks-node mainnet &> stacks-node.log &\n\
-  fi\n\
-  stacks_node_pid=$!\n\
-  wait $stacks_node_pid\n\
-  echo "node exit, restarting..."\n\
-  rkill -9 $stacks_api_pid\n\
-  pg_stop\n\
-  rm -rf $PGDATA\n\
-  sleep 5\n\
-done\n\
-' >> run.sh && chmod +x run.sh
-
+ENV MAINNET_STACKS_CHAIN_ID=0x00000001
+ENV TESTNET_STACKS_CHAIN_ID=0x80000000
+ENV V2_POX_MIN_AMOUNT_USTX=90000000260


Can we combine all of these ENV directives into one directive? It'll result in fewer layers in the image and decrease its size. Also if any of them aren't used in the final image's runtime, you can just use their ARG counterpart due to the same reason in my other comment.

Yes, but it would also be incredibly hard to read. the goal here isn't a small image

That's fine if you want to keep it that way, I don't find it difficult to read with one directive. Also it seems ENV directives don't create additional layers anymore either which is nice.

CharlieC3 · 2021-04-23T21:14:55Z

stx-rosetta.Dockerfile

+
+###################################
+##  runit service files
+RUN printf '#!/bin/sh\nexec 2>&1\n[ ! -d %s ] && mkdir -p %s && chown -R postgres:postgres %s && gosu postgres /usr/lib/postgresql/%s/bin/pg_ctl init -D %s\nexec gosu postgres /usr/lib/postgresql/%s/bin/postmaster -D %s' ${PG_DATA} ${PG_DATA} ${PG_DATA} ${PG_VERSION} ${PG_DATA} ${PG_VERSION} ${PG_DATA} > ${STACKS_SVC_DIR}/postgresql/run \


Have you considered committing the ruinit service files to this repo, and COPYing them into the image? Might be easier to maintain and read that way.

Possible, sure - but changes to the repo would require a full rebuild since the cache layers would no longer be used.
Since there are many parts of this process, the goal was to create an image that can be built using cache layers as quickly as is feasible.

but changes to the repo would require a full rebuild since the cache layers would no longer be used.

If you COPY only those ruinit files, changes to the repo wouldn't require a full rebuild. That only happens when you write COPY . . for example. Either way will work the same, it's just difficult to read or maintain the scripts when they're embedded.

CharlieC3 · 2021-04-23T21:16:14Z

stx-rosetta.Dockerfile

+ENV MAINNET_STACKS_CHAIN_ID=0x00000001
+ENV TESTNET_STACKS_CHAIN_ID=0x80000000
+ENV V2_POX_MIN_AMOUNT_USTX=90000000260
+RUN apt-get update \


Combining this RUN command with the following three will further decrease the number of layers and size of the resulting image.

Same as above - any change in the process/dockerfile would invalidate the cache layers and cause a longer rebuild.
Can they all be combined? yes - iniitially that's what i was doing. but any small change required a full rebuild vs updating a single layer or two.

any change in the process/dockerfile would invalidate the cache layers and cause a longer rebuild.

Only true if you COPY the dockerfile into the image. For RUN directives the docker engine only compares the text of the old RUN vs the text of the new RUN to determine if it can be reused from cache. So if there's a change in the command, it won't use the cache either way you configure it. These are only minor suggestions, so feel free to do what you like.

wileyj · 2021-04-27T13:39:00Z

ping @zone117x

zone117x · 2021-04-27T14:09:14Z

Thanks @wileyj this looks good to me. Do you think we should merge this to master now, or wait until we merge API develop branch to master?

wileyj · 2021-04-27T14:37:03Z

Thanks @wileyj this looks good to me. Do you think we should merge this to master now, or wait until we merge API develop branch to master?

Up to you! I've shared the file from my branch with the folks who needed it, so there's no rush - pinging just to get it off my plate.

zone117x · 2021-04-27T14:54:51Z

Up to you! I've shared the file from my branch with the folks who needed it, so there's no rush - pinging just to get it off my plate.

Alright, there could be some changes required once develop branch is ready, and that's expected this week. So I think holding off until then makes sense.

wileyj · 2021-04-27T15:00:54Z

Up to you! I've shared the file from my branch with the folks who needed it, so there's no rush - pinging just to get it off my plate.

Alright, there could be some changes required once develop branch is ready, and that's expected this week. So I think holding off until then makes sense.

👍 changed the base branch to develop as well.

wileyj · 2021-05-06T15:56:23Z

ping @zone117x

zone117x · 2021-05-06T16:02:39Z

@wileyj the only concern I have about this PR is the move away from pinned versions (of the stacks-node repo at least).
See this discussion for more context on versioning issues: #567

I do think this dockerfile change is a step in the right direction, because the hardcoded versions tend to go stale.

Should we merge this PR with the possibility that this dockerfile could build images with stacks-api and stacks-node versions that are incompatible with each other (when the release cadences don't match up)?

wileyj · 2021-05-06T20:31:23Z

#567

Interesting...I don't have any opinion directly on that discussion, but also - nothing prevents us from setting specific versions in the Dockerfile vs trying to dynamically retrieve the version through github api.

The reason I made the Dockerfile this way was to prevent having to manually update the version numbers each time a
new release is cut for either repo (stacks-blockchain or stacks-blockchain-api). If that's not preferred, I can roll back that idea to hardcode the versions.

Based on the discussion you linked, is there a pair of versions you'd like to start with initially?
i.e.
https://github.com/blockstack/stacks-blockchain-api/releases/tag/v0.58.0
https://github.com/blockstack/stacks-blockchain/releases/tag/2.0.11.0.0

Once I have the specific versions I can update the Dockerfile and we can get this off of our plate.
The caveat being we'll need to ensure we keep the file up to date with current versions.

zone117x · 2021-05-07T16:34:38Z

Based on the discussion you linked, is there a pair of versions you'd like to start with initially?
i.e.
https://github.com/blockstack/stacks-blockchain-api/releases/tag/v0.58.0
https://github.com/blockstack/stacks-blockchain/releases/tag/2.0.11.0.0

Yes these would be good versions to set the dockerfile to for now 👍

The caveat being we'll need to ensure we keep the file up to date with current versions.

Yeah, this is definitely a pain, hopefully something we can figure out in #567 eventually.

hardcoding versions little bit of cleanup

wileyj · 2021-05-08T00:37:02Z

Based on the discussion you linked, is there a pair of versions you'd like to start with initially?
i.e.
https://github.com/blockstack/stacks-blockchain-api/releases/tag/v0.58.0
https://github.com/blockstack/stacks-blockchain/releases/tag/2.0.11.0.0

Yes these would be good versions to set the dockerfile to for now 👍

The caveat being we'll need to ensure we keep the file up to date with current versions.

Yeah, this is definitely a pain, hopefully something we can figure out in #567 eventually.

Updated to use hardcoded versions!

blockstack-devops · 2021-05-10T10:47:11Z

🎉 This PR is included in version 0.59.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Updating rosetta dockerfile

0189ee0

- Refactoring to use runit vs custom script - clone the latest releases vs static versions

wileyj requested a review from zone117x April 21, 2021 19:13

CharlieC3 reviewed Apr 23, 2021

View reviewed changes

wileyj changed the base branch from master to develop April 27, 2021 14:59

Base automatically changed from develop to master April 29, 2021 09:30

Hardcoding versions

41f0ffe

hardcoding versions little bit of cleanup

zone117x approved these changes May 10, 2021

View reviewed changes

zone117x merged commit 9039c20 into master May 10, 2021

zone117x deleted the feature/cleanup-rosetta-dockerfile branch May 10, 2021 10:40

blockstack-devops added the released label May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating rosetta dockerfile #563

Updating rosetta dockerfile #563

wileyj commented Apr 21, 2021

codecov bot commented Apr 21, 2021 •

edited

Loading

wileyj commented Apr 23, 2021

CharlieC3 Apr 23, 2021

zone117x Apr 27, 2021

CharlieC3 Apr 23, 2021

wileyj Apr 23, 2021

CharlieC3 Apr 23, 2021

CharlieC3 Apr 23, 2021

wileyj Apr 23, 2021

CharlieC3 Apr 23, 2021

CharlieC3 Apr 23, 2021

wileyj Apr 23, 2021

CharlieC3 Apr 23, 2021

CharlieC3 Apr 23, 2021

wileyj Apr 23, 2021

CharlieC3 Apr 23, 2021 •

edited

Loading

wileyj commented Apr 27, 2021

zone117x commented Apr 27, 2021

wileyj commented Apr 27, 2021

zone117x commented Apr 27, 2021

wileyj commented Apr 27, 2021

wileyj commented May 6, 2021

zone117x commented May 6, 2021

wileyj commented May 6, 2021

zone117x commented May 7, 2021

wileyj commented May 8, 2021

blockstack-devops commented May 10, 2021

Updating rosetta dockerfile #563

Updating rosetta dockerfile #563

Conversation

wileyj commented Apr 21, 2021

Description

Type of Change

codecov bot commented Apr 21, 2021 • edited Loading

Codecov Report

wileyj commented Apr 23, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlieC3 Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

wileyj commented Apr 27, 2021

zone117x commented Apr 27, 2021

wileyj commented Apr 27, 2021

zone117x commented Apr 27, 2021

wileyj commented Apr 27, 2021

wileyj commented May 6, 2021

zone117x commented May 6, 2021

wileyj commented May 6, 2021

zone117x commented May 7, 2021

wileyj commented May 8, 2021

blockstack-devops commented May 10, 2021

codecov bot commented Apr 21, 2021 •

edited

Loading

CharlieC3 Apr 23, 2021 •

edited

Loading