Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(fuse): path parsing #10243

Merged
merged 1 commit into from
May 6, 2024
Merged

fix(fuse): path parsing #10243

merged 1 commit into from
May 6, 2024

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Dec 4, 2023

This PR fixes missing prefix (closes #10242).

@hacdias hacdias self-assigned this Dec 4, 2023
@hacdias hacdias marked this pull request as ready for review December 4, 2023 14:29
@hacdias hacdias requested a review from a team as a code owner December 4, 2023 14:29
@hacdias hacdias mentioned this pull request Dec 5, 2023
2 tasks
@hacdias hacdias marked this pull request as draft December 5, 2023 11:24
@lidel
Copy link
Member

lidel commented Dec 6, 2023

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. fnd, err = mdag.ProtoNodeConverter(blk, substrate)
  2. https://github.com/ipfs/boxo/blob/08959f281f866bd4197b40bc90cf75c2828c07f0/ipld/merkledag/node.go#L557-L559

but I did not debug deeper (might be similar to #9044, #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
Copy link
Member Author

hacdias commented Dec 6, 2023

@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.

@lidel lidel changed the title fuse: fix ipfs path parsing fix(fuse) path parsing and dag-pb handling Dec 6, 2023
@bmwiedemann
Copy link
Contributor

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

@hacdias
Copy link
Member Author

hacdias commented Dec 6, 2023

@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
Copy link
Contributor

bmwiedemann commented Dec 6, 2023

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

@hacdias
Copy link
Member Author

hacdias commented Dec 6, 2023

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

@hacdias hacdias changed the title fix(fuse) path parsing and dag-pb handling fix(fuse): path parsing and dag-pb handling Dec 6, 2023
@bmwiedemann
Copy link
Contributor

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
Copy link
Contributor

Luflosi commented Dec 6, 2023

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
Copy link
Member

lidel commented Dec 7, 2023

@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 #10243 (comment) – our CI is green because don't have it. Added TODO at the top of this PR so we don't forget.

@bmwiedemann
Copy link
Contributor

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

@hacdias hacdias assigned lidel and unassigned hacdias May 6, 2024
@hacdias hacdias requested a review from lidel May 6, 2024 12:14
@hacdias hacdias marked this pull request as ready for review May 6, 2024 12:15
@hacdias hacdias added the skip/changelog This change does NOT require a changelog entry label May 6, 2024
@lidel lidel changed the title fix(fuse): path parsing and dag-pb handling fix(fuse): path parsing May 6, 2024
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Discussed with @hacdias and decided to merge this (only path fix) + document remaining work in #9044 (comment).

@lidel lidel enabled auto-merge (squash) May 6, 2024 21:18
@lidel lidel merged commit 2841ec0 into master May 6, 2024
19 of 20 checks passed
@lidel lidel deleted the issue-10242 branch May 6, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does NOT require a changelog entry
Projects
No open projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

Fuse functionality broken in v0.24.0
4 participants