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.

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

fix pinning #1010

Merged
merged 2 commits into from
Apr 20, 2015
Merged

fix pinning #1010

merged 2 commits into from
Apr 20, 2015

Conversation

whyrusleeping
Copy link
Member

Addresses #1008

@whyrusleeping whyrusleeping added status/in-progress In progress kind/bug A bug in existing code (including security flaws) topic/commands Topic commands labels Apr 4, 2015
@whyrusleeping whyrusleeping self-assigned this Apr 4, 2015
@whyrusleeping whyrusleeping added codereview and removed status/in-progress In progress labels Apr 4, 2015
@jbenet
Copy link
Member

jbenet commented Apr 4, 2015

@whyrusleeping could you add a test case for this? it's a serious problem, if it re-emerges. would be good to protect against it.

@whyrusleeping
Copy link
Member Author

@jbenet I think trying to write a test case where a pin fails halfway through is going to be more trouble than its worth. We have no easy (or even 'somewhat easy') way of reliably killing a daemon halfway through a 'pin' operation. Its another timing nightmare test waiting to happen

@jbenet
Copy link
Member

jbenet commented Apr 7, 2015

@whyrusleeping can't we:

  • create an object A that links to one object B that exists locally and one that doesn't C
  • run the pin
  • either kill it 1s later (yes, timing problem), or wait until it times out (we maybe could lower the timeout with an env var or something)
  • check the pinning result

this is probably useful to test what happens when we try pinning things that arent around, too. could test both pinning A and C

@jbenet
Copy link
Member

jbenet commented Apr 11, 2015

@whyrusleeping added tests that surface the problems

@whyrusleeping whyrusleeping changed the title fix for #1008 fix pinning Apr 11, 2015
@whyrusleeping
Copy link
Member Author

@jbenet after reading through the add command, it looks like we pin each file of a recursive add recursively, so:

dir/   <- pinned -r
    a  <- pinned -r
    b  <- pinned -r

But running ipfs pin rm -r dir-hash only runs Unpin-recursive(dirhash). leaving the children still pinned recursively. what is the expected behaviour here?

@jbenet jbenet mentioned this pull request Apr 13, 2015
Closed
32 tasks
@jbenet
Copy link
Member

jbenet commented Apr 13, 2015

  • fixed pin lists look good
  • some objects are still there exits with Error: context deadline exceeded ?? looks like gc is resolving objects instead of just checking pins? oh i guess it needs to resolve objects?

@jbenet
Copy link
Member

jbenet commented Apr 14, 2015

@whyrusleeping pushed more to the test case. i'm pretty sure this found problems with pinning.

@whyrusleeping whyrusleeping force-pushed the fix/pin-bug branch 2 times, most recently from dae68c3 to a66a39c Compare April 19, 2015 00:48
@whyrusleeping
Copy link
Member Author

@jbenet this is RFM

@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
@whyrusleeping
functions dont need to be called functions
ef56f4f
jbenet added a commit that referenced this pull request Apr 20, 2015
@jbenet
Merge pull request #1010 from ipfs/fix/pin-bug
b483fdc 
fix pinning
@jbenet jbenet merged commit b483fdc into master Apr 20, 2015
@jbenet jbenet removed the codereview label Apr 20, 2015
@jbenet jbenet deleted the fix/pin-bug branch April 20, 2015 07:21
wking added a commit that referenced this pull request 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]>
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
@jbenet
Merge pull request ipfs/kubo#1010 from ipfs/fix/pin-bug
ca9203b 
fix pinning

This commit was moved from ipfs/kubo@b483fdc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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

Successfully merging this pull request may close these issues.
3 participants
@whyrusleeping @jbenet @chriscool