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.

Sign up for GitHub

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

Jump to bottom

implement Actions around client and retrieval-client commands #1687

Merged
merged 1 commit into from
Jan 25, 2019
Merged

implement Actions around client and retrieval-client commands #1687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions testhelpers/fat/action_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package fat

import (
"context"
"encoding/json"
"fmt"
"io"

"github.com/filecoin-project/go-filecoin/address"
"github.com/filecoin-project/go-filecoin/protocol/storage"

"gx/ipfs/QmR8BauakNcBa3RbE4nbQu76PDiJgoQgz8AJdhJuiU4TAw/go-cid"
"gx/ipfs/QmXWZCd8jfaHmt4UDSnjKmGcrQMw95bDGWqEeVLVJjoANX/go-ipfs-files"
)

// ClientCat runs the client cat command against the filecoin process.
// A ReadCloser is returned representing the data.
// TODO(frrist): address buffering in filecoin plugins to exert appropriate backpressure on the
// reader IPTB returns.
func (f *Filecoin) ClientCat(ctx context.Context, cid cid.Cid) (io.ReadCloser, error) {
out, err := f.RunCmdWithStdin(ctx, nil, "go-filecoin", "client", "cat", cid.String())
if err != nil {
return nil, err
}
return out.Stdout(), err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we always be getting json output so we break less when output changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client cat just dumps the binary blob of the object referenced by the CID. I would say this is the correct behavior and probably shouldn't change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing me to rethink the Output interface on IPTB.

In the filecoin plugins we read the entire output of a command into a byte array and then return it as a reader through the Output interface. In hindsight this seems wasteful and redundant. If we were cat'ing a 12TB file this pattern wouldn't work as it doesn't allow the consumer to read returned data on an "as needed" basis.

To avoid this I am thinking we may want to add something like a StreamingOutput interface to IPTB that would allow us to pass the output pipes all the way through to commands like this. The StreamingOutput interface would expose a Close() method that calls exec.Wait(). This would allow us to wait for the respective I/O loop copying from the process to complete, and would give use the ability to handle large volumes of data when calling IPTB methods from code like this.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the interface already supports this use case. Both Stderr and Stdout are defined to return an io.ReadCloser. I think we just need a better implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is those pipes will be closed when Wait gets called, meaning we cannot read from them unless we allow the caller of the method to call Wait, currently it is handled in RunCmd -- this is the problem I am trying to get at.

}

// ClientImportData runs the client import data command against the filecoin process.
func (f *Filecoin) ClientImportData(ctx context.Context, data files.File) (cid.Cid, error) {
var out cid.Cid
if err := f.RunCmdJSONWithStdin(ctx, data, &out, "go-filecoin", "client", "import"); err != nil {
return cid.Undef, err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a file path isn't going to work for the docker plugin, or other iptb plugins that do not share the same filesystem. To support this we will need a way to create a file in the plugins context.

This is actually just a limitation due to the implementation details of the docker plugin, as we run the client side of the request inside of the docker container. If we were not concerned about binary comparability, we could implement the docker plugin to use a local process, and communicate with the filecoin daemon running in docker.

Any command that uses cmds.FileArg supports reading from stdin which we can use to allow this to work without the filesystem issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, this is something we need to watchout for.

I think file path could work if we make some assumptions about the environment the docker plugin runs in. For example, if the docker containers have a predictable set of files in something like /data/files/* then we could use file path to tell the plugins to load those files. Not saying this is the correct approach, just one to consider.

Copy link
Member Author

@frrist frrist Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think what you are getting at is that ClientImportData could also accept an io.Reader that it passes to RunCmdJSONWithStdin that will be used on an as needed basis, I will add that if thats what you mean

Copy link
Contributor

@travisperson travisperson Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

I just now realized that we are using go-ipfs-files, I thought this was actually golang os.File.

Something to note, a PR landed about a month ago that changed this interface up quite a bit. Though it appears to still maintain the basic functionality.
ipfs/go-ipfs-files#2

I think we can continue to accept the files.File, because a user can create one from an io.ReadCloser, so for a in memory buffer users could use bytes.Buffer wrapped in bufio.NopCloser, and then create the files.File with files.NewReaderFile. The current files.NewReaderFile accepts a path, but if the user leaves it blank, we can use this information to instead pass the file in as stdin (as opposed to passing the filename in), as it implements io.Reader.

The no path idea is also native to the new implementation (the super easy one being NewBytesFile)
https://github.com/ipfs/go-ipfs-files/blob/86b2cb84da8644dab2bd08797731cf12150fedf7/readerfile.go#L19-L34

Copy link
Member Author

@frrist frrist Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I completely follow. Wanna give my rebase a 👍 if I got it right?
https://github.com/filecoin-project/go-filecoin/pull/1682/files#diff-2793111db86639f3cba9e4d114fb10a2R30

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya the update addresses my original comment. The followup was how to support both using stdin, and a file by name. We don't need to worry about that right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Not sure I understand the "No Path Idea" being described above, can you file an issue for it -- it seems like a good idea to have here at some point.

return out, nil
}

// ClientProposeStorageDeal runs the client propose-storage-deal command against the filecoin process.
func (f *Filecoin) ClientProposeStorageDeal(ctx context.Context, data cid.Cid,
miner address.Address, ask uint64, duration uint64, allowDuplicates bool) (*storage.DealResponse, error) {

var out storage.DealResponse
sData := data.String()
sMiner := miner.String()
sAsk := fmt.Sprintf("%d", ask)
sDuration := fmt.Sprintf("%d", duration)

if err := f.RunCmdJSONWithStdin(ctx, nil, &out, "go-filecoin", "client", "propose-storage-deal", sMiner, sData, sAsk, sDuration); err != nil {
return nil, err
}
return &out, nil
}

// ClientQueryStorageDeal runs the client query-storage-deal command against the filecoin process.
func (f *Filecoin) ClientQueryStorageDeal(ctx context.Context, prop cid.Cid) (*storage.DealResponse, error) {
var out storage.DealResponse

if err := f.RunCmdJSONWithStdin(ctx, nil, &out, "go-filecoin", "client", "query-storage-deal", prop.String()); err != nil {
return nil, err
}
return &out, nil
}

// ClientListAsks runs the client list-asks command against the filecoin process.
// A json decoer is returned that asks may be decoded from.
func (f *Filecoin) ClientListAsks(ctx context.Context) (*json.Decoder, error) {
return f.RunCmdLDJSONWithStdin(ctx, nil, "go-filecoin", "client", "list-asks")
}
19 changes: 19 additions & 0 deletions testhelpers/fat/action_retrieval_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package fat

import (
"context"
"io"

"gx/ipfs/QmR8BauakNcBa3RbE4nbQu76PDiJgoQgz8AJdhJuiU4TAw/go-cid"

"github.com/filecoin-project/go-filecoin/address"
)

// RetrievalClientRetrievePiece runs the retrieval-client retrieve-piece commands against the filecoin process.
func (f *Filecoin) RetrievalClientRetrievePiece(ctx context.Context, pieceCID cid.Cid, minerAddr address.Address) (io.ReadCloser, error) {
out, err := f.RunCmdWithStdin(ctx, nil, "go-filecoin", "retrieval-client", "retrieve-piece", minerAddr.String(), pieceCID.String())
if err != nil {
return nil, err
}
return out.Stdout(), nil
}