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

FoundationDB physical backend #4900

Merged
merged 16 commits into from Jul 16, 2018

Conversation

Projects
None yet
2 participants
@jblache
Contributor

jblache commented Jul 11, 2018

This PR implements a FoundationDB physical storage backend for Vault, along with documentation, tests, and build system integration for the FoundationDB Go bindings.

The backend implements the transactional and HA interfaces; locks rely on correct time synchronization of the Vault nodes to operate properly.

Data model

The FoundationDB key is built by decorating the item path handed over to the backend by Vault core, to allow for an efficient list operation implementation using a single range read. The backend stores the data using 3 subspaces under a top-level directory:

  • a data subspace containing the actual data;
  • a shadow copy of the data, consisting of only the keys with nil values, used for the list operation;
  • a subspace dedicated to locks for the HA backend implementation.

The shadow copy is expected to eventually go away when a range read operation for keys only will be available. Bookkeeping for sub-levels employs FoundationDB’s atomic operations to reduce transaction conflicts; this is not true of the delete path at present, pending the availability of a decrease-delete-if-0 type of operation.

Testing

Caveat: there isn’t a readily available FoundationDB Docker image at this time. This is being worked on (apple/foundationdb#355), but until that lands, you will need to run a FoundationDB server locally to run the tests (https://apple.github.io/foundationdb/getting-started-linux.html).

$ export FOUNDATIONDB_CLUSTER_FILE=/etc/foundationdb/fdb.cluster
$ make test TEST=./physical/foundationdb

The test code includes the required setup code for using a Docker image, so a minor touch up should be all that’s needed once a Docker image is available for FoundationDB.

@jefferai

This comment has been minimized.

Member

jefferai commented Jul 11, 2018

Please remove the changes to the Makefile. Cgo must be forced to zero and bootstrap should not bring in outside libraries.

@jblache

This comment has been minimized.

Contributor

jblache commented Jul 11, 2018

Hey Jeff,

On CGO: FoundationDB language bindings are built on top of the C client library, so I'm afraid CGO is a requirement. Please let me know if I'm mistaken in this regard, and I'll be happy to make the necessary changes.

On the bootstrap changes, this could certainly be done differently. I am not sure the FoundationDB Go bindings can be treated similarly to other Go dependencies though, and any advice here would be appreciated. Due to the code that needs to be generated/built and the CGO nature of the bindings, I don't know that a different approach would necessarily result in a better situation overall; the fdb-go-install script is the supported way to get the job done at the moment. Suggestions on how to make this better are welcome!

@jefferai

This comment has been minimized.

Member

jefferai commented Jul 11, 2018

@jblache We don't currently offer dynamic builds of Vault. I don't know how much interest there is in FoundationDB as a storage backend but we could probably have a make target like dynamic-dev or dynamic-bin. Or -- and I'm not sure if this is actually doable -- it'd be cool if you could do something like make dynamic dev where the dynamic target modifies something in the running context such that the next target is then built with cgo enabled. That'd be way better if it's possible.

As for bootstrap, the only things that should be in bootstrap are items necessary for actually building Vault as a whole. If you wanted a separate Makefile in physical/foundationdb to allow for installation of tools that's fine, but we should not require all users to install FoundationDB bindings just to build Vault.

jblache added some commits Jul 11, 2018

@jblache

This comment has been minimized.

Contributor

jblache commented Jul 12, 2018

@jefferai I believe this batch of changes should address your concerns. I've made the backend conditional; the Makefile will enable CGO for all build targets if the backend is enabled, as it is required. By default, the build is exactly as it is today. Attempts to use the backend will fail with a descriptive error message in a build that doesn't have it enabled.

I've removed the Go bindings installation from the bootstrap target and documented that step instead.

.gitignore Outdated
@@ -87,3 +87,7 @@ ui/vault-ui-integration-server.pid
# for building static assets
node_modules
package-lock.json
# FoundationDB
vendor/fdb-go-install.sh

This comment has been minimized.

@jefferai

jefferai Jul 12, 2018

Member

These don't appear to be actual files.

@@ -0,0 +1,330 @@
#!/bin/bash -eu

This comment has been minimized.

@jefferai

jefferai Jul 12, 2018

Member

This should be in physical/foundationdb

- **High Availability** – the FoundationDB storage backend supports high
availability. The HA implementation relies on the clocks of the Vault
nodes inside the cluster being properly sychronized; clock skews are
susceptible to cause issues.

This comment has been minimized.

@jefferai

jefferai Jul 12, 2018

Member

What kinds of issues, specifically?

storage "foundationdb" {
api_version = 510
cluster_file = "/path/to/fdb.cluster"
path = "vault-top-level-directory"

This comment has been minimized.

@jefferai

jefferai Jul 12, 2018

Member

Please add an ha_enabled flag and gate HA support on it. See e.g. dynamo for guidance.

@jblache

This comment has been minimized.

Contributor

jblache commented Jul 12, 2018

@jefferai All done!

.gitignore Outdated
@@ -87,3 +87,6 @@ ui/vault-ui-integration-server.pid
# for building static assets
node_modules
package-lock.json
# FoundationDB Go bindings
vendor/foundation

This comment has been minimized.

@jefferai

jefferai Jul 13, 2018

Member

What is this here for? I assumed it was because go bindings were being put into vendor, but the script below doesn't mention this directory at all so I don't see any indication it's being added to the vendor directory (and I'd argue it's the wrong place for it since the vendor directory is automatically managed by govendor).

This comment has been minimized.

@jblache

jblache Jul 13, 2018

Contributor

Drats, I could have caught that last time around. This is a leftover from my early attempts, and it can go.

#
DESTDIR="${DESTDIR:-}"
FDBVER="${FDBVER:-5.1.0}"

This comment has been minimized.

@jefferai

jefferai Jul 13, 2018

Member

Should this be something like a latest tag? I just wonder whether this will end up just being out of date. Future PRs could bump it, so if it's a good idea to use "known good versions" that's a good argument, but just trying to understand the situation.

This comment has been minimized.

@jblache

jblache Jul 13, 2018

Contributor

Yes, this is a known-good version kind of situation. The script is taken from the 5.1.x branch, so it defaults to that. FDBVER can be set when calling the script to get another version, and it should work just fine.

Having the script in the repo made obvious sense when it was called from bootstrap, but since that isn't the case anymore, I guess we can remove it and update the documentation to match. Let me know which way you are leaning.

This comment has been minimized.

@jefferai

jefferai Jul 16, 2018

Member

I'm happy for the script to be in the repo, just want to understand these questions for the inevitable user queries :-)

Remove gitignore entry for FDB bindings
This was a leftover from early work.
@jefferai

This comment has been minimized.

Member

jefferai commented Jul 16, 2018

Just a note: one of the other Vault team members looked at the code and said LGTM so I didn't examine the code too carefully, just have been looking at overall organization, which I'm happy with now. Going to merge, and I'm hoping you'll be pingable in case people find issues :-)

@jefferai jefferai merged commit e59d193 into hashicorp:master Jul 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jefferai jefferai added this to the 0.10.4 milestone Jul 16, 2018

@jblache

This comment has been minimized.

Contributor

jblache commented Jul 16, 2018

Yay! Thanks for merging :)

@jefferai

This comment has been minimized.

Member

jefferai commented Jul 16, 2018

Thanks for submitting :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment