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.

Sign up for GitHub

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

Deduplicate between Volumes and Mounts in compat API #13540

Merged
merged 3 commits into from
Mar 18, 2022
+40 −7
Merged

Deduplicate between Volumes and Mounts in compat API #13540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0793a58
Deduplicate between Volumes and Mounts in compat API
mheon Mar 17, 2022
bde3ca8
Add tests with Docker Compose v2
mheon Mar 17, 2022
4b359e4
Set names in compose tests based on version
mheon Mar 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions .cirrus.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ apiv2_test_task:


compose_test_task:
name: "compose test on $DISTRO_NV ($PRIV_NAME)"
name: "$TEST_FLAVOR test on $DISTRO_NV ($PRIV_NAME)"
alias: compose_test
only_if: *not_build
skip: *tags
Expand All @@ -438,11 +438,18 @@ compose_test_task:
gce_instance: *standardvm
env:
<<: *stdenvars
TEST_FLAVOR: compose
matrix:
- env:
TEST_FLAVOR: compose
PRIV_NAME: root
- env:
TEST_FLAVOR: compose
PRIV_NAME: rootless
- env:
TEST_FLAVOR: compose_v2
PRIV_NAME: root
- env:
TEST_FLAVOR: compose_v2
PRIV_NAME: rootless
clone_script: *noop # Comes from cache
gopath_cache: *ro_gopath_cache
Expand Down
13 changes: 11 additions & 2 deletions cmd/podman/common/create_opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,21 @@ func ContainerCreateToContainerCLIOpts(cc handlers.CreateContainerConfig, rtc *c
}

// mounts type=tmpfs/bind,source=...,target=...=,opt=val
volSources := make(map[string]bool)
volDestinations := make(map[string]bool)
mounts := make([]string, 0, len(cc.HostConfig.Mounts))
var builder strings.Builder
for _, m := range cc.HostConfig.Mounts {
addField(&builder, "type", string(m.Type))
addField(&builder, "source", m.Source)
addField(&builder, "target", m.Target)

// Store source/dest so we don't add duplicates if a volume is
// also mentioned in cc.Volumes.
// Which Docker Compose v2.0 does, for unclear reasons...
volSources[m.Source] = true
volDestinations[m.Target] = true

if m.ReadOnly {
addField(&builder, "ro", "true")
}
Expand Down Expand Up @@ -328,8 +337,6 @@ func ContainerCreateToContainerCLIOpts(cc handlers.CreateContainerConfig, rtc *c
}

// volumes
volSources := make(map[string]bool)
volDestinations := make(map[string]bool)
for _, vol := range cc.HostConfig.Binds {
cliOpts.Volume = append(cliOpts.Volume, vol)
// Extract the destination so we don't add duplicate mounts in
Expand All @@ -348,6 +355,8 @@ func ContainerCreateToContainerCLIOpts(cc handlers.CreateContainerConfig, rtc *c
// format of `-v` so we can just append them in there.
// Unfortunately, these may be duplicates of existing mounts in Binds.
// So... We need to catch that.
// This also handles volumes duplicated between cc.HostConfig.Mounts and
// cc.Volumes, as seen in compose v2.0.
for vol := range cc.Volumes {
if _, ok := volDestinations[filepath.Clean(vol)]; ok {
continue
Expand Down
4 changes: 4 additions & 0 deletions contrib/cirrus/runner.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ function _run_compose() {
./test/compose/test-compose |& logformatter
}

function _run_compose_v2() {
./test/compose/test-compose |& logformatter
}

function _run_int() {
dotest integration
}
Expand Down
5 changes: 5 additions & 0 deletions contrib/cirrus/setup_environment.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,11 @@ case "$TEST_FLAVOR" in
;;
build) make clean ;;
unit) ;;
compose_v2)
dnf -y remove docker-compose
curl -SL https://github.com/docker/compose/releases/download/v2.2.3/docker-compose-linux-x86_64 -o /usr/local/bin/docker-compose
Copy link
Member

Choose a reason for hiding this comment

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

I'm always leery to have a specified version in tests rather than latest.
@edsantiago probably has a neater way, but you could get/set the version from GitHub into an envar and then use it:

# export VERSION=`curl -s https://github.com/docker/compose/releases/latest | grep "https://github.com/docker/compose/releases/tag/*"  | sed 's/^.*tag\///' | sed 's/\">.*//'`
#  echo $VERSION
v2.3.3
# curl -SL https://github.com/docker/compose/releases/download/${VERSION}/docker-compose-linux-x86_64 -o /usr/local/bin/docker-compose

Copy link
Member

Choose a reason for hiding this comment

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

The latest version can break at any time. Using a fixed version is a good idea IMO.
I think we should download the compose binary at image build time to prevent unnecessary flakes. We could put it at a fixed place and then just mv it here to /usr/bin

Copy link

Choose a reason for hiding this comment

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

Just FYI, if you do end up deciding to download the latest version using the URL https://github.com/docker/compose/releases/latest/download/docker-compose-linux-x86_64 is much easier (and more robust) than greping and seding through GitHub's HTML :)

chmod +x /usr/local/bin/docker-compose
;& # Continue with next item
apiv2)
msg "Installing previously downloaded/cached packages"
dnf install -y $PACKAGE_DOWNLOAD_DIR/python3*.rpm
Expand Down
6 changes: 5 additions & 1 deletion test/compose/ipam_set_ip/tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# -*- bash -*-

podman container inspect ipam_set_ip_test_1 --format '{{ .NetworkSettings.Networks.ipam_set_ip_net1.IPAddress }}'
ctr_name="ipam_set_ip_test_1"
if [ "$TEST_FLAVOR" = "compose_v2" ]; then
ctr_name="ipam_set_ip-test-1"
fi
podman container inspect "$ctr_name" --format '{{ .NetworkSettings.Networks.ipam_set_ip_net1.IPAddress }}'
like "$output" "10.123.0.253" "$testname : ip address is set"
8 changes: 6 additions & 2 deletions test/compose/two_networks/tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
# -*- bash -*-

podman container inspect two_networks_con1_1 --format '{{len .NetworkSettings.Networks}}'
ctr_name="two_networks_con1_1"
if [ "$TEST_FLAVOR" = "compose_v2" ]; then
ctr_name="two_networks-con1-1"
fi
Comment on lines +3 to +6
Copy link
Member

Choose a reason for hiding this comment

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

Well that is is interesting, they changed the container name to using a dash but below for the network names they still use underscores.

podman container inspect "$ctr_name" --format '{{len .NetworkSettings.Networks}}'
is "$output" "2" "$testname : Container is connected to both networks"
podman container inspect two_networks_con1_1 --format '{{.NetworkSettings.Networks}}'
podman container inspect "$ctr_name" --format '{{.NetworkSettings.Networks}}'
like "$output" "two_networks_net1" "$testname : First network name exists"
like "$output" "two_networks_net2" "$testname : Second network name exists"