Skip to content

Pinning fail bug #1008

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.

Sign up for GitHub

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

Jump to bottom
Closed
jbenet opened this issue Apr 3, 2015 · 2 comments
Closed

Pinning fail bug #1008

jbenet opened this issue Apr 3, 2015 · 2 comments
Assignees
@whyrusleeping
Labels
kind/bug A bug in existing code (including security flaws) topic/commands Topic commands

Comments

@jbenet
Copy link
Member

jbenet commented Apr 3, 2015

Found a bug.

  • I pinned a hash:
# i pinned a hash
> ipfs pin add -r <hash>

# pinning failed (context deadline exceeded)

# it ran it again
> ipfs pin add -r <hash>
pinned <hash> recursively
# succeeded immediately

# ran refs just to check
> ipfs refs -r <hash>
<hash1>
<hash2>
<hash3>
<hash4>
<hash5>

# then it got stuck, in the usual "i dont have a ref yet, am fetching it" kind of way
# and proceeded to download more.

THE POINT though, is that ipfs pin add -r <hash> succeeded without all the refs being local

Suggested fix: when adding a recursive pin must check all the refs are there locally. May not need to check the refs themselves, but at least check the object is stored in the repo.
@hosh
Copy link
Contributor

hosh commented Apr 3, 2015

I ran into that, but thought that was normal

@whyrusleeping
Copy link
Member

makes sense, ill fix it

@whyrusleeping whyrusleeping self-assigned this Apr 3, 2015
@whyrusleeping whyrusleeping added kind/bug A bug in existing code (including security flaws) topic/commands Topic commands backlog labels Apr 3, 2015
whyrusleeping added a commit that referenced this issue Apr 4, 2015
@whyrusleeping
fix for #1008
fce3f88
whyrusleeping added a commit that referenced this issue Apr 4, 2015
@whyrusleeping
fix for #1008
5c1e399
@whyrusleeping whyrusleeping mentioned this issue Apr 4, 2015
Merged
whyrusleeping added a commit that referenced this issue Apr 11, 2015
@whyrusleeping
fix for #1008
2411de5
@jbenet jbenet mentioned this issue Apr 13, 2015
Open
whyrusleeping added a commit that referenced this issue Apr 19, 2015
@whyrusleeping
fix for #1008 and other pinning fixes
3cf18e8 
This commit adds a new set of sharness tests for pinning, and addresses
bugs that were pointed out by said tests.

test/sharness: added more pinning tests

Pinning is currently broken. See issue #1051. This commit introduces
a few more pinning tests. These are by no means exhaustive, but
definitely surface the present problems going on. I believe these
tests are correct, but not sure. Pushing them as failing so that
pinning is fixed in this PR.

make pinning and merkledag.Get take contexts

improve 'add' commands usage of pinning

FIXUP: fix 'pin lists look good'

ipfs-pin-stat simple script to help check pinning

This is a simple shell script to help check pinning.

We ought to strive towards making adding commands this easy.
The http api is great and powerful, but our setup right now
gets in the way. Perhaps we can clean up that area.

updated t0081-repo-pinning

- fixed a couple bugs with the tests
- made it a bit clearer (still a lot going on)
- the remaining tests are correct and highlight a problem with
  pinning. Namely, that recursive pinning is buggy. At least:
  towards the end of the test, $HASH_DIR4 and $HASH_FILE4 should
  be pinned indirectly, but they're not. And thus get gc-ed out.
  There may be other problems too.

cc @whyrusleeping

fix grep params for context deadline check

fix bugs in pin and pin tests

check for block local before checking recursive pin
whyrusleeping added a commit that referenced this issue Apr 20, 2015
@whyrusleeping
fix for #1008 and other pinning fixes
0a6b880 
This commit adds a new set of sharness tests for pinning, and addresses
bugs that were pointed out by said tests.

test/sharness: added more pinning tests

Pinning is currently broken. See issue #1051. This commit introduces
a few more pinning tests. These are by no means exhaustive, but
definitely surface the present problems going on. I believe these
tests are correct, but not sure. Pushing them as failing so that
pinning is fixed in this PR.

make pinning and merkledag.Get take contexts

improve 'add' commands usage of pinning

FIXUP: fix 'pin lists look good'

ipfs-pin-stat simple script to help check pinning

This is a simple shell script to help check pinning.

We ought to strive towards making adding commands this easy.
The http api is great and powerful, but our setup right now
gets in the way. Perhaps we can clean up that area.

updated t0081-repo-pinning

