feature: add pouch plugin #1278

shaloulcy · 2018-05-08T02:35:14Z

To use existing docker volume and network plugin. We implement pouch plugin
mechanism. We change the plugin discovery directory to /run/pouch/plugins,
/etc/pouch/plugins, /usr/lib/pouch/plugins. In future, maybe we will use the
grpc protocol to implement the plugin

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

Ⅰ. Describe what this PR did

To use existing docker volume and network plugin. We implement pouch plugin
mechanism. We change the plugin discovery directory to /run/pouch/plugins,
/etc/pouch/plugins, /usr/lib/pouch/plugins. In future, maybe we will use the
grpc protocol to implement the plugin

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io · 2018-05-08T02:49:12Z

Codecov Report

Merging #1278 into master will increase coverage by 0.97%.
The diff coverage is 59.41%.

@@            Coverage Diff             @@
##           master    #1278      +/-   ##
==========================================
+ Coverage   15.31%   16.29%   +0.97%     
==========================================
  Files         174      178       +4     
  Lines       11014    10994      -20     
==========================================
+ Hits         1687     1791     +104     
+ Misses       9208     9065     -143     
- Partials      119      138      +19

Impacted Files	Coverage Δ
plugins/error.go	`0% <0%> (ø)`
plugins/plugins.go	`0% <0%> (ø)`
apis/server/server.go	`0% <0%> (ø)`	⬆️
pkg/httputils/client.go	`27.65% <27.65%> (ø)`
plugins/client.go	`54.32% <54.32%> (ø)`
client/client.go	`66% <66.66%> (+7.05%)`	⬆️
plugins/plugin.go	`72% <72%> (ø)`
plugins/manager.go	`75.17% <75.17%> (ø)`
daemon/mgr/image_utils.go	`0% <0%> (-40%)`	⬇️
pkg/reference/def.go	`33.33% <0%> (-11.12%)`	⬇️
... and 24 more

allencloud · 2018-05-08T06:10:36Z

plugins/plugin.go

+)
+
+// TLSConfig contains information of tls which users can specify
+type TLSConfig struct {


I think we have already defined a TLSConfig in the code base package client. I am wondering if we could avoid the duplication.

plugins.TLSConfig is different from client.TLSConfig

allencloud · 2018-05-08T06:12:38Z

plugins/client.go

+// defaultContentTypeis the default content type accepted and sent by the plugins.
+const defaultContentType = "application/vnd.docker.plugins.v1.1+json"
+
+// PluginClient the client which call Plugin ServiceMethod


Please add every a . in every function's comments.

allencloud · 2018-05-08T06:13:23Z

plugins/client.go

+
+	u, err := url.Parse(addr)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse plugin addr: %s", addr)


fmt.Errorf("failed to parse plugin addr %s: %v", addr, err) ?

allencloud · 2018-05-08T06:14:20Z

plugins/client.go

+		if tlsconfig != nil && tlsconfig.InsecureSkipVerify == false {
+			config, err = client.GenTLSConfig(tlsconfig.KeyFile, tlsconfig.CertFile, tlsconfig.CAFile)
+			if err != nil {
+				return nil, fmt.Errorf("failed to parse plugin tls config")


please throw out the error. I think that will make the error message more explicit.

allencloud · 2018-05-08T06:25:17Z

plugins/client.go

+			return nil, err
+		}
+
+		if resp.StatusCode != http.StatusOK {


Does only 200 make sense?
In my opinion, 2xx and 3xx are both regarded valid.

It seems that the resp should be handled out of this function. 101 / 20x / 30x are the OK.

allencloud · 2018-05-08T06:28:43Z

plugins/client.go

+					return nil, err
+				}
+				counter++
+				logrus.Warnf("plugin %s call %s failed, retry after %d seconds", cli.address, service, delay)


throw out the error in the log message?

it's an warning msg.

allencloud · 2018-05-08T06:39:02Z

plugins/client.go

+
+		resp, err := cli.client.Do(req)
+		if err != nil {
+			if retry {


I would suggest that we could use return fast to reduce more ident, like:

if !retry { return nil, err } delay := backoff(counter) if timeout(start, delay) { return nil, err } ......

allencloud · 2018-05-08T06:52:17Z

plugins/client.go

+	"github.com/sirupsen/logrus"
+)
+
+var defaultTimeout = 30


How about make this type of time.Duration instead of a type of int?

allencloud · 2018-05-08T06:55:52Z

plugins/error.go

+var ErrNotFound = errors.New("plugin not found")
+
+// ErrNotImplement represents that the plugin not implement the given protocol
+var ErrNotImplement = errors.New("plugin not implement")


s/NotImplement/NotImplemented ?

allencloud · 2018-05-08T07:03:25Z

plugins/manager.go

+	for {
+		plugin, err := m.newPlugin(name)
+		if err != nil {
+			if retry {


We can use return fast here.

allencloud · 2018-05-08T07:09:21Z

plugins/manager.go

+	}
+
+	for _, path := range specPaths {
+		fi, err := os.Open(path)


I think we should close this fi when finishing dealing.

allencloud · 2018-05-08T07:20:17Z

Thank @shaloulcy for your work.
While I put forward some minor tips:

I am wondering if we should encapsulate all the httpClient implementation into a single package to avoid duplication. Since I found that in package plugin and package client, we both implemented some structs and some functions.

also cc @fuweid

fuweid

@allencloud , In my option, most functions has been covered in pkg client.
I think we can upgrade the pkg client instead of new pkg plugin.

fuweid · 2018-05-08T09:54:22Z

plugins/client.go

+			return nil, err
+		}
+
+		if resp.StatusCode != http.StatusOK {


It seems that the resp should be handled out of this function. 101 / 20x / 30x are the OK.

rudyfly · 2018-05-09T09:16:56Z

plugins/manager.go

+	"github.com/sirupsen/logrus"
+)
+
+// pluginManager is a pluginManager, which manages all plugin events.


pluginManager is a plugin manager is it better?

rudyfly · 2018-05-09T09:40:18Z

plugins/manager.go

+	"github.com/sirupsen/logrus"
+)
+
+// pluginManager is a pluginManager, which manages all plugin events.


pluginManager is a plugin manager is it better?

rudyfly · 2018-05-09T09:42:03Z

plugins/plugins.go

+var manager = &pluginManager{
+	plugins:         make(map[string]*Plugin),
+	pluginSockPaths: []string{"/run/pouch/plugins"},
+	pluginSpecPaths: []string{"/etc/pouch/plugins", "/usr/lib/pouch/plugins"},


/usr/lib/pouch/plugins I think it should change to /var/lib/pouch/plugins.

rudyfly · 2018-05-09T09:42:35Z

plugins/plugins.go

+	pluginSpecPaths: []string{"/etc/pouch/plugins", "/usr/lib/pouch/plugins"},
+}
+
+// Get return the requested plugin.


s/return/returns

rudyfly · 2018-05-09T09:45:14Z

plugins/plugins.go

+	return manager.getPluginByName(pluginType, name)
+}
+
+// SetPluginSockPaths set the plugin sock paths.


rudyfly · 2018-05-09T09:45:25Z

plugins/plugins.go

+	manager.setPluginSockPaths(paths)
+}
+
+// SetPluginSpecPaths set the plugin spec paths.


allencloud · 2018-05-10T02:37:58Z

pkg/httputils/client.go

+
+// ParseHost inputs a host address string, and output three type:
+// url.URL, basePath and an error.
+func ParseHost(host string) (*url.URL, string, string, error) {


In the comment, you wrote that this function returns three types. While I found that this function actually returns four types (*url.URL, string, string, error).
I am afraid inconsistency exists.

allencloud · 2018-05-10T02:43:14Z

plugins/client.go

+		}
+	}
+
+	if out != nil {


How about

if out == nil { return nil } return json.NewDecoder(resp.Body).Decode(out)

To use existing docker volume and network plugin. We implement pouch plugin mechanism. We change the plugin discovery directory to /run/pouch/plugins, /etc/pouch/plugins, /usr/lib/pouch/plugins. In future, maybe we will use the grpc protocol to implement the plugin Signed-off-by: Eric Li <[email protected]>

allencloud · 2018-05-10T09:22:34Z

LGTM

pouchrobot added kind/feature size/XXL labels May 8, 2018

allencloud reviewed May 8, 2018

View reviewed changes

plugins/manager.go

for {

plugin, err := m.newPlugin(name)

if err != nil {

if retry {

Copy link

Collaborator

allencloud May 8, 2018

Choose a reason for hiding this comment

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

We can use return fast here.

allencloud reviewed May 8, 2018

View reviewed changes

fuweid self-requested a review May 8, 2018 07:25

fuweid reviewed May 8, 2018

View reviewed changes

shaloulcy force-pushed the v1_plugin_manager branch 2 times, most recently from 789b5c2 to 4dcd71b Compare May 9, 2018 10:58

rudyfly reviewed May 9, 2018

View reviewed changes

shaloulcy force-pushed the v1_plugin_manager branch 2 times, most recently from b176f4d to b45eec4 Compare May 10, 2018 02:27

allencloud reviewed May 10, 2018

View reviewed changes

shaloulcy force-pushed the v1_plugin_manager branch from b45eec4 to 2caa0d4 Compare May 10, 2018 03:23

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

allencloud merged commit 49dea30 into AliyunContainerService:master May 10, 2018

pouchrobot mentioned this pull request May 17, 2018

Weekly Report of Pouch from 2018-05-10 to 2018-05-17 #1345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add pouch plugin #1278

feature: add pouch plugin #1278

shaloulcy commented May 8, 2018

codecov-io commented May 8, 2018 •

edited

Loading

allencloud May 8, 2018

shaloulcy May 9, 2018

allencloud May 8, 2018

shaloulcy May 9, 2018

allencloud May 8, 2018

allencloud May 8, 2018

allencloud May 8, 2018

fuweid May 8, 2018

allencloud May 8, 2018

shaloulcy May 9, 2018

allencloud May 8, 2018

allencloud May 8, 2018

allencloud May 8, 2018

allencloud May 8, 2018

allencloud May 8, 2018 •

edited

Loading

allencloud commented May 8, 2018 •

edited

Loading

fuweid left a comment

fuweid May 8, 2018

rudyfly May 9, 2018

rudyfly May 9, 2018

rudyfly May 9, 2018

shaloulcy May 10, 2018

rudyfly May 9, 2018

rudyfly May 9, 2018

rudyfly May 9, 2018

allencloud May 10, 2018 •

edited

Loading

allencloud May 10, 2018

allencloud commented May 10, 2018

feature: add pouch plugin #1278

feature: add pouch plugin #1278

Conversation

shaloulcy commented May 8, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented May 8, 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

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 May 8, 2018 • edited Loading

Choose a reason for hiding this comment

allencloud commented May 8, 2018 • edited Loading

fuweid left a 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 May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented May 10, 2018

codecov-io commented May 8, 2018 •

edited

Loading

allencloud May 8, 2018 •

edited

Loading

allencloud commented May 8, 2018 •

edited

Loading

allencloud May 10, 2018 •

edited

Loading