feature: remote volume driver #1326

shaloulcy · 2018-05-15T13:47:13Z

Signed-off-by: Eric Li [email protected]

Ⅰ. Describe what this PR did

Now pouch only supports local/tempfs/ceph volumes. Docker can support more volumes with the help of volume plugin mechanism. In this PR，we implement remote volume driver, which also use volume plugin mechanism. Pouch volume plugin is compatible with docker, So we can use existing mature docker volumes drivers.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

1. install local-persist volume plugin

installation：https://github.com/CWSpear/local-persist

After start the local-persist volume plugin, it will create local-persist.sock in /run/docker/plugins/

2. create a local-persist volume

pouch volume create -d local-persist --name test_vol -o mountpoint=/data/test_vol
CreatedAt:    2018-5-16 17:31:45
Driver:       local-persist
Labels:       map[]
Mountpoint:   /data/test_vol
Name:         test_vol
Scope:        
Status:       map[mount:/var/lib/pouch/volume mountpoint:/data/test_vol sifter:Default]


#pouch volume ls
DRIVER          VOLUME NAME
local           local1
local-persist   test_vol

3. run container with the volume

#pouch run -d -t -v test_vol:/mydata reg.docker.alibaba-inc.com/busybox:latest sh
d599b56108f67e097a1f9914d79985f524e44eb17e5c394ca9b397818bf0f917

[root@pouch-test-7u-vm /root]
#pouch volume inspect test_vol
[
    {
        "CreatedAt": "2018-5-16 17:31:45",
        "Driver": "local-persist",
        "Labels": {
            "backend": "local-persist",
            "hostname": "pouch-test-7u-vm"
        },
        "Mountpoint": "/data/test_vol",
        "Name": "test_vol",
        "Status": {
            "mount": "/var/lib/pouch/volume",
            "mountpoint": "/data/test_vol",
            "ref": "d599b56108f67e097a1f9914d79985f524e44eb17e5c394ca9b397818bf0f917",
            "sifter": "Default"
        }
    }
]

the volume Status has a ref filed, which is the container id

4. remove container and volume

#pouch rm d599b5 -f
d599b5

#pouch volume remove test_vol
Removed: test_vol

#pouch volume ls
DRIVER   VOLUME NAME
local    local1

Ⅴ. Special notes for reviews

codecov-io · 2018-05-15T14:32:51Z

Codecov Report

Merging #1326 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1326      +/-   ##
==========================================
+ Coverage   17.32%   17.33%   +<.01%     
==========================================
  Files         189      189              
  Lines       11822    11816       -6     
==========================================
  Hits         2048     2048              
+ Misses       9627     9621       -6     
  Partials      147      147

Impacted Files	Coverage Δ
plugins/plugins.go	`0% <ø> (ø)`	⬆️
daemon/mgr/volume.go	`0% <0%> (ø)`	⬆️
cli/container.go	`0% <0%> (ø)`	⬆️
daemon/mgr/container.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_linux.go	`0% <0%> (ø)`	⬆️
cli/common_flags.go	`0% <0%> (ø)`	⬆️
daemon/containerio/jsonfile.go	`1.49% <0%> (+0.06%)`	⬆️

allencloud · 2018-05-17T02:56:40Z

storage/volume/core.go

@@ -76,16 +76,38 @@ func (c *Core) GetVolume(id types.VolumeID) (*types.Volume, error) {
 		},
 	}

+	ctx := driver.Contexts()
+
 	// first, try to get volume from local store.
 	obj, err := c.store.Get(id.Name)
 	if err == nil {


What will happen if err is not nil and err is not metastore.ErrObjectNotFound either?
In the code, I think it will ignore this case. Does it make sense? @shaloulcy

It means the bolt works not well, for example bucket not found, even bolt file deleted. We should throw the out the error. Remote volume metas are also stored in boltdb

allencloud · 2018-05-17T02:57:19Z

storage/volume/core.go

+	}
+
+	// at last, scan all drivers
+	logrus.Debugf("probing all drivers for volume with name: %s", id.Name)


