add remote pinning to ipfs command

[petar]

Part of #7559

Ref.

• Pinning Service API: https://ipfs.github.io/pinning-services-api-spec/
  • go client: https://github.com/ipfs/go-pinning-service-http-client
  • demo server: https://github.com/ipfs-shipyard/rb-pinning-service-api
• Epic: pinning service integration in WebUI: Epic: Pinning service integration ipfs-gui#91, epic: pinning services ipfs-webui#1615

[lidel]

Thank you for starting this. Quick feedback:

• Needs to be updated to https://github.com/ipfs/pinning-services-api-spec/releases/tag/v0.1.0
  • id is renamed to requestid now, for sake fo consistency go-ipfs should replace all occurences of ID with RequestID

[lidel]

@petar is the API under /api/v0/pin/remote/* stable enough to start implementation in js-ipfs-http-client (ipfs/js-ipfs#3293) in parallel to this PR?

cc @Gozala @rafaelramalho19

[petar]

@petar is the API under /api/v0/pin/remote/* stable enough to start implementation in js-ipfs-http-client (ipfs/js-ipfs#3293) in parallel to this PR?

cc @Gozala @rafaelramalho19

@lidel Yes. The additional API for configuring remote services "/pin/remote/service" hasn't been reviewed by others carefully yet. Feel free to criticize it if you get to it. It meets @aschmahmann 's considerations, I believe.

[aschmahmann]

@lidel I haven't had a chance to look at the pin remote service API yet, but will take a look later today.

The rest of the pin remote API should be reasonably stable, although our call on Friday is designed to give that a second pass to make sure we're happy with how it looks and don't want to rename and commands, arguments or options. I suspect any changes that come out of that meeting will be fairly cosmetic though.

[lidel]

@Gozala thanks for feedback! Quick thoughts:

• pushing requestID to options in JS sgtm. but I think that does not impact this PR and will be scoped to feat: support remote pinning services in ipfs-http-client js-ipfs#3293?
  (if so, we should move JS-specific design decisions to that PR)

• Is there reason why name can't be an array of possible matches?

  No strong reason, mostly to keep MVP simple and aligned with upstream API:

  • it is not an array in Pinning Service API, so would require multiple requests here
  • we support comma-separated notation --cid=Qm1,Qm2, in case of name it would not be possible, or would break for names that include ,

• It is also kind of odd that you can ls / rm by requestID + the other options

  When requestID is passed, direct lookup is performed and due to this options are ignored (otherwise we would have to duplicate work and implement filtering in the client).
  @aschmahmann Perhaps the fix here is to return an error when both requestID and query filters are passed, informing user that its either one or another?

• is there good reason to expose a requestId to the user ?

  To make --background=true work and be able to check ongoing pinning status later.
  If you have an idea how we can deliver that without exposing "PinID" (aka "RequestID") this is a good time to write it down.

[Gozala]

• pushing requestID to options in JS sgtm. but I think that does not impact this PR and will be scoped to ipfs/js-ipfs#3293?
  (if so, we should move JS-specific design decisions to that PR)

Well problem is that it would also affect JS CLI which in turn would be bound to the implementation or will have to take a more custom approach

[Gozala]

• When requestID is passed, direct lookup is performed and due to this options are ignored (otherwise we would have to duplicate work and implement filtering in the client).
  @aschmahmann Perhaps the fix here is to return an error when both requestID and query filters are passed, informing user that its either one or another?

Sounds to me that maybe it should be a stat command instead

[aschmahmann]

Well problem is that it would also affect JS CLI which in turn would be bound to the implementation or will have to take a more custom approach

I thought JS didn't have as strong coupling between the CLI and HTTP API as Go did, am I misunderstanding? Fundamentally Go, JS, and Bash all have different each have their own semantics about what's preferred so moving around some things on the interface in JS seems reasonable unless it's a big pain. For example, what happens when we want to extend the number of services to be 1 or more? That's not easily implemented via just a ? in JS anyway.

@aschmahmann Perhaps the fix here is to return an error when both requestID and query filters are passed, informing user that its either one or another?

🤷 I think both options are acceptable. I'm pretty sure we discussed this and @Gozala's suggestion

Sounds to me that maybe it should be a stat command instead

but decided not to since we might want stat for something else and we already have some user expectations via ipfs pin ls <ipfs-path> which is somewhat similar in design.

[lidel]

While discussing issues raised by @Gozala we took a quick step back and looked at pin remote and potential use of requestid.

Below are quick notes from our call, so we have our rationale documented.

With requestid 💫🤔

pin remote rm | ls is confusing because it operates in two modes:

• direct requestid match (which ignores filters like cid,name and status)
• search query (returns matches for passed filters: cid,name and status)

Potential fixes:

1. extract "direct match" to separate pin remote stat <requestid>
   • pro: follows existing convention of dag stat and files stat
   • con: does not solve pin remote rm <requestid> --name=foo
2. return error when both requestid and filters are provided
   • pro: easy and quick fix that works for both ls and rm
   • con: ? (we still have overcomplicated api)

Without requestid 💚

What if we make requestid an internal implementation detail and remove it from pin remote command outputs and filters? (at least for MVP)

pin remote rm | ls already demonstrate we can live without it by accepting search filters such as --cid= and --name

• pros:
  • lower cognitive overhead: pin remote API does not import and expose any third-party opaque identifiers into our api
  • api is simpler and easier to backport to pin local
  • does not lock us into decisions made for (somehow provisional) Pinning Services API
• cons:
  • requires a small refactor (mostly removal of code)

Q&A

Q: how to differentiate between two pins for the same cid
A: use different name

Q: what if unique name is not provided?
A: (now - MVP) just create duplicate (all duplicates can be removed with pin remote rm --cid -f)
A: (idea) we could improve ergonomics by checking for cid+name duplicates, and reuse existing one (this implies cid+name as the de facto unique index for both local and remote pinning - feasibility TBD)

Decision

Remove requestid (and other unused fields) from API output for now.

[Gozala]

One more thing I've noticed with ipfs pin remote service ls:

ipfs pin remote service ls --pin-count 
mock http://localhost:8080 1/0/0/0
test http://localhost:8080 offline

Same call over the HTTP endpoint

curl -X POST 'http://127.0.0.1:5002/api/v0/pin/remote/service/ls?pin-count=true'
{
  "RemoteServices": [
    {
      "Service": "mock",
      "ApiEndpoint": "http://localhost:8080",
      "PinCount": {
        "Queued": 1,
        "Pinning": 0,
        "Pinned": 0,
        "Failed": 0
      }
    },
    {
      "Service": "test",
      "ApiEndpoint": "http://localhost:8080"
    }
  ]
}

Note that in CLI one can tell that service is not reachable, while over HTTP only clue is lack of PinCount. I was wondering maybe we could do better here by renaming --pin-count to --stat and turn the output of HTTP response to following instead:

curl -X POST 'http://127.0.0.1:5002/api/v0/pin/remote/service/ls?stat=true'
{
  "RemoteServices": [
    {
      "Service": "mock",
      "ApiEndpoint": "http://localhost:8080",
      "Stat": {
        "Status": "Online",
        "PinCount": {
          "Queued": 1,
          "Pinning": 0,
          "Pinned": 0,
          "Failed": 0
        }
      }
    },
    {
      "Service": "test",
      "ApiEndpoint": "http://localhost:8080"
      "Stat": {
        "Status": "Offline"
       }
    }
  ]
}

[Gozala]

Oh and also "online" | "offline" may no be a good working here, maybe "available" | "unavailable" or "reachable" | "unreachable" might be a better way to talk about it. Because node may be offline instead of service or maybe service is unreachable because of network fragmentation.

[lidel]

@Gozala good feedback, thanks!
Applied your suggestions (went with shorter statuses: valid|invalid|unknown for now)

Without --stat all services are in unknown state:

{
  "RemoteServices": [
    {
      "Service": "test-valid",
      "ApiEndpoint": "https://testapi1.example.com/psa",
      "Stat": {
        "Status": "unknown"
      }
    },
    {
      "Service": "test-invalid",
      "ApiEndpoint": "https://testapi2.example.com/psa",
      "Stat": {
        "Status": "unknown"
      }
    }
  ]
}

With --stat we see valid and invalid ones:

{
  "RemoteServices": [
    {
      "Service": "test-valid",
      "ApiEndpoint": "https://testapi1.example.com/psa",
      "Stat": {
        "Status": "valid",
        "PinCount": {
          "Queued": 0,
          "Pinning": 0,
          "Pinned": 2,
          "Failed": 0
        }
      }
    },
    {
      "Service": "test-invalid",
      "ApiEndpoint": "https://testapi2.example.com/psa",
      "Stat": {
        "Status": "invalid"
      }
    }
  ]
}

[Gozala]

Thanks @lidel! Do you think it would make sense to only include Stat field if --stat isn't passed ?

[lidel]

@Gozala sgtm – changed in 32f8950 (#7661):

Without --stat :

{
  "RemoteServices": [
    {
      "Service": "test-valid",
      "ApiEndpoint": "https://testapi1.example.com/psa"
    },
    {
      "Service": "test-invalid",
      "ApiEndpoint": "https://testapi2.example.com/psa"
    }
  ]
}