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

Deduplicate between Volumes and Mounts in compat API #13540

Merged
merged 3 commits into from
Mar 18, 2022

Conversation

mheon
Copy link
Member

@mheon mheon commented Mar 17, 2022

Docker Compose v2.0 passes mount specifications in two different places: Volumes (just the destination) and Mounts (full info provided - source, destination, etc). This was causing Podman to refuse to create containers, as the destination was used twice. Deduplicate between Mounts and Volumes, preferring volumes, to resolve this.

Fixes #11822

Still to-do: Get Compose v2 in our test VMs and add tests

Docker Compose v2.0 passes mount specifications in two different
places: Volumes (just the destination) and Mounts (full info
provided - source, destination, etc). This was causing Podman to
refuse to create containers, as the destination was used twice.
Deduplicate between Mounts and Volumes, preferring volumes, to
resolve this.

Fixes containers#11822

Signed-off-by: Matthew Heon <mheon@redhat.com>
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2022
Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add a pair of new Cirrus test suites using Compose v2 instead of
Compose v1 (as is currently packaged in Fedora). They work
identically, and run the same tests, as the Compose v1 tests, but
with the new v2 binary instead.

[NO NEW TESTS NEEDED] This adds an entire Cirrus suite...

Signed-off-by: Matthew Heon <mheon@redhat.com>
@mheon
Copy link
Member Author

mheon commented Mar 17, 2022

Tests added

Compose v2 uses dashes as separators instead of hyphens. This
broke some tests that relied upon container names. Set the name
conditionally to make it safe for both.

Signed-off-by: Matthew Heon <mheon@redhat.com>
@mheon
Copy link
Member Author

mheon commented Mar 17, 2022

Tests going green

@mheon
Copy link
Member Author

mheon commented Mar 17, 2022

@containers/podman-maintainers PTAL

@@ -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 :)

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM
@mheon Do you know in which version the buildkit requirement was dropped? It would help to clarify this in the release notes when we say we support composev2

Comment on lines +3 to +6
ctr_name="two_networks_con1_1"
if [ "$TEST_FLAVOR" = "compose_v2" ]; then
ctr_name="two_networks-con1-1"
fi
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.

@rhatdan
Copy link
Member

rhatdan commented Mar 18, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2022
@openshift-merge-robot openshift-merge-robot merged commit 3853ef9 into containers:main Mar 18, 2022
@mheon
Copy link
Member Author

mheon commented Mar 18, 2022

@Luap99 I do not, but I can look into it further

@Luap99
Copy link
Member

Luap99 commented Mar 18, 2022

I think it was added in v2.1 https://github.com/docker/compose/releases/tag/v2.1.0

Add support for classic builder by @ulyssessouza in docker/compose#8818

@Hurricos
Copy link

Is there any appetite to backport this fix to v4.1? If not, since I'm not as familiar with the release cycle of podman as I'd like to be, when would the next fork off of main into a production branch actually be?

@mheon
Copy link
Member Author

mheon commented Apr 28, 2022

This is in 4.1, which will be released some time next week. It will not be backported to the current 4.0 release.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
8 participants