DI-based core.NewNode #6162

magik6k · 2019-04-01T16:29:33Z

TODO:

magik6k · 2019-04-03T15:49:58Z

Should be ready for a review. Few notes:

I recommend starting with reading the DI library docs, mainly this part - https://godoc.org/go.uber.org/fx#ex-package.
This PR aims to not introduce too many breaking changes, and make the code less intimidating.
The main constructor flow starts in core/builder.NewNode.
core/node is a new package which contains DI-related node construction logic.
core/ package now only provides IpfsNode and wraps DI constructor (will change in constructor interface refactor, not sure how exactly yet).
core/node/groups.go groups providers into larger components for a few reasons:
- It's easier to see what is related
- Groups are exported, so if someone want's to assemble their own node with different components they don't have to pull everything manually
Diff might seem big, but most changes are just moving existing constructor code and wrapping it into smaller units.

Stebalien

🕺

Awesome!

(now we just need to spread the love to libp2p so we can have a unified system)

The only comment there was to add some layer of abstraction so we aren't tied to fx but, well, seeing it in action, I'm not sure if it's worth it (although we likely will want a small library of helper functions).

cc @bigs & @raulk

core/bootstrap/bootstrap.go

Stebalien · 2019-04-04T07:07:09Z

core/builder.go

-	}
+	go func() {
+		// Note that some services use contexts to signal shutting down, which is
+		// very suboptimal. This needs to be here until that's addressed somehow


core/node/core.go

core/node/groups.go

core/node/helpers.go

core/node/libp2p.go

Stebalien · 2019-04-04T10:38:51Z

core/node/libp2p.go

+	//    PSRouter case below.
+	// 3. Introduce some kind of service manager? (my personal favorite but
+	//    that requires a fair amount of work).
+	if dht, ok := out.Routing.(*dht.IpfsDHT); ok {


Can we have different DI units provide different routers and then have a single unit collect them all? (then we can just drop the routing option system)

This seems to run into a circular dependency - If we want to construct routing after the host, we'd also need to somehow defer enabling autorelay.

Autorelay construction depends on routing [1], which in that place can only be provided as libp2p.Option to libp2p.New. We can't get the full routing there as PubsubRouter requires PubSub, which requires Host..

We could try not passing the AutoRelay option to Libp2p constructor, and instead construct it manually here, but it feels hacky. Better ideas?

That sucks... we'll fix this when we add fx support to libp2p but I'd like to merge this first.

What if we had this option depend on IpfsDHT specifically and then constructed the real router after we construct the host?

What if we had this option depend on IpfsDHT specifically and then constructed the real router after we construct the host?

What about NilRouter? (used when --routing=none is set)

I'd say we'll clean this up in the second stage of the refactor (and remove RoutingOption)

core/node/libp2p.go

magik6k · 2019-04-08T14:43:28Z

add some layer of abstraction so we aren't tied to fx

There isn't really that much fx specific code here (DI systems likely won't differ that much), so if we'd really need to switch to something else, it shouldn't be too hard.

Stebalien · 2019-04-09T22:40:38Z

There isn't really that much fx specific code here (DI systems likely won't differ that much), so if we'd really need to switch to something else, it shouldn't be too hard.

Really, my main concern is pluggability but we can probably punt on that.

core/node/libp2p.go

Stebalien · 2019-04-09T22:50:41Z

core/node/libp2p.go

