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

docker-compose up #2699

Merged
merged 1 commit into from Jan 10, 2020
Merged

Conversation

yancyribbens
Copy link
Contributor

@yancyribbens yancyribbens commented Feb 25, 2019

https://i.gifer.com/52an.gif

#2509

build and start both lnd and the btcd backend with a single command docker-compose up. In order to enable docker-compose to bring up the services, I removed service definitions that aren't needed for running the environment. The docs have been updated with new instructions for how to run btcctl now that it's not a separate service.

Also, this PR Moves ltc to a separate docker-compose file. A future PR can allow ltc to be brought up with a single command and inherit from docker-compose.yml.

@yancyribbens yancyribbens changed the title Docker compose up docker-compose up Feb 25, 2019
@Roasbeef Roasbeef added docker Docker-related PRs/Issues P3 might get fixed, nice to have labels Mar 1, 2019
@yancyribbens yancyribbens force-pushed the docker-compose-up branch 2 times, most recently from a1b78ee to 3d9cffd Compare March 15, 2019 13:31
@cfromknecht
Copy link
Contributor

@yancyribbens might need a proper rebase?

@yancyribbens
Copy link
Contributor Author

@cfromknecht I thought I did a proper rebase.. any idea why the diff between this branch and master is so... different?

@cfromknecht
Copy link
Contributor

@yancyribbens it looks like a merge was applied instead of a rebase, judging by bfbe58b

@yancyribbens yancyribbens force-pushed the docker-compose-up branch 2 times, most recently from 1db9326 to e8bedf3 Compare August 12, 2019 20:14
@yancyribbens
Copy link
Contributor Author

@cfromknecht think it was caused by a pull after rebase but before push.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Ran it, works great! Thank you for this feature.

To reduce the diff, could you have docker-compose.ltc.yml inherit from docker-compose.yml and then add the docker litecoin instructions as docker-compose -f docker-compose.yml -f docker-compose.ltc.yml up (or something to that effect)?

@yancyribbens
Copy link
Contributor Author

@valentinewallace I removed the btc specific contents from docker-compose.ltc.yml which reduced the diff a lot. Litecoin can be run now using docker-compose -f docker-compse.ltc.yml up. Does this work ok to reduce the diff size and keep Litcoin functionality?

@yancyribbens
Copy link
Contributor Author

Successfully tested this on mainnet to make a small transaction. It took 4-5days to sync mainnet using BTCD on a dedicated machine with 8cores, 16gb of memory and a SSD drive. I think if this is merged the next thing would be to add the option to use bitcoind instead to improve performance.

@yancyribbens
Copy link
Contributor Author

This PR closes issues: #2829, #2508, #2509

@guggero guggero self-assigned this Nov 14, 2019
@guggero
Copy link
Collaborator

guggero commented Nov 14, 2019

@yancyribbens: We had some changes to these files recently. Could you please rebase then I'm going to have a look. Thanks!

@yancyribbens
Copy link
Contributor Author

@guggero sure i'll see if I can find some time this week to fix the merge conflicts.

@guggero
Copy link
Collaborator

guggero commented Dec 4, 2019

Needs rebase.

@yancyribbens
Copy link
Contributor Author

@guggero done

docker/docker-compose.ltc.yml Show resolved Hide resolved
docker/docker-compose.ltc.yml Show resolved Hide resolved
@yancyribbens
Copy link
Contributor Author

@guggero my bad. I should have test the changes better. I've updated with your suggestions and confirmed docker-compose.ltc.yml builds and syncs with mainnet:

sudo NETWORK=mainnet docker-compose -f ./docker-compose.ltc.yml up
sudo docker exec -it ltcd /start-ltcctl.sh getblockchaininfo

Also, if we want the experience to be similar for both, I recommend updating the docker docs to add instructions for using LTC. #3808

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

We're almost there!
There are some trailing spaces that should be removed. And it would be nice if you could squash everything together into two commits, maybe one for the README and one for the rest. And use the correct docker: prefix in the commit messages.

- lnd:/root/.lnd
entrypoint: ["./start-lnd.sh"]
links:
- "ltcd:blockchain"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: there are a few trailing spaces on this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@yancyribbens
Copy link
Contributor Author

@guggero I squashed the commits and prefixed docker to the message. I think it's preferable to include the readme changes in the same commit, so that if anyone is looking at the commit history can observe that the readme updates are tied together with the docker-compose file changes that go together. I'm happy to split the readme update to a different commit if you have a strong opinion to do so.

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

I tested the newest version and got some collisions. It would be nice if one could start both docker-compose.yml and docker-compose.ltc.yml at the same time.

docker/docker-compose.ltc.yml Show resolved Hide resolved

# lnd volume is used for persisting lnd application data and chain state
# during container lifecycle.
lnd:
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid collision with the other lnd, name this volume lnd_ltc (change also in the volumes section above).

docker/ltcd/start-ltcctl.sh Outdated Show resolved Hide resolved
@yancyribbens
Copy link
Contributor Author

@guggero agreed that would be ideal. I think there's more work todo then your comments to make this happen. For example, L58 of start-lnd.sh needs to be changed so that if both docker-compose.yml and docker-compose.ltc.yml are running they don't both reference blockchain. In addition there are other changes since as renaming the shared volume. Would you be OK with merging this PR since it solves the original issue and then we could create a new issue and PR to run both compose files on the same machine? that way this won't continue to fall behind the master branch as well if we merge these changes as is.

- shared:/rpc
- lnd:/root/.lnd
entrypoint: ["./start-lnd.sh"]
links:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove links, which is deprecated?

Links are a legacy option. We recommend using networks instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johng removing the legacy issues could take place in a separate PR. I think there are enough changes in this PR and better to keep the PR simple and address the github issue imo.

@guggero
Copy link
Collaborator

guggero commented Jan 3, 2020

I agree that more changes are necessary to get both bitcoin and litecoin running in parallel. In my opinion it's OK to postpone these changes (like switching to networks to get rid of the blockchain link/alias) to a separate PR.
But using the same volume for both lnds will lead to problems. So it would be nice if you could rename the ltc one. Then we can get this merged.

@yancyribbens
Copy link
Contributor Author

@guggero the ltc lnd volume name has been updated.

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

@guggero guggero requested a review from carlaKC January 6, 2020 13:04
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Looks good to me 🐳

Only comment is that I ran into a sneaky gotcha when testing some of the block generation commands. docker-compose up only rebuilds if there is a change to the Dockerfile (which there isn't in this change), so the change to use localhost in start-btcctl.sh didn't update when I ran this the first time. Might be worth updating the readme to make a note of that?

Otherwise, awesome PR, glad this one is finally going to go in!

@carlaKC
Copy link
Collaborator

carlaKC commented Jan 8, 2020

Also 😂 👏

https://i.gifer.com/52an.gif

@yancyribbens
Copy link
Contributor Author

@carlaKC thanks for testing out the changes. If I understand your comment, I think if you run docker-compose build --no-cache that should work, even if the images had been previously built. Although, this should only be a temporary problem.

@carlaKC
Copy link
Collaborator

carlaKC commented Jan 9, 2020

@carlaKC thanks for testing out the changes. If I understand your comment, I think if you run docker-compose build --no-cache that should work, even if the images had been previously built. Although, this should only be a temporary problem.

Yeah, I ended up rebuilding the image and it was fine :)
This is a once off problem, but I'm wondering if we should make a comment to warn people that they need to run with --node-cache so they don't run into the same issue. Very small thing though, excited to get this in!

@guggero
Copy link
Collaborator

guggero commented Jan 10, 2020

@carlaKC I think we can just add that warning/hint to the release notes where we describe the changes of this PR. Then we can merge it for 0.9.0-beta-rc2. What do you think?

@carlaKC
Copy link
Collaborator

carlaKC commented Jan 10, 2020

@carlaKC I think we can just add that warning/hint to the release notes where we describe the changes of this PR. Then we can merge it for 0.9.0-beta-rc2. What do you think?

Sure, sounds good to me! Happy to merge as is, it's just a small nit.

@guggero guggero merged commit 0ce2d6f into lightningnetwork:master Jan 10, 2020
@guggero guggero added this to WIP in v0.9.0-beta via automation Jan 14, 2020
@guggero guggero moved this from WIP to Done in v0.9.0-beta Jan 14, 2020
@guggero guggero added this to the 0.9.0 milestone Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker Docker-related PRs/Issues P3 might get fixed, nice to have
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants