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

Fix architecture update for Lambda function #10263

Merged
merged 2 commits into from Feb 17, 2024

Conversation

joe4dev
Copy link
Member

@joe4dev joe4dev commented Feb 16, 2024

Motivation

UpdateFunctionCode does not support updating the Architectures (e.g., x64 => ARM).
This issue has been reported and reproduced by @cabeaulac based on customer input.

Changes

  • Adjust the mixed architecture test test_mixed_architecture to use UpdateFunctionCode
  • Implement Architecture update in UpdateFunctionCode

Testing

Our CI pipeline currently does not support cross-architecture emulation. Therefore, this change can only be tested locally on a machine with cross-architecture support (e.g., ARM MacBook):

  1. Unskip tests.aws.services.lambda_.test_lambda.TestLambdaBehavior.test_mixed_architecture
  2. Run tests.aws.services.lambda_.test_lambda.TestLambdaBehavior.test_mixed_architecture

@joe4dev joe4dev added aws:lambda AWS Lambda semver: patch Non-breaking changes which can be included in patch releases labels Feb 16, 2024
@joe4dev joe4dev self-assigned this Feb 16, 2024
@joe4dev joe4dev marked this pull request as ready for review February 16, 2024 11:42
@coveralls
Copy link

coveralls commented Feb 16, 2024

Coverage Status

coverage: 83.849% (-0.01%) from 83.86%
when pulling 317f958 on fix-lambda-architecture-update
into af377bb on master.

Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 20m 0s ⏱️ - 1m 43s
2 641 tests ±0  2 394 ✅ ±0  247 💤 ±0  0 ❌ ±0 
2 643 runs  ±0  2 394 ✅ ±0  249 💤 ±0  0 ❌ ±0 

Results for commit cb2ec80. ± Comparison against base commit af377bb.

Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

🙌 were just talking about this yesterday, great job

Comment on lines +1140 to +1144
if len(architectures) != 1:
raise ValidationException(
f"1 validation error detected: Value '[{', '.join(architectures)}]' at 'architectures' failed to "
f"satisfy constraint: Member must have length less than or equal to 1",
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if len(architectures) != 1:
raise ValidationException(
f"1 validation error detected: Value '[{', '.join(architectures)}]' at 'architectures' failed to "
f"satisfy constraint: Member must have length less than or equal to 1",
)
if len(architectures) > 1:
raise ValidationException(
f"1 validation error detected: Value '[{', '.join(architectures)}]' at 'architectures' failed to "
f"satisfy constraint: Member must have length less than or equal to 1",
)

Shouldn't be an empty architectures list be fine according to the error message? (would also affect the architectores[0] below since then it won't be safe anymore.

Copy link
Member

Choose a reason for hiding this comment

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

If that's not the case (bc AWS treats it as null and thus returns the error msg below), please add a comment 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment:

# An empty list of architectures is also forbidden. Further exceptions are tested here for create_function:
# tests.aws.services.lambda_.test_lambda_api.TestLambdaFunction.test_create_lambda_exceptions

Limitation: only works on ARM hosts that support x86 emulation.
def test_mixed_architecture(self, create_lambda_function, aws_client, snapshot):
"""Test emulation of a lambda function changing architectures.
Limitation: only works on hosts that support both ARM and AMD64 architectures.
Copy link
Member

Choose a reason for hiding this comment

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

Interesting limitation. Think we should introduce a marker for this

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree that adding marker is better than the plain skip. Maybe adding an arm marker:

only_on_amd64 = pytest.mark.only_on_amd64
only_on_arm64 = pytest.mark.only_on_arm64

This special test would then have both markers.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just visually a bit tricky to distinguish:

amd64
arm64

Copy link
Member

Choose a reason for hiding this comment

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

True but fortunately markers are mostly for automation and filtering purposes, so I'd say that's ok 👍

@joe4dev joe4dev merged commit db3c42f into master Feb 17, 2024
21 of 23 checks passed
@joe4dev joe4dev deleted the fix-lambda-architecture-update branch February 17, 2024 11:23
@joe4dev joe4dev mentioned this pull request Feb 19, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:lambda AWS Lambda semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants