-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: warm containers #2383
feat: warm containers #2383
Conversation
0fd2c78
to
b0d61e8
Compare
060d11a
to
7ae2bec
Compare
c4ed67a
to
63a416e
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.
I didn't get all the way through the tests but wanted to respond early with feedback.
Generally this look good. My biggest callouts relate to how the code is organized. I think we should look at how to break warms containers into its own class. There was lots of control flow spread throughout, which I view as a maintenance risk. We should look towards concrete classes were we can to improve the readability overall.
As an additional note, I would encourage you in the future to think about how to make smaller prs. This is pretty large and makes it hard for a reviewer to look at everything. One way you could break this down is by feature (lazy and warm) or smaller prs with scaffolding that would allow for the basic building blocks to be added and the followed up with deeper implementation.
@@ -36,6 +39,9 @@ class InvokeContext: | |||
This class sets up some resources that need to be cleaned up after the context object is used. | |||
""" | |||
|
|||
WARM_CONTAINER_MODE = "WARM" |
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.
Should we make this into an enum class?
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.
done
@@ -90,6 +98,16 @@ def __init__( | |||
Whether or not to force build the image | |||
aws_region str | |||
AWS region to use | |||
container_mode str |
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 indicates to me, the mode should be an enum. Will make type'ing easier (if we ever go down that path) and make the code a little easier to reason about (less checking needed).
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.
Done
@@ -107,13 +125,20 @@ def __init__( | |||
self._aws_region = aws_region | |||
self._aws_profile = aws_profile | |||
|
|||
self._warm_containers = bool(container_mode) |
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 would suggest maybe adding another 'mode' specific to the old behavior. This will make it a little more expandable, in my opinion.
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.
Done
@@ -129,7 +154,20 @@ def __enter__(self): | |||
self._env_vars_value = self._get_env_vars_value(self._env_vars_file) | |||
self._log_file_handle = self._setup_log_file(self._log_file) | |||
|
|||
self._debug_context = self._get_debug_context(self._debug_ports, self._debug_args, self._debugger_path) | |||
if self._warm_containers and self._debug_ports and not self._debug_function: |
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 if statement is a little dense for me. Can you add a comment about this statement?
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.
Done
self._debug_context = self._get_debug_context(self._debug_ports, self._debug_args, self._debugger_path) | ||
if self._warm_containers and self._debug_ports and not self._debug_function: | ||
if len(self._function_provider.functions) == 1: | ||
[self._debug_function] = self._function_provider.functions.keys() |
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 is self._debug_function
in brackets?
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.
it should be the same as self._debug_function = list(self._function_provider.functions.keys())[0] .. any way .. I changed it to make it more clear
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 see. The brackets make it look like it was trying to set to a list, which is why I was asking. Thanks for updating.
Function: | ||
Environment: | ||
Variables: | ||
MODE: "lazy-511c60c8" |
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.
Can we just make MODE a parameter and pass the value in per test? This is how build works. Less to maintain for us.
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.
done
@@ -191,6 +192,67 @@ def test_provider_parse_stage_name(self): | |||
self.assertEqual(provider.api.stage_name, "dev") | |||
self.assertEqual(provider.api.stage_variables, None) | |||
|
|||
def test_invalid_stage_resource_no_rest_api_id(self): |
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.
How does this relate to warm containers?
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 added some test cases that are not related to this PR to increase the code coverage, but I removed it from this PR and will push it in a separate one.
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 see. Thank you for doing that. A separate PR would be better. :)
@@ -56,6 +58,11 @@ def test_must_get_from_boto_session(self, boto3_mock): | |||
|
|||
boto3_mock.session.Session.assert_called_with(profile_name=self.aws_profile, region_name=self.aws_region) | |||
|
|||
actual = self.local_lambda.get_aws_creds() |
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.
Seems unrelated to warm container?
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 added it to increase the code coverage, I can remove it from this PR, and push it in a separate one.
@@ -271,7 +273,7 @@ def test_local_start_lambda(self, do_cli_mock): | |||
} | |||
|
|||
# NOTE: Because we don't load the full Click BaseCommand here, this is mounted as top-level command | |||
with samconfig_parameters(["start-lambda"], self.scratch_dir, **config_values) as config_path: | |||
with samconfig_parameters(["start_lambda"], self.scratch_dir, **config_values) as config_path: |
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.
should be start-lambda no?
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.
done
@@ -507,102 +421,6 @@ def test_must_not_support_input_data(self): | |||
self.container.start(input_data="some input data") | |||
|
|||
|
|||
class TestContainer_wait_for_result(TestCase): |
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 are all these tests removed?
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 refactored the Container && LambdaContainer classes, so I moved some test cases based on this refactoring
debug_function str | ||
The Lambda function name that will have the debugging options enabled in case of warm containers | ||
option is enabled | ||
lazy_containers_initializing bool |
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: unused
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.
removed
type=click.Choice(["WARM", "LAZY"], case_sensitive=False), | ||
), | ||
click.option( | ||
"--debug-function", |
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 thinking out loud, but wondering if it would make sense to only have a --debug
switch that turns on debugging for all supported runtimes, exposing to any random port on the host. We could either display those ports or let the user docker ps
(they already know SAM CLI is coupled with Docker).
self._socket = socket.socket() | ||
self._socket.bind(("", 0)) | ||
self.rapid_port_host = self._socket.getsockname()[1] | ||
self._socket.close() |
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 is quite specific and meaningful on its own, a separate _get_open_port
could help.
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.
done
samcli/local/lambdafn/runtime.py
Outdated
patterns=["*"], ignore_patterns=["*.swp"], ignore_directories=False | ||
) | ||
self._code_change_handler.on_modified = self._on_code_change | ||
self._lock = threading.Lock() |
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.
Which shared resources are we guarding?
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.
the Observed code dir paths, and containers
samcli/local/lambdafn/runtime.py
Outdated
# observe the build directory instead of the lambda function directory in case of runtime languages | ||
# that needs compilation | ||
if ".aws-sam" in code_path: | ||
code_path = os.path.abspath(os.path.join(code_path, os.pardir)) |
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.
Should we be using pathlib
in new code?
8c295d7
to
4b3ebab
Compare
ab3fe8b
to
131257e
Compare
@@ -107,13 +127,24 @@ def __init__( | |||
self._aws_region = aws_region | |||
self._aws_profile = aws_profile | |||
|
|||
# set the default container mode | |||
if not container_mode: | |||
container_mode = ContainerMode.COLD.value |
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 do we have .value
here, isn't container_mode
an Enum type?
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.
container _mode
is string, I do not think click can define enum options
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.
Should we convert the string we get from the command line args into the Enum and then only use the enum after that point. We should be able to do everything with the Enum directly. We should be able to do something like:
container_mode = ContainerMode(VALUE_FROM_CMD_LINE)
if container_mode is ContainerMode.COLD:
do_cold_mode_things
if not container_mode: | ||
container_mode = ContainerMode.COLD.value | ||
|
||
self._warm_containers = container_mode != ContainerMode.COLD.value |
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.
Will this behave as intended in case of lazy mode?
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.
lazy && warm modes are Warm modes :) .. the difference is the in warm mode will initialize all containers upfront, but in lazy mode the containers will be initialized when invoked
4b3ec81
to
d74bd02
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.
Thanks you for handling the feedback. This revision was much easier to follow.
@@ -107,13 +127,24 @@ def __init__( | |||
self._aws_region = aws_region | |||
self._aws_profile = aws_profile | |||
|
|||
# set the default container mode | |||
if not container_mode: | |||
container_mode = ContainerMode.COLD.value |
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.
Should we convert the string we get from the command line args into the Enum and then only use the enum after that point. We should be able to do everything with the Enum directly. We should be able to do something like:
container_mode = ContainerMode(VALUE_FROM_CMD_LINE)
if container_mode is ContainerMode.COLD:
do_cold_mode_things
async_context.run_async() | ||
LOG.info("Containers Initialization is done.") | ||
except KeyboardInterrupt: | ||
LOG.debug("Ctrl+C was pressed. Aborting containers initialization") |
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.
Is there any cleanup we need to be doing here? Do we need to cancel the async tasks here?
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.
done
click.option( | ||
"--container-mode", | ||
help="Optional. Specifies how SAM cli manages containers.", | ||
type=click.Choice(["WARM", "LAZY"], case_sensitive=False), |
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.
Should we include COLD
here as well? It is part of the Enum.
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.
UX reviewer rejects this suggestion, as he said removing this option (--container-mode) is equivalent to using the COLD option.
@@ -135,7 +135,7 @@ def is_debugging(self): | |||
""" | |||
return bool(self.debug_context) | |||
|
|||
def _get_invoke_config(self, function): | |||
def get_invoke_config(self, function): |
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.
Curious: Is this no longer a private method?
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, It is called in InvokeContext to get the FunctionConfig to send it as part of containers initialization in Runtime.
""" | ||
|
||
warm_containers_options = [ | ||
click.option( |
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 wonder if we should abstract Choices based on Enums into something more concrete. Doesn't look like click supports it out of the box. Not blocking on this but something that came to mind reviewing.
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 am not sure if I got what do you mean.
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.
So we are duplicating the values in line 156 ["EAGER", "LAZY"]
, but we have an Enum that defines this already. If we create a concrete click.option or click.choice that could auto fill out the choices from an Enum, we will never have to keep track or manually add new options here but will just come from the Enum itself.
There is a feature request out to click for this: pallets/click#605
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.
done
samcli/local/docker/container.py
Outdated
@@ -9,6 +9,7 @@ | |||
import docker | |||
import requests | |||
|
|||
from docker.errors import NotFound |
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.
NotFound could be a lot of things. Since we are doing a import directly. I would suggest adding a new name to this.
from docker.errors import NotFound as DockerContainerNotFound
.
Purely for readability.
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.
done
samcli/local/docker/container.py
Outdated
Parameters | ||
---------- | ||
follow: bool | ||
Determines if we need to follow the previous log stream, or get the log stream from the beginning |
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 I follow the follow
flow. Can you explain why it is useful or needed? Just trying to wrap my head around it.
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.
Follow here means if we need to follow on the previous container log stream, or we need to get a new log stream that has all the container logs since it got started.
Mainly, after introducing the warm containers, so we need to follow on the previous log stream, and not to return the log stream since the container was created.
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.
So we reattach to the container? Should we use the logs api to fetch the invoke logs instead? https://docker-py.readthedocs.io/en/stable/containers.html#docker.models.containers.Container.logs Seems like that is what we are trying to do here.
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.
refactored it.
actually find that we should not create a new log thread, as long as there is already running one.
this thread will keep running till we delete the container, so no need to have multiple threads (actually having multiple threads will cause issues)
cls.thread.setDaemon(True) | ||
cls.thread.start() | ||
|
||
@classmethod | ||
def start_api(cls): | ||
def start_api(cls, template_path, port, container_mode=None, parameter_overrides=None): |
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 think all the other tests, pull this from the cls
. Can we keep the same pattern here so all of the tests are in the same format?
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.
done
def setUpClass(cls): | ||
# This is the directory for tests/integration which will be used to file the testdata | ||
# files for integ tests | ||
template = cls.integration_dir + cls.template_path |
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.
Isn't this controlled by StartApiIntegBaseClass
? StartApiIntegBaseClass is starting up a start-api
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.
refactored this part
|
||
@pytest.mark.flaky(reruns=3) | ||
@pytest.mark.timeout(timeout=600, method="thread") | ||
def test_all_containers_are_running(self): |
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.
So by biggest concern here is we hide failures. In a TDD framework, you want to know all the failures at once. This allows you to know the impact of your change. I generally prefer isolated tests as much as possible. I recognize this might not always be possible but something we should try to do everywhere we can.
Would it makes sense to have one Test Class for the first and another for the second? You are testing different use cases. Classes are cheap and can be used to organize different things. Maybe the problem is our naming of the containers? Maybe we shouldn't need to name them at all?
d74bd02
to
e70241a
Compare
@@ -109,6 +129,12 @@ def __init__( | |||
self._aws_region = aws_region | |||
self._aws_profile = aws_profile | |||
|
|||
container_mode_enum = ContainerMode(container_mode) | |||
self._warm_containers = container_mode_enum != ContainerMode.COLD |
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 wonder if there's any unambiguous terminology for this. Currently ContainerMode
suggests "warm", "cold" and "lazy" are separate modes. But this again suggests that both "warm" and "lazy" are "warm". It can be a confusing asymmetry.
(Just brainstorming) Maybe renaming WARM
to WARM_EAGER
and LAZY
to WARM_LAZY
is clearer? Warm/cold describe the container shutdown behavior, and lazy/eager describe the container startup behavior.
(COLD
already implies lazy/cold so that's fine; it makes little sense to have an eager/cold container that starts upfront and stops after the first call...)
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.
COLD is just used internally, but it is not part of the command container-mode parameter options, as actually removing this parameter means the option COLD (which is the current behavior), and this was the UX recommendation (not to add COLD as an option)
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 am not following why shouldn't we add COLD in the command line?
2ba42ae
to
99fef6c
Compare
@@ -15,3 +15,4 @@ requests==2.23.0 | |||
serverlessrepo==0.1.10 | |||
aws_lambda_builders==1.1.0 | |||
tomlkit==0.7.0 | |||
watchdog==0.10.3 |
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.
How does watchdog affect what we need installed for customers? Looking at their github repo, it says you need xcode installed for Mac: https://github.com/gorakhargosh/watchdog#dependencies.
There is no .whl
for this package in pypi as well. How does this dependency impact MSI generation and possible the pyinstaller work that is ongoing for a new way to install linux? We have had issues with libraries written in C before with our installers.
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.
regarding the xcode dependency, I think it should not be a problem, as we use use brew to install SAM, and brew depends on xcode as well.
I tested the MSI generation, and the pyinstaller, and both work fine, so I do not think adding watchdog will affect the release process.
@@ -109,6 +129,12 @@ def __init__( | |||
self._aws_region = aws_region | |||
self._aws_profile = aws_profile | |||
|
|||
container_mode_enum = ContainerMode(container_mode) | |||
self._warm_containers = container_mode_enum != ContainerMode.COLD |
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 am not following why shouldn't we add COLD in the command line?
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.
Getting closer. Thank you for being patient with reviews and handling feedback.
self._warm_containers_initializing_mode = WarmContainersInitializationMode.LAZY | ||
if warm_container_initialization_mode: | ||
self._warm_containers_initializing_mode = WarmContainersInitializationMode( | ||
warm_container_initialization_mode |
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.
Should this be a .upper()
? If the casing is wrong, this will thrown an exception.
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.
Click handles that, it will always send the upper case of the options regardless how the customer enter it.
@@ -144,17 +193,53 @@ def __enter__(self): | |||
"Running AWS SAM projects locally requires Docker. Have you got it installed and running?" | |||
) | |||
|
|||
# initialize all lambda function containers upfront | |||
if self._warm_containers and self._warm_containers_initializing_mode == WarmContainersInitializationMode.EAGER: |
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 can simplify things a little here. If we self._warm_containers_initializing_mode
to default to WarmContainersInitializationMode.COLD
, we can remove the need for self._warm_containers
completely and only check for the mode we care about. This also remains backwards compatible, since currently we only support 'cold'.
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.
done
|
||
def initialize_function_container(function): | ||
function_config = self.local_lambda_runner.get_invoke_config(function) | ||
self.lambda_runtime.run(None, function_config, self._debug_context, None) |
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.
Should we be 'running' or starting
a container/function? Maybe run is the right thing here so we standup aws-lambda-rie
within the container, but want to still ask in case another word makes sense to match the initialize
concept here.
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 think we can use running, as it also the same name use in docker to run the container
|
||
layer_downloader = LayerDownloader(self._layer_cache_basedir, self.get_cwd()) | ||
image_builder = LambdaImage(layer_downloader, self._skip_pull_image, self._force_image_build) | ||
if self._warm_containers: |
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 does work but I would encourage us to make sure things like this are easily expandable. One way to model this is for each mode
we have a LambdaRuntime
. Then here we just do a look up and call. This is how our build workflow lookup is handled: https://github.com/aws/aws-sam-cli/blob/develop/samcli/lib/build/workflow_config.py
What this will give us is each time we add a new mode, we could create a new runtime
or map it to an exiting one. If we do not, the lookup could throw an error (KeyError is we us a dict to implement). This will guard us against setting a new mode but not creating/mapping a new runtime to it and therefore defaulting to the LambdaRuntime
, which is cold started. This is similar to the default
clause in a Java switch case over an Enum.
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.
done
""" | ||
|
||
warm_containers_options = [ | ||
click.option( |
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.
So we are duplicating the values in line 156 ["EAGER", "LAZY"]
, but we have an Enum that defines this already. If we create a concrete click.option or click.choice that could auto fill out the choices from an Enum, we will never have to keep track or manually add new options here but will just come from the Enum itself.
There is a feature request out to click for this: pallets/click#605
samcli/local/docker/container.py
Outdated
Parameters | ||
---------- | ||
follow: bool | ||
Determines if we need to follow the previous log stream, or get the log stream from the beginning |
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.
So we reattach to the container? Should we use the logs api to fetch the invoke logs instead? https://docker-py.readthedocs.io/en/stable/containers.html#docker.models.containers.Container.logs Seems like that is what we are trying to do here.
self._add_function_to_observer(function_config) | ||
return container | ||
|
||
def _on_invoke_done(self, container): |
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 function doesn't have any implementation. Is that expected? Or maybe I am miss reading the diff
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, this function is to delete the created container (or to clear any resources) once the invocation is done .. but in warm containers, we can not delete the containers after the invocation is done, we need to wait till the command termination. That is why, I override this function in the WarmContainersRuntime, and make it do nothing.
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 see now. Thanks for explaining.
samcli/local/lambdafn/runtime.py
Outdated
else: | ||
# observe the build directory instead of the lambda function directory in case of runtime languages | ||
# that needs compilation | ||
if ".aws-sam" in code_path: |
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 are we checking .aws-sam
directly here? Why is it not just the code_path (CodeUri/Code) set in the template?
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.
refactored this part
) | ||
debug_context = None | ||
|
||
container = super().create(function_config, debug_context, None) |
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 are we calling super on ourselves and then create? How does this work, won't this just send us into a infinite loop?
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.
as per documentation super() function is used to access the inherited methods that have been overridden in a class.
So here, I am wrapping the parent create method with some logic that should be done only in case of Warm containers
samcli/local/lambdafn/runtime.py
Outdated
function_config: FunctionConfig | ||
Configuration of the function to create a new Container for it. | ||
""" | ||
with self._lock: |
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.
How does this work for multiple functions getting updated at the same time? Why do we need a lock?
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.
refactored this part
99fef6c
to
4a8730b
Compare
4a8730b
to
4cf52e6
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.
Just a couple of small comments. Really appreciate you taking the feedback and addressing. Great work and customers are going to love the improvement :)
""" | ||
|
||
|
||
class FileObserver: |
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.
Thank you for abstracting this out now. This could come in handy if we ever want an auto/watch build command to be enabled! 🎉
network.connect(self.id) | ||
except NotFound: | ||
# stop and delete the created container before raising the exception | ||
real_container.remove(force=True) |
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.
Ahh. Somehow I missed the word Network. This makes much more sense now. Thanks.
@@ -78,3 +78,6 @@ def __init__( | |||
self.env_vars.handler = self.handler | |||
self.env_vars.memory = self.memory | |||
self.env_vars.timeout = self.timeout | |||
|
|||
def __eq__(self, other): | |||
return self.name == other.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.
Should this only be name to consider the objects equal?
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 think per template, we can not have 2 lambda functions that have the same name, that is why I think name is good for comparison.
self._add_function_to_observer(function_config) | ||
return container | ||
|
||
def _on_invoke_done(self, container): |
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 see now. Thanks for explaining.
self._logs_thread = threading.Thread(target=self.wait_for_logs, args=(stderr, stderr), daemon=True) | ||
self._logs_thread.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.
Are we shutting down this thread in any where else? My worry is if these will keep spin-up, we might end-up with a memory leak.
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 thread will keep running as long as the container is running, and it will finish once the container got deleted.
Thanks for the work @moelasmar, this will be a great addition! 🥳 |
I'd also like to say thanks. This is something we've been after for quite a while to help improve developer experience. |
Thanks for this feature, much appreciated. I have one question about this warm container. Does it simulate the case when lambda container is left static long enough that it will terminate the container until the lambda function is called again? |
No, this does not mimic the 'freeze thaw' behavior of Lambda. This is purely to improve the speed of testing not fidelity with Lambda. |
Best thing AWS released in 2020! Development with SAM is infinitely better. Thanks. |
Is
But I could not find any documentation about this. |
@yskkin ... your explanation is correct, I will discuss with the team to update its documentation. |
Which issue(s) does this change fix? #239
Why is this change necessary?
This change added the warm containers options to enhance the Lambda Function local testing performance.
How does it address the issue?
we added the container-mode option that determine if SAM CLI will handle the containers life cycle in warm mode, or normal mode.
What side effects does this change have?
N/A
Checklist
make pr
passesmake update-reproducible-reqs
if dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.