Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #155: Changes to run ipfs-cluster in the gateways #259

Merged
merged 7 commits into from Dec 4, 2017

Conversation

@hsanjuan
Copy link
Collaborator

hsanjuan commented Nov 30, 2017

These branch has the changes which are needed/useful to run ipfs-cluster in the gateways.

Each commit is a self-contained change, mostly unrelated to the others.

hsanjuan added 5 commits Nov 23, 2017
Allows us to test against latest ipfs (either it's broken and this
helps finding out, or it's not broken anymore and we don't lose time
trying to figure out why).

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
The main container will now run only ipfs-cluster-service.

A new ipfs-cluster-bundle container is built by Dockerfile-bundle
which will provide ipfs-cluster+ipfs.

Fixes #197

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
This also generates a default configuration section when it
doesn't exist, so it's backwards compatible.

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

This adds API, RPC calls to support RecoverAllLocal() (and expose RecoverLocal()
on the Rest API too). cluster-ctl is updated accordingly.

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
@hsanjuan hsanjuan requested a review from ZenGround0 Nov 30, 2017
@ghost ghost assigned hsanjuan Nov 30, 2017
@ghost ghost added the in progress label Nov 30, 2017
@hsanjuan

This comment has been minimized.

Copy link
Collaborator Author

hsanjuan commented Nov 30, 2017

@ZenGround0 might have more things coming here but maybe you can start having a look, no hurries though.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 30, 2017

Coverage Status

Changes Unknown when pulling e824aea on pin-bot into ** on master**.

This allows to call the Rest API's status and sync endpoints with a
"?local=true" parameter. This will trigger operations but only on the
local peer. Cluster *Local and RPC-*Local methods have been accordingly,
although they are aliases for the PinTracker methods (but otherwise they
would not be exposed in external APIs). ipfs-cluster-ctl has been updated to
support the new flag.

The rationaly behind this feature is that sometimes, a single cluster peer
(or the ipfs daemon in it) is misbehaving. The user then wants to Sync,
Recover, or see Status for that single peer. This is specially relevant
when working with big pinsets in larger clusters, as a Status() call will
be considerably more expensive when broadcasted everywhere.

Note that the Rest API keeps returning GlobalPinInfo objects even on local=true
calls. This ensures that the user always gets the same datatype from an endpoint.

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

This comment has been minimized.

Copy link
Collaborator Author

hsanjuan commented Dec 1, 2017

Ok, I think this is it for this branch.

@hsanjuan hsanjuan added needs review and removed in progress labels Dec 1, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 1, 2017

Coverage Status

Changes Unknown when pulling 4922c95 on pin-bot into ** on master**.

Copy link
Member

ZenGround0 left a comment

A few small things, but mostly this LGTM. Thanks for separating into the 6 logical commits, that really improved the review process on my end.

@@ -0,0 +1,2 @@
Dockerfile
Dockerfile-*

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Dec 4, 2017

Member

I am pretty sure this can be Dockerfile* unless you prefer being explicit about matching the dashed ones too.

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Dec 4, 2017

Author Collaborator

ok


RUN mkdir -p $IPFS_CLUSTER_PATH && \
chown 1000:100 $IPFS_CLUSTER_PATH
adduser -D -h $IPFS_CLUSTER_PATH -u 1000 -G users ipfs && \

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Dec 4, 2017

Member

From https://linux.die.net/man/8/adduser, it looks like the -h flag is used to print the help message and exit. It seems like we can get rid of the flag if the manpage is valid for this version of adduser (I know docker tools can be a little different sometimes)

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Dec 4, 2017

Author Collaborator

This is not linux, it's busybox. Here's the reference (sorry, can't link to the actual adduser section): https://busybox.net/downloads/BusyBox.html

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Dec 4, 2017

Member

Thanks for explaining

COPY --from=builder $SRC_PATH/docker/start-daemons.sh /usr/local/bin/start-daemons.sh

RUN mkdir -p $IPFS_CLUSTER_PATH && \
chown 1000:100 $IPFS_CLUSTER_PATH

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Dec 4, 2017

Member

Why is this line not updated to the two step adduser, chown method used in Dockerfile?

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Dec 4, 2017

Author Collaborator

because this one is built from go-ipfs, which does that step already

}

func (api *API) statusHandler(w http.ResponseWriter, r *http.Request) {
queryValues := r.URL.Query()
local := queryValues.Get("local")

if c := parseCidOrError(w, r); c.Cid != "" {

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Dec 4, 2017

Member

Comment on existing code: "c" is a misleading name for the return value of parseCidOrError, my guess is it is named "c" because of cid, but the cid is really a field of the return value. Maybe "p" or "ps" for pinSerial if you want to keep it concise.

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Dec 4, 2017

Author Collaborator

Fair enough, I have renamed those for ps.

error state, usually because the IPFS pin or unpin operation has failed.
The command will wait for any operations to succeed and will return the status
of the item upon completion.
of the item upon completion. Note that, when running on the full sets of tracked
CIDs (without argument), it may take considerable long time.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Dec 4, 2017

Member

typo: "considerable time" or "a considerably long time"

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Dec 4, 2017

Author Collaborator

thx

@@ -301,3 +354,21 @@ func (mock *mockService) ConsensusPeers(in struct{}, out *[]peer.ID) error {
*out = []peer.ID{TestPeerID1, TestPeerID2, TestPeerID3}
return nil
}

// FIXME: dup from util.go

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Dec 4, 2017

Member

Not sure if you are planning to skip this in this PR or forgot this comment so pinging you here.

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Dec 4, 2017

Author Collaborator

planning to skip

cluster.go Outdated
@@ -863,6 +883,12 @@ func (c *Cluster) SyncAllLocal() ([]api.PinInfo, error) {
return syncedItems, err
}

// Sync triggers a LocalSyncCid() operation for a given Cid

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Dec 4, 2017

Member

It looks like LocalSyncCid() was a potential new name for SyncLocal(), however, the name is not changed so this comment is a bit confusing.

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Dec 4, 2017

Author Collaborator

or a potential old name :P. fixed...

@@ -906,6 +916,12 @@ func (c *Cluster) Recover(h *cid.Cid) (api.GlobalPinInfo, error) {
return c.globalPinInfoCid("TrackerRecover", h)
}

// RecoverLocal triggers a recover operation for a given Cid in this peer only.
// It returns the updated PinInfo, after recovery.
func (c *Cluster) RecoverLocal(h *cid.Cid) (api.PinInfo, error) {

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Dec 4, 2017

Member

This is a general point of curiosity that applies to a few of the tracker functions: what's the value in adding this method to the cluster component, when the rpc_api server could just call c.tracker.Recover(h) directly?

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Dec 4, 2017

Author Collaborator

The point is maintaining parallelism between the Rest API and the Go API as to what is the user-facing interface that cluster offers. The rest API or the RPC implementation could call the tracker RPC methods directly, but as a user bringing up cluster programatically you would not be able to access the tracker to do the same (at least via the cluster object). It's easy to do, and avoids people relying on somewhat more internal interfaces.

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
@ghost ghost added the in progress label Dec 4, 2017
Copy link
Member

ZenGround0 left a comment

LGTM

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 4, 2017

Coverage Status

Changes Unknown when pulling d6a7caf on pin-bot into ** on master**.

@hsanjuan hsanjuan merged commit 36f6920 into master Dec 4, 2017
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on master at 72.474%
Details
@hsanjuan hsanjuan deleted the pin-bot branch Dec 4, 2017
@ghost ghost removed the in progress label Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.