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

Docker in docker #387

Merged
merged 1 commit into from
Apr 13, 2020
Merged

Docker in docker #387

merged 1 commit into from
Apr 13, 2020

Conversation

urso
Copy link
Contributor

@urso urso commented Mar 8, 2020

When using docker-in-docker, for example in dev or CI containers, one
normally gives access to the host docker environment to the parent container.
If the container spins up another child container, and wishes
to share a directory, one needs to compute the correct paths from
view of the host system.

How it Works

If the environment variable CROSS_DOCKER_IN_DOCKER=true, we call
docker inspect $HOSTNAME, to learn about details of the current container.
The Mounts field contains information about all mounts from the host
into the current container. The GraphDriver field gives us information
about the storage driver and the location of the contains root directory on the host system. Based on these information we compute
a table of all container mounts (source->destination).

When starting a child container we adapt all paths to be mounted, that have
a prefix in the table, to start with source. For example when mounting a
dev containers /usr/local/cargo directory, we will actually mount
/var/lib/docker/overlay2/<parent container id>/merged/usr/local/cargo.

If the project itself is mounted into the parent container, we will not
use the overlay directory, but find the project path on the host system.

Limitations

Finding the mount point for the containers root directory is only
implemented for the overlayfs2 storage driver.

In order to access the parent containers rust setup, the child container
mounts the parents overlayfs. The parent must not be stopped before the
child container, as the overlayfs can not be unmounted correctly by
docker if the child container still accesses it.

@urso urso requested review from Dylan-DPC-zz and a team as code owners March 8, 2020 18:20
@urso urso mentioned this pull request Mar 8, 2020
7 tasks
src/cli.rs Outdated Show resolved Hide resolved
@reitermarkus
Copy link
Member

Please also add some tests for the parse functions.

@urso
Copy link
Contributor Author

urso commented Mar 20, 2020

Please also add some tests for the parse functions.

I didn't really find any unit tests. It seams that all tests are driven by ci/test.sh. Plus I'm not really familiar with your CI setup. Any recommendations on testing?

@reitermarkus
Copy link
Member

Add them as you would in any other Rust project, so they run when you run cargo test. Then we can add a cargo test before the cargo install in ci/test.sh.

@urso
Copy link
Contributor Author

urso commented Mar 21, 2020

Added some tests + added cargo test the azure pipeline instead of test.sh.

I don't think there is need to run the unit tests with each target in the pipeline. Would it make sense to enable/use stages to the azure pipeline (needs to be explicitely enabled, as its a feature in preview)?

src/cli.rs Outdated
}
}

fn parse_bool_env(val: Result<String, env::VarError>) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be a function, given that it is only used once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Given that there is no dedicated CI test for docker-in-docker, I at least wanted to test that the environment variable is parsed correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reitermarkus Would you prefer me to remove the test again, or keep it as is?

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it. It's mainly testing FromStr for bool, which should be tested plenty already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@TotalKrill
Copy link

Is there any progress on this? I need it for our CI setup

@urso
Copy link
Contributor Author

urso commented Apr 1, 2020

@TotalKrill I currently use cross like this in my development container: https://gist.github.com/urso/033a80a10a9b9656548134adeb373e8d#file-dockerfile-L46

README.md Outdated Show resolved Hide resolved
src/cli.rs Outdated
}
}

fn parse_bool_env(val: Result<String, env::VarError>) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it. It's mainly testing FromStr for bool, which should be tested plenty already.

@urso urso requested a review from reitermarkus April 10, 2020 12:23
@reitermarkus
Copy link
Member

Thanks, @urso! Could you please squash your last two commits?

When using docker-in-docker, for example in dev or CI containers, one
normally gives access to the host docker environment to the docker
container. If the container spins up another child container, and wishes
to share a directory, one needs to compute the pass the right paths from
view of the host system.

If the environment variable CROSS_DOCKER_IN_DOCKER=true, we call `docker
inspect $HOSTNAME`, to learn about details of the current container.
The `Mounts` field contains information about all mounts from the host
into the current container. The `GraphDriver` field gives us information
about the storage driver and the location in the host system of the
current containers root directory. Based on these information we compute
a table of all container mounts (`source->destination`).

When starting a child container we adapt all paths to be mounted having
a prefix in the table, to start with source. For example when mounting a
dev containers `/usr/local/cargo` directory, we will actually mount
`/var/lib/docker/overlay2/<parent container id>/merged/usr/local/cargo`.

If the project itself is mounted into the parent container, we will not
use the overlay directory, but find the project path on the host.

In order to access the parent containers rust setup, the child container
mounts the parents overlayfs. The parent must not be stopped before the
child container, as the overlayfs can not be unmounted correctly by
docker if the child container still accesses it.
@urso
Copy link
Contributor Author

urso commented Apr 11, 2020

Commits have been squashed.

@reitermarkus
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 13, 2020

Build succeeded

  • rust-embedded.cross

@bors bors bot merged commit 8e8615b into cross-rs:master Apr 13, 2020
@reitermarkus
Copy link
Member

Thanks!

for details in v {
let source = make_path(&details["Source"]);
let destination = make_path(&details["Destination"]);
if source != destination {
Copy link
Contributor

Choose a reason for hiding this comment

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

@urso I am not sure to understand why this check is needed?
Actually, I have tried to run latest cross from github with docker-in-docker and if I mount my rust project as -v $PWD:$PWD -w $PWD, source == destination so it is not mounted in the second stage docker and /project is empty.
I might open an issue to discuss it further if you want.
Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a 'translation' table replacing the prefix that matches 'destination' with the prefix that matches 'source'. If source == destination, then this will be a noop.

If you open a shell in your development/CI container you can check the environment via $ echo $CROSS_DOCKER_IN_DOCKER; docker inspect $HOSTNAME. Running cross <command> -v will also print the docker commands as they will be executed. Very helpful for debugging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I did some debugging yesterday and by removing this check in my fork allowed me to use cross in a docker ran by Jenkins.

The issue here is that Jenkins mount the current workspace with the same path in Docker with

-v /path/to/my/workspace:/path/to/my/workspace

So source == destination but it is not a noop, it allows to mount files from host to the second docker stage.

I have managed to reproduce with:

# On host, create a file in current directory
touch testfile
# start a first docker stage
docker run -it --rm -v /var/run/docker.sock:/var/run/docker.sock -v $PWD:$PWD -w $PWD debian:buster bash
# in the first docker stage, install docker
apt update && apt install -y --no-install-recommends docker.io
# check that the test file is there
ls
# it is here, OK
# get the merged path
merge_path=$(docker inspect `hostname` --format '{{.GraphDriver.Data.MergedDir}}')
# start the second docker stage
docker run -it --rm -v $merge_path/$PWD:/project debian:buster bash
# in the second docker stage, check that the test file is here
ls /project
# there is no file, NOK
# exit the second docker
exit
# new start with classic mount
docker run -it --rm -v $PWD:/project debian:buster bash
ls /project
# the testfile is here, OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. If source == destination for mounted dirs, then the replacement will swap out the root path / with the containers merge path. Thanks to default mounting rules the /project will be empty (mounts are not passed through by default). This is why we need the path translation rules in the first place.

You are right, we should remove the if condition and always add the mount infos. I'm no maintainer, nor regular contributor to cross, but would be nice if you can create a PR removing this check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for having a look to it. I will create a PR.

bors bot added a commit that referenced this pull request May 3, 2020
409: Allow to mount host path in docker-in-docker mode r=reitermarkus a=romainreignier

As discussed here: #387 (comment)
The check is not needed to allow to mount paths that are mounted in the first docker with the same path.

Co-authored-by: Romain Reignier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants