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

Scale www database #689

Closed
wants to merge 2 commits into from
Closed

Conversation

fernandoabolafio
Copy link
Member

@fernandoabolafio fernandoabolafio commented Feb 12, 2019

This PR rewrites the database interface and adds another database option, CokroachDB, making it more generic and robust.
The full list of changes implemented are:

  • CockroachDB implementation of the database interface.
  • LevelDB implementation of the database interface.
  • Database Encryption capability.
  • Upgrade of the tool politeiawww_dbutil which now has an implementation similar to politeiawwwcli and has additional commands to handle the database migration.

The setup of cockroachdb can be done by using the same script introduced in the cache PR.

Instructions of how to switch from the old implementation and from leveldb to cockroachdb can be found in the politeiawww_dbutilpackage README.

Other changes implemented in this PR are:

  • The user API for fetching user data from the database was moved over the main package of politeiawww.
  • Users are stored by ID and not by email anymore in the key-value map of the database. This reduces the system dependency on the user email.

closes #545

Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

The database setup process also needs to be updated to reflect the changes that were made in #693.

Its possible to use one or another with politeiawww by specifying the `database` configuration.

#### Using LevelDB
LevelDB requires a minimal configuration. The only required config options is `datadir` which specifies where the directory
Copy link
Member

Choose a reason for hiding this comment

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

datadir is not a required config setting since it has a default.


#### Using CockroachDB
CockroachDB requires the config options `dbhost`, `dbcertdir`, `dbrootcert` to connect to the running db instance. In order to
setup the Database you can run the same script as specified in the cache instructions:
Copy link
Member

Choose a reason for hiding this comment

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

Is the capitalization of Database here intended?

@@ -241,6 +241,45 @@ to setup Politeia and access it through the UI.
* Use the [politeia](https://github.com/decred/politeia/tree/master/politeiad/cmd/politeia) tool to interact directly with politeiad.
* Use any other tools or clients that are listed above.

### Users database setup
Copy link
Member

Choose a reason for hiding this comment

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

The rest of the markdown file is formatted to adhere to the 80 column limit (unless its a link of code snippet). Can you do the same here?

`$ politeiawww_dbutil <command> -h`


## Setup Configuration File
Copy link
Member

Choose a reason for hiding this comment

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

There is an inconsistent naming convention between OS's here. The macOS is putting the conf file in politeiawww/dbutil where as windows and ubuntu put it in politeiawww/cli.

More generally though, politeiawww_dbutil conf and data should probably not be part of the politeiawww directory.

osDataDir/politeiawww_dataload instead of osDataDir/politeiawww/dbutil


### Migrating the database

If you are running an older version of the database you need to migrate
Copy link
Member

Choose a reason for hiding this comment

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

I understand the need for a migration tool if you're migrating from leveldb to cockroachdb, but shouldn't version migrations be handled automatically inside of politeiawww?

)

const (
ErrorNoUserIdentity = "No user identity found. You must login with a user."
Copy link
Member

Choose a reason for hiding this comment

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

Why are these errors here?

DBCertDir string `long:"dbcertdir" description:"Directory containing SSL client certificates"`
DBRootCert string `long:"dbrootcert" description:"File containing SSL root certificate"`
EncryptDB bool `long:"encryptdb" description:"If true the database will encrypt/decrypt for saving and retrieving records"`
RawJSON bool `short:"j" long:"json" description:"Print raw JSON output"`
Copy link
Member

Choose a reason for hiding this comment

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

Why do you have RawJSON and Verbose as config params when they're not actually used anywhere in the tool?


// cleanAndExpandPath expands environment variables and leading ~ in the passed
// path, cleans the result, and returns it.
func cleanAndExpandPath(path string) string {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be updated with the changes in #694.


// NewCDB returns a new cockroachdb context which contains a connection to the
// specified database that was made using the passed in user and certificates.
func NewCDB(user, host, net, rootCert, certDir string, dbKey *database.EncryptionKey, cfg *Config) (*cockroachdb, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Style nit. This function gets called as cockroachdb.NewCDB(). The CDB is redundant since the package name is already cockroachdb.

https://golang.org/doc/effective_go.html#package-names

@@ -86,6 +87,8 @@ type backend struct {

// Following entries require locks
userPubkeys map[string]string // [pubkey][userid]
userEmail map[string]string // [email][userid]
userUsername map[string]string // [username][userid]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be adding in-memory caches to www if we're just going to have to remove them in a few weeks. The way I suggest doing this would be to store the user record as:

type User struct {
	UserID   string // primary key
	Username string 
	Payload  []byte // encrypted blob with the rest of the user details
}

That way we can do user lookups using either the userID or username and eliminate the need for a userUsername in-memory cache. The userEmail in-memory cache would still be required temporarily, but since we want to make email optional anyway, we'll need to switch all email lookups in www to be either username or userID lookups. When we do this we can remove the userEmail in-memory cache completely.

@lukebp
Copy link
Member

lukebp commented May 8, 2019

Implemented by #856.

@lukebp lukebp closed this May 8, 2019
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.

[www] Make www scalable
2 participants