search

ipfs / kubo

An IPFS implementation in Go
https://docs.ipfs.tech/how-to/command-line-quick-start/
Other
15.83k stars 2.96k forks source link

fix(fuse): path parsing #10243

Closed hacdias closed 1 week ago

hacdias commented 5 months ago

This PR fixes missing prefix (closes #10242).

lidel commented 5 months ago

I confirmed the path fix works, I am able to get data from a raw block:

$ GOLOG_LOG_LEVEL="error,fuse/ipfs=debug" ipfs daemon --mount
...

$ cat /ipfs/bafkreide4y52wsmgpu2zulfqnj6iswp4vcng4jyujounbymeq3h2rexzk4
test fuse

but if CID is dag-pb I get could not convert protobuf or raw error (which is a separate bug):

$ cat /ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi > guardian.jpg
cat: /ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi: No such file or directory
# ERROR fuse/ipfs   readonly/readonly_unix.go:113   could not convert protobuf or raw node: expected protobuf dag node

It seems to be caused by (1) failing due to (2):

  1. https://github.com/ipfs/kubo/blob/8ab2de5ff05e1427a22be868b31efbcd58a6ee13/fuse/readonly/readonly_unix.go#L102
  2. https://github.com/ipfs/boxo/blob/08959f281f866bd4197b40bc90cf75c2828c07f0/ipld/merkledag/node.go#L557-L559

but I did not debug deeper (might be similar to https://github.com/ipfs/kubo/issues/9044, https://github.com/ipfs/kubo/pull/10141 landed in September)

@hacdias thoughts?

Maybe timebox this to 1h and if this turns out to be a time sink, ok to push this fix to 0.26? We need to pick our battles before holiday/nucleation break.

hacdias commented 5 months ago

@lidel I tried a few things, including playing with different versions of dependencies, but I wasn't able to figure out the source issue within an hour. Nevertheless, I also checked out v0.23.0 and the same error occurs. So #10141 did not fully fix the FUSE issues.

Could you try reproducing with v0.23.0 and updating the title of #10242 if that's the case? It probably goes further back, but to build previous versions we need an older Go version. I may try that later.

bmwiedemann commented 5 months ago

I tested it. This patch makes fuse work for me again.

hacdias commented 5 months ago

@bmwiedemann it does fix part of the problem, but sadly not the whole problem.

@lidel maybe we can squeeze the path prefix fix for the 0.25.0 and then figure out what's wrong with the dag-pb handling separately? Or do you think it's better to hold and fix both at once.,

bmwiedemann commented 5 months ago

commit a7c65184976e8717ac23d7efaa5b0d477ad15deb was the one that broke FUSE (path prefix part).

hacdias commented 5 months ago

@bmwiedemann yes, we are aware. But we still don't know exactly what's causing the dag-pb handling problem.

bmwiedemann commented 5 months ago

My tests show that the dag-pb handling problem already existed in v0.22.0 and v0.23.0 so maybe the fix was just incomplete.

I used this test for these:

kubo/cmd/ipfs/ipfs daemon --mount & sleep 5
cat /ipfs/QmT78zSuBmuS4z925WZfrqQ1qHaJ56DQaTfyMUF7F8ff5o | wc -c
cat /ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi | wc -c
Luflosi commented 5 months ago

I tried applying this patch to Kubo v0.24.0 in Nixpkgs. With the patch the NixOS test works again. So this fixes #10242 and returns the FUSE functionality to how it was since Kubo v0.13.0. But if I remove the workaround you can see in the test, the test fails again and I get "ERROR fuse/ipfs readonly/readonly_unix.go:113 could not convert protobuf or raw node: expected protobuf dag node" as described in #9044. This has been like that since Kubo v0.13.0, even Kubo v0.23.0 did not work. I think all this information is already in other comments in this PR, I just wanted to tell you what I tested.

lidel commented 5 months ago

@hacdias I think it is better to hold and fix both at once, 0.25 would be nice, but even if fix slips to 0.26, holding is still a lesser evil imo:

If FUSE does not work at all, then users see it and can stay on older version. If it is working "sometimes" (only for raw, but not for dag-pb CIDs), then they get false sense of security, like from the passing test @Luflosi linked above, which tests raw and not dag-pb, and may end up with a broken setup without knowing it (best case wasting time, the worst case losing data and good will for the project).

One thing we should also fix here, is adding tests for both raw and dag-pb, to catch the error described in https://github.com/ipfs/kubo/pull/10243#issuecomment-1841895501 – our CI is green because don't have it. Added TODO at the top of this PR so we don't forget.

bmwiedemann commented 1 month ago

This is still not merged in 0.27 - is there a timeline when we will see working ipfs FUSE again?