feat: introducing the fleet package #3105

TBonnin · 2024-12-03T21:23:39Z

The fleet package will be used to managed the lifecycle of runners

This commit includes:

migration for deployments and nodes tables
deployments and nodes models (with tests)

This commit DOESN'T include (I though that would be enough for a first PR):

the control loop to handle nodes state transition
the routing logic

How to test

The code is not used so it is not possible to run it manually. There is tests though :)

The fleet package will be used to managed the lifecycle of runners This commit includes: - migration for deployments and nodes tables - deployments and nodes models (with tests) This commit DOESN'T include: - the control loop to handle nodes state transistion - the routing logic

linear · 2024-12-03T21:23:49Z

NAN-2301 Feat: runners lifecycle management

bodinsamuel · 2024-12-04T09:45:00Z

packages/fleet/lib/db/migrations/20241102115903_initial_fleet_models.ts

+                routing_id varchar(255) NOT NULL,
+                deployment_id char(40) NOT NULL REFERENCES deployments(commit_id) ON DELETE CASCADE,


Need a unique index for those two no?

I now remember why I didn't make it unique. Because any node can end up in ERROR state and therefore several entry can have the same routingId/deploymentId.
Ex:

Node is created as PENDING

Request to Render to start the new runner fails for some reason. Node is set to ERROR

A new node with same routingID and deploymentId is created as PENDING

Another example: if runner is idled because of inactivity, another runner might potentially be created with the same routingId/deployment

bodinsamuel · 2024-12-04T09:46:17Z

packages/fleet/lib/fleet.ts

+export class Fleet {
+    private dbClient: DatabaseClient;
+    constructor({ fleetId, dbUrl }: { fleetId: string; dbUrl: string }) {
+        this.dbClient = new DatabaseClient({ url: dbUrl, schema: fleetId });


why not passing the DBClient directly?

because I want the db schema to reflect the name/id of the fleet. I could set the schema for the DBClient and then pass the client but it is more encapsulated this way

Are you thinking we would have multiple fleet schemas in production?

not planning to have multiple fleet anytime soon no but nothing would prevent it in the code

TBonnin · 2024-12-04T13:28:55Z

packages/fleet/lib/models/deployments.ts

+                return Err(new Error(`Error: no deployment '${commitId}' created`));
+            }
+            return Ok(DBDeployment.to(inserted));
+        });


very good point. I forgot about rolling back. commitHash is indeed not unique. I will fix

I'm not sure where rolling back is headed, so we may already align, but I think it will be clearer if we're just allowing duplicate commitHash entries with different ids, so that we have a linear history. So a rollback would just be us deploying an old version again, rather than removing anything.

that's exactly what's this is doing. Deployments are immutable and rolling back means inserting a new entry with the same commitHash. No update, no deletion

bodinsamuel · 2024-12-04T09:49:50Z

packages/fleet/lib/models/deployments.ts

+            return Ok(DBDeployment.to(inserted));
+        });
+    } catch (err: unknown) {
+        return Err(new Error(`Error creating deployment '${commitId}': ${stringifyError(err)}`));


Instead of stringifying you can use new Error(msg, { cause }) which was made for this.
Also could be great to have a dedicated Error type (e.g: FleetError) just for clarity

added a FleetError in ee4d0e9 let me know what you think?

packages/fleet/lib/models/nodes.integration.test.ts

+                storageMb: 512
+            })
+        ).unwrap();
+        expect(node).toMatchObject({


packages/fleet/lib/models/nodes.ts

+            return Err(new Error(`Error: no node '${nodeProps.routingId}' created`));
+        }
+        return Ok(DBNode.from(inserted[0]));
+    } catch (err: unknown) {


TBonnin · 2024-12-04T15:01:18Z

packages/fleet/lib/models/nodes.ts

+        }
+        return Ok(DBNode.from(inserted[0]));
+    } catch (err: unknown) {
+        return Err(new Error(`Error creating node '${nodeProps.routingId}': ${stringifyError(err)}`));


you are right. I should try/catch everywhere

bodinsamuel · 2024-12-04T09:58:51Z

packages/fleet/lib/models/nodes.ts

+
+        const transition = NodeStateTransition.validate({ from: node.value.state, to: props.newState });
+        if (transition.isErr()) {
+            return Err(transition.error);


Suggested change

return Err(transition.error);

return transition.error;

A result must be returned. I could maybe do return transition.mapError(...) but I don't think it is more readable

packages/fleet/lib/models/nodes.ts

+    return db.transaction(async (trx) => {
+        const node = await get(trx, props.nodeId, { forUpdate: true });
+        if (node.isErr()) {
+            return Err(new Error(`Node '${props.nodeId}' not found`));


bodinsamuel · 2024-12-04T16:39:08Z

let me know when you want another pass

TBonnin · 2024-12-04T18:32:15Z

let me know when you want another pass

I have addressed all your comments I think so happy to get another pass

nalanj

Pretty cool to see it taking shape. None of the things I commented on were things that I feel too too strongly about, more just asking questions and pondering.

packages/fleet/lib/models/deployments.integration.test.ts

nalanj · 2024-12-04T18:39:43Z

packages/fleet/lib/fleet.integration.test.ts

+    });
+
+    describe('deploy', () => {
+        it('should create a new deployment', async () => {


I'm not 100% sure about the right way for us to isolate this, and it's certainly a philosophical thing, but this test effectively runs the same code as the other deployment creation test. It might be a nice spot for us to use a test stub knowing that fleet can trust that deploy is well tested.

You are right. On the other hand stub everywhere could introduce more issues down the line when underlying logic is changed.

I've added this test to bootstrap the fleet.integration.test file but I might remove it since it is not very useful

nalanj · 2024-12-04T18:40:17Z

packages/fleet/lib/fleet.ts

+export class Fleet {
+    private dbClient: DatabaseClient;
+    constructor({ fleetId, dbUrl }: { fleetId: string; dbUrl: string }) {
+        this.dbClient = new DatabaseClient({ url: dbUrl, schema: fleetId });


Are you thinking we would have multiple fleet schemas in production?

nalanj · 2024-12-04T18:46:08Z

packages/fleet/lib/models/deployments.ts

+                return Err(new Error(`Error: no deployment '${commitId}' created`));
+            }
+            return Ok(DBDeployment.to(inserted));
+        });


I'm not sure where rolling back is headed, so we may already align, but I think it will be clearer if we're just allowing duplicate commitHash entries with different ids, so that we have a linear history. So a rollback would just be us deploying an old version again, rather than removing anything.

Ex: in case of error, a new node with same routingId/deploymentId will be created in case of runner idled because of inactivity, a new node with same routingId/deploymentId will be created

TBonnin requested a review from a team December 3, 2024 21:23

bodinsamuel approved these changes Dec 4, 2024

View reviewed changes

TBonnin added 3 commits December 4, 2024 09:38

fix(fleet): make node routingId/deploymentId unique

a286f60

fix(fleet): support rollback deployment (aka: commitHash is not unique)

996be88

fix(fleet): improve error handling

ee4d0e9

nalanj approved these changes Dec 4, 2024

View reviewed changes

fix: node routingId/deploymentId is not unique

544a1c3

Ex: in case of error, a new node with same routingId/deploymentId will be created in case of runner idled because of inactivity, a new node with same routingId/deploymentId will be created

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introducing the fleet package #3105

feat: introducing the fleet package #3105

TBonnin commented Dec 3, 2024

linear bot commented Dec 3, 2024

bodinsamuel Dec 4, 2024

TBonnin Dec 4, 2024

TBonnin Dec 4, 2024 •

edited

Loading

bodinsamuel Dec 4, 2024

TBonnin Dec 4, 2024

nalanj Dec 4, 2024

TBonnin Dec 4, 2024

This comment was marked as resolved.

TBonnin Dec 4, 2024

nalanj Dec 4, 2024

TBonnin Dec 4, 2024

bodinsamuel Dec 4, 2024

TBonnin Dec 4, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

TBonnin Dec 4, 2024

bodinsamuel Dec 4, 2024

TBonnin Dec 4, 2024

This comment was marked as resolved.

bodinsamuel commented Dec 4, 2024

TBonnin commented Dec 4, 2024

nalanj left a comment

nalanj Dec 4, 2024

TBonnin Dec 4, 2024

nalanj Dec 4, 2024

nalanj Dec 4, 2024

		routing_id varchar(255) NOT NULL,
		deployment_id char(40) NOT NULL REFERENCES deployments(commit_id) ON DELETE CASCADE,

feat: introducing the fleet package #3105

Are you sure you want to change the base?

feat: introducing the fleet package #3105

Conversation

TBonnin commented Dec 3, 2024

How to test

linear bot commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBonnin Dec 4, 2024 • 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

This comment was marked as resolved.

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

bodinsamuel commented Dec 4, 2024

TBonnin commented Dec 4, 2024

nalanj 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

TBonnin Dec 4, 2024 •

edited

Loading