From dcfc962f246f11998254f2e9125dec2550d4fdb3 Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Fri, 19 Jan 2018 13:04:06 +0100 Subject: [PATCH] Feat #277: Address review comments License: MIT Signed-off-by: Hector Sanjuan --- allocate.go | 5 ++++- cluster.go | 2 +- docs/ipfs-cluster-guide.md | 6 +++--- ipfs-cluster-ctl/formatters.go | 2 +- ipfscluster_test.go | 2 +- sharness/t0052-service-state-export.sh | 4 ++-- state/mapstate/map_state.go | 6 +++++- 7 files changed, 17 insertions(+), 10 deletions(-) diff --git a/allocate.go b/allocate.go index 77469ffc..289693c3 100644 --- a/allocate.go +++ b/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) diff --git a/cluster.go b/cluster.go index 3cfea321..cb1c7f12 100644 --- a/cluster.go +++ b/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) { diff --git a/docs/ipfs-cluster-guide.md b/docs/ipfs-cluster-guide.md index e68c6b67..7416a6a7 100644 --- a/docs/ipfs-cluster-guide.md +++ b/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 `. You can use `ipfs-cluster-ctl pin add ` 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 `. You can use `ipfs-cluster-ctl pin add ` 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 `. Retries for pins in error state can be triggered with `ipfs-cluster-ctl recover `. diff --git a/ipfs-cluster-ctl/formatters.go b/ipfs-cluster-ctl/formatters.go index 6a098cc3..ddd96b5a 100644 --- a/ipfs-cluster-ctl/formatters.go +++ b/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) } diff --git a/ipfscluster_test.go b/ipfscluster_test.go index aff29c35..8b220eae 100644 --- a/ipfscluster_test.go +++ b/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") } } diff --git a/sharness/t0052-service-state-export.sh b/sharness/t0052-service-state-export.sh index bb304067..ab4ad900 100755 --- a/sharness/t0052-service-state-export.sh +++ b/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 ' diff --git a/state/mapstate/map_state.go b/state/mapstate/map_state.go index d2f82122..f0a82e36 100644 --- a/state/mapstate/map_state.go +++ b/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 }