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

bugfix: change the order of generating MountPoints #1541

Merged
merged 1 commit into from
Jun 19, 2018

Conversation

shaloulcy
Copy link
Contributor

@shaloulcy shaloulcy commented Jun 15, 2018

Signed-off-by: Eric Li lcy041536@gmail.com

Ⅰ. Describe what this PR did

change the order of generating mountpoints.

  1. generate MountPoints from other containers(volumes-from)
  2. generate MountPoints from binds
  3. generate MountPoints from images
  4. generate MountPoints from Config.Volumes

Why?

Every volume has a destination in container. So if two volume have the same destination, only one volume takes effect.

Now the order of volume taking effect is

  1. generate MountPoints from Config.Volumes
  2. generate MountPoints from images
  3. generate MountPoints from binds
  4. generate MountPoints from other containers(volume-from)

So if Config.Volumes, images, binds and volume-from all have a volume whose destination is the same. Only the Config.Volumes will take effect.

Indeed, the right order is

volumes-from 》 binds 》images 》Config.Volumes

I will add some test-cases in other Prs

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

change the order of generating MountPoints

volumes-from 》 binds 》images 》Config.Volumes

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Signed-off-by: Eric Li <lcy041536@gmail.com>
@pouchrobot pouchrobot added kind/bug This is bug report for project size/S labels Jun 15, 2018
@codecov-io
Copy link

Codecov Report

Merging #1541 into master will increase coverage by 23.67%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #1541       +/-   ##
==========================================
+ Coverage   16.83%   40.5%   +23.67%     
==========================================
  Files         211     254       +43     
  Lines       14062   16750     +2688     
==========================================
+ Hits         2367    6785     +4418     
+ Misses      11533    9099     -2434     
- Partials      162     866      +704
Impacted Files Coverage Δ
daemon/mgr/container.go 49.21% <50%> (+49.21%) ⬆️
cri/v1alpha1/cri.go 0% <0%> (ø) ⬆️
cri/v1alpha2/cri.go 0% <0%> (ø) ⬆️
apis/metrics/metrics.go 100% <0%> (ø)
storage/volume/error/errors.go 28.57% <0%> (ø)
cri/stream/errors.go 0% <0%> (ø)
cri/stream/remotecommand/httpstream.go 0% <0%> (ø)
cri/v1alpha2/service/cri.go 0% <0%> (ø)
storage/quota/set_diskquota.go 9.09% <0%> (ø)
cri/stream/remotecommand/attach.go 0% <0%> (ø)
... and 116 more

@allencloud
Copy link
Collaborator

Could you tell us more about the reason why you change the sequence? @shaloulcy

@rudyfly
Copy link
Collaborator

rudyfly commented Jun 19, 2018

LGTM

@shaloulcy shaloulcy changed the title bugfix: change the sequence of generating MountPoints bugfix: change the order of generating MountPoints Jun 19, 2018
@shaloulcy
Copy link
Contributor Author

@allencloud I have explain the reason in PR Description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants