refactor: refact the pouch hook plugin #2360

rudyfly · 2018-10-25T03:11:53Z

Ⅰ. Describe what this PR did

Refact the pouch hook plugin, add plugins code into binary when
building, instead of hook_plugin.so.

Add the Cri plugin.

More detail you can see: docs/features/pouch_with_plugin.md

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

NONE

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

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

codecov · 2018-10-25T03:11:54Z

Codecov Report

Merging #2360 into master will increase coverage by 0.21%.
The diff coverage is 86.84%.

@@            Coverage Diff            @@
##           master   #2360      +/-   ##
=========================================
+ Coverage   68.29%   68.5%   +0.21%     
=========================================
  Files         265     275      +10     
  Lines       18211   18245      +34     
=========================================
+ Hits        12437   12499      +62     
+ Misses       4357    4324      -33     
- Partials     1417    1422       +5

Flag	Coverage Δ
#criv1alpha1test	`31.85% <61.84%> (+0.21%)`	⬆️
#criv1alpha2test	`35.71% <78.94%> (+0.2%)`	⬆️
#integrationtest	`39.83% <67.1%> (+0.27%)`	⬆️
#nodee2etest	`33.32% <80.26%> (+0.2%)`	⬆️
#unittest	`25.53% <0%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
storage/plugins/error.go	`0% <ø> (ø)`
storage/volume/driver/proxy.go	`79.45% <ø> (ø)`	⬆️
cri/v1alpha2/cri_types.go	`100% <ø> (ø)`	⬆️
storage/plugins/manager.go	`78.72% <ø> (ø)`
storage/plugins/client.go	`60.49% <ø> (ø)`
daemon/config/config.go	`47.94% <ø> (ø)`	⬆️
apis/server/server.go	`58.69% <ø> (ø)`	⬆️
storage/volume/driver/remote.go	`68.96% <ø> (ø)`	⬆️
storage/volume/driver/driver.go	`68.03% <ø> (ø)`	⬆️
storage/plugins/plugins.go	`50% <ø> (ø)`
... and 37 more

zhuangqh · 2018-10-26T03:27:38Z

internal/generator.go

@@ -21,7 +21,7 @@ type DaemonProvider interface {
 	VolMgr() mgr.VolumeMgr
 	NetMgr() mgr.NetworkMgr
 	MetaStore() *meta.Store
-	ContainerPlugin() plugins.ContainerPlugin
+	ContainerPlugin() hookplugins.ContainerPlugin


The definition of DaemonProvider interface should also expose other 3 three plugin? Provide the access method to construct other components. WDYT @rudyfly

Maybe it is unnessary.

zhuangqh · 2018-10-26T03:42:36Z

cri/v1alpha2/cri.go

 	if err != nil {
 		return nil, err
 	}

+	// call cri plugin to update create config
+	if c.CriPlugin != nil {
+		if err := c.CriPlugin.PreCreateContainer(createConfig, sandboxMeta); err != nil {


Why don't you put this hook immediately before c.ContainerMgr.Create? The changes of plugin would be overwrote by other code.

zhuangqh · 2018-10-26T03:46:20Z

daemon/daemon.go

-		if err != nil {
-			return errors.Wrapf(err, "load plugin at %s error", d.config.PluginPath)
-		}
+	//load daemon plugin if exist


missing a space after two slash//

zhuangqh · 2018-10-26T03:46:27Z

daemon/daemon.go

-		} else if s != nil {
-			return fmt.Errorf("not a daemon plugin at %s %q", d.config.PluginPath, s)
-		}
+	//load container plugin if exist


missing a space after two slash//

zhuangqh · 2018-10-26T03:51:57Z

docs/features/pouch_with_plugin.md

-}
+## Which plugins
+
+Currently pouch provides four plugins，they are: `container plugin`，`daemon plugin`，`volume plugin`，`cri plugin`


Suggested change

Currently pouch provides four plugins，they are: `container plugin`，`daemon plugin`，`volume plugin`，`cri plugin`

Currently pouch-container provides four plugins，they are: `container plugin`，`daemon plugin`，`volume plugin`，`cri plugin`.

docs/features/pouch_with_plugin.md

zhuangqh · 2018-10-26T04:10:56Z

docs/features/pouch_with_plugin.md


-define two plugin symbols which only print some logs at correspond point:
+* pre-volume-create volume point, at this point you can change the create volume's config as you want, add your default volume's options.


Suggested change

* pre-volume-create volume point, at this point you can change the create volume's config as you want, add your default volume's options.

* pre-volume-create volume point, at this point you can change the volume's create config as you want, add your default volume's options.

zhuangqh · 2018-10-26T04:12:40Z

docs/features/pouch_with_plugin.md


-var ContainerPlugin ContPlugin
+* pre-create-container cri container point, at this point you can update the container config what it will be created, such as update the container's envs or labels.


Suggested change

* pre-create-container cri container point, at this point you can update the container config what it will be created, such as update the container's envs or labels.

* pre-create-container cri point, at this point you can update the container config to what it will be created, such as update the container's envs or labels.

zhuangqh · 2018-10-26T04:15:22Z

docs/features/pouch_with_plugin.md

-        ]
-    }
+// PreStartHook copy config from /etc/pouch/config.json to /etc/sysconfig/pouch
+// and start plugin processes than daemon depended


mismatched doc.

zhuangqh · 2018-10-26T04:16:05Z

docs/features/pouch_with_plugin.md

+	return nil
+}
+
+// PreStopHook stops plugin processes than start ed by PreStartHook.


Suggested change

// PreStopHook stops plugin processes than start ed by PreStartHook.

// PreStopHook stops plugin processes than started by PreStartHook.

hookplugins/daemonplugin/daemon_hook.go

zhuangqh · 2018-10-29T14:55:29Z

This failure case is strange

FAIL: /home/travis/gopath/src/github.com/alibaba/pouch/test/cli_run_dns_test.go:55: PouchRunSuite.TestRunWithBridgeNetwork
/home/travis/gopath/src/github.com/alibaba/pouch/test/cli_run_dns_test.go:67:
    c.Assert(res.Stdout(), check.Equals, hostRes.Stdout())
... obtained string = "" +
...     "search c.eco-emissary-99515.internal google.internal\n" +
...     "nameserver 127.0.0.11\n" +
...     "options ndots:0\n"
... expected string = "" +
...     "# Dynamic resolv.conf(5) file for glibc resolver(3) generated by resolvconf(8)\n" +
...     "#     DO NOT EDIT THIS FILE BY HAND -- YOUR CHANGES WILL BE OVERWRITTEN\n" +
...     "nameserver 169.254.169.254\n" +
...     "search c.eco-emissary-99515.internal google.internal\n"

However, I can't find a button to restart this job.

allencloud · 2018-10-30T02:43:57Z

cri/v1alpha2/cri.go

@@ -683,6 +686,12 @@ func (c *CriManager) CreateContainer(ctx context.Context, r *runtime.CreateConta
 	sandboxConfig := r.GetSandboxConfig()
 	podSandboxID := r.GetPodSandboxId()

+	// get sandbox
+	sandbox, err := c.ContainerMgr.Get(ctx, podSandboxID)


Is this part of code related to the hookplugin? @rudyfly Just wish confirmation from you.

In addition, the codes added gets instance sandbox, but this sandbox is used in L741, about 50 lines of code away from here.

If you are not putting this code on purpose, I am wondering if we could locate the code just along with L741 to improve aggregation.

Oh, I see you are constructing sandboxMeta for the hook plugin.

But also wish you could answer the second question.

I have moved them together, PTAL.

allencloud · 2018-10-30T02:49:23Z

docs/features/pouch_with_plugin.md

@@ -1,148 +1,159 @@
-# PouchContainer with plugin
+# Pouch-container with plugin


Why to change the name of it?
I think the logo of this project is PouchContainer indeed.

Should pouchcontainer instead of pouch-container?

allencloud · 2018-10-30T03:00:05Z

docs/features/pouch_with_plugin.md

+	// PostUpdate called after update method successful,
+	// the method accepts the rootfs path and envs of container
+	PostUpdate(string, []string) error
+} []()


remove []()?

starnop · 2018-10-30T08:36:45Z

hookplugins/CriPlugin.go

+// CriPlugin defines places where a plugin will be triggered in CRI api lifecycle
+type CriPlugin interface {
+	// PreCreateContainer defines plugin point where receives a container create request, in this plugin point user
+	// could the container's config in cri interface.


Suggested change

// could the container's config in cri interface.

// could update the container's config in cri interface.

or something else? :)

Refact the pouch hook plugin, add plugins code into binary when building, instead of hook_plugin.so. Add the Cri plugin. More detail you can see: `docs/features/pouch_with_plugin.md` Signed-off-by: Rudy Zhang <[email protected]>

zhuangqh · 2018-10-31T01:56:09Z

LGTM

pouchrobot added kind/refactor size/XL labels Oct 25, 2018

rudyfly force-pushed the master_plugin branch from d9706d6 to b430721 Compare October 25, 2018 04:50

pouchrobot added size/XXL and removed size/XL labels Oct 25, 2018

rudyfly force-pushed the master_plugin branch 4 times, most recently from 9b10a69 to 45576fd Compare October 25, 2018 05:03

rudyfly requested review from allencloud, starnop, yyb196, fuweid and HusterWan October 25, 2018 05:04

zhuangqh reviewed Oct 26, 2018

View reviewed changes

rudyfly force-pushed the master_plugin branch 3 times, most recently from 7e5c839 to d116b17 Compare October 29, 2018 09:05

allencloud reviewed Oct 30, 2018

View reviewed changes

rudyfly force-pushed the master_plugin branch 2 times, most recently from 92c4014 to a40d852 Compare October 30, 2018 07:51

starnop reviewed Oct 30, 2018

View reviewed changes

rudyfly force-pushed the master_plugin branch from a40d852 to 2e633ce Compare October 30, 2018 11:51

refactor: refact the pouch hook plugin

e2f48e9

Refact the pouch hook plugin, add plugins code into binary when building, instead of hook_plugin.so. Add the Cri plugin. More detail you can see: `docs/features/pouch_with_plugin.md` Signed-off-by: Rudy Zhang <[email protected]>

rudyfly force-pushed the master_plugin branch from 2e633ce to e2f48e9 Compare October 30, 2018 13:54

zhuangqh merged commit 1ceb1e9 into AliyunContainerService:master Oct 31, 2018

pouchrobot mentioned this pull request Nov 2, 2018

WeeklyReport of PouchContainer from 2018-10-26 to 2018-11-02 #2412

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: refact the pouch hook plugin #2360

refactor: refact the pouch hook plugin #2360

rudyfly commented Oct 25, 2018 •

edited

Loading

codecov bot commented Oct 25, 2018 •

edited

Loading

zhuangqh Oct 26, 2018

rudyfly Oct 29, 2018

zhuangqh Oct 26, 2018

rudyfly Oct 29, 2018

zhuangqh Oct 26, 2018

zhuangqh Oct 26, 2018

zhuangqh Oct 26, 2018

zhuangqh Oct 26, 2018

zhuangqh Oct 26, 2018

zhuangqh Oct 26, 2018

zhuangqh Oct 26, 2018

zhuangqh commented Oct 29, 2018

allencloud Oct 30, 2018 •

edited

Loading

allencloud Oct 30, 2018

rudyfly Oct 30, 2018

allencloud Oct 30, 2018

rudyfly Oct 30, 2018

allencloud Oct 30, 2018

rudyfly Oct 30, 2018

starnop Oct 30, 2018

zhuangqh commented Oct 31, 2018

	Currently pouch provides four plugins，they are: `container plugin`，`daemon plugin`，`volume plugin`，`cri plugin`
	Currently pouch-container provides four plugins，they are: `container plugin`，`daemon plugin`，`volume plugin`，`cri plugin`.


		define two plugin symbols which only print some logs at correspond point:
		* pre-volume-create volume point, at this point you can change the create volume's config as you want, add your default volume's options.


		var ContainerPlugin ContPlugin
		* pre-create-container cri container point, at this point you can update the container config what it will be created, such as update the container's envs or labels.

	* pre-create-container cri container point, at this point you can update the container config what it will be created, such as update the container's envs or labels.
	* pre-create-container cri point, at this point you can update the container config to what it will be created, such as update the container's envs or labels.

	// PreStopHook stops plugin processes than start ed by PreStartHook.
	// PreStopHook stops plugin processes than started by PreStartHook.

		@@ -1,148 +1,159 @@
		# PouchContainer with plugin
		# Pouch-container with plugin

	// could the container's config in cri interface.
	// could update the container's config in cri interface.

refactor: refact the pouch hook plugin #2360

refactor: refact the pouch hook plugin #2360

Conversation

rudyfly commented Oct 25, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Oct 25, 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

zhuangqh commented Oct 29, 2018

allencloud Oct 30, 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

zhuangqh commented Oct 31, 2018

rudyfly commented Oct 25, 2018 •

edited

Loading

codecov bot commented Oct 25, 2018 •

edited

Loading

allencloud Oct 30, 2018 •

edited

Loading