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

Use cleaned destination path for indexing image volumes #5222

Merged
merged 1 commit into from
Feb 20, 2020

Conversation

mheon
Copy link
Member

@mheon mheon commented Feb 16, 2020

We use filepath.Clean() to remove trailing slashes to ensure that when we supercede image mounts with mounts from --volume and --mount, paths are consistent when we compare. Unfortunately, while we used the cleaned path for the destination in the mount, it was accidentally not used to index the maps that we use to identify what to supercede, so our comparisons might be thrown off by trailing slashes and similar.

Fixes #5219

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS labels Feb 16, 2020
@mheon
Copy link
Member Author

mheon commented Feb 16, 2020

I'll work on tests tomorrow, we'll need a new image to actually validate the fix. I think we'll have to build it.

@rhatdan
Copy link
Member

rhatdan commented Feb 17, 2020

@rhatdan
Copy link
Member

rhatdan commented Feb 17, 2020

@hikhvar PTAL

@mheon
Copy link
Member Author

mheon commented Feb 17, 2020

Added a test

We use filepath.Clean() to remove trailing slashes to ensure that
when we supercede image mounts with mounts from --volume and
--mount, paths are consistent when we compare. Unfortunately,
while we used the cleaned path for the destination in the mount,
it was accidentally not used to index the maps that we use to
identify what to supercede, so our comparisons might be thrown
off by trailing slashes and similar.

Fixes containers#5219

Signed-off-by: Matthew Heon <[email protected]>
@hikhvar
Copy link
Contributor

hikhvar commented Feb 20, 2020

It works for me:

bin/podman run --user 9987:9987  \
  --mount type=bind,src=/mnt/HC_Volume_4373159/teamspeak/data,target=/var/ts3server\
   -v /etc/teamspeak/:/var/run/ts3server/  \
   -p 9987:9987/udp \
   -e TS3SERVER_LICENSE=accept \
   --name teamspeak  \
   --network podstack  \
   --ip 10.90.0.5 \
   teamspeak:3.11.0
bin/podman inspect teampeak
[....]
        "Mounts": [
            {
                "Type": "bind",
                "Name": "",
                "Source": "/mnt/HC_Volume_4373159/teamspeak/data",
                "Destination": "/var/ts3server",
                "Driver": "",
                "Mode": "",
                "Options": [
                    "rbind"
                ],
                "RW": true,
                "Propagation": "rprivate"
            },
            {
                "Type": "bind",
                "Name": "",
                "Source": "/etc/teamspeak",
                "Destination": "/var/run/ts3server",
                "Driver": "",
                "Mode": "",
                "Options": [
                    "rbind"
                ],
                "RW": true,
                "Propagation": "rprivate"
            }
        ],
[....]
 bin/podman version
Version:            1.8.1-dev
RemoteAPI Version:  1
Go Version:         go1.10.4
Git Commit:         40fa7e99317a32046fec2442b61dc524f63e52cd
Built:              Thu Feb 20 21:17:37 2020
OS/Arch:            linux/amd64

@mheon
Copy link
Member Author

mheon commented Feb 20, 2020

Restarted tests, let's get this landed

@baude
Copy link
Member

baude commented Feb 20, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2020
@openshift-merge-robot openshift-merge-robot merged commit bfeaabb into containers:master Feb 20, 2020
@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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 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
Development

Successfully merging this pull request may close these issues.

VOLUME from Dockerfile is not overwritten
6 participants