namespaces #575

MarinPostma · 2023-08-02T16:44:32Z

This PR introduces namespaces to sqld.

Namespaces enable multiple database to be managed by a single sqld instance. A primary sqld instance can manage primary databases, and a replica sqld instance can manage replica databases.

Namespaces are lazily created based on the value of the host header. The host header must be present for every request. It is split by . and the first split determines the namespace. For example, if the host is somedatabase.turso.com, then the namespace is somedatabase. For websocket the handshake host header must also be set.

A hrana connection is opened for a specific namespace. In other words, a hrana connection can only open a connection to the database specified during the handshake.

Namespaces are lazily created on either the primary or a replica. When either receives a request to some namespaces, they create it on the fly. In the replica case, a replication stream is created on namespace creation that will trigger namespace creation on the primary. When a namespace is created on the primary, the creation is not eagerly propagated to the replicas. Rather, the replica will start replication from the primary upon the first request it gets for that namespace.

Integration with bottomless works seamlessly. The database id of the bottomless replicator is set to the namespace name, and the primary can lazily recover a database from bottomless if it exists there before responding to a request.

The snapshot script callback is also passed to the namespace. as a second argument

The --allow-default-namespace flag can be used to fall back to the default namespace if the host header can't be parsed properly.

--allow-replica-overwrite has been removed since the handshake is now explicit about the database we are trying to replicate. In case of log incompatibility, the replica is always reset.

Remaining work:

Finer-grained authentication: currently, the same token is used to query any database
Loading dumps: We need an API to load dumps into a namespace
~~Create dump: we need an API to perform a dump for a namespace~~ dumps can be created for specific namespaces by passing the --namespace arguments to the dump subcommand.
Eager namespace creation: namespaces are lazily created. Let's move to a more convenient model where namespaces are created on demand through the admin API
Allow namespace deletion: deleting namespaces can be tricky since we need to ensure consistency between replicas and the primary

penberg · 2023-08-09T10:02:12Z

Finer-grained authentication: currently, the same token is used to query any database

This is fine for now. We can merge without this, let's just open a ticket to remind us that we need to do it.

LucioFranco

Overall looks really good! I think we could use a better PR title and I left a few nits/comments.

sqld/src/hrana/ws/handshake.rs

LucioFranco · 2023-08-09T13:29:37Z

sqld/src/hrana/ws/handshake.rs

+        namespace = match extract_namespace(req) {
+            Ok(ns) => Some(ns),
+            Err(_) if allow_default_ns => Some(DEFAULT_NAMESPACE_NAME.into()),


You can remove the option here right? Since you return in one branch you can make the type the T from the Option and then remove an unwrap?

I don't think you can do that since namespace is set by the callback body, and Rust can't prove that the callback will be invoked at any point, leaving the namespace uninitialized. Or maybe I misunderstood your suggestion?

sqld/src/http/mod.rs

LucioFranco · 2023-08-09T13:32:34Z

sqld/src/http/db_factory.rs

+pub struct DbFactoryExtractor<D>(pub Arc<dyn DbFactory<Db = D>>);
+
+#[async_trait::async_trait]
+impl<F> FromRequestParts<AppState<F>> for DbFactoryExtractor<F::Database>


Whats the benefit of this extractor over using the previous pattern? I assume its less work since you don't need do write out all the structs etc?

We must extract the namespace from the host header in multiple places and retrieve the corresponding DbFactory. I could have a function like retrieve_db_factory(&Request, &Namespaces) and call it everywhere, but I preferred to go with an extractor instead

LucioFranco · 2023-08-09T13:48:55Z

sqld/src/namespace.rs

+};
+
+#[async_trait::async_trait]
+pub trait NamespaceFactory: Sync + Send + 'static {


I think this trait name is super confusing. I would maybe call it NamedspacedDb and just drop the factory since I think we all would know this is a factory :D (or can call it Maker which feels more rusty).

LucioFranco · 2023-08-09T13:49:27Z

sqld/src/namespace.rs

+}
+
+pub struct Namespaces<F: NamespaceFactory> {
+    #[allow(clippy::type_complexity)]


We should just globally allow this lint....its really dumb

LucioFranco · 2023-08-09T13:53:38Z

sqld/src/namespace.rs

+}
+
+#[allow(dead_code)]
+pub struct Namespace<D, T> {


This name as well doesn't make sense to me either would be good to have a doc comment or rename it to something more digestible.

LucioFranco · 2023-08-09T13:54:05Z

sqld/src/namespace.rs

+}
+
+#[async_trait::async_trait]
+pub trait NamespaceCommon {


Should this just be called Namespace and we rename the struct above?

LucioFranco · 2023-08-09T13:54:57Z

sqld/src/http/mod.rs

    enable_console: bool,
    idle_shutdown_layer: Option<IdleShutdownLayer>,
    stats: Stats,
-    logger: Option<Arc<ReplicationLogger>>,
-) -> anyhow::Result<()> {
+    replication_service: Option<S>,


I couldn't figure out why this was changed to be generic? Could you expand on that, I think adding generics if we don't need it can make this API much harder to read.

2 step plan:

don't leak namespace-specific information to HTTP (Namespace<PrimaryNamespaceFactory>)

rn, only the primary can serve replication requests, but, it's easy enough to make a proxy rpc service so that replication request can be served from any node, which is what I want to do in a followup PR.

Would it be better to just enum that service rather than make things more generic?

LucioFranco · 2023-08-09T14:00:05Z

Out of curiosity, how did you test all of this code?

MarinPostma · 2023-08-09T15:58:26Z

Out of curiosity, how did you test all of this code?

I ran plenty of tests:

in standalone mode:
- create multiple namespaces
- add data to those different tables
- check that the data went to the correct namespaces
in primary replica mode:
- Start a primary and 2 replicas
- trigger namespace creation from replica by querying a namespace there
- check that namespace was created on both the primary and replica
- trigger namespace creation from primary by querying namespace there
- check that namespace was created on both primary and replica
- add data from both replica and primary, on different namespaces
- check consistency between primary and replicas for all namespaces
- kill the primary and bring it back up, and check the replicas connect to their respective primaries, attempt to add more data, and check the consistency
In primary/replica mode, with bottomless enabled
- Same as above
- destroy the primary (remove all its files) and start it again, let it recover from bottomless, and check that replicas end in a consistent state, all while replicas are still running.

MarinPostma · 2023-08-10T08:59:01Z

@LucioFranco optionally 1c12b95contains a refactor that should help with clarity. I can split it off in another PR if necessary, lmk.

I tried to make the concepts a bit clearer between database/connection/namespace

LucioFranco

Everything looks really good, we chatted about some naming stuff over slack but no need to fix them now. Almost all my comments are stuff we can do in a follow up so if we don't do them now we should at least make an issue that captures all of the changes we might want to make in the future.

The only comment I want to bring to your attention is the one around the namespace extraction and the fly domain, I think that is a blocker for at least getting embedded replicas somewhat working due to the fly issue. Let me know if you have any comments.

LucioFranco · 2023-08-10T13:19:03Z

sqld/src/connection/libsql.rs

@@ -125,20 +122,20 @@ where
 }

 #[async_trait::async_trait]
-impl<W> DbFactory for LibSqlDbFactory<W>
+impl<W> MakeConnection for LibSqlDbFactory<W>


You do not need to make this change, but MakeConnection throws me off super hard because tower has https://docs.rs/tower/latest/tower/make/trait.MakeConnection.html lol

LucioFranco · 2023-08-10T13:27:03Z

sqld/src/connection/mod.rs

    }
 }

 #[async_trait::async_trait]
-impl<F, DB, Fut> DbFactory for F
+impl<F, C, Fut> MakeConnection for F


Not that we plan to expose this but I would assume generally you would actually want to implement this on a new type like MakeConnectionFn<F> so that it doesn't affect other blanket impls. But since this is private not a big deal.

LucioFranco · 2023-08-10T13:28:22Z

sqld/src/connection/mod.rs

    }
 }

-pub struct TrackedDb<DB> {
+pub struct TrackedConnection<DB> {
    inner: DB,
    #[allow(dead_code)] // just hold on to it
    permit: tokio::sync::OwnedSemaphorePermit,


you can remove the allow and name this field _permit and I think it will get rid of the error.

LucioFranco · 2023-08-10T13:31:37Z

sqld/src/database.rs

+impl Database for PrimaryDatabase {
+    type Connection = TrackedConnection<LibSqlConnection>;
+
+    fn connection_maker(&self) -> Arc<dyn MakeConnection<Connection = Self::Connection>> {


I don't think we should expose Arc<dyn ...> but instead should wrap this in a new type that encapsulates the dyn trait + arc behavior. This should make other code easier to read in the rest of the codebase. The reason I bring this up here is that its encoded into a trait so this is going to leak everywhere.

LucioFranco · 2023-08-10T13:34:23Z

sqld/src/hrana/ws/conn.rs


 use super::super::{ProtocolError, Version};
 use super::handshake::WebSocket;
 use super::{handshake, proto, session, Server, Upgrade};

 /// State of a Hrana connection.
-struct Conn<D> {
+struct Conn<F: MakeNamespace> {


Minor nit: By why F why not M? 😄

LucioFranco · 2023-08-10T13:45:14Z

sqld/src/http/db_factory.rs

+
+use super::AppState;
+
+pub struct MakeConnectionExtractor<D>(pub Arc<dyn MakeConnection<Connection = D>>);


I think I would rename this to NamespaceMakeConnection and drop the extractor part since that is implicit. (Another name option could be NsMakeConnection)

sqld/src/http/db_factory.rs

LucioFranco · 2023-08-10T13:47:38Z

sqld/src/http/db_factory.rs

+
+    match split_namespace(host_str) {
+        Ok(ns) => Ok(ns),
+        Err(_) if allow_default_namespace => Ok(DEFAULT_NAMESPACE_NAME.into()),


I feel like a debug log could be useful here to catch weird issues where we expect a host to be parsed as a namespace but it gets sent to the default one.

LucioFranco · 2023-08-10T13:58:39Z

sqld/src/namespace.rs

+}
+
+/// A namspace isolates the resources pertaining to a database of type T
+pub struct Namespace<T: Database> {


Can drop the bound and leave this to be just T since you don't use any of the trait items in the struct def.

LucioFranco · 2023-08-10T14:02:48Z

sqld/src/rpc/replication_log.rs

-    logger: Arc<ReplicationLogger>,
-    replicas_with_hello: RwLock<HashSet<SocketAddr>>,
+    namespaces: Arc<NamespaceStore<PrimaryNamespaceMaker>>,
+    replicas_with_hello: RwLock<HashSet<(SocketAddr, Bytes)>>,


Is this bytes supposed to be a namespace? If so I would suggest we maybe newtype this to make sure its clear that this bytes was extracted as a namespace, this can be done in a follow up though.

LucioFranco

🔥

MarinPostma force-pushed the namespace branch 3 times, most recently from 6359208 to da49ca2 Compare August 8, 2023 12:48

MarinPostma added 10 commits August 8, 2023 15:03

add namespaces

f9d25f9

update gRPC protocols

5ff8d50

hook namespaces to APIs

afd5355

replace Namspaces::get_or_create with Namespaces::with

98a6e5a

spawn replicator on namespace join set

5966262

namespace reset

ed67632

replica hard reset

90241a0

bottomless replication

92bbece

run fmt

ed75d4a

update bottomless doc

0ca86cc

MarinPostma force-pushed the namespace branch from da49ca2 to 0ca86cc Compare August 8, 2023 13:19

MarinPostma added 6 commits August 8, 2023 15:23

handle rpc error

4630eca

fix tests

8378fb7

fix clippy

733f362

dump namespace

e37e57b

add db_factory extractor

b35fabf

handle host header error

eb81c8f

MarinPostma force-pushed the namespace branch from 19b640f to eb81c8f Compare August 8, 2023 14:57

add optional default namespace

65f02fd

MarinPostma force-pushed the namespace branch from 760978b to 65f02fd Compare August 9, 2023 09:01

MarinPostma added 2 commits August 9, 2023 11:08

fix comment

7863431

fix replica reset

c374f4f

MarinPostma force-pushed the namespace branch from 33a2a34 to c374f4f Compare August 9, 2023 09:44

MarinPostma marked this pull request as ready for review August 9, 2023 09:46

MarinPostma requested review from LucioFranco and psarna and removed request for LucioFranco August 9, 2023 09:48

MarinPostma requested review from LucioFranco and haaawk August 9, 2023 09:48

LucioFranco reviewed Aug 9, 2023

View reviewed changes

apply review suggestions

ef3ec1d

MarinPostma force-pushed the namespace branch 3 times, most recently from 1c12b95 to b014270 Compare August 10, 2023 09:25

refactor database/connection/namespace for clarity

e45152f

MarinPostma force-pushed the namespace branch from b014270 to e45152f Compare August 10, 2023 09:25

MarinPostma mentioned this pull request Aug 10, 2023

Create namespace from dump #589

Merged

LucioFranco reviewed Aug 10, 2023

View reviewed changes

LucioFranco approved these changes Aug 10, 2023

View reviewed changes

MarinPostma added this pull request to the merge queue Aug 10, 2023

This was referenced Aug 10, 2023

Namespaced authentication #590

Open

Enpoints for namespace management #591

Open

Merged via the queue into main with commit 87492b3 Aug 10, 2023

Horusiath mentioned this pull request Aug 21, 2023

bottomless-cli: added support for namespaces #614

Merged

MarinPostma mentioned this pull request Aug 22, 2023

intit bottomless before replication log recovery #577

Closed

MarinPostma deleted the namespace branch August 25, 2023 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

namespaces #575

namespaces #575

MarinPostma commented Aug 2, 2023 •

edited

Loading

penberg commented Aug 9, 2023

LucioFranco left a comment

LucioFranco Aug 9, 2023

MarinPostma Aug 9, 2023

LucioFranco Aug 9, 2023

MarinPostma Aug 9, 2023

LucioFranco Aug 9, 2023

LucioFranco Aug 9, 2023

LucioFranco Aug 9, 2023

LucioFranco Aug 9, 2023

LucioFranco Aug 9, 2023

MarinPostma Aug 9, 2023

LucioFranco Aug 10, 2023

LucioFranco commented Aug 9, 2023

MarinPostma commented Aug 9, 2023

MarinPostma commented Aug 10, 2023 •

edited

Loading

LucioFranco left a comment

LucioFranco Aug 10, 2023

LucioFranco Aug 10, 2023

LucioFranco Aug 10, 2023

LucioFranco Aug 10, 2023

LucioFranco Aug 10, 2023

LucioFranco Aug 10, 2023

LucioFranco Aug 10, 2023

LucioFranco Aug 10, 2023

LucioFranco Aug 10, 2023

LucioFranco left a comment


		use super::AppState;

		pub struct MakeConnectionExtractor<D>(pub Arc<dyn MakeConnection<Connection = D>>);

namespaces #575

namespaces #575

Conversation

MarinPostma commented Aug 2, 2023 • edited Loading

Remaining work:

penberg commented Aug 9, 2023

LucioFranco left a 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

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

LucioFranco commented Aug 9, 2023

MarinPostma commented Aug 9, 2023

MarinPostma commented Aug 10, 2023 • edited Loading

LucioFranco left a 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

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

LucioFranco left a comment

Choose a reason for hiding this comment

MarinPostma commented Aug 2, 2023 •

edited

Loading

MarinPostma commented Aug 10, 2023 •

edited

Loading