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

Add NATS JetStream driver #115

Merged
merged 8 commits into from
Apr 8, 2022

Conversation

matthewdevenny
Copy link
Contributor

This PR proposes to add NATS JetStream as a backend option for kine. NATS adoption on the edge is rapidly growing and backing k3s to NATS is a great fit in environments where NATS is already being employed.

JetStream, which enables new functionalities and higher qualities of service on top of the base 'Core NATS' functionalities and qualities of service, is built-in to nats-server and you only need 1 (or 3 or 5 if you want fault-tolerance against 1 or 2 simultaneous NATS server failures) of your NATS server(s) to be JetStream enabled for it to be available to all the client applications.

@matthewdevenny matthewdevenny force-pushed the feature/add-jetstream-kv branch from 150f7bb to 31f83a1 Compare March 3, 2022 14:25
@matthewdevenny matthewdevenny force-pushed the feature/add-jetstream-kv branch from 31f83a1 to 320f14e Compare March 3, 2022 14:27
@brandond
Copy link
Member

brandond commented Mar 3, 2022

Thanks for the contribution, this is very interesting! I see there are still some bits commented out, and a few things that are waiting on upstream PRs. Did you want to get those worked out before merging?

pkg/endpoint/endpoint.go Outdated Show resolved Hide resolved
@dweomer
Copy link
Contributor

dweomer commented Mar 3, 2022

@boxboatmatt thank you for this contribution! Based on my light reading, I have some concerns that you can likely speak to:

  • as @brandond asked here, I can haz TLS?
  • keyspace seems to be encoded on the wire. will we need a decoder on the nats backend or is this builtin?
  • performance hit for keyspace encoding
    • also i see the two-way hash and it's implied provenance ... are we to assume that a search through potential collision space has been performed exhaustively? (a somewhat silly question to two-way hash/encoding but sometimes my brain goes places)

@matthewdevenny
Copy link
Contributor Author

matthewdevenny commented Mar 3, 2022

@boxboatmatt thank you for this contribution! Based on my light reading, I have some concerns that you can likely speak to:

Yup TLS support is definitely there - I just need to add it... My intention was to revisit the connection but I haven't done that yet :)

  • keyspace seems to be encoded on the wire. will we need a decoder on the nats backend or is this builtin?

  • performance hit for keyspace encoding

    • also i see the two-way hash and it's implied provenance ... are we to assume that a search through potential collision space has been performed exhaustively? (a somewhat silly question to two-way hash/encoding but sometimes my brain goes places)

NATS JetStream reserves . so the key space has to be encoded to work properly with etcd keys which reserves / and can contain . within the keys. I am sure there is some performance hit there but if you look at the load-map test numbers in the build it is still very fast.

http://drone-pr.k3s.io/k3s-io/kine/66

@matthewdevenny
Copy link
Contributor Author

Thanks for the contribution, this is very interesting! I see there are still some bits commented out, and a few things that are waiting on upstream PRs. Did you want to get those worked out before merging?

The nats.go PR doesn't have to get merged it would just make the client a little cleaner.

I should however remove the commented out pieces that are still lingering. I'll address those in addition to other comments

@brandond
Copy link
Member

brandond commented Mar 3, 2022

NATS JetStream reserves . so the key space has to be encoded to work properly with etcd keys which reserves / and can contain . within the keys. I am sure there is some performance hit there but if you look at the load-map test numbers in the build it is still very fast.

I'm curious, would it work to essentially just transliterate periods in the key string to some other non-reserved character? I haven't actually thought this through since I'm not sure what meaning the periods have to JetStream. As of etcdv3 forward slashes actually don't have any meaning to etcd; keys are just arbitrary byte arrays.

https://etcd.io/docs/v3.5/op-guide/v2-migration/#migrate-client-library

Flat key space: There are no directories in API v3, only keys. For example, “/a/b/c/” is a key. Range queries support getting all keys matching a given prefix.

@jnmoyne
Copy link

jnmoyne commented Mar 5, 2022

I'm curious, would it work to essentially just transliterate periods in the key string to some other non-reserved character? I > haven't actually thought this through since I'm not sure what meaning the periods have to JetStream.

Dots separate 'tokens' in a NATS subject such that you can use wildcards (* and >) to match any value for one or more tokens in a subject when subscribing. You could indeed translate the periods in your key to some other non-reserved character (essentially any characters other than .,* and > are ok to use).

Here it's the key is just being base58 encoded which is going to be pretty fast encode/decode.

Signed-off-by: Matthew DeVenny <[email protected]>
Signed-off-by: Matthew DeVenny <[email protected]>
@brandond brandond requested a review from dweomer March 17, 2022 19:05
pkg/drivers/jetstream/kv/kv.go Outdated Show resolved Hide resolved
pkg/drivers/jetstream/jetstream.go Outdated Show resolved Hide resolved
pkg/drivers/jetstream/jetstream.go Outdated Show resolved Hide resolved
Signed-off-by: Matthew DeVenny <[email protected]>
@brandond brandond merged commit c772524 into k3s-io:master Apr 8, 2022
@brandond brandond changed the title Add initial JetStream driver to kine Add NATS JetStream driver Apr 8, 2022
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.

5 participants