feat: Tokenserver: Add node assignment logic #1158

ethowitz · 2021-10-05T18:41:57Z

Description

Adds the logic used to create users and assign them to the correct nodes.

Testing

I added integration and unit tests that provide comprehensive coverage, but the following cases are the important ones:

Users are always allocated to the least-loaded node
When a user is allocated to a node, available and current_load are adjusted accordingly
When no nodes have capacity, Tokenserver attempts to release capacity on nodes where available is zero but current_load is less than capacity

Issue(s)

Closes #1051

ethowitz · 2021-10-28T20:19:43Z

src/tokenserver/extractors.rs

@@ -321,12 +325,12 @@ impl FromRequest for KeyId {

            let (keys_changed_at_string, encoded_client_state) = x_key_id
                .split_once("-")
-                .ok_or_else(|| TokenserverError::invalid_key_id("Invalid X-KeyID header"))?;
+                .ok_or_else(|| TokenserverError::invalid_credentials("Unauthorized"))?;


A few small changes here to fix failing integration tests

Possible future feature, but we've got a few times where we return the same error, and I doubt that we're logging these. It might be worth returning something like an errno to identify which of these got triggered.

Since we're rolling out a new service and there may be unexpected data, getting a response like that could save us time.

I'm wary of changing the API between the old and new Tokenservers; I don't want to give clients something unexpected. Do you think logging an errno would be sufficient?

Yep, logging would be fine. We don't have to return the data to the caller.

ethowitz · 2021-10-28T20:23:02Z

src/tokenserver/db/models.rs

@@ -242,6 +187,83 @@ impl TokenserverDb {
        Ok(result as u64 > 0)
    }

+    /// Gets the least-loaded node that has available slots.
+    fn get_best_node_sync(&self, params: params::GetBestNode) -> DbResult<results::GetBestNode> {


Corresponds to this database method on the old Tokenserver

ethowitz · 2021-10-28T20:24:21Z

src/tokenserver/db/models.rs

    sync_db_method!(replace_users, replace_users_sync, ReplaceUsers);
    sync_db_method!(post_user, post_user_sync, PostUser);
+
+    /// Creates a new user and assigns them to a node.
+    fn allocate_user(&self, params: params::AllocateUser) -> DbFuture<'_, results::AllocateUser> {


Corresponds to this database method on the old Tokenserver

ethowitz · 2021-10-28T20:24:57Z

src/tokenserver/db/models.rs

+
+    /// Gets the user with the given email and service ID, or if one doesn't exist, allocates a new
+    /// user.
+    fn get_or_create_user(


Corresponds (roughly) to this database method on the old Tokenserver

ethowitz · 2021-10-28T20:30:55Z

src/tokenserver/db/models.rs

@@ -845,6 +1094,684 @@ mod tests {
        Ok(())
    }

+    #[tokio::test]
+    async fn test_node_allocation() -> Result<()> {


Most of these new unit tests are rewrites of these unit tests on the old Tokenserver

src/db/error.rs

jrconlin · 2021-10-28T22:33:13Z

tools/integration_tests/tokenserver/test_misc.py

@@ -5,6 +5,8 @@

 from tokenserver.test_support import TestCase

+MAX_GENERATION = 9223372036854775807


heh, interesting choice.

src/tokenserver/handlers.rs

jrconlin · 2021-10-28T22:40:11Z

src/tokenserver/extractors.rs

@@ -321,12 +325,12 @@ impl FromRequest for KeyId {

            let (keys_changed_at_string, encoded_client_state) = x_key_id
                .split_once("-")
-                .ok_or_else(|| TokenserverError::invalid_key_id("Invalid X-KeyID header"))?;
+                .ok_or_else(|| TokenserverError::invalid_credentials("Unauthorized"))?;


Possible future feature, but we've got a few times where we return the same error, and I doubt that we're logging these. It might be worth returning something like an errno to identify which of these got triggered.

Since we're rolling out a new service and there may be unexpected data, getting a response like that could save us time.

ethowitz requested review from a team and removed request for a team October 5, 2021 18:41

ethowitz force-pushed the feat/866-generation-keys-changed-at-validations-handler branch from 41b4b21 to f8db1c9 Compare October 5, 2021 21:25

ethowitz force-pushed the feat/1051-tokenserver-node-assignment-logic branch from 481e153 to 4353165 Compare October 8, 2021 18:07

ethowitz force-pushed the feat/866-generation-keys-changed-at-validations-handler branch from 4131eca to f9da58a Compare October 11, 2021 16:42

Base automatically changed from feat/866-generation-keys-changed-at-validations-handler to master October 28, 2021 18:36

Ethan Donowitz added 2 commits October 28, 2021 18:59

feat: add Tokenserver node assignment logic

2e92b0b

Closes #1051

grammar

6a8fc24

ethowitz force-pushed the feat/1051-tokenserver-node-assignment-logic branch from 4353165 to 6a8fc24 Compare October 28, 2021 20:18

ethowitz commented Oct 28, 2021

View reviewed changes

ethowitz marked this pull request as ready for review October 28, 2021 20:33

ethowitz requested a review from a team October 28, 2021 20:33

jrconlin reviewed Oct 28, 2021

View reviewed changes

add comments and error logging

8213d18

ethowitz requested a review from jrconlin November 3, 2021 20:00

jrconlin approved these changes Nov 4, 2021

View reviewed changes

ethowitz merged commit db739de into master Nov 5, 2021

ethowitz deleted the feat/1051-tokenserver-node-assignment-logic branch November 5, 2021 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Tokenserver: Add node assignment logic #1158

feat: Tokenserver: Add node assignment logic #1158

ethowitz commented Oct 5, 2021

ethowitz Oct 28, 2021

jrconlin Oct 28, 2021

ethowitz Nov 2, 2021

jrconlin Nov 4, 2021

ethowitz Oct 28, 2021

ethowitz Oct 28, 2021

ethowitz Oct 28, 2021

ethowitz Oct 28, 2021

jrconlin Oct 28, 2021

jrconlin Oct 28, 2021

		@@ -5,6 +5,8 @@

		from tokenserver.test_support import TestCase

		MAX_GENERATION = 9223372036854775807

feat: Tokenserver: Add node assignment logic #1158

feat: Tokenserver: Add node assignment logic #1158

Conversation

ethowitz commented Oct 5, 2021

Description

Testing

Issue(s)

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