ipfs service: dataDir backwards compatibility, fix dataDir existance detection #25782
Conversation
|
@infinisil, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mguentner to be a potential reviewer. |
| script = '' | ||
| if [[ ! -d ${cfg.dataDir}/.ipfs ]]; then | ||
| if [[ ! -d ${cfg.dataDir}/config ]]; then |
There was a problem hiding this comment.
The standard IPFS implementation does the same (for now)
There was a problem hiding this comment.
Thanks for checking that!
|
Pinging @mguentner |
| # Before Version 17.09, ipfs would always use "/var/lib/ipfs/.ipfs" as it's dataDir | ||
| defaultDataDir = if versionAtLeast config.system.stateVersion "17.09" then | ||
| "/var/lib/ipfs" else | ||
| "/var/lib/ipfs/.ipfs"; | ||
|
|
There was a problem hiding this comment.
LGTM
However, that means that users need to rename the IPFS directory (manually) when upgrading.
We could also put a [[ -d /var/lib/ipfs/.ipfs ]] && mv /var/lib/ipfs/.ipfs/* /var/lib/ipfs && rmdir /var/lib/ipfs/.ipfs in the
ipfs-init service.
But given that IPFS is not used that much currently in the Nix community (I presume), we should keep that out (this just creates technical debt).
There was a problem hiding this comment.
@mguentner Shouldn't stateVersion generally stay the same? If the user decides to change it they'll have to look through what breaks themselves, because I think NixOS doesn't need to provide any stability guarantees with stateful data beyond this option. Also it's just the default, if the user sets it explicitly later they'll have to move it themselves. I think that's really the intended way of using stateVersion, citing from the description of the option:
Every once in a while, a new NixOS release may change configuration defaults in a way incompatible with stateful data. [...] To prevent such breakage, you can set the value of this option to the NixOS release with which you want to be compatible. [...]
There was a problem hiding this comment.
Ah I know what you mean now, but the script should be more like
if [[ -d /var/lib/ipfs/.ipfs ]] then
mv /var/lib/ipfs/.ipfs/* "${cfg.dataDir}"
rmdir /var/lib/ipfs/.ipfs
fi
Don't know where we would put this though, maybe when cfg.enable == true?
I think such a script wouldn't be too bad, could be run once at build time. Even if there aren't many users, I think it would be a shame to have it break when upgrading.
There was a problem hiding this comment.
I keep thinking about this and I just can't come up with a nice solution, maybe just put it in the release notes?
There was a problem hiding this comment.
Yeah. I forgot the cfg.dataDir again. You are perfectly right.
Let's put it in the release notes. I don't think that any reason justifies adding more unnecessary complexity. IPFS is alpha software, so the NixOS module should be alpha as well. KISS 💋
There was a problem hiding this comment.
Release notes would be located in ./nixos/doc/manual/release-notes/rl-1709.xml
| script = '' | ||
| if [[ ! -d ${cfg.dataDir}/.ipfs ]]; then | ||
| if [[ ! -d ${cfg.dataDir}/config ]]; then |
There was a problem hiding this comment.
Thanks for checking that!
|
Added a release note for this change including the script to migrate, ready to merge. Edit: No scratch that, I still found something wrong, gonna update |
| @@ -128,7 +128,7 @@ in | |||
| install -m 0755 -o ${cfg.user} -g ${cfg.group} -d ${cfg.dataDir} | |||
| ''; | |||
| script = '' | |||
| if [[ ! -d ${cfg.dataDir}/config ]]; then | |||
| if [[ ! -f ${cfg.dataDir}/config ]]; then | |||
There was a problem hiding this comment.
My bash knowledge isn't great, so I didn't notice this before, config is a file, not a directory
|
@fpletz Did it, used squashing for the first time, it's not as bad as I thought it would be :D |
|
Cool, thanks. I just finished some tests. Let's merge this. |
Motivation for this change
Because of #25531, older instances of IPFS directories weren't being detected (see #25759). Also the check for the existance was incorrect originally. This PR fixes both of these issues.
I tested this on the release-17.03 branch, since master currently doesn't build for me, the change is pretty trivial though.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)