-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
implement new runtime framework for bootstrapping localstack #10942
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
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 3m 23s ⏱️ Results for commit 7639225. ♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files ±0 2 suites ±0 1h 35m 42s ⏱️ - 1m 44s Results for commit 7639225. ± Comparison against base commit de41959. This pull request removes 2 and adds 8 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
7cea57e
to
1d9b1e1
Compare
1d9b1e1
to
c91a896
Compare
current state: i think everything works, except i believe the runtime shuts down too eagerly and therefore a CFn cleanup isn't working correctly. |
149a3b9
to
5104264
Compare
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.
Wow! This is truly great, and an awesome push towards a proper separation between the runtime and the emulator specific code.
Currently, these parts are a bit intertwined, which you can see when you think about what code this PR supersedes:
localstack.aws.serving.*
localstack.aws.gateway
The separation into a Runtime
, a RuntimeServer
, as well as a Component
is great!
However, I am not 100% sure if the current separation of concerns is the best way forward (but it's definitely a great step and this thinking should not block a merge of this PR):
You mentioned in the code that at some point it would be great to have the possibility to run multiple components in a single process (I know, it's quite far away from now).
These Component
instances would then be served via different Gateway
instances on a single RuntimeServer
.
But the RuntimeServer
is currently managed by the Component
which basically means there's a 1:1 mapping between the Component
and the RuntimeServer
.
Instead it might be better to have the Runtime manage both, the Component
instances as well as the RuntimeServer
insteances and glue them together?
Another thing we need to think about is the deprecation path for the host mode (which is not supported anymore with the new runtime server).
Unfortunately, a quick and dirty GitHub search shows that there are quite a lot of usages out there in the wild which we need to phase out.
Maybe we could already include a deprecation notice for the usage of the host mode in the CLI in this PR?
Again, truly a great step forward! I'm really looking forward to separating the code into proper namespace packages! 🥳 🚀 🦸🏽
# event flag indicating the infrastructure has been started and that the ready marker has been printed | ||
# TODO: deprecated, use events.infra_ready | ||
INFRA_READY = events.infra_ready | ||
|
||
# event flag indicating that the infrastructure has been shut down | ||
SHUTDOWN_INFRA = threading.Event() |
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.
nit: I don't think this is used anywhere (yet), could this maybe be removed?
Actually, I don't think there's even a usage across all repos in this org:
https://github.com/search?q=org%3Alocalstack+%22INFRA_READY%22&type=code
https://github.com/search?q=org%3Alocalstack+%22SHUTDOWN_INFRA%22&type=code
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.
Fair, I'll remove it as part of the cleanup I'm a second PR.
5104264
to
00bccf3
Compare
FYI: I pushed a small commit with a small adjustment to
All my change does is that it fixes the hook to avoid starting LocalStack if we are in a "collect only" session. |
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.
Awesome progress and I love the much cleaner way of "how to bootstrap an e2e emulator instance".
Great attention to deprecations and I like that even the in memory client has been considered from the start 🚀
There are some lower hanging fruits for follow-ups that we should get in sooner rather than later, especially starting the deprecation warnings and communication around that, so that we can phase them out with 4.0.
The config also seems to become more critical again. Maybe we could do a first step where we split the config into "global" and component config, so that we can then use dynamic component loading based on plugin name as mentioned in one of the comments.
I had a few nits but nothing that would block a merge and couldn't be handled in a follow-up PR, if at all.
image_sha = inspect_result["Image"] | ||
print("LocalStack Docker image sha: %s" % image_sha) |
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.
This needs to be adapted for #10951 but it's not critical and can be done in a follow-up, just shouldn't forget about it 👍
"""Hooks that are executed right before the LocalstackRuntime is created. These can be used to apply | ||
patches or otherwise configure the interpreter before any other code is imported.""" | ||
|
||
on_runtime_start = on_infra_start |
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.
let's start deprecating this asap so that we can remove it with 4.0 at least
# avoid starting up localstack if we only collect the tests (-co / --collect-only) | ||
if session.config.option.collectonly: | ||
return |
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.
Awesome, thanks for adding this. I think I had this in at least one local branch somewhere already when playing around more with filtering tests. 😅
ssl_creds: tuple[str, str] | None = None, | ||
): | ||
""" | ||
Registers the Gateway and the port configuration into the server. Some servers like ``twisted`` or |
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.
Might it not make sense to word this a bit differently then: Attempts to...
to signal that this could also not work depending on the used RuntimeServer plugin?
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.
Did you have a particular potential RuntimeServer implementation in mind that would not allow this? Otherwise I guess let's just leave it as is and assume we can always add multiple gateways
# TODO: rename to RUNTIME_SERVER | ||
server_type = config.GATEWAY_SERVER |
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.
nit: That comment should probably go into config.py
def get_current_runtime() -> "LocalstackRuntime": | ||
with _runtime_lock: | ||
if not _runtime: | ||
raise ValueError("LocalStack runtime has not yet been set") |
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 sure a ValueError is necessarily the clearest Exception here.
What's the reason for not just returning an optional/None here? Downstream typing having to handle the None?
- Manage localstack filesystem directories | ||
- Execute runtime lifecycle hook plugins from ``localstack.runtime.hooks``. | ||
- Manage the localstack SSL certificate | ||
- Serve the gateway (It uses a ``RuntimeServer`` to serve a ``Gateway`` instance coming from the |
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.
Q: What's the reason by the way why we don't just keep calling it the GatewayServer
, like in the (now deprecated) config variable?
Creates a new runtime instance from the current environment. It uses a plugin manager to resolve the | ||
necessary components from the ``localstack.runtime.components`` plugin namespace to start the runtime. | ||
|
||
TODO: perhaps we could control which components should be instantiated with a config variable/constant |
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.
We should just directly map the environment variable to the name of the components plugin.
There's a few other implications there though that have been addressed in other code comments here like the need for a scoped config per component for example. (i.e. the config being part of the components plugin and thus only instantiated with them).
if len(components) > 1: | ||
LOG.warning( | ||
"There are more than one component plugins, using the first one which is %s", | ||
components[0].name, | ||
) |
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.
nit: Feels a bit too lenient for me. I'd personally prefer it to raise in that case. 🤷♂️
Motivation
This is part of the effort to generalize certain aspects of the codebase to make them more re-usable (see #10946). One concept that was difficult to re-use was
infra.py
, which is responsible for starting what we now call the localstack Runtime. The reason is that it imports all sorts of aws-specific code.The Runtime is roughly responsible for
Another annoying thing with the previous runtime is that the server (either hypercorn or twisted), couldn't properly register signal handlers, such as a code reloader, because it can only do that from the main thread, and we were starting the server in a separate thread. Now the runtime server, i.e., the control/event loop of the underlying I/O framework, now (imo correctly) blocks the main thread.
💡 The primary change though is that you now have a dynamic and re-usable
python -m localstack.runtime.main
bootstrapping procedure, which follows the open-closed principle.Usage
Here is an example to build a new localstack runtime distribution without needing to define a new main or new bootstrapping procedure. All you need to do is expose a Components plugin, and (provisionally) disable AWS plugins that the currently leaking from localstack-core using localstack/plux#22.
Then, in your distribution, you can
python -m localstack.runtime.main
, and instead of starting AWS, it will start themy-emulator
.Architecture
This is a high-level diagram 8000 to help explain the components. It's an ad-hoc notation, I hope it helps to get the gist though 😅
Components
One critical aspect to add was an IoC mechanism to dynamically load, specifically the
Gateway
instance, but more generally a type of application context (now theComponents
).We assume that, within a runtime, there is only one
Components
plugin. For instance, there are nowAwsComponents
which is aComponents
plugin. Within the context of the AWS emulator, only this plugin should be available and loaded.RuntimeServer
The
RuntimeServer
represents the lower-level IO control loop that should block the main thread. In the case of twisted, this is the twisted reactor. In the case of hypercorn, this is the asyncio event loop.There was no need to limit the
RuntimeServer
to serve only one Gateway instance. Basically an IO control loop should be able to serve multiple server sockets, so in the future we could serve TCP proxies for instance through the main control loop, rather than instantiating new threads. Thus, theregister(Gateway, ...)
method was conceived. Other thanregister
, theRuntimeServer
only has arun
and ashutdown
method.Instantiation
Which type of
RuntimeServer
is instantiated is currently governed byconfig.GATEWAY_SERVER
. The different server implementations are now registered as plugins instead of using static imports as we do inserve_gateway
.Global access
There are situations where you need global access to the runtime and runtime components (for instance in the in-memory client, we need access to the
Gateway
instance we inject the request into).To this end, the runtime package provides a
localstack.runtime.get_current_runtime()
method, that represents global (or in the future: scoped) access to the currentLocalstackRuntime
object.Bootstrapping
Bootstrapping (the process of creating a localstack runtime and starting it), is done by
localstack.runtime.main
. The idea is that this is generalized. Whether we start AWS, Snowflake, or Azure, we always run the generalizedlocalstack.runtime.main
, which gets its application context from theComponents
explained above.Bootstrapping also involves setting the current runtime (as described above), and registering exit handlers. The runtime and subsequently the
RuntimeServer
's event loop, should now always block the main thread.A special case is the
in_memory_localstack.py
for testing, which works similarly, only that it creates a new runtime running in a thread, which still works.Changes
LEGACY_RUNTIME=0
(changed in the future)Testing
make entrypoints
and thenpython -m localstack.runtime.main
- should already use the new runtime.GATEWAY_SERVER=hypercorn
Limitations / Future work
localstack.services.plugins
which contains theService
andServiceManager
framework, but also imports from botocore. In a second step, we need to generalize theService
concept similar to what I've done in this PR.localstack.runtime.legacy
that I don't like, but were necessary to decouple the new runtime fromlocalstack.services.infra
while maintaining backwards compatibilityget_current_runtime()
. maybe we can make the switch to the new runtime with 3.5, and then removeLEGACY_RUNTIME
with 3.6TODO
What's left to do:
LEGACY_RUNTIME
(maybe?)