ipfs: initialConfig option + fixes #43089

elitak · 2018-07-05T22:15:12Z

added initialConfig option
fix: include wrapped binary in system PATH
fix: init service now attempts to umount mountpoints
fix: properly evaluate overridden extraConfig options.

Motivation for this change

fixes #42672

Addresses #82580 and ipfs/kubo#4214 (#29133 (comment))

initialConfig option is useful for fresh deployments via nixops etc.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

matthewbauer · 2018-07-25T17:20:44Z

@GrahamcOfBorg test ipfs

GrahamcOfBorg · 2018-07-25T17:21:14Z

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: tests.ipfs

Partial log (click to expand)

Cannot nix-instantiate `tests.ipfs' because:
error: attribute 'ipfs' in selection path 'tests.ipfs' not found

GrahamcOfBorg · 2018-07-25T17:21:47Z

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: tests.ipfs

Partial log (click to expand)

Cannot nix-instantiate `tests.ipfs' because:
error: attribute 'ipfs' in selection path 'tests.ipfs' not found

matthewbauer · 2018-07-25T17:22:01Z

/cc @seppeljordan

infinisil · 2018-07-30T19:08:11Z

nixos/modules/services/network-filesystems/ipfs.nix

-            (path: value:
+          (mapAttrsRecursiveCond (value: ! value ? _type)
+            (path: value: let
+              value' = if value ? _type then value.content else value;


Why is this suddenly needed?

Attrsets in configuration.nix evaluate to these kind of attrset wrappers (for purposes of handling mkOverride IIUC), rather than the attrsets literally provided. I had not sufficiently tested using attrsets as values, so I only recently found this change was necessary for proper marshalling.

I don't get it, can you add a comment explaining this with an example of what wouldn't work without it?

services.ipfs.extraConfig.Swarm.AddrFilters = []; wouldn't work right now in master, because the attrset Swarm is not flattened into a simple attrset at the time the recursive map is evaluated. I can't explain why, because I don't understand it myself. I only know that the attrset I'm expecting is in .contents whenever .type is present.

Can't reproduce, with or without this change, the mentioned setting works without problems.

Update on this?

infinisil · 2018-07-30T19:10:07Z

nixos/modules/services/network-filesystems/ipfs.nix

-        install -m 0755 -o ${cfg.user} -g ${cfg.group} -d ${cfg.ipfsMountDir}
-        install -m 0755 -o ${cfg.user} -g ${cfg.group} -d ${cfg.ipnsMountDir}
+        # XXX Sometimes, the daemon stops or crashes without unmounting.
+        umount -l "${cfg.ipfsMountDir}" || true


Why a lazy unmount?

If some process has an open handle in there, I don't want that to prevent a remount. This is a FUSE virtual FS, anyway, so I can't imagine how any harm could result from this.

mmahut · 2019-08-07T21:14:29Z

Any updates on this pull request, please?

elitak · 2020-04-20T12:38:59Z

I've combined the original initialConfig featrue with a few other fixes. This is ready to be reviewed again.

Ericson2314 · 2020-06-12T19:06:12Z

nixos/modules/services/network-filesystems/ipfs.nix

+      umount -l "${cfg.ipnsMountDir}" || true
+      install -m 0755 -o "${cfg.user}" -g "${cfg.group}" -d "${cfg.ipfsMountDir}"
+      install -m 0755 -o "${cfg.user}" -g "${cfg.group}" -d "${cfg.ipnsMountDir}"
+>>>>>>> 03a4ab9399b... ipfs: initialConfig option + fixes


merge artifact

mmahut · 2020-06-12T20:00:52Z

@elitak can we please have this rebase with the fix?

Ericson2314 · 2020-06-12T20:04:26Z

We want to land https://github.com/NixOS/nixpkgs/pull/90157/files, either with a fix, or a fix right after.

elitak · 2020-06-12T20:58:09Z

Huh? This doesn't fix anything crucial. I can rebase it later.

Not sure what the hangup was over this PR; I opened it almost 2 years ago...

fix: include wrapped binary in system PATH fix: init service now attempts to umount mountpoints fix: properly evaluate overridden extraConfig options.

Ericson2314 · 2020-06-12T22:03:10Z

Oh, I meant that I wasn't gonna rebase this myself. But fine if you do, we can ~~merge it or similar~~ consider this PR.

matthewbauer · 2020-06-12T22:12:05Z

nixos/modules/services/network-filesystems/ipfs.nix

@@ -192,6 +207,20 @@ in {
        default = true;
      };

+      initialConfig = mkOption {


Why is initialConfig needed when extraConfig is also available?

matthewbauer · 2020-06-12T22:13:44Z

nixos/modules/services/network-filesystems/ipfs.nix

    serviceConfig.User = cfg.user;
    serviceConfig.Group = cfg.group;
  };

  baseService = recursiveUpdate commonEnv {
    wants = [ "ipfs-init.service" ];
    # NB: migration must be performed prior to pre-start, else we get the failure message!
-    preStart = optionalString cfg.autoMount ''
+    preStart = ''
+      install -m 0755 -o "${cfg.user}" -g "${cfg.group}" -d "${cfg.dataDir}"


is this still needed in ipfs 0.5.1? ideally we would get some more diagnostics and report this as a bug

Ericson2314 · 2020-06-12T22:36:44Z

There is no wrapped binary, and I think the other 3 things would be better to configure separately. We have two bugs that cannot be reproduced, and initialConfig is a feature not a fix.

stale · 2021-06-09T00:40:30Z

I marked this as stale due to inactivity. → More info

Ericson2314 · 2021-06-29T18:17:21Z

I think we've taken everything we should from this.

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jul 5, 2018

matthewbauer requested a review from infinisil July 25, 2018 17:22

infinisil reviewed Jul 30, 2018

View reviewed changes

elitak force-pushed the ipfs branch from 465d9b1 to aa91fe3 Compare April 13, 2020 23:50

ofborg bot added 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 labels Apr 13, 2020

elitak force-pushed the ipfs branch 3 times, most recently from 78fc0c3 to 83aa7da Compare April 20, 2020 12:36

Ericson2314 reviewed Jun 12, 2020

View reviewed changes

matthewbauer mentioned this pull request Jun 12, 2020

Add socket-based IPFS support #90157

Merged

ipfs: initialConfig option + fixes

1eba27e

fix: include wrapped binary in system PATH fix: init service now attempts to umount mountpoints fix: properly evaluate overridden extraConfig options.

elitak force-pushed the ipfs branch from 83aa7da to 1eba27e Compare June 12, 2020 21:02

matthewbauer reviewed Jun 12, 2020

View reviewed changes

ryantm added the 2.status: merge conflict label Oct 3, 2020

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 9, 2021

Ericson2314 closed this Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipfs: initialConfig option + fixes #43089

ipfs: initialConfig option + fixes #43089

elitak commented Jul 5, 2018 •

edited

matthewbauer commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

matthewbauer commented Jul 25, 2018

infinisil Jul 30, 2018

elitak Jul 30, 2018

infinisil Jul 31, 2018

elitak Aug 4, 2018

infinisil Aug 13, 2018

mmahut Jun 12, 2020

infinisil Jul 30, 2018

elitak Jul 30, 2018

mmahut commented Aug 7, 2019

elitak commented Apr 20, 2020

Ericson2314 Jun 12, 2020

mmahut commented Jun 12, 2020

Ericson2314 commented Jun 12, 2020

elitak commented Jun 12, 2020

Ericson2314 commented Jun 12, 2020 •

edited

matthewbauer Jun 12, 2020

matthewbauer Jun 12, 2020

Ericson2314 commented Jun 12, 2020

stale bot commented Jun 9, 2021

Ericson2314 commented Jun 29, 2021

ipfs: initialConfig option + fixes #43089

ipfs: initialConfig option + fixes #43089

Conversation

elitak commented Jul 5, 2018 • edited

Motivation for this change

Things done

matthewbauer commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

matthewbauer commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmahut commented Aug 7, 2019

elitak commented Apr 20, 2020

Choose a reason for hiding this comment

mmahut commented Jun 12, 2020

Ericson2314 commented Jun 12, 2020

elitak commented Jun 12, 2020

Ericson2314 commented Jun 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Jun 12, 2020

stale bot commented Jun 9, 2021

Ericson2314 commented Jun 29, 2021

elitak commented Jul 5, 2018 •

edited

Ericson2314 commented Jun 12, 2020 •

edited