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: fix volume can be removed when using by container #888

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented Mar 13, 2018

Ⅰ. Describe what this PR did

fix volume can be removed when using by container.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

mark container's id into volume option "ref" when container attach volume, and remove option "ref" when container deatch volume.

Ⅳ. Describe how to verify it

create volume and run container.

# pouch volume create -d local -o mount=/data/volume -o size=1g -n test
CreatedAt:    2018-3-13 19:54:58
Driver:       local
Labels:       map[]
Mountpoint:   /data/volume/test
Name:         test
Scope:
Status:       map[mount:/data/volume sifter:Default size:1g]
# pouch run -d -v test:/mnt registry.docker-cn.com/library/busybox:latest top
a72c17c38b865d325dbf2372ff5b4e44eb618e8301d2e68fb86131612b2b348b

check volume option ref

# pouch volume inspect test
{
    "CreatedAt": "2018-3-13 19:54:58",
    "Driver": "local",
    "Labels": {
        "backend": "local",
        "hostname": "ubuntu"
    },
    "Mountpoint": "/data/volume/test",
    "Name": "test",
    "Status": {
        "mount": "/data/volume",
        "ref": "a72c17c38b865d325dbf2372ff5b4e44eb618e8301d2e68fb86131612b2b348b",
        "sifter": "Default",
        "size": "1g"
    }
}

remove container and then check volume option ref.

# pouch rm -f a72c17c38b865d325dbf2372ff5b4e44eb618e8301d2e68fb86131612b2b348b
a72c17c38b865d325dbf2372ff5b4e44eb618e8301d2e68fb86131612b2b348b
# pouch volume inspect test
{
    "CreatedAt": "2018-3-13 19:54:58",
    "Driver": "local",
    "Labels": {
        "backend": "local",
        "hostname": "ubuntu"
    },
    "Mountpoint": "/data/volume/test",
    "Name": "test",
    "Status": {
        "freeTime": "1520942113",
        "mount": "/data/volume",
        "sifter": "Default",
        "size": "1g"
    }
}
# pouch volume rm test
Removed: test

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang [email protected]

@pouchrobot pouchrobot added areas/storage kind/bug This is bug report for project size/M labels Mar 13, 2018
@@ -192,6 +192,10 @@ func (mgr *ContainerManager) Remove(ctx context.Context, name string, option *Co
}
}

if err := mgr.deatchVolumes(ctx, c.meta); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/deatch/detach?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I have made a mistake.

@@ -1028,6 +1048,42 @@ func (mgr *ContainerManager) parseVolumes(ctx context.Context, c *types.Containe
return nil
}

func (mgr *ContainerManager) deatchVolumes(ctx context.Context, c *ContainerMeta) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

detach?

@allencloud
Copy link
Collaborator

CI fails, and it is not a flaky test, since in travis CI we have recorded the following log:

net/http.(*conn).serve.func1(0xc422066280)
	/usr/local/go/src/net/http/server.go:1697 +0xd0
panic(0x19209a0, 0x24166b0)
	/usr/local/go/src/runtime/panic.go:491 +0x283
github.com/alibaba/pouch/volume/types.(*Volume).Option(...)
	/tmp/pouchbuild/src/github.com/alibaba/pouch/volume/types/volume.go:131
github.com/alibaba/pouch/daemon/mgr.(*ContainerManager).parseVolumes(0xc4201360e0, 0x2445e20, 0xc4220921c0, 0xc421f77e00, 0x40, 0xc42205bce0, 0xdfcba9, 0xc422045fae)
	/tmp/pouchbuild/src/github.com/alibaba/pouch/daemon/mgr/container.go:1008 +0x756
github.com/alibaba/pouch/daemon/mgr.(*ContainerManager).Create(0xc4201360e0, 0x2445e20, 0xc4220921c0, 0xc422045fb3, 0x15, 0xc42205bce0, 0x0, 0x0, 0x0)
	/tmp/pouchbuild/src/github.com/alibaba/pouch/daemon/mgr/container.go:327 +0x144
github.com/alibaba/pouch/apis/server.(*Server).createContainer(0xc42021eee8, 0x2445e20, 0xc4220921c0, 0x2444c60, 0xc421f1cc40, 0xc421fd3b00, 0x3, 0x3)
	/tmp/pouchbuild/src/github.com/alibaba/pouch/apis/server/container_bridge.go:131 +0x2da
github.com/alibaba/pouch/apis/server.(*Server).(github.com/alibaba/pouch/apis/server.createContainer)-fm(0x2445e20, 0xc4220921c0, 0x2444c60, 0xc421f1cc40, 0xc421fd3b00, 0xc421fd3b00, 0xc420c40ad0)
	/tmp/pouchbuild/src/github.com/alibaba/pouch/apis/server/router.go:32 +0x5c
github.com/alibaba/pouch/apis/server.(*Server).filter.func1(0x2444c60, 0xc421f1cc40, 0xc421fd3b00)
	/tmp/pouchbuild/src/github.com/alibaba/pouch/apis/server/router.go:120 +0x2ae
net/http.HandlerFunc.ServeHTTP(0xc4202c24e0, 0x2444c60, 0xc421f1cc40, 0xc421fd3b00)
	/usr/local/go/src/net/http/server.go:1918 +0x44
github.com/alibaba/pouch/vendor/github.com/gorilla/mux.(*Router).ServeHTTP(0xc420107200, 0x2444c60, 0xc421f1cc40, 0xc421fd3b00)
	/tmp/pouchbuild/src/github.com/alibaba/pouch/vendor/github.com/gorilla/mux/mux.go:133 +0xed
net/http.serverHandler.ServeHTTP(0xc4208be820, 0x2444c60, 0xc421f1cc40, 0xc422019400)
	/usr/local/go/src/net/http/server.go:2619 +0xb4
net/http.(*conn).serve(0xc422066280, 0x2445e20, 0xc422057880)
	/usr/local/go/src/net/http/server.go:1801 +0x71d
created by net/http.(*Server).Serve
	/usr/local/go/src/net/http/server.go:2720 +0x288

@rudyfly

@codecov-io
Copy link

codecov-io commented Mar 14, 2018

Codecov Report

Merging #888 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #888      +/-   ##
==========================================
- Coverage   13.66%   13.59%   -0.07%     
==========================================
  Files         121      121              
  Lines        7667     7706      +39     
==========================================
  Hits         1048     1048              
- Misses       6529     6568      +39     
  Partials       90       90
Impacted Files Coverage Δ
daemon/mgr/container.go 2.41% <0%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa50191...fef9040. Read the comment docs.

if id != c.ID {
newRef = append(newRef, id)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we wrap a function like this:

func remove(s []string, r string) []string {
    for i, v := range s {
        if v == r {
            return append(s[:i], s[i+1:]...)
        }
    }
    return s
}

Could refer: https://stackoverflow.com/questions/34070369/removing-a-string-from-a-slice-in-go

@Letty5411
Copy link
Contributor

Could you also add tests for it? Or you could open an issue to record this.

fix volume can be removed when using by container.

Signed-off-by: Rudy Zhang <[email protected]>
@rudyfly
Copy link
Collaborator Author

rudyfly commented Mar 19, 2018

@Letty5411 PTAL

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Mar 20, 2018
@allencloud allencloud merged commit 18227f5 into AliyunContainerService:master Mar 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