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 :)

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

I see, could be nice to have this clarified somewhere in the models 👍🏻

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

packages/fleet/lib/models/deployments.ts

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

packages/fleet/lib/models/nodes.ts

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?

packages/fleet/lib/models/deployments.ts

bodinsamuel

Looking good 🦖

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

bodinsamuel · 2024-12-05T09:02:57Z

packages/fleet/lib/utils/errors.ts

@@ -0,0 +1,23 @@
+type Jsonable = string | number | boolean | null | undefined | readonly Jsonable[] | { readonly [key: string]: Jsonable } | { toJSON(): Jsonable };


could be type-fest or could be stored globally in types/utils

didn't want to bring type-fest just for that. Where in types/utils would you put it?

packages/types/lib/utils.ts

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

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

nalanj approved these changes Dec 4, 2024

View reviewed changes

bodinsamuel approved these changes Dec 5, 2024

View reviewed changes

TBonnin added 5 commits December 5, 2024 09:01

fix(fleet): make node routingId/deploymentId unique

8f7f1e7

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

b2768b0

fix(fleet): improve error handling

9831ce4

fix: node routingId/deploymentId is not unique

0a2ded7

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 force-pushed the tbonnin/nan-2301/fleet-init-models branch from 544a1c3 to 0a2ded7 Compare December 5, 2024 14:01

fix: move Jsonable to types package

8e637ec

TBonnin enabled auto-merge (squash) December 5, 2024 14:29

TBonnin merged commit 17f68e6 into master Dec 5, 2024
21 checks passed

TBonnin deleted the tbonnin/nan-2301/fleet-init-models branch December 5, 2024 14:36

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 5, 2024

bodinsamuel Dec 4, 2024

TBonnin Dec 4, 2024

nalanj Dec 4, 2024

TBonnin Dec 4, 2024

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

bodinsamuel left a comment

bodinsamuel Dec 5, 2024

TBonnin Dec 5, 2024

bodinsamuel Dec 5, 2024

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

		@@ -0,0 +1,23 @@
		type Jsonable = string \| number \| boolean \| null \| undefined \| readonly Jsonable[] \| { readonly [key: string]: Jsonable } \| { toJSON(): Jsonable };

feat: introducing the fleet package #3105

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

Choose a reason for hiding this comment

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

bodinsamuel 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

TBonnin Dec 4, 2024 •

edited

Loading