-
Notifications
You must be signed in to change notification settings - Fork 452
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
feat(fleet): add control loop and state management #3116
Conversation
packages/fleet/lib/supervisor.ts
Outdated
import { setTimeout } from 'node:timers/promises'; | ||
import type { NodeProvider } from './node-providers/node_provider.js'; | ||
|
||
type Action = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know Action is an overloaded term in Nango but I couldn't think of a better name. Suggestions are welcomed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operation is more accurate I feel, but still very generic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. changing for operation
packages/fleet/lib/supervisor.ts
Outdated
STARTING: 5 * 60 * 1000, | ||
FINISHING: 24 * 60 * 60 * 1000, | ||
TERMINATED: 7 * 24 * 60 * 60 * 1000, | ||
ERROR: 7 * 24 * 60 * 60 * 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arbitrary values. Happy to change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to lie it's a lot of code and a lot of "hypothetical" code, not everything makes sense to me rn. But looks okay overall
packages/fleet/lib/supervisor.ts
Outdated
import { setTimeout } from 'node:timers/promises'; | ||
import type { NodeProvider } from './node-providers/node_provider.js'; | ||
|
||
type Action = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operation is more accurate I feel, but still very generic
expect(failedNode.state).toBe('ERROR'); | ||
expect(failedNode.error).toBe('my error'); | ||
}); | ||
it('should be able to register a node', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you have against line break 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean empty line between the tests? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes even in general, between logical code blocks but I guess it's keyboard-driven development :p
e5cd89c
to
82cb468
Compare
544a1c3
to
0a2ded7
Compare
82cb468
to
353f8f3
Compare
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
- Implement Supervisor class to manage node states and transitions - Add methods to start, fail, outdate, terminate, and remove nodes - Introduce NodeProvider interface for node operations - Update node state transitions and error handling - Add integration tests for Supervisor functionality
353f8f3
to
868255c
Compare
this.state = 'stopped'; | ||
} | ||
|
||
private async plan(cursor?: number): Promise<Result<Operation[]>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be easier going forward for operations to be an object with a type and a function attached, and then executing the plan just loops and calls the functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not. The advantage of having values instead of opaque functions is that you can optimize the plan before to execute it. I don't think Render API can do it and even if it does I am not planning to support it but we could for instance takes all the CREATE operations fromthe plan and combine them in a unique call to the node provider instead of one by one.
What's not yet implemented:
There are still a few TODOs that I will address in following PRs
How to test
Still not wired. You can run the tests ;)