-
Notifications
You must be signed in to change notification settings - Fork 267
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
Testing CI for moving where the IPFS node is init #329
Changes from 1 commit
31c996a
1522ee8
052bdbe
f9b9629
760d371
e49a6ed
8243b27
8dd6964
24153f7
61969de
563e94e
235da5b
300e0fb
98c9f13
4dcd7a8
e8e50fc
672ce3b
76bae0c
5956145
39b5c75
330e9dd
0d9509a
d4a3bf8
1256a42
edc26f4
f80f731
80f6d75
7847b9d
d7aa886
d0034ec
48a8305
03a4329
24681e6
df5b458
d11f9d4
9d53b41
bc81eff
3e5f077
5918bc0
9dbf03a
d865cc7
8acce6b
a127447
5c39da7
a6c85fc
304537f
717ffd0
2a33518
4c4bad0
74795bf
ee766c7
51d9e95
912a067
d5dbe9a
cc5fd2d
10c3334
818c823
3bc1510
8301d52
6a47a55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,15 +8,19 @@ import ( | |
"net" | ||
"net/http" | ||
_ "net/http/pprof" // nolint: gosec // securely exposed on separate, optional port | ||
"os" | ||
"path/filepath" | ||
"strings" | ||
"time" | ||
|
||
ipfscfg "github.com/ipfs/go-ipfs-config" | ||
ipfscore "github.com/ipfs/go-ipfs/core" | ||
coreapi "github.com/ipfs/go-ipfs/core/coreapi" | ||
"github.com/ipfs/go-ipfs/core/coreapi" | ||
"github.com/ipfs/go-ipfs/core/node/libp2p" | ||
"github.com/ipfs/go-ipfs/plugin/loader" | ||
"github.com/ipfs/go-ipfs/repo/fsrepo" | ||
"github.com/ipfs/interface-go-ipfs-core/options" | ||
tmos "github.com/lazyledger/lazyledger-core/libs/os" | ||
"github.com/lazyledger/lazyledger-core/p2p/ipld/plugin/nodes" | ||
"github.com/prometheus/client_golang/prometheus" | ||
"github.com/prometheus/client_golang/prometheus/promhttp" | ||
|
@@ -651,6 +655,21 @@ func NewNode(config *cfg.Config, | |
logger log.Logger, | ||
options ...Option) (*Node, error) { | ||
|
||
err := InitIpfs(config, logger) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
ipfsNode, err := createIpfsNode(config, true, logger) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to create IPFS node: %w", err) | ||
} | ||
|
||
ipfsAPI, err := coreapi.NewCoreAPI(ipfsNode) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to create an instance of the IPFS core API: %w", err) | ||
} | ||
Comment on lines
+663
to
+671
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks right to me. But I'm wondering if we shouldn't just pass in a Would also like to hear what @Wondertan thinks about this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that idea better than the current implementation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agree. Having dependency initialized outside of a component is also a good practice, which for our case would allow more flexibility to allow users to use custom IPFS nodes until we decide to fully absorb IPFS(not just embed) by using its modules directly. The latter may be possible in the future as an optimization vector. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @liamsi, @evan-forbes, but how do we then ensure that a custom IPFS node includes the needed plugin(s)? That is not clear for me now and may require some investigation. I think we can still do the proposed changed on a code level for now(pass CoreAPI to NewNode), but still not give a user ability to use a custom one, until we understand how we can ensure that, as doing that now would be a waste of time. @liamsi, if you haven't started this yet as part of the cleanup, I can include this change in my PR with API exposure to HTTP. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I have started it only in the sense that I've extracted two methods to a file for initialization and for the creation of the node. The changes that are missing is to move the creation out of node.OnStart (and similarly out of the light client constructor - but that is not a concern on master yet). Regarding the plugins: That code also exist on master (only at a different place). I'm not sure how to assert that the plugin was loaded if we use another approach. |
||
|
||
blockStore, stateDB, err := initDBs(config, dbProvider) | ||
if err != nil { | ||
return nil, err | ||
|
@@ -687,7 +706,7 @@ func NewNode(config *cfg.Config, | |
// If an address is provided, listen on the socket for a connection from an | ||
// external signing process. | ||
if config.PrivValidatorListenAddr != "" { | ||
// FIXME: we should start services inside OnStart | ||
// FIXME: we should start services inside rt | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably a typo here |
||
privValidator, err = createAndStartPrivValidatorSocketClient(config.PrivValidatorListenAddr, genDoc.ChainID, logger) | ||
if err != nil { | ||
return nil, fmt.Errorf("error with private validator socket client: %w", err) | ||
|
@@ -768,6 +787,8 @@ func NewNode(config *cfg.Config, | |
privValidator, csMetrics, stateSync || fastSync, eventBus, consensusLogger, | ||
) | ||
|
||
consensusState.IpfsAPI = ipfsAPI | ||
|
||
// Set up state sync reactor, and schedule a sync if requested. | ||
// FIXME The way we do phased startups (e.g. replay -> fast sync -> consensus) is very messy, | ||
// we should clean this whole thing up. See: | ||
|
@@ -950,23 +971,6 @@ func (n *Node) OnStart() error { | |
return fmt.Errorf("failed to start state sync: %w", err) | ||
} | ||
} | ||
if n.embedIpfsNode { | ||
// It is essential that we create a fresh instance of ipfs node on | ||
// each start as internally the node gets only stopped once per instance. | ||
// At least in ipfs 0.7.0; see: | ||
// https://github.com/lazyledger/go-ipfs/blob/dd295e45608560d2ada7d7c8a30f1eef3f4019bb/core/builder.go#L48-L57 | ||
n.ipfsNode, err = createIpfsNode(n.config, n.areIpfsPluginsAlreadyLoaded, n.Logger) | ||
if err != nil { | ||
return fmt.Errorf("failed to create IPFS node: %w", err) | ||
} | ||
|
||
ipfsAPI, err := coreapi.NewCoreAPI(n.ipfsNode) | ||
if err != nil { | ||
return fmt.Errorf("failed to create an instance of the IPFS core API: %w", err) | ||
} | ||
|
||
n.consensusState.IpfsAPI = ipfsAPI | ||
} | ||
|
||
return nil | ||
} | ||
|
@@ -1526,3 +1530,84 @@ func splitAndTrimEmpty(s, sep, cutset string) []string { | |
} | ||
return nonEmptyStrings | ||
} | ||
|
||
// InitIpfs takes a few config flags from the tendermint config.IPFS | ||
// and applies them to the freshly created IPFS repo. | ||
// The IPFS config will stored under config.IPFS.ConfigRootPath. | ||
// TODO(ismail) move into separate file, and consider making IPFS initialization | ||
// independent from the `tendermint init` subcommand. | ||
// TODO(ismail): add counter part in ResetAllCmd | ||
func InitIpfs(config *cfg.Config, logger log.Logger) error { | ||
repoRoot := config.IPFSRepoRoot() | ||
if fsrepo.IsInitialized(repoRoot) { | ||
logger.Info("IPFS was already initialized", "ipfs-path", repoRoot) | ||
return nil | ||
} | ||
var conf *ipfscfg.Config | ||
|
||
identity, err := ipfscfg.CreateIdentity(os.Stdout, []options.KeyGenerateOption{ | ||
options.Key.Type(options.Ed25519Key), | ||
}) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
logger.Info("initializing IPFS node", "ipfs-path", repoRoot) | ||
|
||
if err := tmos.EnsureDir(repoRoot, 0700); err != nil { | ||
return err | ||
} | ||
|
||
conf, err = ipfscfg.InitWithIdentity(identity) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
applyFromTmConfig(conf, config.IPFS) | ||
if err := setupPlugins(repoRoot, logger); err != nil { | ||
return err | ||
} | ||
|
||
if err := fsrepo.Init(repoRoot, conf); err != nil { | ||
return err | ||
} | ||
return nil | ||
} | ||
|
||
// // Inject replies on several global vars internally. | ||
// // For instance fsrepo.AddDatastoreConfigHandler will error | ||
// // if called multiple times with the same datastore. | ||
// // But for CI and integration tests, we want to setup the plugins | ||
// // for each repo but only inject once s.t. we can init multiple | ||
// // repos from the same runtime. | ||
// // TODO(ismail): find a more elegant way to achieve the same. | ||
// var injectPluginsOnce sync.Once | ||
|
||
// func setupPlugins(path string) error { | ||
// // Load plugins. This will skip the repo if not available. | ||
// plugins, err := loader.NewPluginLoader(filepath.Join(path, "plugins")) | ||
// if err != nil { | ||
// return fmt.Errorf("error loading plugins: %s", err) | ||
// } | ||
|
||
// if err := plugins.Initialize(); err != nil { | ||
// return fmt.Errorf("error initializing plugins: %s", err) | ||
// } | ||
|
||
// injectPluginsOnce.Do(func() { | ||
// err = plugins.Inject() | ||
// }) | ||
// if err != nil { | ||
// return fmt.Errorf("error injecting plugins once: %w", err) | ||
// } | ||
|
||
// return nil | ||
// } | ||
|
||
func applyFromTmConfig(ipfsConf *ipfscfg.Config, tmConf *cfg.IPFSConfig) { | ||
ipfsConf.Addresses.API = ipfscfg.Strings{tmConf.API} | ||
ipfsConf.Addresses.Gateway = ipfscfg.Strings{tmConf.Gateway} | ||
ipfsConf.Addresses.Swarm = tmConf.Swarm | ||
ipfsConf.Addresses.Announce = tmConf.Announce | ||
ipfsConf.Addresses.NoAnnounce = tmConf.NoAnnounce | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's constantly true, maybe change then
createIpfsNode
?