Skip to content

Conversation

@zjumoon01
Copy link
Contributor

CopyData is an action which should be enabled only in mount volumes and images.

For example:
[root@hostname]# pouch run -it --name test2 -v /home/baijia.wr/volume/:/var/ docker.io/library/busybox:latest ls /var/
spool www
[root@hostname]# ls /home/baijia.wr/volume/
spool www

Without this patch, data is copied form container's dir "/var/" to host's dir "/home/baijia.wr/volume/",
that is unexpected. So CopyData should be disabled in bind mount.

Signed-off-by: Wang Rui [email protected]

Ⅰ. Describe what this PR did

Data will be copied from container to host when bind mount is used, that is unexpected.
For example:

[root@hostname]# pouch run -it --name test2 -v /home/baijia.wr/volume/:/var/ docker.io/library/busybox:latest ls /var/
spool www
[root@hostname]# ls /home/baijia.wr/volume/
spool www

Data is copied form container's dir "/var/" to host's dir "/home/baijia.wr/volume/".
This patch will fix that.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

With this patch, DataCopy is disabled in bind mount.

[root@hostname]# pouch run -it --name test2 -v /home/baijia.wr/volume/:/var/ docker.io/library/busybox:latest ls /var/
[root@hostname]# ls /home/baijia.wr/volume/
[root@hostname]#

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Dec 3, 2018

Codecov Report

Merging #2527 into master will increase coverage by 11.98%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2527       +/-   ##
===========================================
+ Coverage   57.23%   69.22%   +11.98%     
===========================================
  Files         278      278               
  Lines       18440    18442        +2     
===========================================
+ Hits        10554    12766     +2212     
+ Misses       6640     4216     -2424     
- Partials     1246     1460      +214
Flag Coverage Δ
#criv1alpha1test 31.09% <100%> (-0.27%) ⬇️
#criv1alpha2test 35.55% <100%> (+0.02%) ⬆️
#integrationtest 40.47% <100%> (?)
#nodee2etest 32.79% <100%> (-0.02%) ⬇️
#unittest 26.94% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
daemon/mgr/container_storage.go 60.32% <100%> (+16.08%) ⬆️
cri/v1alpha1/cri.go 60.26% <0%> (-0.67%) ⬇️
cri/v1alpha2/cri_utils.go 90.04% <0%> (+0.28%) ⬆️
daemon/logger/jsonfile/utils.go 71.54% <0%> (+0.81%) ⬆️
ctrd/watch.go 81.69% <0%> (+1.4%) ⬆️
pkg/user/user.go 40% <0%> (+1.66%) ⬆️
daemon/mgr/spec_mount.go 84.25% <0%> (+1.85%) ⬆️
daemon/mgr/container_utils.go 85.44% <0%> (+1.89%) ⬆️
daemon/containerio/io.go 72.81% <0%> (+1.94%) ⬆️
pkg/utils/utils.go 83.02% <0%> (+2.29%) ⬆️
... and 78 more

@allencloud
Copy link
Collaborator

allencloud commented Dec 3, 2018

Since we can reproduce the issue,
Could we add a test case to cover the bug? @zjumoon01 @rudyfly

@rudyfly
Copy link
Collaborator

rudyfly commented Dec 4, 2018

BTW, can you add a test case to check the bind mount has copyed data or not.

@allencloud allencloud changed the title Fix CopyData in Bind Mount bugfix: Fix CopyData in Bind Mount Dec 4, 2018
@pouchrobot pouchrobot added the kind/bug This is bug report for project label Dec 4, 2018
@pouchrobot pouchrobot added size/S and removed size/XS labels Dec 4, 2018
@zjumoon01
Copy link
Contributor Author

BTW, can you add a test case to check the bind mount has copyed data or not.

OK. Test case is added in the latest patch

@zjumoon01
Copy link
Contributor Author

Since we can reproduce the issue,
Could we add a test case to cover the bug? @zjumoon01 @rudyfly

OK. Test case is added in the latest patch

CopyData is an action which should be enabled only in mount volumes and images.

For example:
 [root@hostname]# pouch run  -it --name test2 -v /home/baijia.wr/volume/:/var/ docker.io/library/busybox:latest ls /var/
 spool  www
 [root@hostname]# ls /home/baijia.wr/volume/
 spool  www

Without this patch, data is copied form container's dir "/var/" to host's dir "/home/baijia.wr/volume/",
that is unexpected. So CopyData should be disabled in bind mount.

Signed-off-by: Wang Rui <[email protected]>
@rudyfly
Copy link
Collaborator

rudyfly commented Dec 5, 2018

LGTM
And waiting for ci pass

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 5, 2018
@Ace-Tang Ace-Tang merged commit be7e002 into AliyunContainerService:master Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

areas/storage kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants