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

Adds data volume to lnd docker service #697

Closed
wants to merge 4 commits into from

Conversation

Osso
Copy link
Contributor

@Osso Osso commented Jan 30, 2018

This is my proposed changes for issue #696

@Osso
Copy link
Contributor Author

Osso commented Jan 30, 2018

This still needs the handling of lncli. It's looking for the macaroon in /root/.lnd
Somehow, this needs to be set --macaroonpath=/data/admin.macaroon

@Osso
Copy link
Contributor Author

Osso commented Jan 31, 2018

I added a symlink creation during the container startup to be able to use lncli again without any argument.

@meshcollider meshcollider added the docker Docker-related PRs/Issues label Jan 31, 2018
@dabura667
Copy link

You should never make a new folder in /

@Osso
Copy link
Contributor Author

Osso commented Feb 3, 2018

Actually, I did not create a new folder. The logs were already stored in /data

That said, Docker is a bit different. Since it's isolated it is quite common to store things in /

@dabura667
Copy link

Why not use ~/.lnd default directory? Then you won’t need to make a symlink.

@Osso
Copy link
Contributor Author

Osso commented Feb 3, 2018

I find it cleaner to store data in /data and that's why I wrote it this way. If this was outside of docker I would store it in /var/lib/lnd. The home directory is not where I usually store application data.

It's definitely possible to make /root/.lnd a volume instead. I'll follow the devs opinion.

@halseth
Copy link
Contributor

halseth commented Feb 5, 2018

I think using ~/.lnd makes the most sense, since that is what is used on Linux. Won't that be the folder used by default for docker?

@Osso
Copy link
Contributor Author

Osso commented Feb 6, 2018

Ok I updated the patch to make /root/.lnd a volume and removed the --datadir option

@Richard87
Copy link

Richard87 commented Feb 7, 2018

What about adding VOLUME /root/.lnd to the lnd dockerfile? Then Docker would create an anonymous volume if not specified when starting a container/image... May prevent dataloss and help users of the image to know what volumes to mount :)

@Osso
Copy link
Contributor Author

Osso commented Feb 7, 2018

Good idea, I added it.

@dabura667
Copy link

This should also contain a fix to the docker example readme.

It breaks the current explanation because when you try to make two lnd containers in the same host they can’t be separated, and use the same volume.

You should modify the docker/README.md to add volume parameters for alice and bob to allow separate volumes.

@Osso
Copy link
Contributor Author

Osso commented Feb 14, 2018

I wrote an update docker-compose.yml to add an alice and bob with different volumes.

I am not sure we want to clutter that file with services that are only useful in the tutorial. It is possible to put them in a different docker compose file but then I need to create network for them to share.

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Possible to do the changes to the README without changing the formatting? Makes it hard to see what has really changed atm 😛

volumes:
- lnd_btc_data:/root/.lnd

alice:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make the changes to this file a separate commit that explains what they achieve?

@Osso
Copy link
Contributor Author

Osso commented Feb 15, 2018

I re-split the changes into 3 commits:

  • the trailing spaces
  • the necessary lnd volumes to prevent data loss
  • the updates to make docker/readme work again

It should be more readable now

@halseth
Copy link
Contributor

halseth commented Apr 20, 2018

@Osso Are these changes/instructions up-to-date with latest master?

@Osso
Copy link
Contributor Author

Osso commented Apr 20, 2018

Mostly, the changes are still needed but the trailing spaces commit must be redone for the new readme.

@Roasbeef Roasbeef added the P3 might get fixed, nice to have label Jul 10, 2018
@halseth
Copy link
Contributor

halseth commented Aug 17, 2018

@Osso Any updates on this? Seems useful (I just cherry-picked the commit to use for some testing) 😅

links:
- "btcd:blockchain"
volumes:
- alice_data:/root/.lnd
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work, as Alice an Bob will share the same folder, and won't be able to run at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alice_data and bob_data are 2 different volumes and won't conflict.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, will need to do some more testing, running into some issues... Will report back!

@Osso
Copy link
Contributor Author

Osso commented Aug 17, 2018

It's quite an old patch. I moved and did not have time to keep it up.

docker/README.md Outdated
@@ -90,7 +90,7 @@ Connect `Bob` node to `Alice` node.

```bash
# Run "Bob" node and log into it:
$ docker-compose run -d --name bob lnd_btc
$ docker-compose run -d bob
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to inlcude the --name parameter, since if not the container will be given a random name.

@@ -34,3 +34,5 @@ RUN apk add --no-cache \
# Copy the entrypoint script.
COPY "docker/lnd/start-lnd.sh" .
RUN chmod +x start-lnd.sh

VOLUME /root/.lnd
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this isn't actually needed if we use a named volume in docker-compose

@@ -82,12 +82,29 @@ services:
container_name: lnd_ltc
links:
- "ltcd:blockchain"
volumes:
- lnd_ltc_data:/root/.lnd
Copy link
Contributor

Choose a reason for hiding this comment

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

The volume shouldn't be required here if you've already defined it in the Dockerfile I believe. Is there any reason why you're also setting it up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am mirroring the way volumes are set up for the other services.
e.g. - litecoin:/data
Also, I prefer having a named volume for ease of management.

Copy link
Contributor

Choose a reason for hiding this comment

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

After doing some further testing it seems docker-compose requires this named volume for management instead of an anonymous volume which is what's created in the Dockerfile.


lnd_btc:
extends: lnd
container_name: lnd_btc
links:
- "btcd:blockchain"
volumes:
- lnd_btc_data:/root/.lnd
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here. shouldn't require it this volume if you create it in lnd/Dockerfile


# Run the "Alice" container and log into it:
$ docker-compose run -d --name alice lnd_btc
$ docker-compose run -d --name alice alice
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea to simplify the docs here, although it would be better to do something like:
docker-compose exec -it --name alice and name the container when it's run.

driver: local

# alice and bob are need for running README examples
alice_data:
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this docker-compose file should be left agnostic to implementation examples.

@guggero
Copy link
Collaborator

guggero commented Nov 7, 2019

This is a very old PR and we probably should have reviewed it a long time ago.
I'm not a fan of adding tutorial specific things into the docker-compose file, since people might use that for their setup outside of just the tutorial.
I left some suggestions in #2533 on how to do this and am closing this PR in favor of the newer one.

@guggero guggero closed this Nov 7, 2019
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants