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

Better Docker Files #372

Closed
18 of 20 tasks
wilwade opened this issue Aug 22, 2022 · 29 comments · Fixed by #408, #417, #433 or #439
Closed
18 of 20 tasks

Better Docker Files #372

wilwade opened this issue Aug 22, 2022 · 29 comments · Fixed by #408, #417, #433 or #439
Assignees

Comments

@wilwade
Copy link
Collaborator

wilwade commented Aug 22, 2022

We need docker file(s) that are able to do the following:

  • Instant sealing
    • frequency-chain/instant-node
    • Runs frequency in instant sealing mode for test containers or other testing situations
  • Node:
    • frequency-chain/node or if we need two frequency-chain/node and frequency-chain/rococo-node
    • Runs a frequency mainnet/rococo node
    • this is close to what we have now

Optimizations to do:

  • Smaller
    • Rather have multiple efficient docker images over one that can do it all
  • Readme on Docker Hub
    • we have a new docker hub to setup for the chain: frequency-chain
    • I or Scott can switch over the GitHub Actions Docker Token env var when ready
  • Better documentation for configuration and how to use it

Tasks:

References:




@wilwade wilwade added this to the Launch Tech Debt milestone Aug 22, 2022
@demisx
Copy link
Collaborator

demisx commented Aug 22, 2022

A couple of things to add:

@demisx demisx self-assigned this Aug 30, 2022
@demisx demisx moved this to 🧊 Icebox in DSNP and Frequency Project Aug 31, 2022
@demisx demisx moved this from 🧊 Icebox to 🪵 Backlog in DSNP and Frequency Project Aug 31, 2022
@demisx
Copy link
Collaborator

demisx commented Aug 31, 2022

@wilwade ❓A couple clarification questions:

  • Q1: The title implies multiple docker files, however I only see this one. Just want to make sure we are on the same page and there is only 1 file to optimize.
  • Q2: Is publishing readme (without the image) on Docker Hub in the scope of this issue? As far as I know, we don't have anything on Docker Hub yet, right?

@demisx demisx moved this from 🪵 Backlog to ⚙️ In Progress in DSNP and Frequency Project Aug 31, 2022
@wilwade
Copy link
Collaborator Author

wilwade commented Aug 31, 2022

@demisx

  • Q1: There is just one right now, but we need two or perhaps three. Updated the issue description
  • Q2: We have one, it is just under the dsnp name instead of frequency: https://hub.docker.com/r/dsnp/frequency CI pushes to it currently.

@demisx
Copy link
Collaborator

demisx commented Aug 31, 2022

@wilwade Thank you for you responses. These are the 3 docker images I have identified with my current assumptions. Please let me know if I missed anything.

N Docker image Target Platforms Description
1 instant-seal-node amd64, arm64 Corresponds to the collator node in instant seal mode as shown in the option 1 of the readme.
2 parachain-node amd64, arm64 This is the Frequency parachain collator node as shown in the option 2 of the readme, i.e. needs
3 relay-chain-validator-node amd64, arm64 Corresponds to the relay chain validator node as shown in the option 2 of the readme under "Rococo Local Testnet". I assume the same image will be used to start a Rococo validator node with either Alice or Bob account based on what's passed in the arguments to docker run.

Update 1:
Please review the proposed naming. I tried to keep it consistent with the diagrams in readme, so each name clearly reflects the purpose of each node started by the docker image instead of using some brand term like "Rococo" that many are still confused about.

Update 2:
The naming has been updated in the table below.

@wilwade
Copy link
Collaborator Author

wilwade commented Aug 31, 2022

A few notes:

  1. While it would be nice to have arm64 images, I don't think they are priority. (Not sure they are possible with standard GitHub Actions, but maybe)
  2. We don't need a relay-chain-validator-node as that is just the polkadot image.
  3. parachain-node is not option 2 of the readme. It is for Networks docs#10
  4. If we can do either mainnet or rococo with the same image and just different configuration, the parachain-node setup works. However, I was hoping we could get a faster setup/smaller image for users if we have separate images for each network. (or just name it node as it would be basically the only one people should use. See below)
  5. Naming. Here is how Purestake, Astar, and Acala name theirs. I think we want to find a pattern we like that is expressive but short is best.

@demisx
Copy link
Collaborator

demisx commented Aug 31, 2022

While it would be nice to have arm64 images, I don't think they are priority. (Not sure they are possible with standard GitHub Actions, but maybe)

This should be doable with buildx and should be pretty straightforward. I'll keep in mind that amd64 is the first priority though.

We don't need a relay-chain-validator-node as that is just the polkadot image.

You are right. I forgot about this part.

Here is my "new" understanding 😄 what needs to be built:

N Docker image Target Platforms Description
1 instant-seal-node amd64 Frequency collator node in instant seal mode as shown in the option 1 of the readme.
2 rococo-node amd64 Frequency collator node which connects to the public Rococo testnet network
3 mainnet-node amd64 Frequency collator node which connects to the mainnet network

Notes:

  • Will evaluate the potential of combining rococo-node and node images into 1 and the side-effects, if any
  • I named the 3-rd image node to be consistent with the current naming convention for Frequency, though my preference would be mainnet-node as I think it communicates the purpose more clear. (Updated to mainnet-node)
  • I will build for the amd64 platform first. I'll add arm64 variant if it's a slam dunk in GitHub actions.

@wilwade
Copy link
Collaborator Author

wilwade commented Sep 1, 2022

@demisx Sounds good. I think mainnet-node is fine if we have them split.

Really even if they combine, it might just be easier for tutorials and documentation to have them separate with different defaults.

@demisx
Copy link
Collaborator

demisx commented Sep 1, 2022

Got it. I'll keep this in mind.

@demisx
Copy link
Collaborator

demisx commented Sep 2, 2022

@wilwade Evaluating different approaches and looking into what others are doing. Need to confirm some assumption here which would apply to all images, including the instant seal one:

  • The built images will copy necessary binary from the Frequency release page artifacts
  • Each published image will be tagged with a version tag corresponding to the Frequency release version
  • Developers will continue to build and run instant seal chain locally as shown in the Option 1 of the readme. In order words, we aren’t providing a dev image in this issue that builds the chain from developer locally checked out repo (out of scope).

@wilwade
Copy link
Collaborator Author

wilwade commented Sep 6, 2022

The built images will copy necessary binary from the Frequency release page artifacts

Correct

Each published image will be tagged with a version tag corresponding to the Frequency release version

Correct

...we aren’t providing a dev image in this issue that builds the chain from developer locally checked out repo (out of scope).

Correct. No building rust inside the image or in Docker.

@demisx
Copy link
Collaborator

demisx commented Sep 6, 2022

@wilwade We currently have one test release published on the releases page. I probably need to publish multiple binaries for different platforms for testing here. Shall I modify the existing test release or create a new one?

@wilwade
Copy link
Collaborator Author

wilwade commented Sep 6, 2022

@demisx Create new ones. We'll end up deleting all the old releases likely once we have the final #282 and start doing real releases

@demisx demisx linked a pull request Sep 7, 2022 that will close this issue
@demisx
Copy link
Collaborator

demisx commented Sep 7, 2022

@wilwade This is what I see running new instant-seal-node image locally. Seems to be working to me. Do you see any red flags by any chance? I don't know if there is a better way to test it's working.

Screen Shot 2022-09-07 at 11 01 35 AM

Screen Shot 2022-09-07 at 11 01 53 AM

# Listening locally on all 3 ports
$ netstat -an | grep LISTEN
tcp46      0      0  *.9944                 *.*                    LISTEN     
tcp46      0      0  *.9933                 *.*                    LISTEN     
tcp46      0      0  *.30333                *.*                    LISTEN 

@demisx
Copy link
Collaborator

demisx commented Sep 7, 2022

Sending money from Alice to Bob appears to be working in instant seal image. I saw "extrinsic success" icon, balances change and the new blocks formed. Moving forward.

Screen Shot 2022-09-07 at 11 51 33 AM

Screen Shot 2022-09-07 at 12 01 03 PM

@demisx
Copy link
Collaborator

demisx commented Sep 8, 2022

The first instant-seal-mode image has been published on Docker Hub. I am going to run some additional checks and submit the first PR. I will populate the "Overview" section towards the end once we know exactly which images we are publishing. I am seeing a bunch of similarities, so wondering in the back of my mind if we could actually get away with less than 3 images. We'll see.

Screen Shot 2022-09-08 at 8 33 51 AM

Repository owner moved this from ⚙️ In Progress to ✅ Accepted in DSNP and Frequency Project Sep 8, 2022
@wilwade wilwade moved this from ✅ Accepted to ⚙️ In Progress in DSNP and Frequency Project Sep 8, 2022
@wilwade wilwade reopened this Sep 8, 2022
Repository owner moved this from ⚙️ In Progress to 🪵 Backlog in DSNP and Frequency Project Sep 8, 2022
@demisx
Copy link
Collaborator

demisx commented Sep 9, 2022

@wilwade Thank you for reopening it. I wonder why the PR got hard linked to this issue and closed it. After all, I've used "Part of ..." in the description, not "Closes ...". 🤔 .

@demisx
Copy link
Collaborator

demisx commented Sep 12, 2022

@wilwade Can you please confirm my assumptions for the rococo-node docker file:

  1. Don't need to pass --bootnodes arg when starting frequency
  2. ./res/genesis/testnet/rococo-testnet-frequency-raw.json will be used as the chain spec
  3. Parachain onboarding step will not be part of the docker file
  4. The docker image will start parachain full node container without collator capabilities

@wilwade
Copy link
Collaborator Author

wilwade commented Sep 12, 2022

  1. Shouldn't have to, but should be able to. Two different ones:
    • Frequency Rococo bootnodes
    • Rococo bootnodes.
  2. I think we want to just use the built in spec, so we should not need a chain spec, just to define the chain as frequency-rococo.
    • MAYBE an option to push in a local file with the spec? Not sure how that would work, but it might be nice to be able to run off of a local spec file instead.
  3. Correct.
  4. Correct.

@demisx
Copy link
Collaborator

demisx commented Sep 13, 2022

After talking to @wilwade, we decided to keep 2 images for now:

N Docker image Target Platforms Description
1 instant-seal-node amd64 Frequency collator node in instant seal mode as shown in the see option 1 of the readme.
2 parachain-node amd64 Frequency parachain node which connects either to the public Rococo testnet or Mainnet networks. The latter is default.

@demisx
Copy link
Collaborator

demisx commented Sep 13, 2022

There was one existing collator node docker file that I moved to the new location. So here is the latest (and hopefully final 🤞🏻) list of docker files for this issue:

N Docker image Target Platforms Description
1 instant-seal-node amd64 Frequency collator node in instant seal mode as shown in the see option 1 of the readme.
2 collator-node-local amd64 Frequency collator node node running locally against the local relay chain.
3 parachain-node amd64 Frequency parachain node which connects either to the public Rococo testnet or Mainnet networks. The latter is default.

Please note that the docker file names and the images will be published on Docker Hub under the same names to avoid any confusion. The Directory Structure Wiki has been updated to reflect the new /docker/ directory.

@wilwade wilwade moved this from 🪵 Backlog to ⚙️ In Progress in DSNP and Frequency Project Sep 14, 2022
@demisx
Copy link
Collaborator

demisx commented Sep 14, 2022

Per @wilwade, need to ensure the parachain-node image is compliant with onFinality docker requirements.

@demisx
Copy link
Collaborator

demisx commented Sep 14, 2022

Just noticed during testing that starting local collator node in docker via the old docker file fails on an M1 laptop. Tested on origin/main. Entered bug #432.

@demisx
Copy link
Collaborator

demisx commented Sep 14, 2022

I've verified parachain-node image against onFinality docker requirements. We are already compliant.

@demisx
Copy link
Collaborator

demisx commented Sep 14, 2022

@wilwade Not sure if you saw my message yesterday in Slack yesterday, but starting parachain-node with --chain=frequency currently outputs this error:

Error: Input("Relay chain argument error: Invalid input: Error opening spec file `TODO`: No such file or directory (os error 2)")

Just want to make sure it is expected as WIP.

demisx added a commit that referenced this issue Sep 15, 2022
# Goal
The goal of this PR is create a docker image for running Frequency
parachain full node.

Part of #372
@demisx
Copy link
Collaborator

demisx commented Sep 15, 2022

The Directory Structure Wiki has been updated to reflect the addition of the DockerHub overview markdown files.

Screen Shot 2022-09-14 at 7 31 56 PM

demisx added a commit that referenced this issue Sep 15, 2022
# Goal
The goal of this PR is to populate DockerHub overview pages from
dedicated readme files via GitHub actions.

Part of #372
@demisx
Copy link
Collaborator

demisx commented Sep 15, 2022

Just a heads-up. Strange thing I've noticed while configuring "Frequency Rococo - Dmitri" test network spec on OnFinality.io. Setting up the collator node spec doesn't have --collator in the list of the arguments:

Screen Shot 2022-09-15 at 9 51 03 AM

@wilwade
Copy link
Collaborator Author

wilwade commented Sep 15, 2022

Just a heads-up. Strange thing I've noticed while configuring "Frequency Rococo - Dmitri" test network spec on OnFinality.io. Setting up the collator node spec doesn't have --collator in the list of the arguments:

@demisx I guess we will need to put that one in manually

@demisx
Copy link
Collaborator

demisx commented Sep 15, 2022

@wilwade Yeah, we might. Maybe a bug. I am testing different combinations right now. Another thing I am noticing is that no matter how I try to change the base path to /data it's still get reset to /chain-data 🤷🏻 after saving the network spec. We may need to update our docker images to match.

Repository owner moved this from ⚙️ In Progress to ✅ Accepted in DSNP and Frequency Project Sep 16, 2022
@demisx
Copy link
Collaborator

demisx commented Sep 16, 2022

@wilwade I've created a sample Frequency Rococo network spec (Frequency Rococo 202216-01) with the latest published parachain-node image. I've used the default arguments given by onFinality, except:

  • The ----rpc-methods was set to safe for each node
  • The --name arg has been populated with frequency-full, frequency-archive and frequency-collator respectively. I wasn't sure what the --name arg should be for relay chain, so it's currently an empty string
  • The --collator argument has been added to the collator node

Please feel free to delete this spec any time. I am moving to #282, but let me know if you need my help with anything here or with onFinality.

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