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

volume: fix WCOW volume mounts #10090

Merged
merged 1 commit into from Dec 16, 2022
Merged

volume: fix WCOW volume mounts #10090

merged 1 commit into from Dec 16, 2022

Conversation

milas
Copy link
Member

@milas milas commented Dec 16, 2022

What I did
Do not use the older Volumes field in the API; instead rely on the more robust Mounts. For Linux containers, it seems that it's fine to set both of these. For Windows containers (WCOW), however, there appears to be a Moby bug that causes it to normalize the anonymous (Volumes) variant to lowercase, which can result in duplicative volume definitions and an error when trying to start the container.

Related issue
Fixes #9577.

(not mandatory) A picture of a cute animal, if possible in relation to what you did
two cats that look a lot alike

@milas milas requested a review from a team December 16, 2022 14:42
@milas milas self-assigned this Dec 16, 2022
@milas milas requested review from nicksieger, ndeloof, StefanScherer, ulyssessouza, glours and laurazard and removed request for a team December 16, 2022 14:42
@codecov
Copy link

codecov bot commented Dec 16, 2022

Codecov Report

Base: 75.79% // Head: 76.98% // Increases project coverage by +1.19% 🎉

Coverage data is based on head (ffb9544) compared to base (0eaa249).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10090      +/-   ##
==========================================
+ Coverage   75.79%   76.98%   +1.19%     
==========================================
  Files           2        2              
  Lines         252      252              
==========================================
+ Hits          191      194       +3     
+ Misses         53       51       -2     
+ Partials        8        7       -1     
Impacted Files Coverage Δ
pkg/e2e/framework.go 75.31% <0.00%> (+1.27%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Do not use the older `Volumes` field in the API; instead rely on
the more robust `Mounts`. For Linux containers, it seems that it's
fine to set both of these. For Windows containers (WCOW), however,
there appears to be a Moby bug that causes it to normalize the
anonymous (`Volumes`) variant to lowercase, which can result in
duplicative volume definitions and an error when trying to start
the container.

Fixes docker#9577.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@thaJeztah
Copy link
Member

/cc @neersighted - wondering if this is related to what you mentioned the other day (although in your case it didn't fail).

(Wondering if we need to fix normalising (or duplicate detection at least) for Windows daemons)

@neersighted
Copy link
Member

I'm wondering if you're talking about my issue with Desktop finding the wrong daemon/uninstalling the WCOW daemon during upgrade, or my issue with drvfs mounts disappearing using the WSL2 backend? It doesn't look like any of them to be, but maybe I'm thinking of the wrong thing?

@thaJeztah
Copy link
Member

@neersighted oh! I misunderstood; thought you had issues with volumes 😂 ☺️

@milas milas merged commit c37182b into docker:v2 Dec 16, 2022
@milas milas deleted the fix-wcow-volume branch December 16, 2022 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"docker compose up" does not start if volumes are used
4 participants