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

Removing or listing pins gives inconsistent results between cluster CLI and proxied API. #380

Closed
hickscorp opened this issue Apr 13, 2018 · 7 comments
Assignees
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up

Comments

@hickscorp
Copy link

hickscorp commented Apr 13, 2018

We have two test clusters each talking to its individual IPFS node. When using ipfs-cluster-ctl on either one of the clusters, we're able to pin and unpin, and then when issuing a ipfs-cluster-ctl pin ls on the other cluster we see that it propagated. We know the clusters are communicative.

However, here is something very weird that happens. Using the proxied API, we list pins, we get:

%{"Keys" => %{"QmQHmGya11K65DyjsaGTfi9s5LXrCZT2AF4H3JkBBNAZma" => %{"Type" => "recursive"}}}

So far so good, and verified using ipfs-cluster-ctrl pin ls too. Then we issue an unpinning order on proxied :9095. This is what we get:

%{"Pins" => ["QmQHmGya11K65DyjsaGTfi9s5LXrCZT2AF4H3JkBBNAZma"]}

Brilliant. Now for sanity check, we issue the same pin rm on the proxied API:

Error status code: 500, {\"Message\":\"not pinned\",\"Code\":0}\n

Perfect, expected. Let's verify the pinning status using the cli tool:

ipfs-cluster-ctl pin ls
QmQHmGya11K65DyjsaGTfi9s5LXrCZT2AF4H3JkBBNAZma |  | Repl. Factor: -1 | Allocations: [everywhere]

Oops. Examining the cluster logs doesn't show unpinning either!

ipfs-cluster-two_1  | 15:14:27.538  INFO  consensus: Current Raft Leader: Qmc6H2UwRty245dRnSNJs2CANZHmZU92kVsLMhwenjB7c5 raft.go:271
ipfs-cluster-two_1  | 15:14:27.638  INFO    cluster: Cluster Peers (without including ourselves): cluster.go:461
ipfs-cluster-two_1  | 15:14:27.638  INFO    cluster:     - QmeybQebcM9LwyyHcg7RM3ZbAgQmXKyQEGRCmHJRxycdUT cluster.go:467
ipfs-cluster-two_1  | 15:14:27.638  INFO    cluster: ** IPFS Cluster is READY ** cluster.go:472
ipfs-cluster-one_1  | 15:14:27.787  INFO  consensus: Current Raft Leader: Qmc6H2UwRty245dRnSNJs2CANZHmZU92kVsLMhwenjB7c5 raft.go:271
ipfs-cluster-one_1  | 15:14:27.787  INFO    cluster: Cluster Peers (without including ourselves): cluster.go:461
ipfs-cluster-one_1  | 15:14:27.787  INFO    cluster:     - Qmc6H2UwRty245dRnSNJs2CANZHmZU92kVsLMhwenjB7c5 cluster.go:467
ipfs-cluster-one_1  | 15:14:27.787  INFO    cluster: ** IPFS Cluster is READY ** cluster.go:472
ipfs-cluster-two_1  | 15:14:32.639  INFO    cluster: peerset change detected cluster.go:393
ipfs-cluster-one_1  | 15:14:32.788  INFO    cluster: peerset change detected cluster.go:393
ipfs-cluster-two_1  | 15:14:33.522  INFO     config: Saving configuration config.go:317
ipfs-cluster-one_1  | 15:14:33.768  INFO     config: Saving configuration config.go:317
ipfs-cluster-one_1  | 15:16:43.400 ERROR    cluster: no state has been agreed upon yet cluster.go:985
ipfs-cluster-one_1  | 15:16:48.980 ERROR    cluster: no state has been agreed upon yet cluster.go:985
ipfs-cluster-one_1  | 15:16:53.825  INFO    cluster: IPFS cluster pinning QmQHmGya11K65DyjsaGTfi9s5LXrCZT2AF4H3JkBBNAZma everywhere: cluster.go:1068
ipfs-cluster-two_1  | 15:16:53.834  INFO  consensus: pin committed to global state: QmQHmGya11K65DyjsaGTfi9s5LXrCZT2AF4H3JkBBNAZma consensus.go:284
ipfs-cluster-one_1  | 15:16:53.957  INFO   ipfshttp: IPFS Pin request succeeded: QmQHmGya11K65DyjsaGTfi9s5LXrCZT2AF4H3JkBBNAZma ipfshttp.go:632
ipfs-cluster-two_1  | 15:16:53.964  INFO   ipfshttp: IPFS Pin request succeeded: QmQHmGya11K65DyjsaGTfi9s5LXrCZT2AF4H3JkBBNAZma ipfshttp.go:632

What to believe? How to reliably pin ls using the :9095 proxy?

@hickscorp hickscorp changed the title Pin statuses are wrong when comparing them with the IPFS daemon API ones. Removing or listing pins gives inconsistent results between cluster CLI and proxied API. Apr 13, 2018
@hsanjuan hsanjuan added kind/bug A bug in existing code (including security flaws) exp/intermediate Prior experience is likely helpful status/ready Ready to be worked P1 High: Likely tackled by core team if no one steps up labels Apr 15, 2018
@hsanjuan
Copy link
Collaborator

I think you didn't issue your unpin against :9095 but against :5001 instead.

I know because cluster never returns: Error status code: 500, {\"Message\":\"not pinned\",\"Code\":0}\n. Cluster always returns a successful unpin, even if the content wasn't pinned.

Can you double check it? I can't reproduce this.

@hickscorp
Copy link
Author

@hsanjuan I'll test ASAP - but I am pretty positive all requests were performed on 9095, the reason being that I'm not doing the requests - a piece of software of ours is, and is configured once and for all at boot time with the IPFS details (Host, port). I'll report back.

@hickscorp
Copy link
Author

hickscorp commented Apr 16, 2018

Good afternoon,

I apologise as I had to take care of personal things and couldn't report before.
I am confirming that the pinning registers when done through 9095, but doesn't register in the cluster when performed through 9095. Proof in the following logs:

EDIT and possible solution: I discovered that the proxy catches the pins when the CID is passed in ?arg=:cid instead of a path component. It's absolutely fine in the sense that IPFS doc states that the the CID should be in a query variable, what puzzles me now is the fact that it also accepts /pin/add/cid. I guess this issue could be closed.

iex(one@127.0.0.1)1> alias App.IPFS, as: IPFS
IPFS
iex(one@127.0.0.1)2> %IPFS{}   
%IPFS{base: "api/v0", host: "localhost", port: 9095, scheme: "http"}
iex(one@127.0.0.1)3> conn |> IPFS.pin_ls()
{:ok, %{"Keys" => %{}}}
iex(one@127.0.0.1)4> conn |> IPFS.add("/home/doodloo/Pictures/Very Nice Great Success.jpg")
{:ok,
 %{
   "Hash" => "QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3",
   "Name" => "QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3",
   "Size" => "44722"
 }}
iex(one@127.0.0.1)5> conn |> IPFS.pin_ls()                                                 
{:ok,
 %{
   "Keys" => %{
     "QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3" => %{
       "Type" => "recursive"
     }
   }
 }}
iex(one@127.0.0.1)6> conn |> IPFS.pin_rm("QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3")
{:ok, %{"Pins" => ["QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3"]}}
iex(one@127.0.0.1)7> conn |> IPFS.pin_rm("QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3")
{:error, "Error status code: 500, {\"Message\":\"not pinned\",\"Code\":0}\n"}
iex(one@127.0.0.1)8> conn |> IPFS.pin_ls()                                                
{:ok,
 %{
   "Keys" => %{
     "QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3" => %{
       "Type" => "recursive"
     }
   }
 }}

At this point, pinning statuses start to differ like I stated in my original post. You will notice that the client uses the same connection all the way through (conn), which I debugged at the beginning so you can see the that it indeed hits 9095.

Here are the associated logs from the cluster:

ipfs-cluster-one_1  | 13:52:13.406 ERROR    cluster: no state has been agreed upon yet cluster.go:985
ipfs-cluster-one_1  | 13:52:39.381  INFO    cluster: IPFS cluster pinning QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3 on [<peer.ID eybQeb> <peer.ID c6H2Uw>]: cluster.go:1070
ipfs-cluster-one_1  | 13:52:39.386  INFO  consensus: pin committed to global state: QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3 consensus.go:284
ipfs-cluster-one_1  | 13:52:39.414  INFO   ipfshttp: IPFS Pin request succeeded: QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3 ipfshttp.go:632
ipfs-cluster-two_1  | 13:52:39.484  INFO   ipfshttp: IPFS Pin request succeeded: QmQChpnJJ5am4HRiW7b1KZtBEBeWy3azovVMCL3xsFVUL3 ipfshttp.go:632

As a side note, I have to very strongly advocate against having IPFS cluster require the developer to hit something else than the proxy - an IPFS client should be completely agnostic to whether or not it's talking to IPFS or IPFS cluster, and the pinning / unpinning magic should be done automatically IMO.

Maybe the way we pin isn't caught by the proxy - we use GET /pin/add/:cid and /pin/rm/:cid. Is that compatible with what the proxy should intercept?

@ghost ghost removed the status/ready Ready to be worked label Apr 16, 2018
@hsanjuan
Copy link
Collaborator

@hickscorp did you close this for some reason?

OK, I know the problem now. Cluster intercepts /api/v0/pin/add?arg=<cid> (or rm) (which was the documented api endpoint when this was done and what go-ipfs cli uses). We'll have to add support when the argument is passed as an extra /<arg> (this is for all commands). @lanzafame can you take care of this?

Relevant code: https://github.com/ipfs/go-ipfs-cmds/blob/master/http/parse.go#L42

I have to very strongly advocate against having IPFS cluster require the developer to hit something else than the proxy - an IPFS client should be completely agnostic to whether or not it's talking to IPFS or IPFS cluster

The IPFS API does not support nor play well with cluster functionality. The proxy is provided as convenience to help transition to cluster on on side, but it's mostly to enable composition.

@hsanjuan hsanjuan added exp/novice Someone with a little familiarity can pick up status/ready Ready to be worked and removed exp/intermediate Prior experience is likely helpful needs feedback from submitter labels Apr 17, 2018
@hsanjuan hsanjuan reopened this Apr 17, 2018
@hickscorp
Copy link
Author

hickscorp commented Apr 17, 2018

@hsanjuan I understand that the IPFS API does not support or play well with cluster functionality. I'm talking about the other way around - that a cluster is an overlay on top of IPFS, and that the proxying feature should receive top notch support, because it would allow IPFS cluster to be a drop-in replacement of IPFS for software that hit the IPFS API without any changes to these softwares.
Hope I'm making more sense ;)

@hsanjuan
Copy link
Collaborator

@hickscorp yes, we agree :)

@lanzafame lanzafame self-assigned this Apr 18, 2018
@hsanjuan
Copy link
Collaborator

hsanjuan commented May 1, 2018

Fixed in #392

@hsanjuan hsanjuan closed this as completed May 1, 2018
@ghost ghost removed the status/ready Ready to be worked label May 1, 2018
@hsanjuan hsanjuan mentioned this issue May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

No branches or pull requests

3 participants