Flat keyspace directory backend. #2062

russjones · 2018-07-11T01:08:02Z

Purpose

tsh ssh (and other commands like tsh ls) take a long time on large clusters. For example, on a 4k node cluster with a local etcd instance as the backend and auth, proxy, node all running in the same process, tsh ssh can take over 6 seconds. These numbers are larger for real world deployments where auth, proxy, and node run on different machines as well as etcd.

This is due to two reasons:

During login, tsh calls GetNodes several times. Each time GetNodes is called, it's validates the schema of the services.Server it returns. In the example 4k cluster, this in of itself takes up to 4 seconds. The solution to this is make schema validation optional in calls to GetNodes.
The remaining second is spent by the state cache re-upserting 4k items back into the dir backend. For example, writing 600 bytes to a new file takes about 200 us but writing 600 bytes to an open file takes about 10 us. To reduce the time spent re-upserting 4k items into the dir backend, the dir backend needs to support bulk insertion.

Fixes to these two problems should reduce login time to 500 ms to 1 second for 4k clusters.

Implementation

For the first problem, the API for GetNodes(namespace string) was updated to GetNodes(namespace string, skipValidation bool) allowing the caller to control if schema validation is run or not. In addition, json-iterator was used to speed up marshalling and unmarshalling.

For the second problem, the directory backend was re-written to support a flat keyspace. Once support for a flat keyspace was added, then the directory backend could store buckets with content being a list of keys. Then backend operations would simply be reading in all the keys in a bucket, operating on the map of items in the bucket, and then re-writing it back out. This reduces the time a upsert takes from 200 us to 10 us for each item, which for 4k nodes reduces total time from 800 ms to 40 ms.

Related Issues

Fixes #2061

klizhentas · 2018-07-11T16:41:51Z

lib/auth/api.go

@@ -39,7 +39,7 @@ type AccessPoint interface {
 	GetNamespace(name string) (*services.Namespace, error)

 	// GetServers returns a list of registered servers
-	GetNodes(namespace string) ([]services.Server, error)
+	GetNodes(namespace string, skipValidation bool) ([]services.Server, error)


let's make this a functional argument to keep the API consistent

so the signature will look like this:

GetNodes(namespace, SkipValidation())

klizhentas · 2018-07-11T16:45:25Z

lib/backend/dir/impl.go

+	var out []backend.Item
+
+	// Get a list of all buckets in the backend.
+	files, err := ioutil.ReadDir(path.Join(bk.rootDir))


you know here in advance the list of buckets files to open, why do a full scan?

At this point, the actual bucket to open is not known. Take for example a backend with the following items in it:

/roles/admin/params /roles/dev/params /namespaces/default/nodes/123

If bucket is /namespaces/default/nodes, then GetItems don't not need to do a full directory scan and can return the exact bucket (or return trace.NotFound if it does not exist).

However if the caller asks for /roles (which is what GetRoles does), that's when the directory scan needs to occur perform a prefix match then extract the first matching bucket and return it as a key.

klizhentas · 2018-07-11T16:48:04Z

lib/backend/dir/impl.go

+			// If the bucket item is expired, update the bucket, and don't include
+			// it in the output.
+			if bk.isExpired(v) {
+				delete(b.items, k)


I would prefer this logic withitemsUpdated and delete operations to be encapsulated in the bucket, so

b.deleteItem(k) should handle this internal property itemsUpdated

klizhentas · 2018-07-11T16:49:25Z

lib/backend/dir/impl.go

+		// backend and see if any match the prefix. If any match the prefix return
+		// trace.BadParameter, otherwise return the original error. This is
+		// consistent with our DynamoDB implementation.
+		files, er := ioutil.ReadDir(path.Join(bk.rootDir))


we should not do a full scan of the bucket in this case, why not try to open the files with partial prefix instead?

This is because GetVal has to return trace.BadParameter when specifically requesting a bucket. Take for example a backend with a single item in it:

/foo/bar/baz

If the bucket requested was /foo/bar/baz then we can just check for that particular bucket and return trace.BadParameter if we are unable to open a bucket.

However, if the caller asks for /foo there is no way to differentiate the bucket does not exist at all from the caller requesting a bucket that does exist (when we would return trace.BadParameter.

klizhentas · 2018-07-11T16:50:27Z

lib/backend/dir/impl.go

+
+	// Update items in bucket.
+	for _, e := range newItems {
+		b.items[e.Key] = bk.itemWithTTL(e.Value, e.TTL)


this should be setItem that updates internal property itemsUpdated

klizhentas · 2018-07-11T16:52:17Z

lib/backend/dir/impl.go

+
+	// If the items were updated, write them out to disk.
+	if b.itemsUpdated {
+		err = writeOut(b.file, b.items)


if you failed to write, still close the bucket, otherwise you will get a deadlock situation on busy file system

klizhentas · 2018-07-11T16:52:35Z

lib/backend/dir/impl.go

+
+// readIn will read in the bucket and return a map of keys. The second return
+// value returns true to false to indicate if the file was empty or not.
+func readIn(f *os.File) (map[string]bucketItem, error) {


klizhentas · 2018-07-11T16:52:41Z

lib/backend/dir/impl.go

+// writeOut will truncate the file and write out the items to the file f.
+func writeOut(f *os.File, items map[string]bucketItem) error {
+	// Marshal items to disk format.
+	bytes, err := json.Marshal(items)


writeBucket

klizhentas · 2018-07-11T16:54:32Z

lib/utils/jsontools.go

 	"github.com/ghodss/yaml"
+	"github.com/json-iterator/go"


dependency is not vendored

I was actually surprised by this as well, turns out it's already vendored.

kontsevoy · 2018-07-13T00:02:20Z

@russjones does this require a migration?

klizhentas · 2018-07-13T00:03:27Z

yes, that's why it will go to 2.7

russjones · 2018-07-13T00:04:24Z

@kontsevoy Yes, it's in lib/backend/dir/migrate.go. It's relatively safe but there is a scenario where if the backend directories were poorly chosen it could fail.

all backends to support bulk insertion. Added UpsertNodes endpoint, which is used by the state cache to speed up GetNodes.

russjones requested a review from klizhentas July 11, 2018 01:08

klizhentas requested changes Jul 11, 2018

View reviewed changes

russjones force-pushed the rjones/dir-perf branch 2 times, most recently from 064e1ae to 3a592d2 Compare July 11, 2018 22:53

Updated dir backend to a flat keyspace. Added UpsertItems endpoint to

ce1c747

all backends to support bulk insertion. Added UpsertNodes endpoint, which is used by the state cache to speed up GetNodes.

russjones force-pushed the rjones/dir-perf branch from 3a592d2 to ce1c747 Compare July 13, 2018 20:12

russjones merged commit 71ffacd into master Jul 13, 2018

russjones deleted the rjones/dir-perf branch July 13, 2018 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flat keyspace directory backend. #2062

Flat keyspace directory backend. #2062

russjones commented Jul 11, 2018

klizhentas Jul 11, 2018

klizhentas Jul 11, 2018

russjones Jul 11, 2018 •

edited

Loading

klizhentas Jul 11, 2018

klizhentas Jul 11, 2018

russjones Jul 11, 2018 •

edited

Loading

klizhentas Jul 11, 2018

klizhentas Jul 11, 2018

klizhentas Jul 11, 2018

klizhentas Jul 11, 2018

klizhentas Jul 11, 2018

russjones Jul 11, 2018

kontsevoy commented Jul 13, 2018

klizhentas commented Jul 13, 2018

russjones commented Jul 13, 2018 •

edited

Loading

		"github.com/ghodss/yaml"
		"github.com/json-iterator/go"

Flat keyspace directory backend. #2062

Flat keyspace directory backend. #2062

Conversation

russjones commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kontsevoy commented Jul 13, 2018

klizhentas commented Jul 13, 2018

russjones commented Jul 13, 2018 • edited Loading

russjones Jul 11, 2018 •

edited

Loading

russjones Jul 11, 2018 •

edited

Loading

russjones commented Jul 13, 2018 •

edited

Loading