interface'ifying things #61

frrist · 2018-07-16T20:26:23Z

What's in the PR?

Core interface changes
Plugin system (localipfs & dockeripfs)
- The plugin implementation is baebones, improvements to the *ipfs plugins are desirable.
Reworked CLI
Increased Test Coverage

whyrusleeping · 2018-07-16T22:36:33Z

main.go

 		killCmd,
 		restartCmd,
-		setCmd,


where is set going?

Hopefully nowhere. A lot of code is being stripped to just limit the total amount of code that needs to be rewritten to get things working. Both setCmd and getCmd will be coming back in, as well as the flags I stripped from init in some way.

With the latest updates though the IPFS local plugin actually works. Some code massaging needs to take place as a few things are a bit ruff around the edges.

Proper plugin loading

Better command output handling

whyrusleeping · 2018-07-17T04:48:28Z

@travisperson could you take a look at incorporating these changes too while youre at it? 08f0993

It separates the init process into two steps (optionally) then makes the normal init process explicitly do both. This will make certain setups easier to play with in the future.

travisperson · 2018-07-17T04:59:48Z

@whyrusleeping ah nice. Ya should be easy to incorporate.

whyrusleeping · 2018-07-18T20:13:32Z

testbed/spec.go

+	}
+
+	plugs, err := ioutil.ReadDir(dir)
+


style nit: don't put spaces between creating and checking an error

whyrusleeping · 2018-07-18T20:14:25Z

testbed/spec.go

+
+	PluginName := *(PluginNameSym.(*string))
+
+	plugins[PluginName] = iptbplugin{


probably want to check if a plugin by this name exists already

whyrusleeping · 2018-07-18T20:17:28Z

We should make sure its possible to build iptb by default with support for some 'default plugins'. I want to not have to worry about where my ipfs plugin is located

travisperson · 2018-07-18T21:53:13Z

Building in some plugins is a great idea, I updated the current "plugins" to get built into the binary and allow for them to be overridden.

Also exposed them as a plugin package for easy hacking / building your own 🔨

whyrusleeping · 2018-07-19T18:00:16Z

plugins/ipfs/util.go

+	return []string{attrId, attrPath, attrBwIn, attrBwOut}
+}
+
+func GetAttrDesc(attr string) (string, error) {


duplicate code?

whyrusleeping · 2018-07-19T18:02:53Z

testbed/interfaces/node.go

+type GetAttrDescFunc func(attr string) (string, error)
+
+// TestbedNode specifies the interface to a process controlled by iptb
+type TestbedNode interface {


This interface is getting pretty big, I wonder if we should break it up, and make some of the methods optional (by specialization, or similar)

The return types and parameters are a little off, but a rough idea:
https://gist.github.com/frrist/01405d6b36aae85108fad80c2c40d3fb

whyrusleeping · 2018-07-19T18:05:13Z

plugins/ipfs/docker/dockeripfs.go

+)
+
+func init() {
+	NewNode = func(dir string, extras map[string]interface{}) (testbedi.TestbedNode, error) {


You don't have to assign these like this, it should work the same if you just define it as:

func NewNode(dir string, extras map[string]interface{}) (testbedi.TestbedNode, error) { ... ... }

frrist · 2018-07-19T16:49:17Z

main.go

 	"fmt"
 	"io"
-	"net/http"
+


nit: whitespace

frrist · 2018-07-19T16:54:17Z

main.go

+	Action: func(c *cli.Context) error {
+		if c.Int("count") == 0 {
+			fmt.Printf("please specify number of nodes: '%s init -n 10'\n", os.Args[0])
+			os.Exit(1)


lets return errors for these and have the exit here

frrist · 2018-07-19T16:55:56Z

testbed/testbed.go

+			return nil
+		}
+
+		err = os.RemoveAll(dir)


nit: can just return os.RemoveALl(dir) here

frrist · 2018-07-19T16:58:26Z

testbed/testbed.go

+
+	for i := 0; i < count; i++ {
+		dir := path.Join(base, fmt.Sprint(i))
+		err := os.MkdirAll(dir, 0775)


nit: combine with if

frrist · 2018-07-19T16:59:58Z

testbed/testbed.go

+			return nil, err
+		}
+
+		var spec *NodeSpec


nit: I don't think this is necessary

frrist · 2018-07-19T17:37:52Z

main.go

+	Usage: "stream events from a given node",
+	Action: func(c *cli.Context) error {
+		if len(c.Args()) != 1 {
+			return fmt.Errorf("'iptb logs' accepts at exactly 1 argument")


iptb ~~logs~~ events

frrist · 2018-07-19T17:38:14Z

main.go

@@ -598,3 +822,34 @@ var logsCmd = cli.Command{
 		return nil
 	},
 }
+
+var eventsCmd = cli.Command{


I like this!

frrist · 2018-07-19T17:40:47Z

plugins/ipfs/docker/dockeripfs.go

+		return nil, fmt.Errorf("error getting env: %s", err)
+	}
+
+	cmd := exec.Command(l.repobuilder, "init")


can use exec.CommandContext here

frrist · 2018-07-19T17:52:48Z

testbed/testbed.go

+			defer wait.Done()
+			_, err := nd.Init(context.TODO())
+			if err != nil {
+				panic(err)


we should try and avoid panics, could use an error channel here. We could also just do this in a loop without a go-routine.

frrist · 2018-07-19T18:04:07Z

main.go

 		if err != nil {
 			return err
 		}

-		out, err := nd.RunCmd(c.Args()[1:]...)
+		out, err := nd.RunCmd(context.TODO(), c.Args()[1:]...)


can pass a context with a timeout here

whyrusleeping · 2018-07-19T18:37:45Z

plugins/ipfs/docker/dockeripfs.go

+		return fmt.Errorf("node is already running")
+	}
+
+	cmd := exec.Command("docker", "run", "-d", "-v", l.dir+":/data/ipfs", l.image)


Interesting future work here would be to use the docker api package: https://docs.docker.com/develop/sdk/#sdk-and-api-quickstart

whyrusleeping · 2018-07-23T06:55:26Z

Before merging, let's make sure that the tests all pass, and that if we use this in the go-ipfs sharness tests, they all still pass

frrist · 2018-07-26T23:43:22Z

commands/attr.go

+
+		tb := testbed.NewTestbed(path.Join(flagRoot, "benches", flagTestbed))
+
+		if flagSave {


Should we first attempt to set the attribute on the node before persisting it to the nodespec file?

frrist · 2018-07-26T23:48:32Z

commands/fast.go

+		}
+
+		runCmd := func(node testbedi.Core) (testbedi.TBOutput, error) {
+			return node.Init(context.TODO(), ([]string{})...)


can remove ([]string{})...

frrist · 2018-07-26T23:52:36Z

commands/start.go

+		},
+	},
+	Before: func(c *cli.Context) error {
+		if present := isTerminatorPresent(c); present {


let's leave a comment describing what terminator means

I added a comment on isTerminatorPresent as this is used on any command which takes a node range.

frrist · 2018-07-26T23:55:14Z

commands/utils.go

+			results[i] = Result{
+				Node:   n,
+				Output: out,
+				Error:  errors.Wrapf(err, "node[%d]", n),


will this this be incorrect if list is a range like [4-8]?

travisperson · 2018-08-07T20:56:42Z

To make sure everything was working correctly I did a quick update to go-ipfs sharness tests as well using the new cli.

You can see those changes here https://github.com/ipfs/go-ipfs/compare/master...travisperson:update-iptb?expand=1

Stebalien

Mostly random suggestions and interface comments. Overall, this looks like a reasonable design.

Stebalien · 2018-08-07T20:45:35Z

README.md

-	dump-stack		get a stack dump from the given daemon
-	help, h			show a list of subcommands, or help for a specific subcommand
+     fast     create default bench and initialize
+     bench    create, remove, list bench setups


"manage test (?) bench setups". I assume these aren't benchmarks.

Ya, not benchmarks, just different setups. I'll try to make that clear. There is some discussion around the introduction of bench in issue #62. Basically this stemmed from having both a init and a create command, and trying to make it less ambiguous as to which to run.

The thought was to place the concept of "create" under a category, and a suggestion was made to call them benches, like having a toolbench where you would work on something.

In the current implementation init no longer makes the node specification. Instead it's a direct map to the Init method on the node interface. This allows iptb to have it's own configuration (defining number of nodes, types, etc), and still exposing the direct call to the interface.

bench is really unfortunate name, I think pretty much anything would be better, given its strong association with benchmarks

Stebalien · 2018-08-07T20:47:19Z

README.md

-	connect			connect two nodes together
-	dump-stack		get a stack dump from the given daemon
-	help, h			show a list of subcommands, or help for a specific subcommand
+     fast     create default bench and initialize


"auto"? Or maybe just a iptb init --create[=default] --start flags? "fast" makes me think you want IPTB to run fast, or something.

Right now there is iptb bench create [other flags] --init. The fast command was added (name mostly a placeholder, though auto might be a good choice) to bring back the functionality of iptb init, as currently it maps directly to the node interface method Init. It would be nice to keep that exposed as a directly way to invoke that method without overloading it too much. This could also be fixed by changing out the Core#Init method name something else, and keeping init as the quick and easy way to get started with iptb.

Basically iptb fast is almost a direct mapping to iptb bench create --init, it just doesn't accept the idea of additional configuration through --attr flags.

+1 for sth else than fast, I do like the idea of using default somehow

Stebalien · 2018-08-07T20:54:25Z

commands/connect.go

+		for _, f := range from {
+			for _, t := range to {
+				ctx, cancel := context.WithTimeout(context.Background(), timeout)
+				defer cancel()


Shouldn't the timeout be applied to the entire command?

Actually, I'd also consider doing this in parallel anyways.

Agreed, but this can happen in a follow on PR

Stebalien · 2018-08-07T20:56:14Z

commands/connect.go

+	Category:  "CORE",
+	Name:      "connect",
+	Usage:     "connect nodes together",
+	ArgsUsage: "<nodes> <nodes>",


I'd also expect this command to take 1 arg (connecting all the nodes) and 0 args (connecting all nodes). Is there already another command for that?

Nope, nothing like that yet. Sounds like a good addition though 👍

Stebalien · 2018-08-07T21:00:40Z

plugins/ipfs/util.go

+	TotalOut int
+}
+
+func GetBW(l testbedi.Libp2p) (*BW, error) {


Might as well take a context for all of these functions up-front (unless I'm missing something about how they're supposed to be used).

Stebalien · 2018-08-07T21:11:03Z

testbed/testbed.go

+	return NodesFromSpecs(specs)
+}
+
+func InitNodes(nodes []testbedi.Core, args ...string) error {


I'd just pass in the context.

dead code actually, deleting...

Stebalien · 2018-08-07T21:12:53Z

testbed/spec.go

+}
+
+// SetAttr sets an attribute on the NodeSpec
+func (ns *NodeSpec) SetAttr(attr string, val interface{}) {


The Attribute interface takes/returns a string value. I assume these should match.

Stebalien · 2018-08-07T21:14:30Z

plugins/ipfs/docker/dockeripfs.go

+
+/// Core Interface
+
+func (l *Dockeripfs) Init(ctx context.Context, agrs ...string) (testbedi.Output, error) {


s/agrs/args.

Stebalien · 2018-08-07T21:17:15Z

plugins/ipfs/util.go

+	return nil
+}
+
+func SwarmAddr(lt ListenType, addr string, port int) string {


We should really just use normal multiaddrs. This'll be a pain to maintain.

Ya, actually that is a good idea. I just changed it so that the plugin itself can take an apiaddr / swarmaddr, and the user can just throw in what ever they want.

They now just have defaults, and we can later be a little smarter and parse into and rewrite from the config to change individual values.

local
apiaddr => /ip4/127.0.0.1/tcp/0
swarmaddr => /ip4/127.0.0.1/tcp/0

local
apiaddr => /ip4/0.0.0.0/tcp/5001
swarmaddr => /ip4/0.0.0.0/tcp/4001

Note: we can also add profiles and such

Stebalien · 2018-08-07T21:18:20Z

testbed/interfaces/node.go

+	Libp2p
+	// Allows a node to run any initialization it may require
+	// Ex: Installing additional dependencies / setting up configuration
+	Init(ctx context.Context, args ...string) (Output, error)


I assume that "args" are additional command line arguments, right?

dignifiedquire

Nothing major, mostly small things that could make things a bit nicer.

One thing I noticed, while the interfaces are nicely documented a lot of the rest of the code doesn't have comments on methods, which makes it a bit harder to read & understand, than necessary.

Overall this is a big improvement 💯

dignifiedquire · 2018-08-08T13:42:30Z

README.md

@@ -1,22 +1,19 @@
 # IPTB

-`iptb` is a program used to create and manage a cluster of sandboxed IPFS nodes locally on your computer. Spin up 1000s of nodes! It exposes various options, such as different bootstrapping patterns. `iptb` makes testing IPFS networks easy!
+`iptb` is a program used to create and manage a cluster of sandboxed IPFS nodes


I thought the idea was to allow for other things than IPFS as well, so maybe make this description a bit more generic?

dignifiedquire · 2018-08-08T13:43:15Z

README.md

-	connect			connect two nodes together
-	dump-stack		get a stack dump from the given daemon
-	help, h			show a list of subcommands, or help for a specific subcommand
+     fast     create default bench and initialize


+1 for sth else than fast, I do like the idea of using default somehow

dignifiedquire · 2018-08-08T13:44:29Z

README.md

-	dump-stack		get a stack dump from the given daemon
-	help, h			show a list of subcommands, or help for a specific subcommand
+     fast     create default bench and initialize
+     bench    create, remove, list bench setups


bench is really unfortunate name, I think pretty much anything would be better, given its strong association with benchmarks

dignifiedquire · 2018-08-08T13:45:03Z

README.md

 ```

 ### Install

+`iptb` uses [gx](https://github.com/whyrusleeping/gx)


can we make it such that it also works without gx, pretty please?

We can do that as long as we drop the ipfs plugins from being built into the binary by default due to its heavy use of gx. Currently the go-libp2p package needs to be versioned.

dignifiedquire · 2018-08-08T13:47:46Z

commands/connect.go

+var ConnectCmd = cli.Command{
+	Category:  "CORE",
+	Name:      "connect",
+	Usage:     "connect nodes together",


It would be great to have a little more text here, with some examples. I was really unclear on how this works, especially with ranges, until I saw a demo from @whyrusleeping

Agreed, I propose we address this in a follow on PR since similar changes will likely be required.

dignifiedquire · 2018-08-08T14:02:32Z

plugins/ipfs/local/localipfs.go

+}
+
+func (l *Localipfs) Shell(ctx context.Context, nodes []testbedi.Core) error {
+	shell := os.Getenv("SHELL")


would be nice if these env variables are global constants, such that other plugins could reuse them

dignifiedquire · 2018-08-08T14:06:56Z

testbed/interfaces/node.go

+type Attribute interface {
+	Core
+	// GetAttr returns the value of attr
+	GetAttr(attr string) (string, error)


This is inconsistent, with both go best practices and the metric interface, and should be just Attr and SetAttr.

dignifiedquire · 2018-08-08T14:07:26Z

testbed/interfaces/node.go

+	// Ex: Installing additional dependencies / setting up configuration
+	Init(ctx context.Context, args ...string) (Output, error)
+	// Starts the node
+	Start(ctx context.Context, wait bool, args ...string) (Output, error)


missing explanation of what wait does

I think it needs to be removed actually. At the moment it doesn't serve much of a purpose. We can add it back if we find that it's actually needed for something.

Actually, I guess start has a purpose still. It can be to wait for the node to be reachable (api online). Stop is the one that doesn't really fit at the moment. It was added per a mention in #10, but I can't seem to think why it would be needed.

dignifiedquire · 2018-08-08T14:08:04Z

testbed/interfaces/node.go

+	Shell(ctx context.Context, ns []Core) error
+
+	// Writes a log line to stdout
+	Infof(format string, args ...interface{})


why does a plugin need to implement this?

Yeah, I was thinking that this one could probably be left off.

Ya, I think we talked about this previously, it can be dropped. Original thought was to make it easier to use the interface in other applications (embedded iptb), and provide logging.

dignifiedquire · 2018-08-08T14:08:56Z

testbed/interfaces/node.go

+	// Examples localipfs, dockeripfs, etc.
+	Type() string
+
+	String() string


what is String supposed to do/where is it useful?

String is a builtin go interface. It defines how this object gets printed if someone passes it to a Printf type function

whyrusleeping · 2018-08-14T19:50:11Z

sharness/t0020-stop.sh

 '

 test_expect_success "iptb stop works" '
-	../bin/iptb stop
+	../bin/iptb stop && sleep 1


we could have iptb sit around and wait for the processes to be really dead

at least make an issue for it, fine to fix later

whyrusleeping

Very large PR, i'm not going to be able to review in-depth, but i trust you two to do the right thing. Let's get this merged, and try to integrate things upwards, fixing any issues discovered along the way.

travisperson · 2018-08-14T19:51:41Z

@Stebalien @dignifiedquire @whyrusleeping

Is there any other feedback that should block this PR? There is additional work I want to do to the plugins. I can remove that code from this PR, or simply follow up.

frrist assigned travisperson and frrist Jul 16, 2018

frrist requested review from travisperson and whyrusleeping July 16, 2018 20:26

whyrusleeping reviewed Jul 16, 2018

View reviewed changes

travisperson mentioned this pull request Jul 17, 2018

Use plugins to manage nodes #62

Closed

whyrusleeping reviewed Jul 18, 2018

View reviewed changes

testbed/spec.go Outdated

}

plugs, err := ioutil.ReadDir(dir)

Copy link

Member

whyrusleeping Jul 18, 2018

Choose a reason for hiding this comment

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

style nit: don't put spaces between creating and checking an error

whyrusleeping reviewed Jul 18, 2018

View reviewed changes

whyrusleeping reviewed Jul 19, 2018

View reviewed changes

frrist commented Jul 19, 2018

View reviewed changes

whyrusleeping reviewed Jul 19, 2018

View reviewed changes

travisperson force-pushed the feat/interface-plugins branch 2 times, most recently from 289baf9 to fd43b32 Compare July 26, 2018 19:51

frrist commented Jul 26, 2018

View reviewed changes

travisperson force-pushed the feat/interface-plugins branch from d487790 to b55e21f Compare July 28, 2018 00:10

travisperson mentioned this pull request Jul 31, 2018

Add stat collection to IPTB #65

Closed

travisperson force-pushed the feat/interface-plugins branch 3 times, most recently from e82eb61 to 451df6b Compare August 1, 2018 00:59

frrist changed the title ~~[WIP] interface'ifying things~~ interface'ifying things Aug 1, 2018

frrist added the kind/enhancement A net-new feature or improvement to an existing feature label Aug 1, 2018

frrist requested review from Stebalien and magik6k August 1, 2018 01:06

frrist mentioned this pull request Aug 1, 2018

iptb shell fails #4

Closed

frrist requested a review from whyrusleeping August 1, 2018 01:11

travisperson force-pushed the feat/interface-plugins branch from 451df6b to dcd10d7 Compare August 2, 2018 17:46

Stebalien reviewed Aug 7, 2018

View reviewed changes

dignifiedquire reviewed Aug 8, 2018

View reviewed changes

travisperson force-pushed the feat/interface-plugins branch from 018fefc to ae7b7d1 Compare August 9, 2018 21:14

This was referenced Aug 11, 2018

Connect nodes in parallel #67

Open

Allow connecting *all* nodes or a cluster of nodes fully #68

Closed

whyrusleeping reviewed Aug 14, 2018

View reviewed changes

whyrusleeping approved these changes Aug 14, 2018

View reviewed changes

travisperson added 6 commits August 14, 2018 16:10

New interfaces

219b210

New cli

739c8be

Remove old

20de896

Add ipfs base helpers

49e52b5

Add dockeripfs

1cc6889

Add localipfs

cb205fb

travisperson force-pushed the feat/interface-plugins branch from 60c8209 to 34b6f51 Compare August 14, 2018 23:12

Address review feedback

5e19097

travisperson force-pushed the feat/interface-plugins branch from 34b6f51 to 5e19097 Compare August 14, 2018 23:14

travisperson merged commit e136336 into master Aug 15, 2018

Stebalien deleted the feat/interface-plugins branch August 15, 2018 23:06

This was referenced Aug 20, 2018

create a go-filecoin plugin for IPTB filecoin-project/venus#768

Closed

add iptb plugin for localfilecoin filecoin-project/venus#762

Merged

frrist mentioned this pull request Sep 18, 2018

consider replacing daemon tests with iptb filecoin-project/venus#916

Closed


		PluginName := (PluginNameSym.(string))

		plugins[PluginName] = iptbplugin{


		tb := testbed.NewTestbed(path.Join(flagRoot, "benches", flagTestbed))

		if flagSave {


		/// Core Interface

		func (l *Dockeripfs) Init(ctx context.Context, agrs ...string) (testbedi.Output, error) {

interface'ifying things #61

interface'ifying things #61

Conversation

frrist commented Jul 16, 2018 • edited Loading

What's in the PR?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Jul 17, 2018

travisperson commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Jul 18, 2018

travisperson commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping Jul 19, 2018 • edited Loading

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

whyrusleeping commented Jul 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

travisperson commented Aug 7, 2018

Stebalien 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

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

dignifiedquire 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

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

frrist commented Jul 16, 2018 •

edited

Loading

whyrusleeping Jul 19, 2018 •

edited

Loading