Please remove : or use logrus.Debugf("probing all drivers for volume with name(%s)", id.Name)

allencloud · 2018-05-17T04:30:30Z

storage/volume/core_util.go

-		return errors.Errorf("Backend driver: %s not found", v.Spec.Backend)
+	dv, err := driver.Get(v.Spec.Backend)
+	if err != nil {
+		return errors.Errorf("Backend driver: %s not found, err: %v", v.Spec.Backend, err)


("failed to get backend driver %s: %v", v.Spec.Backend, err)?

allencloud · 2018-05-17T04:31:58Z

storage/volume/driver/driver.go

-func (t driverTable) Add(name string, d Driver) {
-	t[name] = d
+// driverTable contains all volume drivers
+type driverTable struct {


You could use type SafeMap struct in package pkg/collect, instead of declaring a brand new one.
If we use SafeMap, lots of functions could be reduced since they may be duplicated with functions in package collect.

WDYT?

Now driverTable only has a field named drivers except sync.Mutex. In future we may add more fields, like alias drivers and so on. And the SafeMap may not safisfy my requirements. For example we want add a driver, first we will check whether the driver existed. If not, we add the driver. This action is atomic. Of course, we can change the SafeMap

In future we may add more fields

You can define like this, I think the following :

type driverTable struct { sync.Mutex drivers collect.SafeMap alias xxxType }

Yes, of course. We could enhance SafeMap. 😄

allencloud · 2018-05-17T04:34:55Z

storage/volume/driver/proxy.go

+)
+
+const (
+	remoteVolumeCreateService       = "/VolumeDriver.Create"


What is the usage of all the const variables? Could you add a little bit annotation for these?
Since it would be quite hard to understand this part of codes with no guidance.

These const variables is the part of remote volume protocols, I will add more annotations

allencloud · 2018-05-17T04:36:51Z

storage/volume/driver/proxy.go

+
+	var resp remoteVolumeCreateResp
+
+	if err := proxy.client.CallService(remoteVolumeCreateService, &req, &resp, true); err != nil {


I am wondering if we could change the function, just making the response in the returned value, instead of passing a pointer of resp in parameter? Does it to meed some particular demand?

To simplize the caller logic. Return io.Reader?

allencloud · 2018-05-17T04:39:50Z

storage/volume/types/volume_util.go

+	}
+}
+
+func buildVolumeConfig(options map[string]string) (*VolumeConfig, error) {


I think we could add some unit test for this file.

allencloud · 2018-05-17T05:29:15Z

Is there any way to add some integration test? @shaloulcy

shaloulcy · 2018-05-17T06:34:01Z

@allencloud I think we can add some integration tests using the local-persist volume driver

Signed-off-by: Eric Li <[email protected]>

shaloulcy · 2018-05-18T03:37:08Z

modify some logging and add more annotations. I will add more unit-tests and integration tests in other pull requests

allencloud · 2018-05-18T03:44:35Z

LGTM

rudyfly · 2018-05-18T06:28:31Z

LGTM

commit 75aab2b Merge: 5c8993b 2f2e9c1 Author: Starnop <[email protected]> Date: Sat May 19 21:51:30 2018 +0800 Merge branch 'cri-compatibility' of github.com:Starnop/pouch into cri-compatibility commit 2f2e9c1 Merge: bd9c4fd acbd19a Author: Starnop <[email protected]> Date: Fri May 18 15:32:31 2018 +0800 Merge branch 'master' of https://github.com/alibaba/pouch into cri-compatibility commit acbd19a Merge: 6cef601 8e862f9 Author: Yao Zengzeng <[email protected]> Date: Fri May 18 15:27:01 2018 +0800 Merge pull request AliyunContainerService#1351 from fuweid/feature_allow_use_image_by_digest_id feature: allow use image by digest id commit 6cef601 Merge: 4423313 caf45ec Author: Wei Fu <[email protected]> Date: Fri May 18 15:10:30 2018 +0800 Merge pull request AliyunContainerService#1350 from Ace-Tang/version build: generate version information at build time commit caf45ec Author: Ace-Tang <[email protected]> Date: Fri May 18 13:31:25 2018 +0800 build: generate version information at build time auto-generate git commit, build time at build time, fix go-version hard code. Signed-off-by: Ace-Tang <[email protected]> commit 8e862f9 Author: Wei Fu <[email protected]> Date: Fri May 18 13:50:42 2018 +0800 feature: allow use image by digest id Basically, the user can use sha256:xyz to inspect image or run container. Signed-off-by: Wei Fu <[email protected]> commit 4423313 Merge: 8410064 f67cf08 Author: Allen Sun <[email protected]> Date: Fri May 18 11:44:39 2018 +0800 Merge pull request AliyunContainerService#1326 from shaloulcy/core_volume feature: remote volume driver commit 8410064 Merge: 8af60a9 852d4f4 Author: Wei Fu <[email protected]> Date: Fri May 18 10:39:15 2018 +0800 Merge pull request AliyunContainerService#1339 from rhinoceros/master docs: Update INSTALLATION.md apt-key fingerprint BE2F475F commit f67cf08 Author: Eric Li <[email protected]> Date: Mon May 14 12:19:46 2018 +0800 feature: remote volume driver Signed-off-by: Eric Li <[email protected]> commit bd9c4fd Merge: 8d01e30 521ca7c Author: Starnop <[email protected]> Date: Thu May 17 11:11:40 2018 +0800 Merge branch 'master' of https://github.com/alibaba/pouch into cri-compatibility commit 8af60a9 Merge: ecbe3b6 13a17b5 Author: Allen Sun <[email protected]> Date: Fri May 18 09:43:40 2018 +0800 Merge pull request AliyunContainerService#1349 from pouchrobot/auto-doc-2018-05-18 docs: auto generate pouch cli/api docs via code commit 13a17b5 Author: pouchrobot <[email protected]> Date: Fri May 18 01:28:51 2018 +0000 docs: auto generate pouch cli docs via code Signed-off-by: pouchrobot <[email protected]> commit 5c8993b Merge: 8d01e30 521ca7c Author: Starnop <[email protected]> Date: Thu May 17 11:11:40 2018 +0800 Merge branch 'master' of https://github.com/alibaba/pouch into cri-compatibility commit ecbe3b6 Merge: 3374daf 7fc11df Author: Wei Fu <[email protected]> Date: Thu May 17 18:53:20 2018 +0800 Merge pull request AliyunContainerService#1227 from Ace-Tang/full_spec_params feature: add pidslimit implement commit 7fc11df Author: Ace-Tang <[email protected]> Date: Fri May 11 16:25:32 2018 +0800 feature: add pidslimit implement Signed-off-by: Ace-Tang <[email protected]> commit 3374daf Merge: 89a5ae5 fd8e0f7 Author: Allen Sun <[email protected]> Date: Thu May 17 13:44:52 2018 +0800 Merge pull request AliyunContainerService#1341 from Letty5411/0517-assertfix test: enhance cli related tests commit 89a5ae5 Merge: 521ca7c 8469cea Author: Wei Fu <[email protected]> Date: Thu May 17 13:17:47 2018 +0800 Merge pull request AliyunContainerService#1331 from Letty5411/0516-doc doc: update test.md about how to run test commit fd8e0f7 Author: letty <[email protected]> Date: Thu May 17 11:23:16 2018 +0800 test: enhance test Signed-off-by: letty <[email protected]> commit 521ca7c Merge: 19c956b 6b2a8b4 Author: Yao Zengzeng <[email protected]> Date: Thu May 17 10:39:06 2018 +0800 Merge pull request AliyunContainerService#1340 from ZouRui89/race_off bugfix: move IO part behind to ensure execConfig result assignment before IO closes commit 6b2a8b4 Author: Zou Rui <[email protected]> Date: Wed May 16 18:03:27 2018 +0800 bugfix: move IO part behind to ensure execConfig result assignment before IO closes Signed-off-by: Zou Rui <[email protected]> commit 8d01e30 Merge: 9339a70 19c956b Author: Starnop <[email protected]> Date: Wed May 16 12:57:14 2018 +0800 Merge branch 'master' of https://github.com/alibaba/pouch into cri-compatibility commit 852d4f4 Author: rhinoceros.xn <[email protected]> Date: Wed May 16 17:37:51 2018 +0800 Update INSTALLATION.md $ apt-key fingerprint BE2F475F pub 4096R/BE2F475F 2018-02-28 Key fingerprint = F443 EDD0 4A58 7E8B F645 9C40 CF68 F84A BE2F 475F uid opsx-admin <[email protected]> commit 8469cea Author: letty <[email protected]> Date: Wed May 16 10:08:32 2018 +0800 doc: update test.md about how to run test Signed-off-by: letty <[email protected]> commit 9339a70 Author: Starnop <[email protected]> Date: Tue May 15 17:35:53 2018 +0800 cri com and up

pouchrobot added areas/storage kind/feature size/XXL labels May 15, 2018

shaloulcy force-pushed the core_volume branch 2 times, most recently from 604e78b to 4b21275 Compare May 15, 2018 14:14

shaloulcy requested a review from rudyfly May 16, 2018 06:59

allencloud reviewed May 17, 2018

View reviewed changes

feature: remote volume driver

f67cf08

Signed-off-by: Eric Li <[email protected]>

shaloulcy force-pushed the core_volume branch from 4b21275 to f67cf08 Compare May 18, 2018 02:00

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 18, 2018

allencloud merged commit 4423313 into AliyunContainerService:master May 18, 2018

pouchrobot mentioned this pull request May 24, 2018

Weekly Report of Pouch from 2018-05-17 to 2018-05-24 #1408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: remote volume driver #1326

feature: remote volume driver #1326

shaloulcy commented May 15, 2018 •

edited

Loading

codecov-io commented May 15, 2018 •

edited

Loading

allencloud May 17, 2018

shaloulcy May 17, 2018

allencloud May 17, 2018

allencloud May 17, 2018

allencloud May 17, 2018 •

edited

Loading

shaloulcy May 17, 2018

allencloud May 17, 2018

allencloud May 17, 2018

shaloulcy May 17, 2018

allencloud May 17, 2018

shaloulcy May 17, 2018

allencloud May 17, 2018

shaloulcy May 17, 2018

allencloud commented May 17, 2018

shaloulcy commented May 17, 2018

shaloulcy commented May 18, 2018 •

edited

Loading

allencloud commented May 18, 2018

rudyfly commented May 18, 2018


		var resp remoteVolumeCreateResp

		if err := proxy.client.CallService(remoteVolumeCreateService, &req, &resp, true); err != nil {

feature: remote volume driver #1326

feature: remote volume driver #1326

Conversation

shaloulcy commented May 15, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

1. install local-persist volume plugin

2. create a local-persist volume

3. run container with the volume

4. remove container and volume

Ⅴ. Special notes for reviews

codecov-io commented May 15, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented May 17, 2018

shaloulcy commented May 17, 2018

shaloulcy commented May 18, 2018 • edited Loading

allencloud commented May 18, 2018

rudyfly commented May 18, 2018

shaloulcy commented May 15, 2018 •

edited

Loading

codecov-io commented May 15, 2018 •

edited

Loading

allencloud May 17, 2018 •

edited

Loading

shaloulcy commented May 18, 2018 •

edited

Loading