Skip to content
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

Add Lambda runtime parity configuration for ulimit #7871

Merged
merged 14 commits into from Mar 20, 2023

Conversation

joe4dev
Copy link
Member

@joe4dev joe4dev commented Mar 15, 2023

Addresses #6942

This PR makes ulimit configurable for Lambda as an opt-in option and refactors the Docker flags parser:

  • Make ulimit configurable through LAMBDA_DOCKER_FLAGS=--ulimit nofile=1024:1024 --ulimit nproc=735:735 --ulimit core=-1:-1
  • Add parity test for ulimit
  • Add unit tests for ulimit, platform, and user flags
  • Refactor custom implementation of Docker flags parser using argparse. The custom subclass NoExitArgumentParser prevents the parser from exiting (default behavior for interactive CLI).
  • Split monolithic unit test case into logical segments and improve structure (mostly alphabetical ordering)

@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 15, 2023 00:09 — with GitHub Actions Inactive
@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 15, 2023 00:35 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Mar 15, 2023

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 35m 17s ⏱️ -16s
1 829 tests +1  1 441 ✔️ +1  388 💤 ±0  0 ±0 
2 553 runs  +3  1 808 ✔️ +2  745 💤 +1  0 ±0 

Results for commit 4301369. ± Comparison against base commit 10e77c0.

♻️ This comment has been updated with latest results.

@joe4dev joe4dev force-pushed the lambda-runtime-parity-ulimit branch from f249eb4 to 5ff046c Compare March 15, 2023 08:36
@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 15, 2023 08:36 — with GitHub Actions Inactive
@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 15, 2023 09:47 — with GitHub Actions Inactive
@coveralls
Copy link

coveralls commented Mar 15, 2023

Coverage Status

Coverage: 85.146% (+0.01%) from 85.135% when pulling 4301369 on lambda-runtime-parity-ulimit into 10e77c0 on master.

@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 15, 2023 11:43 — with GitHub Actions Inactive
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


if args.envs:
for env in args.envs:
lhs, _, rhs = env.partition("=")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems we are doing this partitioning in some cases. We could define an argparse custom action for it https://docs.python.org/3/library/argparse.html#action , like demonstrated here: https://www.geeksforgeeks.org/python-key-value-pair-using-argparse/ (was the first link for it but it demonstrates what's possible).

Just a thought, nothing urgent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully agree 👍
I saw it but just tackled the main argument parsing in the first iteration. This looks like a low-hanging fruit though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added a comment for now. A quick test revealed that more tests and refinements are needed to ensure equivalent parsing, especially given some flag-specific logic.

localstack/utils/no_exit_argument_parser.py Show resolved Hide resolved
Comment on lines 382 to 386
@pytest.mark.skipif(
not use_docker(),
reason="Monkeypatching of Docker flags not applicable if run locally",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for the other PR, why wouldn't it be applicable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Monkeypatching works but Docker-flags are not used with the local executor in the old lambda provider because no new Docker container is started. Hence, DOCKER flags are not applicable and Docker tests with the local executor fail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I clarified the reason and added a new is_old_local_executor() util. This needs to be extended to the new local executor replacement (i.e., the self-managed workers) because they do not spawn new containers either.

The list of optional argument is becoming hard to maintain when using implicit ordering.
Explicit key/value pairs make the usage of `parse_additional_flags` much clearer and easier.
We don't need to offer a no-privileged flag. Hence, we can simplify parsing of a boolean flag while maintaining compatibility for Python 3.7
* Use Typings for older Python version
* Convert TypedDict into dataclass
Requires more testing to ensure consistent behavior (e.g., with multiple arguments, empty arguments, and combinations).
@joe4dev joe4dev force-pushed the lambda-runtime-parity-ulimit branch from 62ee45d to 4301369 Compare March 20, 2023 13:57
@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 20, 2023 13:57 — with GitHub Actions Inactive
@joe4dev joe4dev merged commit 77bd2c2 into master Mar 20, 2023
8 checks passed
@joe4dev joe4dev deleted the lambda-runtime-parity-ulimit branch March 20, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants