8000 Add lambda config to ignore architecture by joe4dev · Pull Request #7890 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

Add lambda config to ignore architecture #7890

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

Merged
merged 5 commits into from
Mar 20, 2023
Merged

Conversation

joe4dev
Copy link
Member
@joe4dev joe4dev commented Mar 16, 2023

Add an environment variable LAMBDA_IGNORE_ARCHITECTURE to ignore the architecture (i.e., x86_64 or arm64) of a Lambda function and let Docker select a matching image based on the host operating system and architecture.

Motivation

  • Improve performance by natively running cross-platform compatible lambda functions.
  • Facilitate cross-platform testing with different test runners. We might need to introduce pytest markers to indicate compatibility restrictions (e.g., amd64_only and arm64_only).

Further changes

  • Use standardized get_arch() to match aarch64 platform on Linux
  • Move is_arm_compatible() to platform utils
  • Make the PULLED_IMAGES cache platform-aware to prevent running into timeouts upon invocation

@joe4dev joe4dev self-assigned this Mar 16, 2023
@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 16, 2023 21:49 — with GitHub Actions Inactive
@github-actions
Copy link
github-actions bot commented Mar 16, 2023

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 43m 12s ⏱️ - 8m 7s
1 828 tests +2  1 439 ✔️ ±0  389 💤 +2  0 ±0 
2 550 runs  +6  1 805 ✔️ ±0  745 💤 +6  0 ±0 

Results for commit 9df0b23. ± Comparison against base commit 3b41e13.

♻️ This comment has been updated with latest results.

@joe4dev joe4dev force-pushed the lambda-native-platform-flag branch from 37684bc to 965d2ae Compare March 17, 2023 12:32
@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 17, 2023 12:32 — with GitHub Actions Inactive
@coveralls
Copy link
coveralls commented Mar 17, 2023

Coverage Status

Coverage: 85.135% (-0.008%) from 85.142% when pulling 9df0b23 on lambda-native-platform-flag into 3b41e13 on master.

@joe4dev joe4dev force-pushed the lambda-native-platform-flag branch from 965d2ae to a8bdc0e Compare March 17, 2023 14:49
@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 17, 2023 14:49 — with GitHub Actions Inactive
Copy link
Member
@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM!

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, just some minor questions for tests, nothing to hold back the merge though.

Comment on lines 382 to 386
@pytest.mark.skipif(
not use_docker(),
reason="Monkeypatching of Docker-related flag 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.

Why wouldn't this work?

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
@dfangl dfangl Mar 20, 2023

Choose a reason for hiding this comment

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

Ah, now i understand. The if run locally confused me, as I thought of running the test locally, not running lambdas in the local executor. Overlooked the use_docker and thought of in_docker.

Still, use_docker might be problematic, as it would skip the tests if I set (by accident) something weird like LAMBDA_EXECUTOR=local PROVIDER_OVERRIDE_LAMBDA=v2, which should work (as it uses the new provider) but will skip as it thinks of the old provider only.

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 skip is currently redundant for test_ignore_architecture given the old provider is unsupported anyways but needs to be extended to the new local executor replacement (i.e., the self-managed workers) because they do not spawn new containers either.

Comment on lines +413 to +417
@pytest.mark.skipif(
not is_arm_compatible() and not is_aws(),
reason="ARM architecture not supported on this host",
)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine for now, but emulation is thinkable in both ways, also arm64 on amd64.

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting point. That could even be added to is_arm_compatible() if it becomes relevant in the future.

@joe4dev joe4dev force-pushed the lambda-native-platform-flag branch from a8bdc0e to 9df0b23 Compare March 20, 2023 11:35
@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 20, 2023 11:36 — with GitHub Actions Inactive
@joe4dev joe4dev merged commit 10e77c0 into master Mar 20, 2023
@joe4dev joe4dev deleted the lambda-native-platform-flag branch March 20, 2023 13:42
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.

4 participants
0