Fix: Add ARM64 support for provided.al2023 runtime#13
Open
bnusunny wants to merge 32 commits into
Open
Conversation
Add a manually-triggered GitHub Actions workflow that runs test_download_two_layers 5 times with debug instrumentation to diagnose why layers sometimes return 'Layer1' instead of 'Layer2' on Docker (but not Finch). Instrumentation logs: - Layer ordering in download_all() input/output - Generated Dockerfile ADD command sequence - Tarball construction order - Image build vs reuse decisions
Instead of running isolated iterations, run the exact same pytest command as the CI local-invoke suite with added debug logging. This reproduces the real test ordering and cross-test interactions. Enhanced instrumentation now also captures: - Per-layer download decisions (cached vs fresh) - Extracted layer file contents after download - Docker build stream output (cache hits show as CACHED)
Run just the TestLayerVersion class instead of the full local-invoke suite. This covers the failing test_download_two_layers plus the other tests in the class that may cause cross-test contamination.
debug: add workflow to investigate flaky layer ordering tests
debug: remove repo guard for fork
debug: use direct OIDC auth
debug: add BY_CANARY=true
…rm container tests * Update notify-slack.yml to add sleep (aws#8646) * fix: use tag prefix matching to clean up samcli/lambda-* images (aws#8647) Docker's images.list(name='samcli/lambda') does exact repository matching and won't match repositories like 'samcli/lambda-python'. This caused stale images to persist across parameterized test classes, leading to flaky test_download_two_layers failures where Layer2 should overwrite Layer1 but the image was never rebuilt. Fix by: 1. Adding _cleanup_samcli_images() that lists all images and filters by 'samcli/lambda-' tag prefix 2. Using this method in both tearDown and tearDownClass 3. Fixing the same pattern in TestLayerVersionThatDoNotCreateCache * fix: remove fallback in count_running_containers that causes flaky warm container tests The count_running_containers method had a fallback that returned the count of ALL SAM CLI containers when MODE env var filtering found no matches. This caused AssertionError: 3 != 2 when stale containers from other tests were present. Now it strictly counts only containers matching this test's unique MODE UUID and uses exact string matching. * fix: add AWS_DEFAULT_REGION and use -k pattern for parameterized tests * fix: add BY_CANARY=true to enable Docker tests on CI * fix: exclude RemoteLayers tests that need AWS credentials --------- Co-authored-by: seshubaws <116689586+seshubaws@users.noreply.github.com>
When build_in_source is used with Node.js, the build directory contains a node_modules symlink. During local invoke, SAM CLI resolves this symlink and creates an additional bind mount. Docker tolerates creating a mountpoint over a symlink, but Finch (containerd/runc) fails with 'not a directory'. This fix temporarily replaces symlinks with empty directories before container creation, then restores them afterward. This ensures: - Finch/runc gets a valid directory mountpoint - Docker continues to work as before - Repeated invocations work because symlinks are restored - Host filesystem is left unchanged even if container creation fails
fix: replace symlinks with dirs for Finch container mount compatibility
fix: restore BY_CANARY to run docker tests on CI
Containerd/Finch resolves bind mounts at start time, not create time. Moving the symlink restore to after start() ensures the empty directory mountpoints are still present when the container runtime sets up mounts.
fix: move symlink restore to after container start
Bind mounts must remain valid for the container's entire lifetime. Both Docker and Finch need the empty directory mountpoint to persist until the container is stopped and deleted. Restoring symlinks in the delete() method's finally block ensures proper cleanup alongside the existing host_tmp_dir cleanup.
fix: restore symlinks at container delete time
Implement a local CloudFormation Language Extensions processor supporting: - Fn::ForEach loop expansion in Resources, Conditions, and Outputs - Fn::Length, Fn::ToJsonString intrinsic functions - Fn::FindInMap with DefaultValue support - Conditional DeletionPolicy/UpdateReplacePolicy - Nested ForEach depth validation (max 5 levels) - Partial resolution mode preserving unresolvable references Pipeline architecture: TemplateParsingProcessor -> ForEachProcessor -> IntrinsicResolverProcessor -> DeletionPolicyProcessor -> UpdateReplacePolicyProcessor Includes comprehensive unit tests and CloudFormation compatibility suite.
Wire the language extensions library into SAM CLI with two-phase architecture: - Phase 1: expand_language_extensions() -> LanguageExtensionResult - Phase 2: SamTranslatorWrapper.run_plugins() (SAM transform only) Key components: - expand_language_extensions() canonical entry point with template-level cache keyed on (path, mtime, params_hash) - SamTranslatorWrapper receives pre-expanded template (Phase 2 only) - SamLocalStackProvider.get_stacks() calls expand_language_extensions() - SamTemplateValidator calls expand_language_extensions() - DynamicArtifactProperty dataclass for Mappings transformation - Fn::ForEach guards in artifact_exporter, normalizer, cdk/utils - clear_expansion_cache() for warm container file change events
- _get_template_for_output() preserves Fn::ForEach in build output - _update_foreach_artifact_paths() generates Mappings for dynamic artifact properties with per-function build paths - Recursive nested Fn::ForEach support - ForEach-aware path resolution skips Docker image URIs Test templates: static CodeUri, dynamic CodeUri, parameter collections, nested stacks, nested ForEach, dynamic ImageUri, depth validation.
Package:
- _export() calls expand_language_extensions() for Phase 1
- Preserves Fn::ForEach in packaged template with S3 URIs
- Generates Mappings for dynamic artifact properties
- _find_artifact_uri_for_resource() handles all export formats:
string, {S3Bucket,S3Key}, {Bucket,Key}, {ImageUri}
- Recursive nested Fn::ForEach support
- Warning for parameter-based collections
Deploy:
- Uploads original unexpanded template to CloudFormation
- Clear error for missing Mapping keys
Integration tests for CodeUri, ContentUri, DefinitionUri, ImageUri,
BodyS3Location across all packageable resource types.
- sam validate: valid ForEach, invalid syntax, cloud-dependent collections, dynamic CodeUri, nested depth validation (5 valid, 6 invalid) - sam local invoke: expanded function names from ForEach - sam local start-api: ForEach-generated API endpoints
Add make test-lang-ext and make test-all targets so the 1695 language extensions unit tests only run when needed, keeping the default make test fast for unrelated PRs.
The expand_language_extensions() cache stored references to template dicts that were later mutated in-place by ApplicationBuilder.update_template() (which changes nested stack Location properties to build-output paths). On cache hit, the mutated dict was returned, causing TemplateNotFoundException during the second infra sync in sam sync --watch. Remove the cache entirely since deep-copying on hit negates the performance benefit and adds complexity. Keep clear_expansion_cache() as a no-op for backward compatibility. Fixes TestSyncInfraNestedStacks_0 and TestSyncInfraNestedStacks_1 integration test failures.
fix: remove expansion cache to fix sync watch nested stack failures
fix: use OIDC credentials directly for sync test workflow
Owner
Author
|
@cortex-assistant please review this PR |
![cortex-assistant[bot]](https://avatars.githubusercontent.com/u/12918?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add ARM64 support for provided.al2023 runtime
Summary
This PR adds a new arm64_fix.py file at the repo root to address issue #12. However, after reviewing the existing codebase, this file is dead code — it is never imported by the SAM CLI, and the fix it attempts to apply already exists in the canonical source.
Key Finding
The existing file samcli/lib/utils/architecture.py already includes ARM64 support for both provided.al2 and provided.al2023:
"provided.al2": [ARM64, X86_64],
"provided.al2023": [ARM64, X86_64],This means either the bug in #12 doesn't exist in the current develop branch, or the root cause is elsewhere (e.g., Docker image selection, build workflow, or a different code path) — and this PR doesn't address it.
Issues
| # | Severity | Description |
|---|---|---|
| 1 | 🔴 Critical | Wrong location: arm64_fix.py is at the repo root and is never imported by the SAM CLI. The "fix" has zero effect. |
| 2 | 🔴 Critical | Duplicates existing logic: Recreates SUPPORTED_RUNTIMES from samcli/lib/utils/architecture.py with a different name, only a subset of runtimes, and hardcoded strings instead of constants. |
| 3 | 🟡 Medium | Incomplete runtime list: Only covers 5 runtimes while the canonical map covers 25+. |
| 4 | 🟡 Medium | No tests: No unit tests added for the new code. |
| 5 | 🟠 Low | Hardcoded strings: Uses "x86_64" and "arm64" literals instead of the project's X86_64/ARM64 constants. |
Recommendations
- Remove
arm64_fix.py— it's dead code. - Investigate the actual root cause of issue #12. The existing architecture mapping already supports ARM64 for custom runtimes, so the bug likely lives elsewhere (build workflow, Docker image selection, etc.).
- If a code change is needed, modify the existing modules directly (e.g.,
samcli/lib/utils/architecture.pyor the relevant build module). - Add a test that reproduces the reported failure scenario.
| @@ -0,0 +1,13 @@ | |||
| # Fix: Add arm64 support for provided.al2023 runtime | |||
There was a problem hiding this comment.
🔴 Dead code: This file is at the repo root and is never imported by the SAM CLI application. It will have no effect on the build behavior.
The canonical architecture mapping already lives in samcli/lib/utils/architecture.py and already includes ARM64 for provided.al2023 and provided.al2. Any fix should modify the existing module, not add a standalone file.
| "python3.13": ["x86_64", "arm64"], | ||
| "nodejs20.x": ["x86_64", "arm64"], | ||
| "provided.al2023": ["x86_64", "arm64"], # Fixed: was missing arm64 | ||
| "provided.al2": ["x86_64", "arm64"], |
There was a problem hiding this comment.
🟡 Incomplete & duplicated: This only covers 5 runtimes, while the existing SUPPORTED_RUNTIMES dict in samcli/lib/utils/architecture.py covers 25+. Also uses hardcoded strings ("arm64", "x86_64") instead of the project's ARM64 and X86_64 constants.
|
I've completed the review of PR #13 and submitted Request Changes. Here's the TL;DR:
I left two inline comments on the file with specific details. |
Owner
Author
|
@cortex-assistant Please review this PR. |
|
The PR has not changed since my previous review — it's still the same single commit ( My previous review still stands. Here's a summary of the findings: Verdict: 🔴 Request Changes (already submitted) The core issues remain:
To move forward, the author should:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #12
Adds arm64 to the supported architectures map for provided.al2023 and provided.al2 runtimes.