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

(aws-s3-deployment): Switch Lambda architecture to ARM_64 #29996

Open
2 tasks
eruvanos opened this issue Apr 29, 2024 · 11 comments
Open
2 tasks

(aws-s3-deployment): Switch Lambda architecture to ARM_64 #29996

eruvanos opened this issue Apr 29, 2024 · 11 comments
Labels
@aws-cdk/aws-s3-deployment effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@eruvanos
Copy link

eruvanos commented Apr 29, 2024

Describe the feature

To reduce costs and improve sustainability, it would be great to switch CDKBucketDeployment Lambda to ARM_64.

Use Case

Reduced costs.

Proposed Solution

The code looks compatible to ARM_64, switching architecture should be enough.

Other Information

To give more information, it would be great to be able to have little more control about created Lambdas.
E.g. our environment enforces Lambdas to be deployed into a VPC. This is only possible using an Aspect.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.102.0

Environment details (OS name and version, etc.)

independent

@eruvanos eruvanos added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Apr 29, 2024
@pahud
Copy link
Contributor

pahud commented Apr 29, 2024

Make perfect sense to me. Welcome to submit a PR. Thank you.

@pahud pahud added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Apr 29, 2024
@tmokmss
Copy link
Contributor

tmokmss commented May 4, 2024

Is Graviton Lambda available in all the regions and partitions? I could not find relevant coverage data.

@eruvanos
Copy link
Author

eruvanos commented May 4, 2024

@tmokmss That is a good question, did not think about that. I will check, otherwise we would have to introduce a property.

@nmussy
Copy link
Contributor

nmussy commented May 6, 2024

The documentation states that we should be using the pricing region dropdown to get the ARM availability. It seems to be globally available as of today, comparing the dropdown options with a browser inspector, they both have the following 31 regions, including Gov:

us-east-1
us-east-2
us-west-1
us-west-2
af-south-1
ap-east-1
ap-south-2
ap-southeast-3
ap-southeast-4
ap-south-1
ap-northeast-3
ap-northeast-2
ap-southeast-1
ap-southeast-2
ap-northeast-1
ca-central-1
ca-west-1
eu-central-1
eu-west-1
eu-west-2
eu-south-1
eu-west-3
eu-south-2
eu-north-1
eu-central-2
il-central-1
me-south-1
me-central-1
sa-east-1
us-gov-east-1
us-gov-west-1

The CN regions are not included, but they also both seem to support the ARM arch, see pricing.

It should also be safe to assume newly introduced regions will get this feature, given ca-west-1 is the latest release. I don't think this is a concern, but good call @tmokmss

@daschaa
Copy link
Contributor

daschaa commented May 13, 2024

The linked PR looks good imo.
Does this change need an entry in the README or can the exemption request be approved? I think the pr-linter still marks the pull request as "not ready".

@github-actions github-actions bot added p1 and removed p2 labels May 19, 2024
Copy link

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

@shikha372 shikha372 self-assigned this May 21, 2024
@eruvanos
Copy link
Author

eruvanos commented Jun 5, 2024

Update: Our PR was basically rejected, because the proposed solution does not cover all requirements.

I changed the issue description accordingly.

@jfuss
Copy link
Contributor

jfuss commented Jun 11, 2024

@eruvanos I think there is still value but we cannot just change it for everyone. We have to assume someone somewhere relied on this being x86-64.

The path to getting this added to CDK would be:

  1. Making the AWSCLI layer Construct support both architectures.
  2. Adding a property that accepts the architecture for CDKBucketDeployment Lambda.
  3. Picking the correct AWS CLI Layer based on teh architecture.

This adds more work but removes risks from just changing the default to something else without worry about breaking any existing customers.

@daschaa
Copy link
Contributor

daschaa commented Jun 14, 2024

@jfuss That sounds like a good plan. Is this "feature" already planned on you side and if so, can you give an ETA?

@shikha372 shikha372 removed their assignment Jun 26, 2024
@jfuss
Copy link
Contributor

jfuss commented Jul 16, 2024

@daschaa Not planned at this time from our side to my knowledge. You or another community member is free to pick it up, if anyone is passionate about it.

@daschaa
Copy link
Contributor

daschaa commented Jul 18, 2024

@jfuss So to implement this feature it's also expected from a community member to create a pull request on the layer repo to support both architectures? Or is this something that is implemented by AWS?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3-deployment effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants