feat(cli): parallel asset publishing #19367
Conversation
|
|
|
Here's what publishing looks like on my test project: Parallel Asset Publishing (18 seconds) publish-parallel.mp4 |
misterjoshua
changed the title
rix0rrr
changed the title
rix0rrr
added
the
pr-linter/exempt-readme
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
1 similar comment
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
It appears that this change has exposed a race condition when publishing multiple ECS images from a Mac. It is causing our deployments to fail with:
It appears that this issue is caused by a combination of the CDK performing a “docker login” on each published asset, along with specific logic in the docker project for storing credentials in the OSX keychain. The source code for docker-credentials-osxkeychain doesn’t update existing OSX keychain entries if present. Instead it always does a SecKeychainAddInternetPassword call, which fails if the entry is already in the keychain. That error isn’t caught and handled. Instead the code attempts to avoid this issue by deleting the entry in the keychain before each add attempt. This logic works fine for the normal use case of doing a single login and then making multiple authenticated docker calls (single-threaded or in parallel). In fact, the logic has been in place for five-plus years. Unfortunately, the CDK calls “docker login” before each “docker push” call instead of once for the batch. Two publish calls started at the same time will (most of the time) result in a credentials sequence of “delete, delete, add, add” such that the second add will fail because the first add created the entry. The best method for fixing this issue in the CDK would involve refactoring the publishing code to perform a single “docker login” for the batch of container assets. Unfortunately this would be a significant redesign as the logic is shared between lambda assets and container assets. Another option might be to catch the failed login and try again after a small delay, with or without detecting the specific error message. This doesn’t feel super clean, but is less effort and is less likely to cause side effects. That said, while it should work fine for small numbers of container assets, it may become problematic when there are many publish calls all logging in and retrying all at once. |
I wonder whether we could track logins here and only run |
|
Update: Downgrading to version 2.16.0 lets me continue the 2.0 upgrade. |
|
@mitchlloyd @brianfrantz I've created a dedicated bug report issue to track this issue: #20116 I've also proposed a change that seems to resolve this problem on a mac I can access. #20117. It would benefit from your review and testing. |
Once again, any change to anything anywhere broke someone. In this particular case, parallel asset publishing (#19367, and in particular building) broke a use case where someone uses a tool to build assets that is not concurrency-safe. So -- now we need to make that configurable. Command line: ``` cdk deploy --no-asset-parallelism ``` cdk.json: ``` { "assetParallelism": false } ``` Environment variables: ``` export CDK_ASSET_PARALLELISM=false ```
Once again, any change to anything anywhere broke someone. In this particular case, parallel asset publishing (#19367, and in particular building) broke a use case where someone uses a tool to build assets that is not concurrency-safe. So -- now we need to make that configurable. Command line: ``` cdk deploy --no-asset-parallelism ``` cdk.json: ``` { "assetParallelism": false } ``` Environment variables: ``` export CDK_ASSET_PARALLELISM=false ``` ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds parallel asset publishing. In a 60-asset/60-Lambda test project, this change decreases the total hot-swap time from 57 seconds to 18 seconds.
Fixes #19193
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license