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 #732: Introduce native pin/update #869

Merged
merged 4 commits into from Aug 12, 2019

Conversation

@hsanjuan
Copy link
Collaborator

commented Aug 6, 2019

This introduces a pin/update operation which allows to Pin a new item to
cluster indicating that said pin is an update to an already-existing pin.

When this is the case, all the configuration for the existing pin is copied to
the new one (including allocations). The IPFS connector will then trigger
pin/update directly in IPFS, allowing an efficient pinning based on
DAG-differences. Since the allocations where the same for both pins,
the pin/update can proceed.

PinUpdate does not unpin the previous pin (it is not possible to do this
atomically in cluster like it happens in IPFS). The user can manually do it
after the pin/update is done.

Internally, after a lot of deliberations on what the optimal way for this is,
I opted for adding a PinUpdate option to the PinOptions type (carries the
CID to update from). In order to carry this option from the REST API to the
IPFS Connector, it is serialized in the Protobuf (and stored in the
datastore). There is no other way to do this in a simple fashion since the Pin
object is piece of information that is sent around.

Additionally, making it a PinOption plays well with the Pin/PinPath APIs which
need little changes. Effectively, you are pinning a new thing. You are just
indicating that it should be configured from an existing one.

Fixes #732

@hsanjuan hsanjuan requested review from kishansagathiya and Stebalien Aug 6, 2019
@hsanjuan hsanjuan marked this pull request as ready for review Aug 6, 2019
@hsanjuan

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2019

@Stebalien you said you wanted to review sometimes... this is an interesting one by the number of places it touches...

@hsanjuan hsanjuan self-assigned this Aug 6, 2019
cmd/ipfs-cluster-ctl/main.go Show resolved Hide resolved
ipfsconn/ipfshttp/ipfshttp_test.go Show resolved Hide resolved
ipfsconn/ipfshttp/ipfshttp_test.go Show resolved Hide resolved
@@ -88,6 +88,14 @@ test_expect_success IPFS,CLUSTER "pin data to cluster with user allocations" '
echo $allocations | grep -q ${pid}
'

test_expect_success IPFS,CLUSTER "pin update a pin" '
cid1=`docker exec ipfs sh -c "echo test | ipfs add -q"`
ipfs-cluster-ctl pin add "$cid1"

This comment has been minimized.

Copy link
@kishansagathiya

kishansagathiya Aug 9, 2019

Member

We can't replace these two lines with echo test | ipfs-cluster-ctl add -q. Would it be helpful to have that someday?

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Aug 9, 2019

Author Collaborator

Yes, it would be useful. New feature request?

@hsanjuan hsanjuan force-pushed the fix/732-pin-update-the-good-way branch from 81e6365 to 9715435 Aug 9, 2019
@kishansagathiya

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

This is good to go after make check is fixed

@hsanjuan

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 9, 2019

ouch there's a bug, not all tests are being run

hsanjuan added 3 commits Jul 12, 2019
This introduces a pin/update operation which allows to Pin a new item to
cluster indicating that said pin is an update to an already-existing pin.

When this is the case, all the configuration for the existing pin is copied to
the new one (including allocations). The IPFS connector will then trigger
pin/update directly in IPFS, allowing an efficient pinning based on
DAG-differences. Since the allocations where the same for both pins,
the pin/update can proceed.

PinUpdate does not unpin the previous pin (it is not possible to do this
atomically in cluster like it happens in IPFS). The user can manually do it
after the pin/update is done.

Internally, after a lot of deliberations on what the optimal way for this is,
I opted for adding a `PinUpdate` option to the `PinOptions` type (carries the
CID to update from). In order to carry this option from the REST API to the
IPFS Connector, it is serialized in the Protobuf (and stored in the
datastore). There is no other way to do this in a simple fashion since the Pin
object is piece of information that is sent around.

Additionally, making it a PinOption plays well with the Pin/PinPath APIs which
need little changes. Effectively, you are pinning a new thing. You are just
indicating that it should be configured from an existing one.

Fixes #732
@hsanjuan hsanjuan force-pushed the fix/732-pin-update-the-good-way branch from 2c7d078 to fb2d427 Aug 9, 2019
@hsanjuan

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 9, 2019

I will rewrite the pin/update test in upcoming PR

This was referenced Aug 9, 2019
@hsanjuan hsanjuan requested a review from kishansagathiya Aug 9, 2019
Copy link
Member

left a comment

Test failure (could be intermittent seems consistent) https://travis-ci.com/ipfs/ipfs-cluster/jobs/223951134#L487

error could be different, say waiting for leader, context cancelled or waiting to become a Voter, context cancelled (but consensus related)
How is a context with no deadline or cancelFunc getting cancelled?

@hsanjuan hsanjuan merged commit d8c20ad into master Aug 12, 2019
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
@hsanjuan hsanjuan deleted the fix/732-pin-update-the-good-way branch Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.