Added data_dir configuration options.

[mhrheaume]

In certain deployment scenarios, we may not want (or be able) to write to the default ~/.lndk data directory. For example, the home directory may be mounted as read-only, or mounted using ephemeral storage.

This change adds a new data_dir configuration option to specify where LNDK should store data. If this option isn't specified, then falls back to the default ~/.lndk.

[kannapoix]

I tested both CLI flag and config file, it seems good. Thank you ✨
Maybe we have to think about log_dir. Should we put logs relative to data_dir when it specified? I think LNDK still create ~/.lndk directory and put logs in it, even if data_dir is not ~/.lndk.

[mhrheaume]

I tested both CLI flag and config file, it seems good. Thank you ✨ Maybe we have to think about log_dir. Should we put logs relative to data_dir when it specified? I think LNDK still create ~/.lndk directory and put logs in it, even if data_dir is not ~/.lndk.

For my particular use case I'm just setting both data_dir and log_dir to somewhere other than ~/.lndk, and that works for me, but I like your suggestion that we fall back to data_dir if log_dir isn't specified? Something like log_dir -> data_dir -> ~/.lndk?

[kannapoix]

For my particular use case I'm just setting both data_dir and log_dir to somewhere other than ~/.lndk, and that works for me, but I like your suggestion that we fall back to data_dir if log_dir isn't specified? Something like log_dir -> data_dir -> ~/.lndk?

Thank you addressing it. I think that is better!

But I found something strange. It seems log_dir is treated as a file in the code🤔 If pass --log-dir=lndk-log, a log file is named as lndk-log. So in the new code, if we didn't set a log directory explicitly but only data directory, lndk creates the data directory and then try to create a log file with the same name to the data directory. This causes error.
My apologies for not recognizing this behavior. Maybe we can fix this the other PR 🙇

[mhrheaume]

Thank you addressing it. I think that is better!

But I found something strange. It seems log_dir is treated as a file in the code🤔 If pass --log-dir=lndk-log, a log file is named as lndk-log. So in the new code, if we didn't set a log directory explicitly but only data directory, lndk creates the data directory and then try to create a log file with the same name to the data directory. This causes error. My apologies for not recognizing this behavior. Maybe we can fix this the other PR 🙇

Ah, right, I forgot that log_dir is actually expected to be a file rather than a directory. I've updated it to append lndk.log to the path if we're falling back to data_dir and tested locally to make sure it's doing the right thing in that scenario.

[a-mpch]

Ran and everything seems to work and found that we might want to set a broad definition (as I'll need to change the lndk.conf pr #193 too 😓 my bad).

First we must separate data_dir with lndk_dir to follow "standarness" of other projects (Bitcoin and LND):

lndk_dir: main repository, where lndk.conf could be stored (as defaults) and other dirs could be created or mounted, as data_dir
data_dir: directory where you must have a write permission as the deamon will write data (logs or TLS) inside. Usually it separates by networks.

Currently they are the same directory, lndk_dir does not exists but mounting data_dir in .lndk means that a conf in .lndk could be writable (not that nice). Therefore, we might want to in another PR do some more changes as we would like to add networks to the data_dir or we could use another approach of adding a prefix or something else (just thinking how to be a bit more future proof).

So what would that mean?

We might wanna do a breaking change on logs and app data, fix that log_dir to be a dir and add maybe log_file to be used. It will be needed anyway. Next configuration might wanna use a direct reference to the previous log (and wouldn't change) until we do something like rotating logs and sadly it will need a change. That might be really possible as stated here we could have a lot of onion traffic (and would be double of logs, lnd's and lndk's for the same pressure 😅).

Enough blabla, we could just go ahead with this PR and maybe a following one addressing the other issues or whatever works best.

@orbitalturtle or @dunxen might have a thought on doing breaking changes, but as LNDK is experimental I think it would be just acceptable.

[dunxen]

Enough blabla, we could just go ahead with this PR and maybe a following one addressing the other issues or whatever works best.

@orbitalturtle or @dunxen might have a thought on doing breaking changes, but as LNDK is experimental I think it would be just acceptable.

Yeah, relatively frequent breaking changes are still fine IMO even if it relates to persistence. They'll be documented in the following release.

Sorry for the delay here! I'll be attending to a handful of LNDK PRs this weekend!

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (ea35b47) to head (d6c665f).

Files with missing lines	Patch %	Lines
src/main.rs	0.00%	12 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #192   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           1       1           
  Lines         128     135    +7     
======================================
- Misses        128     135    +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dunxen]

We might wanna do a breaking change on logs and app data, fix that log_dir to be a dir and add maybe log_file to be used. It will be needed anyway. Next configuration might wanna use a direct reference to the previous log (and wouldn't change) until we do something like rotating logs and sadly it will need a change. That might be really possible as stated here we could have a lot of onion traffic (and would be double of logs, lnd's and lndk's for the same pressure 😅).

Thanks for the suggestions here, @a-mpch!

A few things:

1. I'm happy to have these breaking changes for the sake of clarity and accuracy. So we can go ahead and just outright replace all mentions and naming of log_dir with log_file. This can happen now in this PR.
2. In what case would we want to also keep a separate log_dir configuration option? Do we even need it? How would that play with log_file?
3. I agree that we can work at having separate configs for lndk_dir and data_dir. This can happen in a separate PR before the next release and we can discuss specifics. So let's leave the name data_dir as is for now to cover the single directory used by lndk for any conf to be stored and data to be written. We will align them in that new PR with more standard practices.

@a-mpch, @kannapoix, and @mhrheaume, what are your feelings on this?

tl;dr: Rename log_dir to log_file throughout the entire project in this PR and have follow-up PR discussing a split for lndk_dir & data_dir configuration.

cc @orbitalturtle

[a-mpch]

@a-mpch, @kannapoix, and @mhrheaume, what are your feelings on this?

@dunxen works for me!

[mhrheaume]

Ugh, running into some hiccups running the integration tests. Assuming those are from my changes, I'll hopefully be able to reproduce locally shortly.

[mhrheaume]

Ok, whew! MDM running on my machine didn't like the fact that the itests were downloading and running bitcoind, so I ended up running the tests in a Docker container. I'll post it in a separate PR if there's interest. Anyways, latest revision should pass!

[dunxen]

Thanks! Yeah it would be great if you could open a PR for running those tests in a container as an option, as it would help others in the same situation.

Everything looks good but just a comment on commit ordering.

[dunxen]

Could we reorder the commits so this renaming one is first? Then we can avoid adding and removing this comment.

[orbitalturtle]

Ran some tests and looks good once the commit ordering comment is addressed. Thanks for the PR!