refactor: network manager #210
Conversation
loks0n
force-pushed
the
refactor-network-manager
branch
2 times, most recently
from
ac7a95f to
17f156a
Compare
loks0n
force-pushed
the
refactor-network-manager
branch
from
17f156a to
9427aad
Compare
| export COMPOSE_INTERACTIVE_NO_CLI | ||
| export DOCKER_BUILDKIT=1 | ||
| export COMPOSE_DOCKER_CLI_BUILD=1 | ||
| export BUILDKIT_PROGRESS=plain |
There was a problem hiding this comment.
These are modern defaults, can remove
| ); | ||
|
|
||
| Http::get('/v1/health') | ||
| ->groups(['api']) |
| /* Create desired networks if they don't exist */ | ||
| $networks = explode(',', System::getEnv('OPR_EXECUTOR_NETWORK') ?: 'openruntimes-runtimes'); | ||
| $runner = new Docker($orchestration, new Runtimes(), $networks); | ||
| $networkManager = new NetworkManager($orchestration, $networks); |
There was a problem hiding this comment.
Key change: create networks and connect executor container outside of the 'Docker' runner. This achieves two things:
- Simplifies the Docker god class
- Prepares us for Coroutines, where each coroutine needs it's own instance of Docker class. If we don't we will attempt to create networks every request
| try { | ||
| $this->orchestration->networkConnect($container->getName(), $network); | ||
| } catch (\Throwable) { | ||
| // TODO: Orchestration library should throw a distinct exception for "already connected" |
There was a problem hiding this comment.
:(
we need better exceptions here on orchestration library. not much else i could do
| * Remove residual runtimes and networks | ||
| */ | ||
| Console::info('Removing orphan runtimes and networks...'); | ||
| $this->cleanUp(); |
There was a problem hiding this comment.
I don't think we need to cleanup on restart, sounds dangerous. We should do what k8s runner does instead: resume managing anything labeled with right tag
| Process::signal(SIGINT, fn () => $this->cleanUp()); | ||
| Process::signal(SIGQUIT, fn () => $this->cleanUp()); | ||
| Process::signal(SIGKILL, fn () => $this->cleanUp()); | ||
| Process::signal(SIGTERM, fn () => $this->cleanUp()); |
There was a problem hiding this comment.
need to revisit this, not sure we need to handle all of these
There was a problem hiding this comment.
should we add a comment here to not forget about the revisit?
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
this is nice, should we have it for other repos too?
No description provided.