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: miss mount point mode when volume from other container #2576

Merged
merged 1 commit into from
Dec 20, 2018

Conversation

rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented Dec 19, 2018

Ⅰ. Describe what this PR did

When the mountpoints of container are come from other container,
use volumes-from to set mountpoints, if the old mountpoint has set
volume mode, new container must keep the volume mode.

Ⅱ. Does this pull request fix one issue?

NONE

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

add test TestRunVolumesFromWithDR

Ⅳ. Describe how to verify it

run test

cd test
go test -v -check.f TestRunVolumesFromWithDR

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang rudyflyzhang@gmail.com

@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@3470d89). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2576   +/-   ##
=========================================
  Coverage          ?   69.23%           
=========================================
  Files             ?      278           
  Lines             ?    18689           
  Branches          ?        0           
=========================================
  Hits              ?    12939           
  Misses            ?     4276           
  Partials          ?     1474
Flag Coverage Δ
#criv1alpha1test 31.26% <50%> (?)
#criv1alpha2test 35.79% <50%> (?)
#integrationtest 40.73% <100%> (?)
#nodee2etest 32.8% <50%> (?)
#unittest 26.66% <50%> (?)
Impacted Files Coverage Δ
apis/opts/mountpoint.go 100% <100%> (ø)
daemon/mgr/container_storage.go 61.12% <100%> (ø)

@chuanchang
Copy link
Contributor

chuanchang commented Dec 19, 2018

@rudyfly
CI issues:

FILE: ./docs/features/pouch_with_dragonfly.md
[✖] https://github.com/dragonflyoss/Dragonfly/blob/master/docs/quick_start/_index.md

ERROR: dead links found!

BTW, #2580

// run bak container
command.PouchRun("run", "-d", "--name", cNameBak,
"-v", vName+":/var:dr",
"-v", vName+":/data", busyboxImage, "top").Assert(c, icmd.Success)
Copy link
Contributor

Choose a reason for hiding this comment

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

defer command.PouchRun("rm", "-vf", cNameBak)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK,you are right, but my test steps are run bak container -> stop bak container -> run test container -> remove bak container, I want to check whether the step remove bak container is successful.
Let me see how to write the steps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sunyuan3 I have modified the test case, PTAL. 🍺

When the mountpoints of container are come from other container,
use `volumes-from` to set mountpoints, if the old mountpoint has set
volume mode, new container must keep the volume mode.

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
@HusterWan
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 20, 2018
@sunyuan3
Copy link
Contributor

LGTM

@HusterWan HusterWan merged commit 7b08848 into AliyunContainerService:master Dec 20, 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/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants