Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

fix: Fixes substrate-node-template docker issues #13039

Conversation

ltfschoen
Copy link
Contributor

@ltfschoen ltfschoen commented Jan 1, 2023

  • Description:
    • Updates to allow use of either docker-compose v1 or docker compose v2. See difference between each here https://stackoverflow.com/questions/66514436/difference-between-docker-compose-and-docker-compose, noting that a comment at that link suggests users may be able to resolve it by installing compose standalone with docker-compose syntax https://docs.docker.com/compose/install/other/ which installs Docker Compose version v2.14.2 when you run docker-compose version, but i don't think that should be necessary, it needs support for latest docker compose v2. Previously only docker-compose was supported, so if you had installed Docker CE on a linux machine it wouldn't have that executable but it would have the docker compose ... executable and the script would panic. Note that I encountered the issue because I had the latest docker compose from installing Docker CE (i didn't have Docker Desktop because I was using a cloud provider that didn't have the required KVM support, see https://docs.docker.com/desktop/install/linux-install/)
    • Updates readme to mention what to do if get error listen tcp4 0.0.0.0:9944: bind: address already in use that port 9944 not available in this PR (for example i was running a moonbase collator on that same port, so i changed the port used by that service, but would have been easier just to modify the port mapping in docker-compose.yml to use a different local port)
    • Previously after you clone the Substrate repository and run ./bin/node-template/scripts/docker_run.sh it would give error error: could not find Cargo.tomlin/var/www/node-template or any parent directory on Ubuntu 22.04, so docker-compose.yml file has been updated in this PR
    • Previously when it started building with cargo build --release it gave warning warning: use of deprecated associated function chrono::Local::today: use Local::now() instead, so that change has been made in this PR
    • Added bind mount for Git to fix warning when running cargo build --release where warning is warning: Git command failed with status: exit status: 128; warning: Could not find .git/HEAD searching from '/var/www/node-template/node' upwards!
    • Using printf instead of echo so newline characters work (alternative is echo -e)
    • Uses multiline yaml format for docker-compose.yml command so not too many chars per line
    • What important points should reviewers know?
      • In docker-compose.yml I have used bind mounts instead of volumes. If I used volumes instead then I would have added the following to the nested volumes in docker-compose.yml instead of using type: bind for each instead.
        - ../../.git:/var/www/node-template/.git
        - ../../frame:/var/frame
        - ../../utils:/var/utils
        - ../../client:/var/client
        - ../../primitives:/var/primitives
      
      • Note that if I had used volumes instead of bind mounts then I wouldn't have had to add mkdir -p "$PARENT_DIR/.local" in the docker_run.sh file. If neither approaches are followed then it gives error Error response from daemon: invalid mount config for type "bind": bind source path does not exist: ~/substrate/bin/node-template/.local
    • Is there something left for follow-up PRs?
      • Suggested further change in docker_run.sh file is to change docker compose run --rm --service-ports dev $@ by removing the --rm so it doesn't delete the container, that way if the user stops the container that automatically starts running they don't lose the container and have to create it again, instead they could just run the following to find and enter the container using the last container that was created, and then inside the container optionally run commands
      docker ps -a
      docker compose -f ./bin/node-template/docker-compose.yml up -d
      docker exec -it $(docker ps -n=1 -q) /bin/sh
      
  • Related Issues: You mentioned a related issue if this PR is related to it, e.g. Fixes #228 or Related #1337.
  • 2 Reviewers: You asked at least two reviewers to review. If you aren't sure, start with GH suggestions.
  • Style Guide: Your PR adheres to the style guide
    • In particular, mind the maximal line length of 100 (120 in exceptional circumstances).
    • There is no commented code checked in unless necessary.
    • Any panickers in the runtime have a proof or were removed.
  • N/A [X] Runtime Version: You bumped the runtime version if there are breaking changes in the runtime.
  • N/A [X] Docs: You updated any rustdocs which may need to change.
  • N/A [X] Polkadot Companion: Has the PR altered the external API or interfaces used by Polkadot?
    • N/A [X] If so, do you have the corresponding Polkadot PR ready?
    • N/A [X] Optionally: Do you have a corresponding Cumulus PR?

@bkchr
Copy link
Member

bkchr commented Jan 1, 2023

You committed the lock file. Please remove.

@ltfschoen
Copy link
Contributor Author

ltfschoen commented Jan 1, 2023

You committed the lock file. Please remove.

Thanks! I've removed it now

@ggwpez
Copy link
Member

ggwpez commented Jan 2, 2023

I tried it with the docker-compose but it errored:

$ docker-compose version
docker-compose version 1.25.0, build unknown
docker-py version: 4.1.0
CPython version: 3.9.2
OpenSSL version: OpenSSL 1.1.1n  15 Mar 2022

$ ./scripts/docker_run.sh
*** Start Substrate node template ***
Searching for docker-compose executable...
Detected docker-compose executable.
ERROR: Version in "./docker-compose.yml" is unsupported. You might be seeing this error because you're using the wrong Compose file version. Either specify a supported version (e.g "2.2" or "3.3") and place your service definitions under the `services` key, or omit the `version` key and place your service definitions at the root of the file to use version 1.
For more on the Compose file format versions, see https://docs.docker.com/compose/compose-file/

Would it not be easier to decommission the old compose and just recommend the new one?

@ltfschoen
Copy link
Contributor Author

I tried it with the docker-compose but it errored:

$ docker-compose version
docker-compose version 1.25.0, build unknown
docker-py version: 4.1.0
CPython version: 3.9.2
OpenSSL version: OpenSSL 1.1.1n  15 Mar 2022

$ ./scripts/docker_run.sh
*** Start Substrate node template ***
Searching for docker-compose executable...
Detected docker-compose executable.
ERROR: Version in "./docker-compose.yml" is unsupported. You might be seeing this error because you're using the wrong Compose file version. Either specify a supported version (e.g "2.2" or "3.3") and place your service definitions under the `services` key, or omit the `version` key and place your service definitions at the root of the file to use version 1.
For more on the Compose file format versions, see https://docs.docker.com/compose/compose-file/

Would it not be easier to decommission the old compose and just recommend the new one?

thanks for your feedback. that appears to be because i increased the version in docker-compose.yml from 3.2 to 3.8 without realising its impact https://github.com/paritytech/substrate/pull/13039/files#diff-5e9a8fb875be970e9f3ad6ffe27a734686cd8540a1dbd7a0079cee876e833bbdL1, since i needed a newer version when i was using ONBUILD but i'm not using that in the Dockerfile anymore. i'll try and revert back to 3.2 to see if that works with the changes or find a compromise

@ltfschoen
Copy link
Contributor Author

ltfschoen commented Jan 3, 2023

@ggwpez ggwpez i've got Docker CE installed on Ubuntu 20.04, by following these instructions https://www.vultr.com/docs/install-docker-ce-on-ubuntu-18-04/, which allows me to run docker compose version since it included the Compose plugin from https://docs.docker.com/compose/install/linux/

$ docker version
Client: Docker Engine - Community
 Version:           20.10.22
 API version:       1.41
 Go version:        go1.18.9
 Git commit:        3a2c30b
 Built:             Thu Dec 15 22:28:04 2022
 OS/Arch:           linux/amd64
 Context:           default
 Experimental:      true

Server: Docker Engine - Community
 Engine:
  Version:          20.10.22
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.18.9
  Git commit:       42c8b31
  Built:            Thu Dec 15 22:25:49 2022
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.6.14
  GitCommit:        9ba4b250366a5ddde94bb7c9d1def331423aa323
 runc:
  Version:          1.1.4
  GitCommit:        v1.1.4-0-g5fd4c4d
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0

It includes docker compose

$ docker --help
...
Management Commands:
  app*        Docker App (Docker Inc., v0.9.1-beta3)
  ...
  buildx*     Docker Buildx (Docker Inc., v0.9.1-docker)
  compose*    Docker Compose (Docker Inc., v2.14.1)
  ...
  scan*       Docker Scan (Docker Inc., v0.23.0)
  ...

Then if I ran the script it detected docker compose was available and used it successfully:

$ ./scripts/docker_run.sh 
*** Start Substrate node template ***
Searching for docker-compose executable...
Detected docker executable.
Detected docker compose subcommand.
...

But to be able to run docker-compose version (with dash syntax) it was also necessary to install the Compose standalone https://docs.docker.com/compose/install/other/

curl -SL https://github.com/docker/compose/releases/download/v2.14.2/docker-compose-linux-x86_64 -o /usr/local/bin/docker-compose
sudo ln -s /usr/local/bin/docker-compose /usr/bin/docker-compose
chmod +x /usr/local/bin/docker-compose
docker-compose version
# Docker Compose version v2.14.2

Then if I ran the script it detected docker-compose was available and used it successfully:

$ ./scripts/docker_run.sh 
*** Start Substrate node template ***
Searching for docker-compose executable...
Detected docker-compose executable.
...

Then I reverted to installing the older docker-compose v1.25.0 from https://github.com/docker/compose/releases/tag/1.25.0 and I got the same error as you. It was because the old Compose file format v1 didn't even support using the "version" field at all, as mentioned here https://docker-docs.netlify.app/compose/compose-file/compose-versioning/#version-1.
But the Docker Engine version depends on the output of docker version, and I think we'd want them to have the latest version, so we'd want to use the latest Compose file format, and in the latest version the "version" field is deprecated https://github.com/compose-spec/compose-spec/blob/master/spec.md#compose-file, so I think we should just remove it entirely.

Then if I removed that field running it gave error ERROR: The Compose file './docker-compose.yml' is invalid because: Unsupported config option for services: 'dev', so I removed the services: line too, and then it gave error ERROR: The Compose file './docker-compose.yml' is invalid because: dev.volumes contains an invalid type, it should be a string because it appears Compose file format v1 didn't support bind mount syntax like - type: bind.
So I tried conditionally running a ./docker-compose-legacy.yml file specifically reformatted for that syntax.
Then I tried downloading docker compose v2.0.0 and found that it would only run with docker-compose compose version but not with docker-compose version, and I found that version v2.14.2 supported both, so I'll revert to using docker-compose compose version to detect that version.
I also tried v1.0.0 where the docker-compose --verbose --version outputs fig 1.0.0 but that version didn't even support container_name: node-template in the Dockerfile and doesn't support down option so too much mucking around, so I don't think we should support that far back. So even though v1.25.0 supports both docker-compose --version and docker-compose version commands, i'm only going to use the later, and assume anything that uses docker-compose --version isn't supported.

i have tested it with docker-compose syntax as follows, and it uses docker compose otherwise (which i think should have been installed first anyway, but at least users can use either):

# 2.14.2
curl -SL https://github.com/docker/compose/releases/download/v2.14.2/docker-compose-linux-x86_64 -o /usr/local/bin/docker-compose \
    && sudo ln -s /usr/local/bin/docker-compose /usr/bin/docker-compose \
    && chmod +x /usr/local/bin/docker-compose \
    && docker-compose version \
    && ./scripts/docker_run.sh

-> Detected legacy docker-compose version 2.x. Using Compose File Format 2+.

# 2.0.0
curl -L https://github.com/docker/compose/releases/download/v2.0.0/docker-compose-linux-amd64 -o /usr/local/bin/docker-compose \
    && docker-compose compose version \
    && ./scripts/docker_run.sh

-> Detected legacy docker-compose version 2.x. Using Compose File Format 2+.

# 1.25.0
curl -L https://github.com/docker/compose/releases/download/1.25.0/docker-compose-Linux-x86_64 -o /usr/local/bin/docker-compose \
    && docker-compose version \
    && ./scripts/docker_run.sh

-> Detected legacy docker-compose version 1.x. Using Compose File Format 1.

# 1.0.0
curl -L https://github.com/docker/compose/releases/download/1.0.0/fig-Linux-x86_64 -o /usr/local/bin/docker-compose \
    && docker-compose --verbose --version \
    && ./scripts/docker_run.sh

-> Unknown or unsupported version of docker-compose. Skipping...
    Detected docker executable.
    Detected docker compose subcommand.

@bkchr
Copy link
Member

bkchr commented Jan 3, 2023

If the legacy format supports everything, why not just stick to this? Why do we need two files which are doing the same?

@ltfschoen
Copy link
Contributor Author

https://docker-docs.netlify.app/compose/compose-file/compose-versioning/#version-1

great idea! some things come to mind:

  • yes, i think we could use just the legacy format shown in docker-compose-legacy.yml (aka FILE_A) for the development template, although if the user is using version 1.0.0 (unlikely), then the script won't use docker-compose for various reasons (it doesn't appear to support various commands), so instead it will use docker compose (which requires services: to be at the top of the file otherwise it gives error (root) Additional property dev is not allowed, so if we only keep FILE_A, we could just inject that line to the new file temporarily only if its being called by docker compose. also if the user is using version 2.0.0 or 2.14.2, it will use docker-compose (which also requires services: so we could inject it in that case too.
    so we only don't need services: at the top if we're using 1.25.0, because otherwise it gives error ERROR: The Compose file './docker-compose.yml' is invalid because: Unsupported config option for services: 'dev'

  • based on reading this https://docs.docker.com/storage/, it seems i'm actually using bind mounts for both the docker-compose.yml and docker-compose-legacy.yml files, although the docker-compose-legacy.yml is entirely using the short-form syntax. although volumes are likely preferred in production, this is for development purposes.

i've pushed a commit that now just uses a single docker-compose.yml file, and in the script i've used a sed command to conditionally comment out or uncomment the first line services: of that file depending on whether or not that line is supported by the version of docker-compose or docker compose being used.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Worked now with old and new version that I tried.

The bash code is a bit 🙈 but If you think its worth it.

bin/node-template/scripts/docker_run.sh Show resolved Hide resolved
SCRIPT_DIR=$(realpath "$(dirname "${BASH_SOURCE[0]}")")
PARENT_DIR=$(dirname "$SCRIPT_DIR")
mkdir -p "$PARENT_DIR/.local"
mkdir -p "$PARENT_DIR/.cargo/registry"
Copy link
Member

Choose a reason for hiding this comment

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

Could you maybe put them into the target/ dir, so I wont forget deleting them eventually?
Also do you know if there is a way to not make docker chown these folders with root?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cheers, i'm working on fixing all that, i'll push some commits and tell you when its done

Copy link
Contributor Author

@ltfschoen ltfschoen Jan 4, 2023

Choose a reason for hiding this comment

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

i've pushed changes that address your comments.

in the changes when i added a workspace bin/node-template/Cargo.toml file instead of only building the bin/node-template/node directory that fixed the location of the /target directory, and adding to docker-compose.yml CARGO_HOME=/var/www/node-template/target/.cargo allows .cargo files to be kept in the /target dir.
in the entrypoint.sh script i use usermod to load the existing non-root user called nonroot that has already been added to the prebuilt docker image 'paritytech/ci-linux:production', then i use chown -R nonroot:nonroot ... to change ownership of the relevant folders like the chain directory and other hidden folders that get used like:
/var/www/node-template/target/debug/.cargo-lock
/var/www/node-template/target/.cargo/registry/index/github.mirror.nvdadr.com-x/.git/config.lock
/var/www/node-template/target/debug/build/anyhow-x/invoked.timestamp

i also create a symlink from the chain data folder to /data, so we can have a single static dev chain folder, which you can specify with --base-path=/data, which makes it easier to access to prune since otherwise the directory might change each time and clutters up storage.

i also run arguments that the user passes into the script with su -c "$args" nonroot, where args=$@.

@ltfschoen
Copy link
Contributor Author

ltfschoen commented Jan 6, 2023

an alternative to the ugly docker-compose.yml hack where add or remove the services: line using the sed command depending on what version of docker compose they have installed could be to not use the docker-compose.yml file at all and just use the cli with docker build ... && docker run ... instead of docker compose ... or docker-compose ... as shown in the example here tangle-network/dapp#848 (comment).
or perhaps like @ggwpez earlier suggested, only support recent versions of docker so we don't have to do the hack at all.

i even got caught out recently when i switched to a different cloud server and installed docker on the new machine following this guide https://www.vultr.com/docs/install-docker-ce-on-ubuntu-18-04/, but then i got error: ***unknown shorthand flag: 'f' in -f. and when i ran docker compose --version it output Docker version 20.10.22, build 3a2c30b but docker compose version that has that flag didn't work. but only a previous machine where that flag was working but i forgot how i installed docker i was able to run docker compose version and it output Docker Compose version v2.14.1.
the issue was that those instructions skipped the step of installing the docker-compose-plugin as mentioned here https://docs.docker.com/engine/install/ubuntu/ with apt-get install -y docker-compose-plugin or apt-get install docker-compose

another unfortunate thing about using the legacy Compose file format it doesn't support top-level volume:
so if i wanted to replicate VOLUME ["/data"] from here, where you create a volume with docker volume create data, it'll give an error if the user is using the older version docker-compose 1.25.0 ERROR: yaml.parser.ParserError: expected '<document start>', but found '<block mapping start>' in "./docker-compose.yml" because it doesn't support named volumes volumes:, as they mention here https://docker-docs.netlify.app/compose/compose-file/compose-versioning/#version-1-to-2x, so you can't do this:

    volumes:
      ...
      - data:/data
    ...
volumes:
  data:
    external: true

also the reason why the node template dockerfile is proposed to located in the parent Substrate folder here ./docker/substrate_node_template_builder.Dockerfile with the substrate dockerfile is because inside that dockerfile itself i needed to COPY the ./client, ./frame, /primitives, and ./utils folders from the top-level Substrate directory into the container as follows:

COPY ./client /client
COPY ./frame /frame
COPY ./primitives /primitives
COPY ./utils /utils

and that required the ./bin/node-template/docker-compose.yml file to specify the context to be context: "../../".

but if i had instead located that dockerfile in ./bin/node-template/docker/substrate_node_template_builder.Dockerfile, then it wouldn't work if the context was context: "." because you can't refer to parent directories in the host like this COPY ../../client /client in the dockerfile, otherwise you get an error like COPY failed: file not found in build context .... the ./bin/node-template/docker-compose.yml file could however be moved into the ./docker/docker-compose.yml.

i also tried combining both the node-template and substrate dockerfiles together, using an ONBUILD approach where you pass in ARG BUILD_ENV=${BUILD_ENV} where BUILD_ENV would be "node_template" or "" and then conditionally run either FROM docker.io/paritytech/ci-linux:production as build_ or FROM docker.io/paritytech/ci-linux:production as build_node_template and then later continue with say FROM build_node_template but it wasn't working and it would look horrible anyway.

@ggwpez ggwpez mentioned this pull request Jan 16, 2023
@ltfschoen
Copy link
Contributor Author

@ggwpez so should we get rid of support for old docker versions so we can get rid of the ugly bash code?

@ggwpez
Copy link
Member

ggwpez commented Feb 13, 2023

@ggwpez so should we get rid of support for old docker versions so we can get rid of the ugly bash code?

Yea, just require the new version. Don't see a problem with that.

@paritytech-processbot paritytech-processbot bot deleted the branch paritytech:ds/template-readme February 21, 2023 22:24
@ltfschoen
Copy link
Contributor Author

happy to push the changes but i was busy with a hackathon.
is the bot closing the PR an indication that this PR isn't going to get approved even if changes are provided? don't really want to waste time contributing more if it's just going to get closed as stale...
i noticed another simpler docker related issue is about to be closed too #13072.
possibly a sign that the docker templates are going to get deprecated anyway?

@nuke-web3
Copy link
Contributor

@ltfschoen was this correctly closed or still a wip?

@ltfschoen
Copy link
Contributor Author

ltfschoen commented Feb 21, 2023

@ltfschoen was this correctly closed or still a wip?

i think the PR would help the community use docker. sorry i was doing the polkadot europe hackathon so haven't had time the past couple of weeks.
@ggwpez suggested removing support for old docker versions, since that'd mean the PR wouldn't introduce the ugly docker-compose.yml hack (can't figure out a less ugly way to do it), and the bin/node-template/scripts/docker_run.sh file wouldn't be as complex (checking different things to work out what old version they might have so it works for them) as it'd be as simple as either working seamlessly or notifying the user to just update to the latest docker version first. but i haven't pushed any changes to remove support for old docker versions yet.
i'm not sure if @bkchr agrees with removing old docker version support though? if @bkchr agrees we should remove old docker version support then i'll make and push the changes.

ltfschoen added a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
ltfschoen added a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
ltfschoen added a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
ltfschoen added a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
@ltfschoen
Copy link
Contributor Author

ltfschoen commented Feb 22, 2023

@nukemandan actually now that i think about it you were correct in closing this PR.
i'll create a new PR to update the Substrate Docker instructions, since we'll remove the Substrate Node Template Docker instructions and redirect users to just use the Substrate Docker instructions to play with the Substrate Node Template.
The Substrate Docker instructions use Docker rather than Docker Compose.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants