Skip to content

fix: fix docker start command#1237

Merged
jcstein merged 8 commits intomainfrom
jcs/update-docker
Nov 10, 2023
Merged

fix: fix docker start command#1237
jcstein merged 8 commits intomainfrom
jcs/update-docker

Conversation

@jcstein
Copy link
Copy Markdown
Member

@jcstein jcstein commented Nov 2, 2023

Overview

Resolves #1228
running with persistent storage logs

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 2, 2023

PR Preview Action v1.4.4
🚀 Deployed preview to https://celestiaorg.github.io/docs/pr-preview/pr-1237/
on branch gh-pages at 2023-11-10 16:06 UTC

@jcstein jcstein self-assigned this Nov 2, 2023
@jcstein jcstein added the bug Something isn't working label Nov 2, 2023
Comment thread nodes/docker-images.md
Comment thread nodes/docker-images.md
@@ -160,21 +161,21 @@ An example init command will look similar to below:

```bash-vue [Mainnet Beta]
docker run -e NODE_TYPE=$NODE_TYPE -e P2P_NETWORK=$NETWORK \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be added here:

-e CELESTIA_HOME=/celestia

or a section above should be added exporting a variable that can be used here like the others.

Or the other option would be to modify the default docker environment variable to use the /celestia path instead of /home/celestia.

Or you may want to switch the other commands back to using /home/celestia for the mounted volume instead of /celestia to maintain persistence.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i tested the full command when i edited this doc and it worked as expected. -v $HOME/my-node-store:/celestia \ does the trick

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

what OS are you on? i think that may be the issue. this doc works for mac and linux.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've executed it both within windows and Ubuntu WSL. Are you sure it is actually persisting in your testing? The command executes successfully as written for me as well, it just doesn't actually persist anything. So after running it you can check in the $HOME/my-node-store folder on your host machine and find no files or directories were created (of course you should wipe the directory first to ensure it is empty to begin with and isn't using the data from previous runs).

So while I think all of the commands in the docs will not currently throw any errors, I do believe they are not correct as currently written as they will not achieve the intended goal of a persistent node. Perhaps I messed up somehow though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

got it, i’ll test to verify!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The original comment in this thread was to fix the persistence issue I had encountered.

Comment thread nodes/docker-images.md
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd make a note that trying to run the docker images in windows is likely to run into issues due to the way NTFS handles permissions and that referencing a volume that supports linux permissions (such as within WSL2) is a good way for Windows users to be able to run a node with persistent storage.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

please feel free to make a PR to add the instructions that work for you on Windows!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

for the scope of this PR, i'd just like to fix the docker commands that do work. hopefully you understand 🙏

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For sure that's totally reasonable. I do think--like I mentioned above--that the current commands do not achieve the intended objective of persisting a node although they may not throw any errors but perhaps you are referencing a different version of the docker container or something like that which works.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jcstein I created this PR #1249 feel free to modify/improve wording as much as you feel is appropriate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The additional parameter I recommend adding to the docker commands should fix this so that it does persist correctly. That, or the other options I mentioned in the other comment thread above should work as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Understood! What is the full start command then? As I understand the -v $HOME/my-node-store:/home/celestia flag would still be used?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

docker run -e NODE_TYPE=$NODE_TYPE -e P2P_NETWORK=$NETWORK -e CELESTIA_HOME=/celestia -v $HOME/my-node-store:/celestia ghcr.io/celestiaorg/celestia-node:v0.12.0 celestia $NODE_TYPE init --p2p.network $NETWORK
Initializing Celestia Node with command:
celestia light init --p2p.network celestia
2023-11-09T10:36:03.394Z	INFO	node	nodebuilder/init.go:31	Initializing Light Node Store over '/celestia/.celestia-light'
Error: mkdir /celestia/.celestia-light: operation not permitted

Copy link
Copy Markdown
Member Author

@jcstein jcstein Nov 9, 2023

Choose a reason for hiding this comment

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

it appears to be something related to permissions in the container itself afaiu. here are full logs

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

after changing the path back to /home/celestia and using your -e CELESTIA_HOME=/home/celestia flag, i hit an error not being able to create the directory in the container?

@jcstein jcstein requested a review from smuu November 3, 2023 11:56
@smuu
Copy link
Copy Markdown
Contributor

smuu commented Nov 9, 2023

So my findings:

  • when using /home/celestia instead of /celestia, the keys and the data are persisted (the node uses this if not specified otherwise
  • when using Mainnet Beta, we don't need to set P2P_NETWORK as this points by default to the celestia network
  • NODE_TYPE is not needed at all, I think, as this is only used when automatically calling init when starting, but as we have persistent data and we init manually, this is not needed in this section

This should resolve the bug and make this section a little cleaner.

@jcstein
Copy link
Copy Markdown
Member Author

jcstein commented Nov 9, 2023

copying this for vis @smuu

after changing the path back to /home/celestia and using your -e CELESTIA_HOME=/home/celestia flag, i hit an error not being able to create the directory in the container?

@jcstein
Copy link
Copy Markdown
Member Author

jcstein commented Nov 9, 2023

i plan to use p2p network and node type so that users can set up env once in above section in order to not have to explain/set the same variables twice

@smuu
Copy link
Copy Markdown
Contributor

smuu commented Nov 9, 2023

i plan to use p2p network and node type so that users can set up env once in above section in order to not have to explain/set the same variables twice

I understand, then I propose to use $NODE_TYPE also in the init and start commands, as those are hardcoded to light at the moment.

@smuu
Copy link
Copy Markdown
Contributor

smuu commented Nov 9, 2023

copying this for vis @smuu

after changing the path back to /home/celestia and using your -e CELESTIA_HOME=/home/celestia flag, i hit an error not being able to create the directory in the container?

I can't reproduce this behavior.
When following the steps of creating the folder and changing the permissions, I can run the container with the init.

@keyneom
Copy link
Copy Markdown
Contributor

keyneom commented Nov 9, 2023

copying this for vis @smuu
after changing the path back to /home/celestia and using your -e CELESTIA_HOME=/home/celestia flag, i hit an error not being able to create the directory in the container?

I can't reproduce this behavior. When following the steps of creating the folder and changing the permissions, I can run the container with the init.

That is my experience as well. As long as the permissions are set with the prior command (chown) then I don't encounter any errors and things persist correctly.

@keyneom
Copy link
Copy Markdown
Contributor

keyneom commented Nov 9, 2023

So my findings:

  • when using /home/celestia instead of /celestia, the keys and the data are persisted (the node uses this if not specified otherwise
  • when using Mainnet Beta, we don't need to set P2P_NETWORK as this points by default to the celestia network
  • NODE_TYPE is not needed at all, I think, as this is only used when automatically calling init when starting, but as we have persistent data and we init manually, this is not needed in this section

This should resolve the bug and make this section a little cleaner.

I believe the docs as written are intended to support running on test nets, etc so I'd leave the P2P_NETWORK as is. I do think it makes sense to just use /home/celestia since it is the default. I wasn't clear on the other factors that may have been driving a switch to /celestia.

@jcstein
Copy link
Copy Markdown
Member Author

jcstein commented Nov 10, 2023

I think we're in good shape now!

@jcstein jcstein requested a review from keyneom November 10, 2023 16:06
Copy link
Copy Markdown
Contributor

@smuu smuu left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Contributor

@keyneom keyneom left a comment

Choose a reason for hiding this comment

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

Lgtm

@jcstein jcstein merged commit 8137ba5 into main Nov 10, 2023
@jcstein jcstein deleted the jcs/update-docker branch November 10, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docker Light Node fails to start second time and Docker Docs Are Incorrect for Start

3 participants