feature: add container's volume manager #15

rudyfly · 2017-10-25T10:49:01Z

Add container's volume manager in pouch.

Ultron: (Container Storage Manager), used to manager the container's
volume, it supports local and remote storages. Local storage is local
disk on the server, such as hdd disk, memory disk, tmpfs and so on.
Remote storage is connecting to central storage or distributed storage,
such as ceph, nas, aws EBS.

Ultron supports local/remote mode, remote mode can schedue the volume
base on storage which is the the optimal storage.

More information about ultron is coming soon...

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

allencloud · 2017-10-30T05:03:36Z

cli/http_request.go

@@ -36,7 +36,7 @@ func (r *Request) Send() *Response {
 		response.Err = ErrHTTP5xx
 		return response
 	}
-	if resp.StatusCode != http.StatusOK {
+	if resp.StatusCode < 200 || resp.StatusCode > 300 {


I think we had better compare with 400 instead of 300. Since 3xx is also reasonable for a request. WDYT? @rudyfly @skoo87

allencloud · 2017-10-30T05:05:32Z

cli/volume.go

+	"github.com/spf13/cobra"
+)
+
+// VolumeCommand use to implement 'volume' command.


I strongly suggest that the comment of a func should use "第三人称单数". for example, VolumeCommand uses, Init initializes...

yeah, I always use this format "xxx is used to do something", is it ok?

Yeah, that is much better. 👍

allencloud · 2017-10-30T05:07:25Z

cli/volume.go

+
+// Run is the entry of VolumeCreateCommand command.
+func (v *VolumeCreateCommand) Run(args []string) {
+	logrus.Infof("create a volume: %s, options: %v, labels: %v, selectors: %v",


I do not think that write an info log in command line is reasonable.
Always, we get the correct result returned or some error message.
How about that?

ok, just for debug, forgot to change log level :)

allencloud · 2017-10-30T05:14:00Z

cli/volume.go

+	for _, l := range v.labels {
+		label := strings.Split(l, "=")
+		if len(label) != 2 {
+			logrus.Warnf("unknown label: %s", l)


Yeah, I totally agree with that we should tell user the error message.
But to be honest, I think maybe there are some other ways to meet user's demand. Maybe it is worth considering.

For user, if he inputs an incorrect label like "asdfgh" by mistake, not the format of "a=b", of course throwing a warn log is also telling him. Meanwhile. the pouchd is also creating a mistaken volume which does not meet the demand. So, if there any possibility to return error directly to user and stop processing it with remote pouchd?

allencloud · 2017-10-30T05:14:19Z

cli/volume.go

+	for _, o := range v.options {
+		option := strings.Split(o, "=")
+		if len(option) != 2 {
+			logrus.Warnf("unknown option: %s", o)


see comments above 🍺

allencloud · 2017-10-30T05:14:38Z

cli/volume.go

+	for _, s := range v.selectors {
+		selector := strings.Split(s, "=")
+		if len(selector) != 2 {
+			logrus.Warnf("unknown selector: %s", s)


same comments above 🍺

allencloud · 2017-10-30T05:16:31Z

daemon/mgr/volume.go

+	// Create is used to create volume.
+	Create(ctx context.Context, name, driver string, options, labels map[string]string) error
+
+	// Remove is used delete an exist volume.


s/delete/to delete/
s/exist/existing/

allencloud · 2017-10-30T05:17:15Z

daemon/mgr/volume.go

+	return vm.Core.CreateVolume(id)
+}
+
+// Remove is used delete an exist volume.


s/delete/to delete/
s/exist/existing/

allencloud · 2017-10-30T05:22:03Z

pkg/utils/command.go

+		if err != nil || ret == nil {
+			time.Sleep(interval)
+			continue
+		} else if ret.ExitStatus != 0 {


We are not fans of else. 😃
If the if block ends up with continue or return, we suggest that we could eliminate else.

You are right, it can change like these:
if err != nil || ret == nil || ret.ExitStatus != 0 {
time.Sleep(interval)
continue
}

allencloud · 2017-10-30T05:25:14Z

I left some comments, while it is still too large review.
I am afraid I need some other maintainer's help to finish this.

In addition, I have one question for us?
Do we have any plan to add some unit tests for the added code? @rudyfly 🐯

rudyfly · 2017-10-30T15:30:00Z

I will add some unit tests in the next few days.

skoowoo · 2017-10-27T06:32:35Z

pkg/serializer/serialize.go

@@ -0,0 +1,55 @@
+package serializer


Can remove the serializer package ??? Use json package directly ????

skoowoo · 2017-10-27T06:38:39Z

pkg/node/idc.go

@@ -0,0 +1,7 @@
+// +build linux


这个文件的代码感觉不太好，应该抽象成工具，而不是 idc.

skoowoo · 2017-10-27T06:40:42Z

cli/http_request.go

@@ -36,7 +36,7 @@ func (r *Request) Send() *Response {
 		response.Err = ErrHTTP5xx
 		return response
 	}
-	if resp.StatusCode != http.StatusOK {
+	if resp.StatusCode < 200 || resp.StatusCode > 300 {


'> 300' to '>= 300' ????

skoowoo · 2017-10-31T02:05:46Z

cli/main.go

+	cli.AddCommand(base, &StartCommand{})
+	cli.AddCommand(base, &VersionCommand{})
+	cli.AddCommand(base, &ImageCommand{})
+	cli.AddCommand(base, &VolumeCommand{})


Why define the "base" command, and pass it to "AddCommand" ??? If you need to define the "base" command, I think that You may be put it into "AddCommand", so don't need to pass it here.

Oh, I understand what you mean, please ignore the above comment.

skoowoo · 2017-10-31T02:14:18Z

daemon/mgr/volume.go

+	_ "github.com/alibaba/pouch/volume/modules/ceph"
+	_ "github.com/alibaba/pouch/volume/modules/local"
+	_ "github.com/alibaba/pouch/volume/modules/tmpfs"
+	_ "github.com/alibaba/pouch/volume/store/boltdb"


Please put these imports in some "*_builtin.go", Is it better ????

eg:
ceph_builtin.go

// +build ceph
package mrg

_ "github.com/alibaba/pouch/volume/modules/ceph"

local_builtin.go

// +build !local

_ "github.com/alibaba/pouch/volume/modules/local"

and so on.

skoowoo · 2017-10-31T02:17:45Z

daemon/mgr/volume.go

+// VolumeManager is the default implement of interface VolumeMgr.
+type VolumeManager struct {
+	Core  *volume.Core
+	Store *meta.Store


s / "Core" and "Store" / "core" and "store" ?????

skoowoo · 2017-10-31T02:58:47Z

pkg/utils/command.go

@@ -0,0 +1,74 @@
+package utils


Could you put these codes ("command.go") into "pkg/exec/exec.go"， then remove the "command.go" file.

I think that is more uniform on the realization.

Add container's volume manager in pouch. Ultron: (Container Storage Manager), used to manager the container's volume, it supports local and remote storages. Local storage is local disk on the server, such as hdd disk, memory disk, tmpfs and so on. Remote storage is connecting to central storage or distributed storage, such as ceph, nas, aws EBS. Ultron supports local/remote mode, remote mode can schedue the volume base on storage which is the the optimal storage. More information about ultron is coming soon... Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>

skoowoo · 2017-11-01T02:01:34Z

LGTM

rudyfly added the volume label Oct 25, 2017

rudyfly self-assigned this Oct 25, 2017

allencloud added DO-NOT-MERGE kind/feature size/L size/XXL and removed size/L labels Oct 25, 2017

allencloud reviewed Oct 30, 2017

View reviewed changes

skoowoo removed the DO-NOT-MERGE label Oct 31, 2017

skoowoo reviewed Oct 31, 2017

View reviewed changes

allencloud added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 1, 2017

skoowoo merged commit bb359be into AliyunContainerService:master Nov 1, 2017

allencloud mentioned this pull request Jan 4, 2018

[enhancement request] volume management implementation #139

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add container's volume manager #15

feature: add container's volume manager #15

rudyfly commented Oct 25, 2017 •

edited

Loading

allencloud Oct 30, 2017

allencloud Oct 30, 2017

rudyfly Oct 30, 2017

allencloud Oct 30, 2017

allencloud Oct 30, 2017

rudyfly Oct 30, 2017

allencloud Oct 30, 2017

allencloud Oct 30, 2017

allencloud Oct 30, 2017

allencloud Oct 30, 2017

allencloud Oct 30, 2017

rudyfly Oct 30, 2017

allencloud Oct 30, 2017

rudyfly Oct 30, 2017

allencloud commented Oct 30, 2017 •

edited

Loading

rudyfly commented Oct 30, 2017

skoowoo Oct 27, 2017

skoowoo Oct 27, 2017

skoowoo Oct 27, 2017

skoowoo Oct 31, 2017

skoowoo Oct 31, 2017

skoowoo Oct 31, 2017

skoowoo Oct 31, 2017

skoowoo Oct 31, 2017

skoowoo commented Nov 1, 2017

feature: add container's volume manager #15

feature: add container's volume manager #15

Conversation

rudyfly commented Oct 25, 2017 • 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

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 Oct 30, 2017 • edited Loading

rudyfly commented Oct 30, 2017

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

skoowoo commented Nov 1, 2017

rudyfly commented Oct 25, 2017 •

edited

Loading

allencloud commented Oct 30, 2017 •

edited

Loading