-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(core): add volumes-from option to docker run command for bundling #22829
Conversation
its unclear how that would be reliably testable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
The failure is sort of expected:
This due to the fact, that the CodeBuild pipeline does not work like the CICD systems that this PR is targeting to enable to build. |
this is overall unfavourable as it makes the test very complex. its not reproducible locally so i need to push this to AWS ci
Co-authored-by: Momo Kornher <mail@moritzkornher.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you made it working, but I really don't see any point in the test. It literally just tests docker. Sorry about that!
To get this one merged asap, let's remove the integ test and revert customCommands
as well.
I'd also think we should create parity of bundling options for all three functions we have (node, go, python). So what I'm pretty much asking you is to revert all python changes here and than create second PR that adds unified support for that as well as some of the other Docker options. Hope that's okay.
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
this is all stripped down now to the minimum. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @webratz , I appreciate you working through this with me. If you're interested, you could look at extending/aligning the function bundling interfaces next.
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
Pull request has been modified.
@webratz I'll take it from here. No need to keep updating the branch (in fact makes everything slower). |
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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…all lambda variants (#23318) This continues the work started in #22829 by exposing the underlying docker run options of the container bundling. With that the bundling feature can be used in a wider range of setups that the current defaults can not support out of the box. The removed properties are covered in the same way by the one the interface extends from. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-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*
aws#22829) relates to aws#8799 follow up to stale aws#21660 ## Describe the feature Ability to add [--volumes-from](https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container---volumes-from) flag when bundling assets with docker. This enabled people using Docker in Docker to use CDKs bundling functionality, which is currently not possible. ## Use Case CICD systems often run within a docker container already. Many systems mount the ` /var/run/docker.sock` from the host system into the CICD container. When running bundling within such a container it currently breaks, as docker assume the path is from the host system, not within the CICD container. The options allows to mount the data from any other container. Very often it will be the current one which can be used by using the `HOSTNAME` environment variable ## Proposed Solution Add optional property to [DockerRunOptions](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.DockerRunOptions.html) and [BundlingOptions](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.BundlingOptions.html) that would translate into --volumes-from {user provided option} This change would not reflect in any CloudFormation changes, but only with the docker commands performed when bundling. Due to using the `--volumes-from` option, docker will instead of trying to find the path on the host (where it does not exist) try to use the volume that is created by the container C1 that is actually running the CDK. With that it is able to access the files from CDK and can continue the build. ![Docker volumes from](https://user-images.githubusercontent.com/2162832/193787498-de03c66c-7bce-458b-9776-7ba421b9d929.jpg) The following plain docker steps show how this works from the docker side, and why we need to adjust the `--volumes-from` parameter. ```sh docker volume create builds docker run -v /var/run/docker.sock:/var/run/docker.sock -v builds:/builds -it docker ``` Now within the just created docker container, run the following commands. ```sh echo "testfile" > /builds/my-share-file.txt docker run --rm --name DinDContainer --volumes-from="${HOSTNAME}" ubuntu bash -c "ls -hla /builds" ``` We see that the second container C2 (here `DinDContainer`) has the same files available as the container C1. ## Alternative solutions I'm not aware of alternative solutions for this docker in docker use cases, besides of not relying on docker at all, which is out of scope for this MR. ---- ### All Submissions: * [X] 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)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? I ran it, but it seems not to have generated something, i might need some guidance there. *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…all lambda variants (aws#23318) This continues the work started in aws#22829 by exposing the underlying docker run options of the container bundling. With that the bundling feature can be used in a wider range of setups that the current defaults can not support out of the box. The removed properties are covered in the same way by the one the interface extends from. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-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*
aws#22829) relates to aws#8799 follow up to stale aws#21660 ## Describe the feature Ability to add [--volumes-from](https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container---volumes-from) flag when bundling assets with docker. This enabled people using Docker in Docker to use CDKs bundling functionality, which is currently not possible. ## Use Case CICD systems often run within a docker container already. Many systems mount the ` /var/run/docker.sock` from the host system into the CICD container. When running bundling within such a container it currently breaks, as docker assume the path is from the host system, not within the CICD container. The options allows to mount the data from any other container. Very often it will be the current one which can be used by using the `HOSTNAME` environment variable ## Proposed Solution Add optional property to [DockerRunOptions](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.DockerRunOptions.html) and [BundlingOptions](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.BundlingOptions.html) that would translate into --volumes-from {user provided option} This change would not reflect in any CloudFormation changes, but only with the docker commands performed when bundling. Due to using the `--volumes-from` option, docker will instead of trying to find the path on the host (where it does not exist) try to use the volume that is created by the container C1 that is actually running the CDK. With that it is able to access the files from CDK and can continue the build. ![Docker volumes from](https://user-images.githubusercontent.com/2162832/193787498-de03c66c-7bce-458b-9776-7ba421b9d929.jpg) The following plain docker steps show how this works from the docker side, and why we need to adjust the `--volumes-from` parameter. ```sh docker volume create builds docker run -v /var/run/docker.sock:/var/run/docker.sock -v builds:/builds -it docker ``` Now within the just created docker container, run the following commands. ```sh echo "testfile" > /builds/my-share-file.txt docker run --rm --name DinDContainer --volumes-from="${HOSTNAME}" ubuntu bash -c "ls -hla /builds" ``` We see that the second container C2 (here `DinDContainer`) has the same files available as the container C1. ## Alternative solutions I'm not aware of alternative solutions for this docker in docker use cases, besides of not relying on docker at all, which is out of scope for this MR. ---- ### All Submissions: * [X] 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)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? I ran it, but it seems not to have generated something, i might need some guidance there. *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…all lambda variants (aws#23318) This continues the work started in aws#22829 by exposing the underlying docker run options of the container bundling. With that the bundling feature can be used in a wider range of setups that the current defaults can not support out of the box. The removed properties are covered in the same way by the one the interface extends from. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-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*
relates to #8799
follow up to stale #21660
Describe the feature
Ability to add --volumes-from flag when bundling assets with docker.
This enabled people using Docker in Docker to use CDKs bundling functionality, which is currently not possible.
Use Case
CICD systems often run within a docker container already. Many systems mount the
/var/run/docker.sock
from the host system into the CICD container. When running bundling within such a container it currently breaks, as docker assume the path is from the host system, not within the CICD container.The options allows to mount the data from any other container. Very often it will be the current one which can be used by using the
HOSTNAME
environment variableProposed Solution
Add optional property to DockerRunOptions and BundlingOptions that would translate into --volumes-from {user provided option}
This change would not reflect in any CloudFormation changes, but only with the docker commands performed when bundling.
Due to using the
--volumes-from
option, docker will instead of trying to find the path on the host (where it does not exist) try to use the volume that is created by the container C1 that is actually running the CDK. With that it is able to access the files from CDK and can continue the build.The following plain docker steps show how this works from the docker side, and why we need to adjust the
--volumes-from
parameter.Now within the just created docker container, run the following commands.
We see that the second container C2 (here
DinDContainer
) has the same files available as the container C1.Alternative solutions
I'm not aware of alternative solutions for this docker in docker use cases, besides of not relying on docker at all, which is out of scope for this MR.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?I ran it, but it seems not to have generated something, i might need some guidance there.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license