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: add attach volume when container start #1483

Merged
merged 1 commit into from
Jun 19, 2018

Conversation

rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented Jun 7, 2018

Ⅰ. Describe what this PR did

Add attach volume when container start. Now volume attaching is
at container create stage, but if host have been restarted, container won't be
created, so volume can't be attached.

Add Config() for volume driver, it makes volume driver can use the
configure of volume in daemon. Now set volume-meta-dir and
volume-timeout into driver config.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

A running container can start again after host have been restarted.

Ⅴ. Special notes for reviews

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

@codecov-io
Copy link

codecov-io commented Jun 7, 2018

Codecov Report

Merging #1483 into master will increase coverage by 0.06%.
The diff coverage is 71.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1483      +/-   ##
==========================================
+ Coverage   41.25%   41.32%   +0.06%     
==========================================
  Files         255      255              
  Lines       16837    16874      +37     
==========================================
+ Hits         6946     6973      +27     
- Misses       9024     9031       +7     
- Partials      867      870       +3
Impacted Files Coverage Δ
daemon/mgr/volume.go 79.16% <100%> (-1.25%) ⬇️
storage/volume/modules/local/local.go 74.24% <100%> (+3.55%) ⬆️
daemon/mgr/container.go 49.26% <57.69%> (+0.21%) ⬆️
storage/volume/core.go 52.91% <77.77%> (+1.24%) ⬆️
daemon/daemon.go 54.37% <0%> (-1.88%) ⬇️
daemon/mgr/network.go 69.43% <0%> (+0.48%) ⬆️
ctrd/watch.go 78.12% <0%> (+3.12%) ⬆️

@rudyfly rudyfly requested a review from shaloulcy June 8, 2018 01:59
@@ -26,6 +26,9 @@ type Driver interface {
type Opt interface {
// Options return module customize volume options.
Options() map[string]types.Option

// Config is used to pass the daemon volume configure into driver.
Config(Context, map[string]interface{}) error
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the Driver interface should implement Config method, not the Opt interface. WDYT? @rudyfly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have refacted it, PTAL 😄

continue
}

attachedVolumes := map[string]struct{}{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it necessary to add a defer in a loop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, it should defer outside the loop

@rudyfly rudyfly force-pushed the volume branch 2 times, most recently from 85a41b9 to 05c8472 Compare June 15, 2018 02:41
Add attach volume  when container start. Now volume attaching is
at container create stage, but if host have been restarted, container won't be
created, so volume can't be attached.

Add Config() for volume driver, it makes volume driver can use the
configure of volume in daemon. Now set `volume-meta-dir` and
`volume-timeout` into driver config.

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
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 size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants