re-introduce eager service loading #12657
Conversation
Test Results (amd64) - Integration, Bootstrap 5 files ±0 5 suites ±0 2h 21m 26s ⏱️ -34s Results for commit 047a213. ± Comparison against base commit 8b3dcdd. This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| wait_for_localstack_ready, | ||
| aws_client_factory, | ||
| ): | ||
| # this is undocumented behavior, to allow eager loading of specific services while not restricting services loading |
There was a problem hiding this comment.
FYI I arrived at this PR because I was specifically trying to figure out how to eager load specific services in a specific order without loading everything or preventing services from ever being enabled. Lucky issue searching turned up this PR.
My use case is building integration tests that have isolated localstack containers and building/deploying images to ecr for lambda as well as spinning up RDS. I ended up getting tripped up because depending on whether RDS or ECR was started first, ECR might be 4510 or 4511.
All that is to say, at least one person (me) finds this behavior useful and would appreciate the behavior being documented!
Looking forward to see this PR land either way.
There was a problem hiding this comment.
@jeremygiberson please keep in mind though that we still don't technically guarantee that these port assignments would be deterministic.
There was a problem hiding this comment.
Thanks @bentsku for tackling this and for particularly for making sure we have regression tests in place now to avoid a similar occurence in the future! 🚀
We can get input from @thrau once he's back but having the hook live close to the service plugins / manager seems like a sensible choice to me. 👍
I updated the eager loading logic to not eager load all services if set, as it will most likely just wait forever.
As far as I understood, this would be a major change though, since it would break how users expect EAGER_SERVICE_LOADING=1 to work in isolation. 🤔 Did you find any occurence where we communicated this differently?
| assert services.get("cloudwatch") == "disabled" | ||
|
|
||
|
|
||
| def test_eager_service_loading( |
There was a problem hiding this comment.
nice 👏 thanks for adding these.
Might also be good to add a case for {"EAGER_SERVICE_LOADING": "0", "SERVICES": "s3,sqs,sns"} just to make sure we have no regression there.
There was a problem hiding this comment.
I think this would be covered by the test above: test_strict_service_loading. STRICT_SERVICE_LOADING is set to 1 by default in LocalStack, and EAGER_SERVICE_LOADING to 0.
If something were to change with eager loading, I believe the test above would start failing as we're asserting that the services are available and not running? But we could make that explicit as well, here we are depending on default values.
| wait_for_localstack_ready, | ||
| aws_client_factory, | ||
| ): | ||
| # this is undocumented behavior, to allow eager loading of specific services while not restricting services loading |
There was a problem hiding this comment.
@jeremygiberson please keep in mind though that we still don't technically guarantee that these port assignments would be deterministic.
@dominikschubert this is fair. Knowing that we lost the feature for almost a year now and had a major release since, I thought that this would be a fair change that we could document going forward. I had a I'm not sure we ever communicated about I tried to start LocalStack with I'm also fine to leave the behavior as it was before, and prepare this change for |
|
@bentsku Even though it's extremely expensive, I'd argue we should keep the behavior for now since someone might depend on it. We can discuss next week and onward if we want to change that behavior and if we want to do so via a major or minor release. Protecting users from potentially unintended mistakes is of course great, but the use case of trying to make sure everything is up and running is a valid one as well 🤷♂️ , so we need to consider that in a bit more detail. In the meantime we can also investigate why it's never actually starting up when eagerly loading all services. I'd argue that's a bug to address separately 🐞 |
551b4ab to
047a213
Compare
|
@dominikschubert got it, I've updated the PR now 👍 I see it is as half a bug and half a feature, as starting each individual service will take a long time, maybe I didn't have a lot of patience either 😄 but not sure we have users using every single service we have, but that's another question! We can keep this discussion offline. Thanks for the review 🙏 |
|
just reading the title i thought we're moving away from lazy loading. before i had all the context my heart rate went up significantly, lol. but this is a great PR, thanks for fixing this @bentsku! the implementation is really clean, i think the hook you added is one of the cleanest ways to do this 👍 |
Motivation
It seems we deleted eager service loading behavior with #11202 by accident.
Now with the new runtime framework, we can simply eager load services via hooks.
I've also added a proper bootstrap test to avoid deleting the behavior again without realizing it.
I updated the eager loading logic to not eager load all services if set, as it will most likely just wait forever.
My biggest question is where this hook declaration should live?
\cc @thrau @alexrashed
Changes