Feat #277: Address review comments

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

allocate.go

@@ -41,7 +41,10 @@ import (
 // it should only be used with valid replicationFactors (rplMin and rplMax
 // which are positive and rplMin <= rplMax).
 // It always returns allocations, but if no new allocations are needed,
-// it will return the current ones.
+// it will return the current ones. Note that allocate() does not take
+// into account if the given CID was previously in a "pin everywhere" mode,
+// and will consider such Pins as currently unallocated ones, providing
+// new allocations as available.
 func (c *Cluster) allocate(hash *cid.Cid, rplMin, rplMax int, blacklist []peer.ID) ([]peer.ID, error) {
 	// Figure out who is holding the CID
 	currentPin, _ := c.getCurrentPin(hash)

cluster.go

@@ -941,7 +941,7 @@ func (c *Cluster) Pins() []api.Pin {
 // PinGet returns information for a single Cid managed by Cluster.
 // The information is obtained from the current global state. The
 // returned api.Pin provides information about the allocations
-// assigned for the requested Cid, but does not provide indicate if
+// assigned for the requested Cid, but does not indicate if
 // the item is successfully pinned. For that, use Status(). PinGet
 // returns an error if the given Cid is not part of the global state.
 func (c *Cluster) PinGet(h *cid.Cid) (api.Pin, error) {

docs/ipfs-cluster-guide.md

@@ -218,7 +218,7 @@ Static clusters expect every member peer to be up and responding. Otherwise, the
 
 We call a dynamic cluster, that in which the set of `cluster.peers` changes. Nodes are bootstrapped to existing cluster peers (`cluster.bootstrap` option), the "peer rm" operation is used and/or the `cluster.leave_on_shutdown` configuration option is enabled. This option allows a node to abandon the consensus membership when shutting down. Thus reducing the cluster size by one.
 
-Dynamic clusters allow greater flexibility at the cost of stablity. Leave and, specially, join operations are tricky as they change the consensus membership. They are likely to fail in unhealthy clusters. All operations modifying the peerset require an elected and working leader. Note that peerset modifications may also trigger pin re-allocations if any of the pins from the departing cluster crosses the `replication_factor_min` threshold.
+Dynamic clusters allow greater flexibility at the cost of stablity. Leave and, specially, join operations are tricky as they change the consensus membership. They are likely to fail in unhealthy clusters. All operations modifying the peerset require an elected and working leader. Note that peerset modifications may also trigger pin re-allocations if any of the pins from the departing cluster crosses below the `replication_factor_min` threshold.
 
 Peers joining an existing cluster should not have any consensus state (contents in `./ipfs-cluster/ipfs-cluster-data`). Peers leaving a cluster are not expected to re-join it with stale consensus data. For this reason, **the consensus data folder is renamed** when a peer leaves the current cluster. For example, `ipfs-cluster-data` becomes `ipfs-cluster-data.old.0` and so on. Currently, up to 5 copies of the cluster data will be left around, with `old.0` being the most recent, and `old.4` the oldest.
 
@@ -261,11 +261,11 @@ This involves:
 
 Errors in the first part of the process (before the entry is commited) will be returned to the user and the whole operation is aborted. Errors in the second part of the process will result in pins with an status of `PIN_ERROR`.
 
-Deciding where a CID will be pinned (which IPFS daemon will store it - receive the allocation) is a complex process. In order to decide, all available peers (those reporting valid/non-expired metrics) are sorted by the `allocator` component, in function of the value of their metrics (provided by the `informer` component). If a CID is already allocated to some peers (in the case of a re-pinning operation), those allocations are kept.
+Deciding where a CID will be pinned (which IPFS daemon will store it - receive the allocation) is a complex process. In order to decide, all available peers (those reporting valid/non-expired metrics) are sorted by the `allocator` component, depending on the value of their metrics. These values are provided by the configured `informer`. If a CID is already allocated to some peers (in the case of a re-pinning operation), those allocations are kept.
 
 New allocations are only provided when the allocation factor (healthy peers holding the CID) is below the `replication_factor_min` threshold. In those cases, the new allocations (along with the existing valid ones), will attempt to total as much as `replication_factor_max`. When the allocation factor of a CID is within the margins indicated by the replication factors, no action is taken. The value "-1" and `replication_factor_min` and `replication_factor_max` indicates a "replicate everywhere" mode, where every peer will pin the CID.
 
-Default replication factors are specified in the configuration, but every pinned as them associated to its entry in the *shared state*. Changing the replication factor of existing pins requires re-pinning them (it does not suffice to change the configuration). You can always check the details of a pin, including its replication factors, using `ipfs-cluster-ctl pin ls <cid>`. You can use `ipfs-cluster-ctl pin add <cid>` to re-pin at any time with different replication factors. But note that the new pin will only be commited if it differs from the existing one.
+Default replication factors are specified in the configuration, but every Pin object carries them associated to its own entry in the *shared state*. Changing the replication factor of existing pins requires re-pinning them (it does not suffice to change the configuration). You can always check the details of a pin, including its replication factors, using `ipfs-cluster-ctl pin ls <cid>`. You can use `ipfs-cluster-ctl pin add <cid>` to re-pin at any time with different replication factors. But note that the new pin will only be commited if it differs from the existing one in the way specified in the paragraph above.
 
 In order to check the status of a pin, use `ipfs-cluster-ctl status <cid>`. Retries for pins in error state can be triggered with `ipfs-cluster-ctl recover <cid>`.
 

ipfs-cluster-ctl/formatters.go

@@ -166,7 +166,7 @@ func textFormatPrintPin(obj *api.PinSerial) {
 	} else {
 		var sortAlloc sort.StringSlice = obj.Allocations
 		sortAlloc.Sort()
-		fmt.Printf("Repl. Factor: %d->%d | Allocations: %s\n",
+		fmt.Printf("Repl. Factor: %d--%d | Allocations: %s\n",
 			obj.ReplicationFactorMin, obj.ReplicationFactorMax,
 			sortAlloc)
 	}

ipfscluster_test.go

@@ -1149,7 +1149,7 @@ func TestClustersReplicationMinMaxRealloc(t *testing.T) {
 		t.Errorf("Inssufficient reallocation, could have allocated to %d peers but instead only allocated to %d peers", expected, lenSA)
 	}
 
-	if len(secondAllocations) < 3 {
+	if lenSA < 3 {
 		t.Error("allocations should be more than rplMin")
 	}
 }

sharness/t0052-service-state-export.sh

@@ -9,8 +9,8 @@ test_cluster_init
 
 
 test_expect_success IPFS,CLUSTER "state export fails without snapshots" '
-    cluster_kill
-    sleep 5
+    cluster_kill &&
+    sleep 5 &&
     test_expect_code 1 ipfs-cluster-service --debug --config "test-config" state export
 '
 

state/mapstate/map_state.go

@@ -151,5 +151,9 @@ func (st *MapState) Unmarshal(bs []byte) error {
 	// snapshot is up to date
 	buf := bytes.NewBuffer(bs[1:])
 	dec := msgpack.Multicodec(msgpack.DefaultMsgpackHandle()).Decoder(buf)
-	return dec.Decode(st)
+	err := dec.Decode(st)
+	if err != nil {
+		logger.Error(err)
+	}
+	return err
 }