api,client,http,main,prod,storage: Add query param for publisher and solve flooding in recovery #2202

santicomp2014 · 2020-06-11T11:13:02Z

This PR resolves request flooding by only triggering content repair for the chunks related to the requested hash, as described in #2205.

The publisher is passed from query params or by SwarmPublisher on download by CLI.

It also removes Publisher from the manifest as it's not needed now.

The bzz-list is modified to receive the publisher in the query so it can also get the manifest before the actual download takes place. If this is not done the Download in CLI will fail as its first step is to issue a List command.

closes #2193
closes #2205

…on download cli and query param for download; injected publisher into ctx

… from manifest as its passed along ctx in the download path

…ry purposes

mortelli

no major issues, i think.

the PR branch is behind the base branch by 1 commit, don't forget to update it and re-do the tests.

api/client/client.go

mortelli · 2020-06-12T17:25:44Z

api/client/client.go

+func (c *Client) List(hash, prefix, credentials, publisher string) (*api.ManifestList, error) {
+	uri := c.Gateway + "/bzz-list:/" + hash + "/" + prefix
+	if publisher != "" {
+		uri = uri + "?publisher=" + publisher
+	}
+	req, err := http.NewRequest(http.MethodGet, uri, nil)


could you please briefly comment on why you needed to make these changes to the List func?

Sure, I will add to the description of the PR.
But to resume as the download command is invoked
first, the List is issued and this will fail if the contents are GC'd

thank you!

i would like to ask if @zelig has any comments regarding this change.

cmd/swarm/flags.go

prod/prod.go

storage/netstore.go

mortelli · 2020-06-12T17:35:53Z

storage/netstore.go

@@ -201,7 +205,8 @@ func (n *NetStore) Get(ctx context.Context, mode chunk.ModeGet, req *Request) (c
 			if ok {
 				ch, err = n.RemoteFetch(ctx, req, fi)
 				if err != nil {
-					if n.recoveryCallback != nil {
+					if n.recoveryCallback != nil && publisher != "" {
+						log.Debug("gp netstore recovery triggered")
 						n.recoveryCallback(ctx, ref)
 						time.Sleep(500 * time.Millisecond) // TODO: view what the ideal timeout is


did you try measuring how long this actually takes?

i think we should either remove this retry, or use a Sleep amount which could realistically work (500 is too little for even 3 nodes).

we can also look into outputting a "recovery triggered, retry later" message into the CLI, although this could be done in a different issue

relevant: #2172

It takes around 2-3 seconds which is more than this Sleep.
The problem is that a 3-second delay will halt the execution and also it can differ on other setups.

I would prefer to remove this and emit a message with a status, so the client can retry at their own retry interval.
What do you think @zelig?

in the book i resurrected error code 420 ('enhance your calm' of twitter fame) to be returned if recovery was initiated. Indeed the client could then just retry

in the book i resurrected error code 420 ('enhance your calm' of twitter fame) to be returned if recovery was initiated. Indeed the client could then just retry

love this idea

…nd fixed naming issues

santicomp2014

I will pull and update as well

api/client/client.go

santicomp2014 · 2020-06-12T17:53:12Z

api/client/client.go

+func (c *Client) List(hash, prefix, credentials, publisher string) (*api.ManifestList, error) {
+	uri := c.Gateway + "/bzz-list:/" + hash + "/" + prefix
+	if publisher != "" {
+		uri = uri + "?publisher=" + publisher
+	}
+	req, err := http.NewRequest(http.MethodGet, uri, nil)


Sure, I will add to the description of the PR.
But to resume as the download command is invoked
first, the List is issued and this will fail if the contents are GC'd

cmd/swarm/flags.go

prod/prod.go

storage/netstore.go

santicomp2014 · 2020-06-12T18:06:54Z

storage/netstore.go

@@ -201,7 +205,8 @@ func (n *NetStore) Get(ctx context.Context, mode chunk.ModeGet, req *Request) (c
 			if ok {
 				ch, err = n.RemoteFetch(ctx, req, fi)
 				if err != nil {
-					if n.recoveryCallback != nil {
+					if n.recoveryCallback != nil && publisher != "" {
+						log.Debug("gp netstore recovery triggered")
 						n.recoveryCallback(ctx, ref)
 						time.Sleep(500 * time.Millisecond) // TODO: view what the ideal timeout is


It takes around 2-3 seconds which is more than this Sleep.
The problem is that a 3-second delay will halt the execution and also it can differ on other setups.

I would prefer to remove this and emit a message with a status, so the client can retry at their own retry interval.
What do you think @zelig?

santicomp2014 · 2020-06-12T18:10:55Z

Updated with Base branch

zelig · 2020-06-13T09:05:09Z

storage/netstore.go

@@ -201,7 +205,8 @@ func (n *NetStore) Get(ctx context.Context, mode chunk.ModeGet, req *Request) (c
 			if ok {
 				ch, err = n.RemoteFetch(ctx, req, fi)
 				if err != nil {
-					if n.recoveryCallback != nil {
+					if n.recoveryCallback != nil && publisher != "" {
+						log.Debug("gp netstore recovery triggered")
 						n.recoveryCallback(ctx, ref)
 						time.Sleep(500 * time.Millisecond) // TODO: view what the ideal timeout is


in the book i resurrected error code 420 ('enhance your calm' of twitter fame) to be returned if recovery was initiated. Indeed the client could then just retry

cmd/swarm/flags.go

api/client/client.go

…to Query().Add, return 402 when recovery attempt is being processed

…llback to async

…l return recovery attempt instead and should not be absorbed

mortelli

nice, almost there.

make sure to switch 402 status with 420 and i think we will be good to go.

have you manually tested this current implementation for both down and curl?

api/client/client.go

api/http/server.go

mortelli · 2020-06-16T01:33:09Z

api/manifest.go

@@ -243,7 +242,6 @@ func readManifest(mr storage.LazySectionReader, addr storage.Address, fileStore
 	if err != nil { // size == 0
 		// can't determine size means we don't have the root chunk
 		log.Trace("manifest not found", "addr", addr)
-		err = fmt.Errorf("Manifest not Found")


leaving a comment here mainly for @zelig:

the Manifest not Found error created here was swallowing up the 420 that was trying to be returned.

in any case, i believe without line 246 the returned error will be 500 or something of the sort (@santicomp2014 can confirm/refute this)

Yes exactly, the error code becomes Manifest not found and the error code is 500.
The error manifest not found is already being thrown in the error.

mortelli · 2020-06-16T01:35:35Z

storage/netstore.go

-						return n.RemoteFetch(ctx, req, fi)
+					if n.recoveryCallback != nil && publisher != "" {
+						log.Debug("content recovery callback triggered", "ref", ref.String())
+						go n.recoveryCallback(ctx, ref)


based on some tests, we think it might be necessary to call the recovery callback with go here.

do you see any issues with this @zelig?

zelig · 2020-06-15T23:20:23Z

api/http/server.go

+	postPinFail        = metrics.NewRegisteredCounter("api/http/post/pin/fail", nil)
+	deletePinCount     = metrics.NewRegisteredCounter("api/http/delete/pin/count", nil)
+	deletePinFail      = metrics.NewRegisteredCounter("api/http/delete/pin/fail", nil)
+	errRecoveryAttempt = errors.New("recovery was initiated")


define this in netstore once

I was worried about circular dependency, moved it.
Thanks

zelig · 2020-06-16T11:04:35Z

api/http/server.go

@@ -252,6 +254,11 @@ func (s *Server) HandleBzzGet(w http.ResponseWriter, r *http.Request) {
 				respondError(w, r, err.Error(), http.StatusUnauthorized)
 				return
 			}
+			if isRecoveryAttemptError(err) {
+				w.Header().Set("WWW-Authenticate", fmt.Sprintf("Basic realm=%q", uri.Address().String()))
+				respondError(w, r, err.Error(), http.StatusPaymentRequired)


no i meant 420 not 402, you can just construct the response as:

respondError(w, r, err.Error(), 420)

My bad, I was confused it did not make sense.
Changed

zelig · 2020-06-16T11:07:17Z

api/client/client.go

 	req, err := http.NewRequest("GET", uri, nil)
+	values := req.URL.Query()


why not just

req.URL.Query().Add("publisher", publisher)

I used req.URL.Query().Add("publisher", publisher) and the query param was not reaching the other side.
I looked at other places of the code using Add/Set and it was encoding after adding it.

After changing it to .Encode() it works correctly.
@mortelli saw the same thing

…rRecoveryAttempt into netstore

mortelli

LGTM, GJ

api/client,api:http,cmd/swarm,prod,storage: added flag for publisher …

540187d

…on download cli and query param for download; injected publisher into ctx

santicomp2014 changed the base branch from master to global-pinning June 11, 2020 11:13

santicomp2014 added 3 commits June 11, 2020 12:37

storage: changed recovery only when publisher is set

0463f7a

prod,netstore: added logs for flood test

0e8b18e

storage: remove publisher after recovery

ca488fd

mortelli assigned santicomp2014 Jun 11, 2020

mortelli added global pinning experimental implementation of global pinning in progress labels Jun 11, 2020

santicomp2014 added 3 commits June 11, 2020 16:07

storage: removed test publisher removal

f5236ec

prod: added log for publisher in getPinners

754739b

api,api/client,apit/http,cmd/swarm,cmd/swarm-smoke: removed publisher…

0787628

… from manifest as its passed along ctx in the download path

mortelli mentioned this pull request Jun 12, 2020

global pinning: phase 3 #2173

Closed

4 tasks

santicomp2014 added 2 commits June 12, 2020 10:07

cmd/swarm: remove unused log in download path

f1b0fbf

api/client,api/http,cmd/swarm: added publisher to bzz-list for recove…

e25597e

…ry purposes

santicomp2014 marked this pull request as ready for review June 12, 2020 14:58

santicomp2014 requested review from mortelli and zelig June 12, 2020 14:58

mortelli added ready for review and removed in progress labels Jun 12, 2020

mortelli suggested changes Jun 12, 2020

View reviewed changes

api/client,api/http,cmd/swarm,prod,storage: removed unused comments a…

6a9a30a

…nd fixed naming issues

santicomp2014 commented Jun 12, 2020

View reviewed changes

Merge branch 'global-pinning' into global-pinning-fix-request-flooding

d5d3de9

storage: changed comments in netstore for clarity in global pinning

19edb39

santicomp2014 changed the title ~~Add query param for publisher and solve flooding in recovery~~ api,client,http,main,prod,storage: Add query param for publisher and solve flooding in recovery Jun 12, 2020

mortelli mentioned this pull request Jun 12, 2020

main, swarm, http, prod, api, client: repair globally pinned content through CLI publisher #2194

Closed

zelig suggested changes Jun 13, 2020

View reviewed changes

santicomp2014 added 2 commits June 15, 2020 12:29

api/client,api/http,cmd/swarm,storage: changed publisher query param …

aecc382

…to Query().Add, return 402 when recovery attempt is being processed

api/client: encoded publisher in query param

41cfbe5

santicomp2014 added 2 commits June 15, 2020 18:43

api/http: added isRecoveryAttempt to List, storage changed recoveryCa…

688122a

…llback to async

api: removed Manifest not found from readManifest, global pinning wil…

79f3867

…l return recovery attempt instead and should not be absorbed

santicomp2014 added the ready for another review label Jun 15, 2020

mortelli reviewed Jun 16, 2020

View reviewed changes

zelig suggested changes Jun 16, 2020

View reviewed changes

santicomp2014 added 3 commits June 16, 2020 10:20

api/http,storage: changed StatusRecoveryAttempt to 420, refactored Er…

5e57f81

…rRecoveryAttempt into netstore

api/http: added isRecoveryAttemptError for HandleGetFile

3494897

api/http: changed StatusRecoveryAttempt to StatusEnhanceYourCalm

6cc23bf

mortelli approved these changes Jun 16, 2020

View reviewed changes

santicomp2014 merged commit 53c0fcc into global-pinning Jun 16, 2020

mortelli deleted the global-pinning-fix-request-flooding branch June 16, 2020 19:02

This was referenced Jun 16, 2020

repair globally pinned content without manifest #2193

Closed

fix content recovery request flooding #2205

Closed

mortelli restored the global-pinning-fix-request-flooding branch June 17, 2020 02:36

This was referenced Jun 23, 2020

flood of recovery requests #2209

Open

lookup timeout on feed content recovery #2210

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api,client,http,main,prod,storage: Add query param for publisher and solve flooding in recovery #2202

api,client,http,main,prod,storage: Add query param for publisher and solve flooding in recovery #2202

santicomp2014 commented Jun 11, 2020 •

edited by mortelli

Loading

mortelli left a comment

mortelli Jun 12, 2020

santicomp2014 Jun 12, 2020

mortelli Jun 12, 2020

mortelli Jun 12, 2020

mortelli Jun 12, 2020 •

edited

Loading

santicomp2014 Jun 12, 2020

zelig Jun 13, 2020

mortelli Jun 16, 2020

santicomp2014 left a comment

santicomp2014 Jun 12, 2020

santicomp2014 Jun 12, 2020

santicomp2014 commented Jun 12, 2020

zelig Jun 13, 2020

mortelli left a comment

mortelli Jun 16, 2020 •

edited

Loading

santicomp2014 Jun 16, 2020

mortelli Jun 16, 2020

zelig Jun 15, 2020

santicomp2014 Jun 16, 2020

zelig Jun 16, 2020

santicomp2014 Jun 16, 2020

zelig Jun 16, 2020

santicomp2014 Jun 16, 2020 •

edited

Loading

mortelli left a comment

		req, err := http.NewRequest("GET", uri, nil)
		values := req.URL.Query()

api,client,http,main,prod,storage: Add query param for publisher and solve flooding in recovery #2202

api,client,http,main,prod,storage: Add query param for publisher and solve flooding in recovery #2202

Conversation

santicomp2014 commented Jun 11, 2020 • edited by mortelli Loading

mortelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortelli Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santicomp2014 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santicomp2014 commented Jun 12, 2020

Choose a reason for hiding this comment

mortelli left a comment

Choose a reason for hiding this comment

mortelli Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santicomp2014 Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

mortelli left a comment

Choose a reason for hiding this comment

santicomp2014 commented Jun 11, 2020 •

edited by mortelli

Loading

mortelli Jun 12, 2020 •

edited

Loading

mortelli Jun 16, 2020 •

edited

Loading

santicomp2014 Jun 16, 2020 •

edited

Loading