- fixed a couple bugs with the tests
- made it a bit clearer (still a lot going on)
- the remaining tests are correct and highlight a problem with
  pinning. Namely, that recursive pinning is buggy. At least:
  towards the end of the test, $HASH_DIR4 and $HASH_FILE4 should
  be pinned indirectly, but they're not. And thus get gc-ed out.
  There may be other problems too.

cc @whyrusleeping

fix grep params for context deadline check

fix bugs in pin and pin tests

check for block local before checking recursive pin
wking added a commit that referenced this issue Jun 9, 2015
@wking
core/commands/unixfs/ls: Don't recurse into chunked files
bdefbbb 
Folks operating at the Unix-filesystem level shouldn't care about that
level of Merkle-DAG detail.  Before this commit we had:

  $ ipfs unixfs ls /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox
  QmW1HetBAhJPQgLMpYXyRb8JV4AU8d8WFvK9wkiNc4ki31 File 262144
  QmcG1bJ3dtKdEqKv4pyzNZaNdFGb4eS4TYP3MNXyj1K4sD File 262144
  QmYuAwy4tEeRkWe2BG8CAUW6amjgDQAQXJ2Efikqip2r66 File 262144
  QmccDNKasKMBZ7jRGfYwUwqaQrHu2GMUaD9pdYcAVjzfKV File 262144
  QmYHZJzWo4UTTtsbFJU6Ls79tp8t1AxsRsG6siNefVCf17 File 262144
  QmXWXzjBB9i9NkpUEausaWPE1UoZhMfhyjVeUhZAFoHmr6 File 262144
  Qmasn9Hzj5PsMXoy7bnu7WVXMLq1jYg7uTs2eWoyVbyiE3 File 262144
  QmTwygpqpUkCq68YLPuzwVVW2QWkt7StAPW6DvK3aQTL5v File 112616

And with this commit we have:

  $ ipfs unixfs ls /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox
  QmPbjmmci73roXf9VijpyQGgRJZthiQfnEetaMRGoGYV5a File 1947624 /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox

I also reworked the argument-prefixing (object.Argument) in the output
marshaller to avoid redundancies like:

  $ ipfs unixfs ls /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox
  /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox:
  QmPbjmmci73roXf9VijpyQGgRJZthiQfnEetaMRGoGYV5a File 1947624 /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox

As a side-effect of this rework, we no longer have the trialing blank
line that we used to have after the final directory listing.

I also dropped the minute-timeout context which landed in 'ipfs ls
...' with 0a6b880, fix for #1008 and other pinning fixes, #1010).
Instead, I just pass through this command's context.

The new ErrImplementation is like Python's NotImplementedError, and is
mostly a way to guard against external changes that would need
associated updates in this code.  For example, once we see something
that's neither a file nor a directory, we'll have to update the switch
statement to handle those objects.

License: MIT
Signed-off-by: W. Trevor King <[email protected]>
ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this issue Apr 7, 2022
@whyrusleeping
fix for ipfs#1008 and other pinning fixes
99bdf19 
This commit adds a new set of sharness tests for pinning, and addresses
bugs that were pointed out by said tests.

test/sharness: added more pinning tests

Pinning is currently broken. See issue ipfs#1051. This commit introduces
a few more pinning tests. These are by no means exhaustive, but
definitely surface the present problems going on. I believe these
tests are correct, but not sure. Pushing them as failing so that
pinning is fixed in this PR.

make pinning and merkledag.Get take contexts

improve 'add' commands usage of pinning

FIXUP: fix 'pin lists look good'

ipfs-pin-stat simple script to help check pinning

This is a simple shell script to help check pinning.

We ought to strive towards making adding commands this easy.
The http api is great and powerful, but our setup right now
gets in the way. Perhaps we can clean up that area.

updated t0081-repo-pinning

- fixed a couple bugs with the tests
- made it a bit clearer (still a lot going on)
- the remaining tests are correct and highlight a problem with
  pinning. Namely, that recursive pinning is buggy. At least:
  towards the end of the test, $HASH_DIR4 and $HASH_FILE4 should
  be pinned indirectly, but they're not. And thus get gc-ed out.
  There may be other problems too.

cc @whyrusleeping

fix grep params for context deadline check

fix bugs in pin and pin tests

check for block local before checking recursive pin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@whyrusleeping whyrusleeping

Labels
kind/bug A bug in existing code (including security flaws) topic/commands Topic commands
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@hosh @jbenet @whyrusleeping @dylanPowers