+	//    PSRouter case below.
+	// 3. Introduce some kind of service manager? (my personal favorite but
+	//    that requires a fair amount of work).
+	if dht, ok := out.Routing.(*dht.IpfsDHT); ok {


That sucks... we'll fix this when we add fx support to libp2p but I'd like to merge this first.

What if we had this option depend on IpfsDHT specifically and then constructed the real router after we construct the host?

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien

We'll have some cleanup work ahead of us but this looks good to merge. Let's do so before we start collecting conflicts.

Stebalien · 2019-04-17T19:20:22Z

core/bootstrap/bootstrap.go

+	"github.com/libp2p/go-libp2p-net"
+	"github.com/libp2p/go-libp2p-peer"
+	"github.com/libp2p/go-libp2p-peerstore"
+	"github.com/libp2p/go-libp2p-routing"


Let's leave the explicit names for now.

(meh, actually, I don't care much either way)

Stebalien · 2019-04-17T22:46:46Z

core/node/libp2p.go

+
+func P2PAddrFilters(cfg *config.Config) (opts Libp2pOpts, err error) {
+	for _, s := range cfg.Swarm.AddrFilters {
+		f, err := mask.NewMask(s)


Stebalien · 2019-04-17T22:47:33Z

core/node/libp2p.go

+	mplex "github.com/whyrusleeping/go-smux-multiplex"
+	yamux "github.com/whyrusleeping/go-smux-yamux"
+	"github.com/whyrusleeping/multiaddr-filter"
+	mamask "github.com/whyrusleeping/multiaddr-filter"


Duplicate imports.

Stebalien · 2019-04-17T22:49:57Z

core/node/libp2p.go

+		if err != nil {
+			return nil, err
+		}
+		noAnnAddrs[maddr.String()] = true


FIXME later: We shouldn't convert to strings here. We should just run string(maddr.Bytes()).

Stebalien · 2019-04-17T23:54:00Z

core/node/libp2p.go

+	fx.Out
+
+	Opts []libp2p.Option `group:"libp2p"`
+}


We should be able to get rid of this helper entirely (libp2p/go-libp2p#603). I'll file a followup PR after merging this.

Stebalien · 2019-04-18T00:06:14Z

🚀 🌕

Stebalien · 2019-04-18T00:07:15Z

@magik6k,

Next up: all of these options need to be documented.

DI-based core.NewNode This commit was moved from ipfs/kubo@c3a7bc2

magik6k requested a review from Kubuxu as a code owner April 1, 2019 16:29

ghost assigned magik6k Apr 1, 2019

ghost added the status/in-progress In progress label Apr 1, 2019

magik6k changed the title ~~DI-based core.NewNode~~ [WIP] DI-based core.NewNode Apr 1, 2019

magik6k force-pushed the feat/dig branch 3 times, most recently from 3781a67 to f3f0962 Compare April 3, 2019 14:58

magik6k requested review from Stebalien and hannahhoward April 3, 2019 15:50

magik6k changed the title ~~[WIP] DI-based core.NewNode~~ DI-based core.NewNode Apr 3, 2019

Stebalien reviewed Apr 4, 2019

View reviewed changes

magik6k force-pushed the feat/dig branch from f3f0962 to 3b21ecf Compare April 4, 2019 23:42

Stebalien reviewed Apr 9, 2019

View reviewed changes

magik6k mentioned this pull request Apr 15, 2019

Host/Service interface design libp2p/go-libp2p#467

Open

magik6k force-pushed the feat/dig branch from fc3bc4f to 50848d9 Compare April 15, 2019 15:10

magik6k added 12 commits April 17, 2019 16:56

gomod: import dig

7c6a920

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

gomod: import uber/fx

2379787

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Initial DI node implementation

0fd2f80

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Organize NewNode a bit

361ba69

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Properly set IsOnline in NewNode

65d8fad

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

More constructor fixes

ccc576b

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Fix goprocess / lifecycle / ctx relations

cc2be2e

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Fix bootstrap

bfaffb2

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Fix some blockstore type mixups

1acb450

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Fix context propagation sortof

0ba7661

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Rewire teardown routines to lifecycles

d0670f2

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Move ConnMgr construction into DI provider

5299299

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k and others added 15 commits April 17, 2019 16:56

fuse: fix govet warning

c5f887d

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Remove old constructor code

adbc85b

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Cleanup core package

d35dac7

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Move pathresolve

7046626

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Remove DI module dependency on BuildCfg

fd0c06a

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Separate function to parse BuildCfg into Options

3ac6057

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Move option parsing to BuildCfg; fix imports

e4cf660

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

bootstrap: cleanup randomSubsetOfPeers

0e6f8d4

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Invoke Provider.Run in lc.OnStart

803512e

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Rename lcProcess.Run to Append, add docs

bb4b99e

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Minor cleanups in libp2p construction

23f50ab

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Cleanup routing related units

bf380b8

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Handle Provider closing in lifecycle

d574658

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Remove relay address filtering logic

19cc7c9

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

chore: remove duplicate import

c9ea728

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the feat/dig branch from b1e9f63 to c9ea728 Compare April 17, 2019 23:56

ghost assigned Stebalien Apr 17, 2019

Stebalien approved these changes Apr 17, 2019

View reviewed changes

Stebalien merged commit c3a7bc2 into master Apr 18, 2019

ghost removed the status/in-progress In progress label Apr 18, 2019

Stebalien deleted the feat/dig branch April 18, 2019 00:06

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#6162 from ipfs/feat/dig

f0387ae

DI-based core.NewNode This commit was moved from ipfs/kubo@c3a7bc2

gammazero pushed a commit to ipfs/boxo that referenced this pull request Sep 29, 2023

Merge pull request ipfs/kubo#6162 from ipfs/feat/dig

ae7d192

DI-based core.NewNode This commit was moved from ipfs/kubo@c3a7bc2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DI-based core.NewNode #6162

DI-based core.NewNode #6162

magik6k commented Apr 1, 2019 •

edited

Loading

magik6k commented Apr 3, 2019

Stebalien left a comment

Stebalien Apr 4, 2019

Stebalien Apr 4, 2019

magik6k Apr 8, 2019

Stebalien Apr 9, 2019

magik6k Apr 15, 2019

magik6k commented Apr 8, 2019

Stebalien commented Apr 9, 2019

Stebalien Apr 9, 2019

Stebalien left a comment

Stebalien Apr 17, 2019

Stebalien Apr 17, 2019

Stebalien Apr 17, 2019

Stebalien Apr 17, 2019

Stebalien Apr 17, 2019

Stebalien Apr 17, 2019

Stebalien Apr 17, 2019

Stebalien commented Apr 18, 2019

Stebalien commented Apr 18, 2019

DI-based core.NewNode #6162

DI-based core.NewNode #6162

Conversation

magik6k commented Apr 1, 2019 • edited Loading

magik6k commented Apr 3, 2019

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

magik6k commented Apr 8, 2019

Stebalien commented Apr 9, 2019

Choose a reason for hiding this comment

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

Stebalien commented Apr 18, 2019

Stebalien commented Apr 18, 2019

magik6k commented Apr 1, 2019 •

edited

Loading