add remote pinning policy for mfs #7798

petar · 2020-12-01T23:26:15Z

This builds on top of #7559

Needed for #7559 & ipfs/ipfs-gui#91
Needs ipfs/go-ipfs-config#116

Closes #7818

cmd/ipfs/daemon.go

core/commands/pin/remotepin.go

cmd/ipfs/daemon.go

gammazero

couple of minor comments

cmd/ipfs/daemon.go

fix: replace pre-existing pins for MFS root

This refactors `ipfs config replace` commands to ensure pinning service section does not break WebUI: - allow tweaking Policies via WebUI - return an error if API credentials are modified - restore API credentials if missing from input file - make it impossible to add/remove pinning services via config replace - make it impossble to proble if API.Key is valid (we return error even if API.Key does not change)

lidel · 2021-01-23T02:31:47Z

API.Key restore in config replace was nearly ok, but I had to refactor it to comply with output of ipfs config show
(webui reads config via config show and when user edits it, it is saved with config replace, so we can't break the cycle)

I cleaned up func replaceConfig a bit, should be easier to follow now + added tests and the sharness should be green (will check on Monday).

Remaining TODO:

confirm the sharness is green
add MFS policy docs to docs/config.md
test with Pinata again (and decide if we want to cleanup duplicate pins)

aschmahmann

Taking another look through the main part of this PR, but added some comments on config + API key scrubbing

core/commands/config.go

test/sharness/t0700-remotepin.sh

aschmahmann · 2021-01-25T20:44:35Z

test/sharness/t0700-remotepin.sh

 '

-test_expect_success "'ipfs config replace' injects remote services back" '
-  test_expect_code 1 grep -q -E "test_.+_svc" show_config &&
+test_expect_success "'ipfs config replace' injects Pinning.RemoteServices[*].API.Key back" '


WDYT about another test to make sure that we can change or add a policy in the config and the keys + endpoints stay the same?

cmd/ipfs/pinmfs.go

cmd/ipfs/daemon.go

cmd/ipfs/pinmfs.go

core/commands/config.go

test/sharness/t0700-remotepin.sh

aschmahmann · 2021-01-25T21:22:15Z

cmd/ipfs/daemon.go

+	pinErr := make(chan error)
+	err = pinMFSOnChange(daemonConfigPollInterval, cctx, &ipfsPinMFSNode{node}, pinErr)


I suspect we can move all of this code into core/node and referenced in the Online group in groups.go. There's no reason we need to treat this much differently then whether to start up the DHT or not.

I'm not sure if this is painful or trivial. If it's painful we can refactor in a separate PR.

aschmahmann · 2021-01-25T21:40:08Z

cmd/ipfs/pinmfs.go

+	pinTime := time.Now().UTC()
+	for ps := range lsPinCh {
+		existingRequestID = ps.GetRequestId()
+		if ps.GetPin().GetCid() == cid && ps.GetStatus() != pinclient.StatusFailed {


@lidel @petar WDYT about if the state is StatusPinning performing a connection to the Delegates? This would help us out if we submitted an MFS pin but then went offline/router blip before it was fully pinned.

aschmahmann · 2021-01-26T14:53:55Z

cmd/ipfs/pinmfs.go

+}
+
+type ipfsPinMFSNode struct {
+	node *core.IpfsNode


if we move into the constructor code then we may be able to take an interface here instead of the raw IpfsNode

aschmahmann · 2021-01-26T15:03:37Z

cmd/ipfs/pinmfs.go

+}
+
+func pinMFSOnChange(configPollInterval time.Duration, cctx pinMFSContext, node pinMFSNode, errCh chan<- error) error {
+	go func() {


This goroutine seems to be the only thing we do. Any reason not to have the function be synchronous and then call go pinMFSOnChange?

Also, this function seems pretty long/difficult to follow, perhaps we can separate out the timer + state polling logic from the update logic or separate out parts of the update logic. This may also help reduce the number of channels for us to consider at any given point.

lidel

I took it for a spin and works as expected with our reference pinning service implementation at rb-pinning-service-api. 👍

Found some minor issues around edge cases (not blockers, we can fix them in separate PRs/RC, but would be nice to include in this one if possible):

Swallowed errors from pinning service client

I also tested with Pinata and they had a small bug in message format (fix wip), which breaks our client with remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string error, which makes it retry pin over and over again, which accumulates errors in memory until I exit the process.

Those pending errors are then returned in bulk when one exits the process :

2021-01-28T14:11:16.251+0100	DEBUG	remotepinning/mfs	pinning loop is awake, 1 remote services
2021-01-28T14:11:16.251+0100	DEBUG	remotepinning/mfs	pinning considering service pinata-test for mfs pinning
2021-01-28T14:11:16.251+0100	DEBUG	remotepinning/mfs	pinning MFS root QmebJqoF9hdcmr2XwfHHv6FjQxkWSvjN266E7PPu9aZboP to pinata-test
2021-01-28T14:11:17.489+0100	DEBUG	remotepinning/mfs	pinning to pinata-test: replacing existing MFS root pin with QmebJqoF9hdcmr2XwfHHv6FjQxkWSvjN266E7PPu9aZboP
^C
Received interrupt signal, shutting down...
(Hit ctrl-c again to force-shutdown the daemon.)

Error: 36 errors occurred:
	* remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string
	* remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string
	* remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string
[...]

Pinata is working on a fix and this should not be a blocker, but I am not sure if silently swallowing this type of errors is a good idea in general.

👉 We should log those error responses via remotepinning/mfs or remotepinning/client logger to save a lot of time in debugging.

Bogus error on disabling MFS policy via `ipfs config`

This is cosmetic, but when I disable MFS policy I get this error (but it gets disabled just fine):

$ ipfs config --json Pinning.RemoteServices.rb-pinning-service.Policies.MFS.Enable true
$ ipfs config --json Pinning.RemoteServices.rb-pinning-service.Policies.MFS.Enable false
Error: failed to get config value: "Pinning.RemoteServices.rb-pinning-service.Policies.MFS key has no attributes"

petar · 2021-01-28T17:47:45Z

Fixed the swallowing issue.

# Conflicts: # core/commands/pin/remotepin.go # go.mod # go.sum

lidel

💚 config replace works fine with http://127.0.0.1:5001/webui/

💚 logger remotepinning/mfs shows errors correctly now (but as debug – I think we should change this, see below)

2021-01-28T20:45:07.412+0100	DEBUG	remotepinning/mfs	pinning MFS root QmdLUV2zPmXpZGyb3ASyxyFwP9fHJZom87KF9Lpr3bCVA9 to pinata-test error (remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string)

🤔 If there were pinning issues, I still see the accumulated errors when i interrupt ipfs daemon:

^C
Received interrupt signal, shutting down...
(Hit ctrl-c again to force-shutdown the daemon.)

Error: 5 errors occurred:
  * remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string
  * remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string
  * remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string
  * remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string
  * remote pinning service returned http error 400: json: cannot unmarshal object into Go struct field FailureError.error.details of type string

I believe printing errors in realtime is better (see below)

cmd/ipfs/pinmfs.go

lidel

🚢 RC2

* remote pinning service MFS policy * update go-ipfs-config * hardening secret sanitization in `ipfs config` commands Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com> Co-authored-by: Marcin Rataj <lidel@lidel.org>

petar requested a review from aschmahmann December 1, 2020 23:26

aschmahmann changed the base branch from master to petar/pincli December 1, 2020 23:31

aschmahmann requested changes Dec 1, 2020

View reviewed changes

cmd/ipfs/daemon.go Outdated Show resolved Hide resolved

core/commands/pin/remotepin.go Outdated Show resolved Hide resolved

cmd/ipfs/daemon.go Outdated Show resolved Hide resolved

cmd/ipfs/daemon.go Outdated Show resolved Hide resolved

aschmahmann reviewed Dec 1, 2020

View reviewed changes

cmd/ipfs/daemon.go Outdated Show resolved Hide resolved

gammazero reviewed Dec 2, 2020

View reviewed changes

cmd/ipfs/daemon.go Outdated Show resolved Hide resolved

cmd/ipfs/daemon.go Outdated Show resolved Hide resolved

cmd/ipfs/daemon.go Outdated Show resolved Hide resolved

petar force-pushed the petar/pinmfs branch from d2aad1f to 8e4aca7 Compare December 2, 2020 03:38

aschmahmann force-pushed the petar/pinmfs branch from 91f3cc0 to 3928faf Compare December 3, 2020 01:31

petar force-pushed the petar/pinmfs branch 4 times, most recently from 014cfd3 to 9864c13 Compare December 4, 2020 15:00

aschmahmann force-pushed the petar/pinmfs branch from 53ec641 to 8315aa7 Compare December 9, 2020 02:26

aschmahmann changed the base branch from petar/pincli to master December 9, 2020 02:26

This was referenced Dec 9, 2020

add support for pinning mfs ipfs/go-ipfs-config#116

Merged

Add support for remote Pinning Services #7559

Closed

lidel changed the title ~~Petar/pinmfs~~ Add remote pinning policy for MFS Dec 9, 2020

lidel changed the title ~~Add remote pinning policy for MFS~~ add remote pinning policy for mfs Dec 9, 2020

lidel requested changes Dec 9, 2020

View reviewed changes

cmd/ipfs/daemon.go Outdated Show resolved Hide resolved

petar and others added 11 commits December 9, 2020 09:35

remote pinning service MFS policy

d9095bc

comment

d9d2c56

fixes

5e7399b

test mfs is being pinned

fa157b6

add debug logging

1cf6f73

add MFSRepinInterval parameter

cb93750

explicitly set repin interval in tests

97309c4

logging

079ec5c

fix repinning logic

7bfda0f

fix outdated pinning config names

7ba059e

update go-ipfs-config

07a4939

aschmahmann force-pushed the petar/pinmfs branch from 1d8ff71 to 07a4939 Compare December 9, 2020 14:35

aschmahmann and others added 2 commits January 21, 2021 13:00

Merge pull request #7864 from ipfs/fix/replace-duplicate-mfs-pins

58898cc

fix: replace pre-existing pins for MFS root

aschmahmann requested changes Jan 25, 2021

View reviewed changes

document matchesGlobPrefix

1d650c6

gammazero reviewed Jan 26, 2021

View reviewed changes

aschmahmann requested changes Jan 26, 2021

View reviewed changes

petar and others added 9 commits January 26, 2021 08:22

fix error messages

33d6c02

style

2d8338d

style+cleanup

0564c0b

add test

99d87b0

fix

ad3d8cd

pull "go" outside of pinMFSOnChange

d1873f4

break up pinMFSOnChange; add pinAllMFS

eba0076

fix sharness

f132ffe

use mfs pinning specific logger and log with Debugf instead of Infof

8ba35b6

lidel requested changes Jan 28, 2021

View reviewed changes

log all pinning errors

dc56cf5

aschmahmann added 2 commits January 28, 2021 14:21

bump go-ipfs-config to remove omitempty from MFS config options

c8fefbd

Merge branch 'master' into petar/pinmfs

7e385a8

# Conflicts: # core/commands/pin/remotepin.go # go.mod # go.sum

lidel requested changes Jan 28, 2021

View reviewed changes

cmd/ipfs/pinmfs.go Outdated Show resolved Hide resolved

petar and others added 3 commits January 28, 2021 13:19

consume mfs pinning errors in a separate thread

36827e0

fix dup logs

b87597d

update go-ipfs-config

453d240

aschmahmann approved these changes Jan 28, 2021

View reviewed changes

lidel approved these changes Jan 28, 2021

View reviewed changes

aschmahmann merged commit 8794928 into master Jan 28, 2021

aschmahmann deleted the petar/pinmfs branch January 28, 2021 23:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add remote pinning policy for mfs #7798

add remote pinning policy for mfs #7798

petar commented Dec 1, 2020 •

edited by lidel

Loading

gammazero left a comment •

edited

Loading

lidel commented Jan 23, 2021 •

edited

Loading

aschmahmann left a comment

aschmahmann Jan 25, 2021

aschmahmann Jan 25, 2021

aschmahmann Jan 25, 2021

aschmahmann Jan 26, 2021

aschmahmann Jan 26, 2021

lidel left a comment •

edited

Loading

petar commented Jan 28, 2021

lidel left a comment •

edited

Loading

lidel left a comment

		pinErr := make(chan error)
		err = pinMFSOnChange(daemonConfigPollInterval, cctx, &ipfsPinMFSNode{node}, pinErr)

add remote pinning policy for mfs #7798

add remote pinning policy for mfs #7798

Conversation

petar commented Dec 1, 2020 • edited by lidel Loading

gammazero left a comment • edited Loading

Choose a reason for hiding this comment

lidel commented Jan 23, 2021 • edited Loading

aschmahmann left a comment

Choose a reason for hiding this comment

aschmahmann Jan 25, 2021

Choose a reason for hiding this comment

aschmahmann Jan 25, 2021

Choose a reason for hiding this comment

aschmahmann Jan 25, 2021

Choose a reason for hiding this comment

aschmahmann Jan 26, 2021

Choose a reason for hiding this comment

aschmahmann Jan 26, 2021

Choose a reason for hiding this comment

lidel left a comment • edited Loading

Choose a reason for hiding this comment

Swallowed errors from pinning service client

Bogus error on disabling MFS policy via ipfs config

petar commented Jan 28, 2021

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

petar commented Dec 1, 2020 •

edited by lidel

Loading

gammazero left a comment •

edited

Loading

lidel commented Jan 23, 2021 •

edited

Loading

lidel left a comment •

edited

Loading

Bogus error on disabling MFS policy via `ipfs config`

lidel left a comment •

edited

Loading