Feat: pintracker revamp #383

lanzafame · 2018-04-16T10:11:05Z

Still, need to add tests, but this PR desperately needs to see the light of day. @hsanjuan The main component, I would love feedback on is the operationTracker? Cheers

#308 (edit, added correct issue)

coveralls · 2018-04-16T10:22:18Z

Coverage increased (+0.4%) to 67.96% when pulling 19257ad on feat/pintracker-revamp into a0a0898 on master.

hsanjuan

@lanzafame , this looks very nice and it's very clean! And yes I like the operationTracker!

hsanjuan · 2018-04-17T10:31:57Z

architecture.md

@@ -80,7 +80,7 @@ This sections gives an overview of how some things work in Cluster. Doubts? Some

 * Initialize the P2P host.
 * Initialize Consensus: start looking for a leader asap.
-* Setup RPC in all componenets: allow them to communicate with different parts of the cluster.
+* Setup RPC in all components: allow them to communicate with different parts of the cluster.


Thanks for all these

hsanjuan · 2018-04-17T10:33:59Z

ipfsconn/ipfshttp/config_test.go

-      "pin_method": "pin"
+      "pin_method": "pin",
+      "client_timeout": "10s",
+      "client_post_timeout": "1m0s"


Please keep in mind to PR a documentation update (new website) when this is merged.

hsanjuan · 2018-04-17T10:35:53Z

ipfsconn/ipfshttp/config.go

+	ClientTimeout time.Duration
+
+	// IPFS Daemon HTTP Client POST timeout
+	ClientPostTimeout time.Duration


Hmm, does it make sense to have 2 when the IPFS HTTP Client always makes POST?

Ah if it always makes POSTs then, no it doesn't make sense 👍 I will fix this up

hsanjuan · 2018-04-17T10:38:46Z

ipfsconn/ipfshttp/config.go

@@ -23,6 +23,8 @@ const (
 	DefaultProxyWriteTimeout      = 10 * time.Minute
 	DefaultProxyIdleTimeout       = 60 * time.Second
 	DefaultPinMethod              = "pin"
+	DefaultClientTimeout          = 10 * time.Second
+	DefaultClientPostTimeout      = 60 * time.Second


Rather than general timeouts, I would like to have a standard timeout (say 5m) and then specific for pin and another for unpin operations. Where the pinTimeout is like 24h by default, and then unpin timeout something like 3h.

hsanjuan · 2018-04-17T10:43:34Z

ipfsconn/ipfshttp/ipfshttp.go


-	res, err := client.Post(url, "", nil)
+	ctx, cancel := context.WithCancel(ipfs.ctx)


context.WithTimeout(ipfs.ctx, timeout) ?

Also, defer cancel() so we cleanup as soon as we return.

Yeah, there was a reason but I have forgotten it and also I have learnt a lot more about the context pkg since I wrote this, so my guess is that my reason isn't necessarily valid anymore anyway

hsanjuan · 2018-04-17T10:51:41Z

pintracker/maptracker/operationtracker.go

+	operationRecover
+)
+
+//go:generate stringer -type=phase


uooo i didn't know this, maybe we can use it in more places!

hsanjuan · 2018-04-17T10:52:48Z

pintracker/maptracker/operationtracker.go

+		return
+	}
+	opc.cancel()
+	logger.Infof("operation '%s' on cid '%s' has been cancelled", opc.op.String(), c.String())


Note to self: not sure if these deserve INFO importance.

hsanjuan · 2018-04-17T10:54:40Z

pintracker/maptracker/operationtracker.go

+	operationUnknown operation = iota
+	operationPin
+	operationUnpin
+	operationSync


do you plan to do something with sync or recover? otherwise comment or remove.

hsanjuan · 2018-04-17T11:08:44Z

pintracker/maptracker/maptracker.go

+	opc, ok := mpt.optracker.get(c.Cid)
+	if !ok {
+		logger.Debug("pin operation wasn't being tracked")
+		ctx = mpt.ctx


I'm not quite seeing something here.

If the pin was queued, but the operation is cancelled (because an unpin request is received), this will still run when the pin comes out of the queue. Since the previous operation was cancelled, it was deleted and will show the warning that it wasn't tracked. Right?

Right now it would seem that it only cancels ongoing pins (by killing the context), but not queued pins. If an unpin requests comes and the pin is queued, the optracker should mark it as cancelled (phaseCancelled?), and this function should probably exit in that case.

This may force you to carry several operationCtx per Cid (one directly associated to each Track/Untrack) and have any new operation for that Cid affect the others when it runs (an unpin would cancel all pins). A new pin would probably not even enter the queue if there is a pin operationCtx for that Cid already

Probably, the operationCtx can only be removed from the tracker here, once it's no longer queued.

Please correct me if I missed something and things work differently.

Thanks for catching this, I had forgotten to perform the 'filtering' of pin/unpins in the pin/unpinWorkers. I have pushed that change up. Please let me know if that doesn't solve the issue you are raising?

I think it does solve it. Looking forward to tests.

hsanjuan

Oh, I almost forget, I would like to have TrackerStatusPinQueue and TrackerStatusUnpinQueue types to reflect that something is pinning (in ipfs) or just waiting to be pinned (in our queue).

lanzafame · 2018-04-18T02:03:23Z

@hsanjuan hmmm this is for use in the syncStatus function, correct? So that things that are queued don't get timed out?

hsanjuan · 2018-04-18T02:05:50Z

@lanzafame anything that returns api.PinInfo stuff (which carries a status)

hsanjuan · 2018-04-23T18:28:40Z

There are some errors in the tests (jenkins: config_test.go:27: error parsing ipfshttp.pin_timeout: time: invalid duration).

Other than that, last commit lgtm.

hsanjuan · 2018-04-24T07:29:27Z

ipfsconn/ipfshttp/config.go

@@ -69,6 +81,9 @@ type jsonConfig struct {
 	ProxyWriteTimeout       string `json:"proxy_write_timeout"`
 	ProxyIdleTimeout        string `json:"proxy_idle_timeout"`
 	PinMethod               string `json:"pin_method"`
+	ClientPostTimeout       string `json:"client_post_timeout"`


ClientPost seems a too internal name for a user. Better IPFSTimeout or IPFSRequestTimeout

hsanjuan · 2018-04-24T07:30:13Z

ipfsconn/ipfshttp/config_test.go

+      "pin_method": "pin",
+      "client_post_timeout": "5m0s",
+	  "pin_timeout": "24h",
+	  "unpin_timeout": "3h"


indentation is funny here

hsanjuan · 2018-04-24T07:33:15Z

ipfsconn/ipfshttp/ipfshttp.go

@@ -713,14 +725,25 @@ func (ipfs *Connector) PinLsCid(hash *cid.Cid) (api.IPFSPinStatus, error) {
 	return api.IPFSPinStatusFromString(pinObj.Type), nil
 }

-func doPost(client *http.Client, apiURL, path string) (*http.Response, error) {
+func (ipfs *Connector) doPost(client *http.Client, apiURL, path string) (*http.Response, error) {


if do post is no longer used for anything, then just rename doPostCtx to doPost.

hsanjuan · 2018-04-24T07:33:45Z

ipfsconn/ipfshttp/ipfshttp.go

+	return ipfs.postDiscardBodyCtx(ipfs.ctx, path)
+}
+
+func (ipfs *Connector) postDiscardBodyCtx(ctx context.Context, path string) error {


Same as with doPost. Less trivial unused code marginally better than more.

hsanjuan · 2018-04-24T07:47:05Z

pintracker/maptracker/maptracker_test.go

+		if !ok {
+			t.Fatal()
+		}
+		if opc.phase == phaseInProgress {


Likely to fail if the item is pinned before you can check if it's in progress right? You'll need a RPC mock which, for example, locks the pinning operation for a few seconds to simulate a long one.

hsanjuan · 2018-04-24T07:51:41Z

pintracker/maptracker/maptracker_test.go

+			t.Fatal()
+		}
+		if opc.phase == phaseInProgress {
+			err = mpt.Untrack(h1)


Once you untrack, it would be good to find a way to check that the operation was cancelled. I'm not sure about the best approach though. Probably the mock RPC can report whether the context in the operation was cancelled.

hsanjuan · 2018-04-24T07:52:34Z

pintracker/maptracker/maptracker_test.go

+			}
+		}
+	}()
+


You need to wait for above goroutine to return before finishing the test.

hsanjuan · 2018-04-24T07:53:27Z

pintracker/maptracker/maptracker_test.go

+	go func() {
+		for {
+			opc, _ := mpt.optracker.get(c.Cid)
+			if opc.phase == phaseQueued {


same comment as above, you might just miss phaseQueued

hsanjuan · 2018-04-24T10:18:35Z

Update config with new keys and sensible values here: https://github.com/ipfs/ipfs-cluster/blob/master/config_test.go

Pin tracker pin_timeout and unpin_timeout are now redundant because the ipfsconnector includes timeouts.

lanzafame · 2018-04-26T01:42:14Z

@hsanjuan the pin_timeout and unpin_timeout are still you used by the MapPinTracker.syncStatus function. I think until the whole status map concept gets reworked, it is better off staying and just keeping them in sync with the timeouts listed in ipfsconn, imho. Thoughts?

hsanjuan · 2018-04-26T06:48:47Z

@lanzafame I think syncStatus should not be anymore setting things in error using those timeouts. Since things are going to transition to error correctly by themselves when the ipfsconn timeouts trigger. So yeah, I think those cases in syncStatus become now nops.

hsanjuan · 2018-04-26T06:58:55Z

Btw. You can adjust a manual delay in maptracker_test.go:62 (it's just 100ms). I think the rest of errors are affected pinDelay mostly.

TestTrackUntrackWithCancel hangs undefinitely for me some times (i guess because of the problem I pointed above).

hsanjuan · 2018-04-26T06:59:35Z

Also, one way to make sure we are not messing something with the locks is to switch to sync.Map and see if the errors still happen.

lanzafame · 2018-05-02T06:30:53Z

@hsanjuan If you are happy with the code itself, then I am happy to hand it over to you to see if you can get those windows tests passing.

License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

ipfshttp: cancel POST request when timeout reached ipfshttp/config: fix config test ipfshttp: use struct styling for multi-line func calls ipfshttp/config: add general ClientTimeout License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

The TrackerStatuses were starting to be used to convey the inflight status of an 'operation', instead of just the status of the Pin. I have separated out any thing related to 'operations' and an operation's 'phases'. License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

to the ipfscluster.IPFSConnector interface and then to the implementation of that interface in ipfsconn/ipfshttp. This allows calls from MapPinTracker to cancel requests made to the local IPFS node. License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

come off the pin/unpin channels. Also fix a race condition in the operationTracker. License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

and get the tests passing and add Pin/UnpinQueued tracker statuses back in. License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

This creates a race condition where the items may have been already pinned before the operation is registered in the tracker. This may result in operations being left in the tracker and potentially never completed. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Pinning/unpinning timeouts are controlled by the ipfs connector component. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Avoid writing tests which will hang indefinitely on failure conditions. Introduce TODOs. Rename some vars to more explicit names. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan

@lanzafame

I have fixed a couple of bugs:

race condition which caused that adding the operation to the tracker came after pinning, so pinning had no effect and things ended marked as queued.
cancelling the operation contexts in every case before deleting them. Otherwise this would be leaking memory.

I still get occasional failures but the worst ones are fixed.

I have added some TODOs to the tests. We need to check that cancellations of ongoing operations actually happen in the right way. This can be achieved by letting the mock RPCServer keep a list with the times a method was called, and whether the passed context was cancelled before returning from it, and then checking that from the tests.

Other than that I haven't been able to find any more problems with the code..

License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

hsanjuan · 2018-05-07T05:26:56Z

Yai! @lanzafame let me know if you want to do anything here, otherwise I'll merge

lanzafame · 2018-05-07T05:29:47Z

@hsanjuan Nothing else. All good to go 🎉

lanzafame requested a review from hsanjuan April 16, 2018 10:11

ghost assigned lanzafame Apr 16, 2018

ghost added the status/in-progress In progress label Apr 16, 2018

hsanjuan requested changes Apr 17, 2018

View reviewed changes

hsanjuan requested changes Apr 18, 2018

View reviewed changes

lanzafame force-pushed the feat/pintracker-revamp branch from 95a67cd to ff513a5 Compare April 24, 2018 07:07

hsanjuan requested changes Apr 24, 2018

View reviewed changes

lanzafame force-pushed the feat/pintracker-revamp branch from 4f31d6d to 1a11c71 Compare April 26, 2018 04:19

lanzafame force-pushed the feat/pintracker-revamp branch from 1a11c71 to be642f5 Compare April 26, 2018 06:55

lanzafame force-pushed the feat/pintracker-revamp branch 7 times, most recently from f3b9b8a to 348bd9f Compare May 2, 2018 05:53

hsanjuan changed the title ~~WIP: Feat/pintracker revamp~~ Feat/pintracker revamp May 2, 2018

hsanjuan changed the title ~~Feat/pintracker revamp~~ Feat: pintracker revamp May 2, 2018

ghost assigned hsanjuan May 2, 2018

hsanjuan force-pushed the feat/pintracker-revamp branch from 1ca55bf to 63835fb Compare May 2, 2018 10:54

hsanjuan force-pushed the feat/pintracker-revamp branch from 9f4e392 to b564fe6 Compare May 2, 2018 13:04

lanzafame and others added 11 commits May 2, 2018 15:24

typos and style nitpicks

5316c3b

License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

pintracker: add filtering of operationCtxs as they

1eade86

come off the pin/unpin channels. Also fix a race condition in the operationTracker. License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

ipfshttp: add pin/unpin specific timeouts

f68c7f5

and get the tests passing and add Pin/UnpinQueued tracker statuses back in. License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

Pintracker: remove timeouts

9856bcd

Pinning/unpinning timeouts are controlled by the ipfs connector component. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Pintracker: rename and fmting.

8b08dfe

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Undo extra delays

e186dbe

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Pintracker: always cancel operation contexts

877e65a

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan force-pushed the feat/pintracker-revamp branch from b564fe6 to 877e65a Compare May 2, 2018 13:24

Pintracker: improve tests

859cf75

Avoid writing tests which will hang indefinitely on failure conditions. Introduce TODOs. Rename some vars to more explicit names. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan force-pushed the feat/pintracker-revamp branch from 7506973 to 859cf75 Compare May 2, 2018 14:13

hsanjuan requested changes May 2, 2018

View reviewed changes

ipfs-cluster deleted a comment from GitCop May 2, 2018

lanzafame added 2 commits May 4, 2018 21:18

rename operationCtx functions

401eb40

License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

finish maptracker tests

19257ad

License: MIT Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

lanzafame mentioned this pull request May 7, 2018

WIP: Feat/pintracker revamp optracker sync map #399

Closed

hsanjuan approved these changes May 7, 2018

View reviewed changes

hsanjuan added RFM Ready for Merge and removed status/in-progress In progress labels May 7, 2018

hsanjuan mentioned this pull request May 7, 2018

Release 0.4.0 #372

Closed

hsanjuan merged commit bfcf700 into master May 7, 2018

hsanjuan deleted the feat/pintracker-revamp branch May 7, 2018 05:40


		res, err := client.Post(url, "", nil)
		ctx, cancel := context.WithCancel(ipfs.ctx)

Feat: pintracker revamp #383

Feat: pintracker revamp #383

Conversation

lanzafame commented Apr 16, 2018 • edited by hsanjuan

coveralls commented Apr 16, 2018 • edited

hsanjuan 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

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

Choose a reason for hiding this comment

hsanjuan left a comment

Choose a reason for hiding this comment

lanzafame commented Apr 18, 2018

hsanjuan commented Apr 18, 2018

hsanjuan commented Apr 23, 2018

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

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

hsanjuan commented Apr 24, 2018

lanzafame commented Apr 26, 2018

hsanjuan commented Apr 26, 2018

hsanjuan commented Apr 26, 2018

hsanjuan commented Apr 26, 2018

lanzafame commented May 2, 2018

hsanjuan left a comment

Choose a reason for hiding this comment

hsanjuan commented May 7, 2018

lanzafame commented May 7, 2018

lanzafame commented Apr 16, 2018 •

edited by hsanjuan

coveralls commented Apr 16, 2018 •

edited