hyperledger-archives / fabric

THIS IS A READ-ONLY historic repository. Current development is at https://gerrit.hyperledger.org/r/#/admin/projects/fabric . pull requests not accepted
https://gerrit.hyperledger.org/
Apache License 2.0
1.17k stars 1.01k forks source link

consensus layer seems to depend on src-tree presence to operate properly #1648

Open ghaskins opened 8 years ago

ghaskins commented 8 years ago

The method to install the peer binary as a stand-alone application is not readily apparent because the current code appears to depend on certain things being found within $GOPATH/src/github.com/hyperledger/fabric, like consensus/obcpbft/config.yaml.

This should be cleaned up before production operation will be possible.

Describe How to Reproduce

Take the peer binary and ./peer/core.yaml, install them within a clean-room environment (sans hyperledger/fabric git src) and try to run "peer help". It will crash with:

panic: Error reading CORE_PBFT plugin config: Unsupported Config Type ""

goroutine 1 [running]:
panic(0xd00680, 0xc8201aa440)
    /opt/go/src/runtime/panic.go:464 +0x3e6
github.com/hyperledger/fabric/consensus/obcpbft.loadConfig(0xc8201371e0)
    /opt/gopath/src/github.com/hyperledger/fabric/consensus/obcpbft/obc-pbft.go:90 +0x7dd
github.com/hyperledger/fabric/consensus/obcpbft.init.1()
    /opt/gopath/src/github.com/hyperledger/fabric/consensus/obcpbft/obc-pbft.go:39 +0x18
github.com/hyperledger/fabric/consensus/obcpbft.init()
    /opt/gopath/src/github.com/hyperledger/fabric/consensus/obcpbft/viewchange.go:588 +0xa3
github.com/hyperledger/fabric/consensus/controller.init()
    /opt/gopath/src/github.com/hyperledger/fabric/consensus/controller/controller.go:48 +0x54
github.com/hyperledger/fabric/consensus/helper.init()
    /opt/gopath/src/github.com/hyperledger/fabric/consensus/helper/helper.go:346 +0x53
main.init()
    /opt/gopath/src/github.com/hyperledger/fabric/peer/main.go:1055 +0xbf

Running strace on the startup reveals the process is searching as follows:

stat("/config.yaml", 0xc8201b05e8)      = -1 ENOENT (No such file or directory)
stat("/consensus/obcpbft/config.yaml", 0xc8201b0b98) = -1 ENOENT (No such file or directory)
stat("/opt/gopath/src/github.com/hyperledger/fabric/consensus/obcpbft/config.yaml", 0xc8201b1148) = -1 ENOENT (No such file or directory)

You can fake it by artificially satisfying the search (i.e. make sure you have both core.yaml and config.yaml in the $CWD, but this is sloppy. A better, more unified way to specify the configuration should be supplied, or the dependency needs to be embedded at build time.

mastersingh24 commented 8 years ago

at a minimum, there needs to be a central place to store config file(s) and a unified way for accessing config. When it comes to consensus, it is silly that each module loads its own config even when not being used

kchristidis commented 8 years ago

When it comes to consensus, it is silly that each module loads its own config even when not being used.

(Emphasis mine.) Not sure where this comes from, and whether I'm parsing the statement properly, but if package my-custom-consensus is tasked with loading its config file (example), and package my-custom-consensus is never imported, its config file will not be loaded, correct?

At a minimum, there needs to be a central place to store config file(s) and a unified way for accessing config.

Agreed. Any nays for a <consensus-module-name>.yaml in the peer package?

I'd like to avoid adding a consensus section in peer/core.yaml so as to make testing different modules as frictionless as possible (i.e. remove the need to rewrite the section).

A consensus.yaml config in the peer package with <consensus-module-name> sections could also work I guess.

ghaskins commented 8 years ago

@kchristidis I suspect what @mastersingh24 was getting at is that a command line "peer help" in theory probably doesn't need features like the consensus subsystem, so why is it trying to load its config? I suppose that is indeed a good goal, but I am less worried about optimizing the runtime in this manner and more worried about supporting a production-like deployment (binary+config in standard places like /usr/sbin and /etc, no source).

To that point, I have no specific objection to the feature-scoped configuration elements nor avoiding the pollution of the top-level configuration file. I think the problem is more of default/overridable search paths more than anything.

To be more concrete, a hypothetical installation of a package "foobar" might consist of a binary in /usr/sbin/foobard, a config in /etc/foobard.conf, and init-script in /etc/init.d that invokes "foobard -c /etc/foobard.conf".

More complicated applications (take apache, for instance) might have a hierarchy of config in a subdirectory (e.g. /etc/apache/*).

What I propose is that we support some kind of "config-prefix" that might expect to find the following tree relative to the prefix:

hyperledger/
└── fabric
    ├── consensus
    │   └── obcpbft
    │       └── config.yaml
    └── peer
        └── core.yaml

The default for the prefix could be "$GOPATH/src/github.com", and distros could do something like "peer -c /etc node start" for production. Thoughts?

mastersingh24 commented 8 years ago

@kchristidis @ghaskins - yes - I meant that the current design tries to load the pbft config even if you are doing noops. Maybe there is not a better way to do that, but given that you can only "run" with one consensus mode at any given time, it might make sense to load the config when you determine which consensus mode you are using.

We can also have multiple config files but I would prefer that we either reference their file path or search directory from the main config (e.g. core.yaml ) or we simply follow the config directory structures of many products out there and have the main config in a some base directory and other configs / overrides in some sub-directory - e.g. this is what typical web servers like apache, nginx, haproxy, etc do.

corecode commented 8 years ago

Most of consensus config should be tied down in the genesis block, and must not be changed after starting the network (#999).

I agree that config file locations need to be reworked. Maybe together with fixing config file verification? (#1309)