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.

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

Decouple backend DB and BigchainDB Docker containers #1174

Merged
merged 3 commits into from Feb 15, 2017
Merged

Conversation

krish7919
Copy link
Contributor

Solved #1153.

@krish7919 krish7919 self-assigned this Feb 8, 2017
@krish7919 krish7919 requested review from sbellem, ttmc and sohkai and removed request for sohkai February 8, 2017 16:12
@codecov-io
Copy link

codecov-io commented Feb 8, 2017

Codecov Report

Merging #1174 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1174   +/-   ##
=======================================
  Coverage   96.92%   96.92%           
=======================================
  Files          49       49           
  Lines        2279     2279           
=======================================
  Hits         2209     2209           
  Misses         70       70

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e479d8...2a9042f. Read the comment docs.

@@ -34,8 +34,6 @@ ENV BIGCHAINDB_SERVER_BIND 0.0.0.0:9984
# but maybe our Docker or Docker Compose stuff does?
# ENV BIGCHAINDB_API_ENDPOINT http://bigchaindb:9984/api/v1

ENTRYPOINT ["bigchaindb", "--dev-start-rethinkdb", "--dev-allow-temp-keypair"]
ENTRYPOINT ["bigchaindb"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why you can remove the --dev-allow-temp-keypair flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand, --dev-allow-temp-keypair flag is used to create a temp keypair.
If we go into production with this, we should be able to set a keypair ourselves rather than let bigchaindb generate it.
We also need to do key management and periodic recycling of the keys.
I have removed it as a mandatory entrypoint option, and let devs specify it explicitly during testing, as is documented below.


CMD ["start"]

EXPOSE 8080 9984 28015 29015
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why port 9984 isn't exposed. (The other ones are for RethinkDB, which is gone from this container, so that much is clear.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a hint to docker to expose ports for container networking.
The real option required is --publish over here. We were exposing non-required ports before.

* `bigchaindb/bigchaindb` the image to use. All the options after the container name are passed on to the entrypoint inside the container.
* `-y configure` execute the `configure` sub-command (of the `bigchaindb`
command) inside the container, with the `-y` option to automatically use all the default config values
* `--dev-allow-temp-keypair` specifies that this is a dev environment and
Copy link
Contributor

Choose a reason for hiding this comment

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

The --dev-allow-temp-keypair option generates one temporary keypair if none is found, but it's usually used with the bigchaindb start command, not the bigchaindb configure command, e.g. see: https://docs.bigchaindb.com/projects/server/en/master/server-reference/bigchaindb-cli.html#bigchaindb-start

Doesn't bigchaindb -y configure generate a keypair?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would want a temp keypair created when we set up the configuration file.
Is it really require a --dev-allow-temp-keypair option? It can fail even if no conf file is found.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's what happens when the file $HOME/.bigchaindb file does not exist:

$ bigchaindb -y configure rethinkdb
Generating keypair
Generating default configuration for backend rethinkdb
Configuration written to /home/troy/.bigchaindb
Ready to go!

It generates a keypair.

The --dev-allow-temp-keypair option exists (as an option to go with bigchaindb start) for the cases when you want to be able to do bigchaindb start without doing a bigchaindb -y configure <backend> first. Most BigchainDB configuration variables have default values: the only exception is the keypair: there is no default keypair.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, I see you removed the --dev-allow-temp-keypair from the actual docker command (above); this comment about --dev-allow-temp-keypair should also be removed.

### Run the backend database
From v0.9 onwards, you can run either RethinkDB or MongoDB.

We use the virtual interface created by the docker daemon to allow
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a small thing, but "Docker" is a proper noun and should be capitalized when used in regular English prose. The exception is when you're referring specifically to the docker command.


We use the virtual interface created by the docker daemon to allow
communication between the BigchainDB and database containers.
It has an IP address of 172.17.0.1 by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting! Can you link to the Docker documentation about 172.17.0.1? I can't help but wonder if there's a less arcane way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will not use the docker vnic in production, as both containers need routable IPs.
From here:
The bridge network represents the docker0 network present in all Docker installations. Unless you specify otherwise with the docker run --network=<NETWORK> option, the Docker daemon connects containers to this network by default. You can see this bridge as part of a host’s network stack by using the ifconfig command on the host.

@ttmc ttmc changed the title Decouple backend DB and BigchainDB Decouple backend DB and BigchainDB Docker containers Feb 9, 2017
@krish7919 krish7919 merged commit e71ee0d into master Feb 15, 2017
@vrde vrde deleted the issue-1153 branch March 21, 2018 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants