Make libnetwork understand pluginv2. #1472

anusha-ragunathan · 2016-09-26T22:02:05Z

As part of daemon init, network and ipam drivers are passed a
pluginstore object that implements the plugin/getter interface. Use this
interface methods in libnetwork to interact with network plugins. This
interface provides the new and improved pluginv2 functionality and falls
back to pluginv1 (legacy) if necessary.

Signed-off-by: Anusha Ragunathan anusha@docker.com

anusha-ragunathan · 2016-09-26T22:08:56Z

Companion PR to moby/moby#26860.

mavenugo · 2016-09-26T22:16:59Z

controller.go

@@ -163,7 +165,7 @@ type initializer struct {
 }

 // New creates a new instance of network controller.
-func New(cfgOptions ...config.Option) (NetworkController, error) {
+func New(getter getter.PluginGetter, cfgOptions ...config.Option) (NetworkController, error) {


getter.PluginGetter can be one of the config.Option. That will save the New API from changing.

Agree, please see if you can pass the plugin getter via a config option setter.

mavenugo · 2016-09-26T22:19:16Z

drivers/bridge/bridge.go

@@ -144,7 +144,7 @@ func newDriver() *driver {
 }

 // Init registers a new instance of bridge driver
-func Init(dc driverapi.DriverCallback, config map[string]interface{}) error {
+func Init(dc driverapi.DriverCallback, ps interface{}, config map[string]interface{}) error {


why is this an interface{} ? why cant it be the concrete type ?

plugingetter is only used in remote driver code. So it can be abstracted from other code paths. i.e bridge code doesnt need to know about a plugingetter, since it doesnt use it.

got it. Can you instead use the above DriverCallback for this purpose ?
You can return the plugingetter as a GetPluginGetter API as part of DriverCallback.
BTW, Controller implements DriverCallback and hence you can get this done cleanly without having to change the Init API signature.

PluginGetter naturally falls in DriverRegistry rather than the Controller. I've made necessary updates.

mavenugo · 2016-09-26T22:21:05Z

drivers/remote/driver.go

+			}
+		})
+	} else {
+		plugins.Handle(driverapi.NetworkPluginEndpointType, func(name string, client *plugins.Client) {


The callback function seems identical. Can you pls reuse it instead of duplicating it here ?

mavenugo · 2016-09-26T22:21:30Z

ipams/remote/remote.go

+		})
+	} else {
+		plugins.Handle(ipamapi.PluginEndpointType, func(name string, client *plugins.Client) {
+			a := newAllocator(name, client)


mavenugo · 2016-09-26T22:25:41Z

@anusha-ragunathan thanks for the vendor updates. Are these changes done to be in sync with docker/docker ?

anusha-ragunathan · 2016-09-26T22:26:59Z

@mavenugo : vendor update has 2 fold reason. 1. get the docker/docker/plugin/getter package 2. syncup with docker/docker

mavenugo

Thanks for the vendor update again :-)

@aboch can u pls check if the vendoring work is fine ?

mavenugo · 2016-09-26T22:32:01Z

drivers/bridge/bridge.go

@@ -144,7 +144,7 @@ func newDriver() *driver {
 }

 // Init registers a new instance of bridge driver
-func Init(dc driverapi.DriverCallback, config map[string]interface{}) error {
+func Init(dc driverapi.DriverCallback, ps interface{}, config map[string]interface{}) error {


got it. Can you instead use the above DriverCallback for this purpose ?
You can return the plugingetter as a GetPluginGetter API as part of DriverCallback.
BTW, Controller implements DriverCallback and hence you can get this done cleanly without having to change the Init API signature.

anusha-ragunathan · 2016-09-26T22:44:51Z

CI failure on

--- FAIL: TestCreateMultipleNetworks (0.13s)
    bridge_test.go:468: iptables failed: iptables --wait -nvL DOCKER-ISOLATION: iptables: No chain/target/match by that name.

The test is not on the gh open issues and looks unrelated to the PR. I can open an issue if necessary.

mavenugo · 2016-09-27T02:10:10Z

@anusha-ragunathan thats alright. can u pls address the comments ? I will check what is going on with the test failure.

aboch · 2016-09-27T02:49:42Z

@anusha-ragunathan
Please take care of these failures:

./macvlan_test.go:34: not enough arguments in call to Init
./macvlan_test.go:41: not enough arguments in call to Init
./macvlan_test.go:52: not enough arguments in call to Init

Run make to run all the tests offline.

anusha-ragunathan · 2016-09-27T15:33:53Z

@aboch : I did, before opening the PR, but ran into errors modprobing ipvsadm although the package is installed and ip_vs is modprobed. A strange error where the test container cannot find modprobe and ipvsadm in $PATH, although its present. I'm debugging that right now.

anusha-ragunathan · 2016-09-27T15:55:21Z

There were 2 libnetwork test containers that were stuck causing modprobe, ipvsadm PATH errors. Removing those fixed the issue, but now modprobe is unable to open modules.dep.bin. Rebooting host doesnt help. File exists on host.

$ ls -als /lib/modules/3.19.0-6 
8-generic/modules.dep.bin                                                                              
604 -rw-r--r-- 1 root root 618268 Sep 10 10:13 /lib/modules/3.19.0-68-generic/modules.dep.bin

RUN TestGetFamily
--- FAIL: TestGetFamily (0.00s)
Location: ipvs_test.go:124
Error: No error is expected but got no such file or directory

=== RUN TestService
WARN[0000] Could not load necessary modules for IPSEC rules: Running modprobe xfrm_user failed with message: modprobe: ERROR: ../libkmod/libkmod.c:557 kmod_search_moddep() could not open moddep file '/lib/modules/3.19.0-68-generic/modules.dep.bin', error: exit status 1
WARN[0000] Running modprobe ip_vs failed with message: modprobe: ERROR: ../libkmod/libkmod.c:557 kmod_search_moddep() could not open moddep file '/lib/modules/3.19.0-68-generic/modules.dep.bin', error: exit status 1
ERRO[0000] Could not get ipvs family information from the kernel. It is possible that ipvs is not enabled in your kernel. Native loadbalancing will not work until this is fixed.
--- FAIL: TestService (0.01s)
Location: ipvs_test.go:91
Error: No error is expected but got exit status 127

=== RUN TestDestination
WARN[0000] Could not load necessary modules for IPSEC rules: Running modprobe xfrm_user failed with message: modprobe: ERROR: ../libkmod/libkmod.c:557 kmod_search_moddep() could not open moddep file '/lib/modules/3.19.0-68-generic/modules.dep.bin', error: exit status 1
--- FAIL: TestDestination (0.04s)
Location: ipvs_test.go:91
Error: No error is expected but got exit status 127

aboch · 2016-09-27T16:04:49Z

@anusha-ragunathan
~~try first to export CIRCLECI then run make~~

For now, do not worry for the local make failures, just take care of the compiling issues in macvlan test code, and re-push.

If you want to run the CI locally, run make circle-ci

aboch · 2016-09-27T16:31:13Z

drivers/remote/driver.go

-	})
+func Init(dc driverapi.DriverCallback, ps interface{}, config map[string]interface{}) error {
+	// Unit test code is unaware of a true PluginStore. So we fall back to v1 plugins.
+	if ps != nil {


Please define a local function pointer which will point to ps.(plugingetter.PluginGetter) if ps != nil, plugins.Handle() otherwise

aboch · 2016-09-27T16:36:31Z

drvregistry/drvregistry.go

@@ -37,7 +37,7 @@ type DrvRegistry struct {
 // Functors definition

 // InitFunc defines the driver initialization function signature.
-type InitFunc func(driverapi.DriverCallback, map[string]interface{}) error
+type InitFunc func(driverapi.DriverCallback, interface{}, map[string]interface{}) error


Would it make sense to have the plugin getter be a member of DrvRegistry ?
At the end all the remote drivers register with the registry, he can hold the plugin getter for them to be called in the init function.

Yes, DrvRegistry is where PluginGetter belongs. Updated.

aboch · 2016-09-27T16:38:28Z

controller.go

@@ -192,12 +195,12 @@ func New(cfgOptions ...config.Option) (NetworkController, error) {
 			dcfg = c.makeDriverConfig(i.ntype)
 		}

-		if err := drvRegistry.AddDriver(i.ntype, i.fn, dcfg); err != nil {
+		if err := drvRegistry.AddDriver(i.ntype, i.fn, dcfg, c.plugingetter); err != nil {


If you store the plugin getter in the drvRegistry, AddDriver() does prototype does not need to change

Done. I've also split the PR into godep vendoring and plugin related code, so its easier to review.

Signed-off-by: Anusha Ragunathan <anusha@docker.com>

aboch · 2016-09-27T21:24:02Z

controller.go

@@ -1074,7 +1078,7 @@ func (c *controller) loadDriver(networkType string) error {
 }

 func (c *controller) loadIPAMDriver(name string) error {
-	if _, err := plugins.Get(name, ipamapi.PluginEndpointType); err != nil {
+	if _, err := c.plugingetter.Get(name, ipamapi.PluginEndpointType, getter.LOOKUP); err != nil {


Here we can modify to c.driverRegistry.GetPluginGetter().

c has to implement a GetPluginGetter to satisy DriverAPICallback which is a wrapper to drvRegistry.GetPluginGetter(). So this will be c.GetPluginGetter()

aboch · 2016-09-27T21:24:18Z

controller.go

@@ -154,6 +155,7 @@ type controller struct {
 	agentInitDone          chan struct{}
 	keys                   []*types.EncryptionKey
 	clusterConfigAvailable bool
+	plugingetter           getter.PluginGetter


I think we no longer need this member in controller, rather we can just keep it confined to drvRegistry

aboch · 2016-09-27T21:27:07Z

drivers/remote/driver.go

@@ -29,7 +29,15 @@ func newDriver(name string, client *plugins.Client) driverapi.Driver {
 // Init makes sure a remote driver is registered when a network driver
 // plugin is activated.
 func Init(dc driverapi.DriverCallback, config map[string]interface{}) error {
-	plugins.Handle(driverapi.NetworkPluginEndpointType, func(name string, client *plugins.Client) {
+	var handleFunc func(string, func(string, *plugins.Client))


I think you could just rewrite the whole block as:

handleFunc := plugins.Handle if pg := dc.GetPluginGetter(); pg != nil { handleFunc = pg.Handle }

aboch · 2016-09-27T21:28:07Z

drvregistry/drvregistry.go

+	ipamDrivers  ipamTable
+	dfn          DriverNotifyFunc
+	ifn          IPAMNotifyFunc
+	plugingetter getter.PluginGetter


Please follow the camel case convention as in ipamDrivers

Yikes! Done.

aboch · 2016-09-27T21:29:09Z

ipams/remote/remote.go

@@ -30,7 +30,15 @@ func newAllocator(name string, client *plugins.Client) ipamapi.Ipam {

 // Init registers a remote ipam when its plugin is activated
 func Init(cb ipamapi.Callback, l, g interface{}) error {
-	plugins.Handle(ipamapi.PluginEndpointType, func(name string, client *plugins.Client) {
+	var handleFunc func(string, func(string, *plugins.Client))


Same here, you should be able to reduce the footprint of this change, make it less verbose.

anusha-ragunathan · 2016-09-27T22:04:04Z

CI failure on #1385

As part of daemon init, network and ipam drivers are passed a pluginstore object that implements the plugin/getter interface. Use this interface methods in libnetwork to interact with network plugins. This interface provides the new and improved pluginv2 functionality and falls back to pluginv1 (legacy) if necessary. Signed-off-by: Anusha Ragunathan <anusha@docker.com>

aboch · 2016-09-27T22:54:24Z

LGTM

mavenugo · 2016-09-28T02:11:53Z

@anusha-ragunathan thanks for taking care of all the comments. LGTM

GordonTheTurtle added the status/0-triage label Sep 26, 2016

anusha-ragunathan mentioned this pull request Sep 26, 2016

Make libnetwork use pluginv2. moby/moby#26860

Merged

anusha-ragunathan force-pushed the plugins branch 2 times, most recently from a392cf7 to a18957e Compare September 26, 2016 22:17

mavenugo reviewed Sep 26, 2016

View reviewed changes

anusha-ragunathan force-pushed the plugins branch from a18957e to c626c8a Compare September 26, 2016 22:32

mavenugo reviewed Sep 26, 2016

View reviewed changes

aboch reviewed Sep 27, 2016

View reviewed changes

Godep changes.

25df221

Signed-off-by: Anusha Ragunathan <anusha@docker.com>

anusha-ragunathan force-pushed the plugins branch 4 times, most recently from 90c8a63 to 2fcee1c Compare September 27, 2016 21:21

aboch reviewed Sep 27, 2016

View reviewed changes

anusha-ragunathan force-pushed the plugins branch from 2fcee1c to fa0a5a1 Compare September 27, 2016 21:28

aboch reviewed Sep 27, 2016

View reviewed changes

anusha-ragunathan force-pushed the plugins branch from fa0a5a1 to 9e8e375 Compare September 27, 2016 21:54

anusha-ragunathan force-pushed the plugins branch from 9e8e375 to 7b26d7e Compare September 27, 2016 22:29

mavenugo merged commit d34488a into moby:master Sep 28, 2016

anusha-ragunathan deleted the plugins branch September 29, 2016 16:24

Make libnetwork understand pluginv2. #1472

Make libnetwork understand pluginv2. #1472

Conversation

anusha-ragunathan commented Sep 26, 2016

anusha-ragunathan commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo Sep 26, 2016 • 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

mavenugo commented Sep 26, 2016

anusha-ragunathan commented Sep 26, 2016

mavenugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusha-ragunathan commented Sep 26, 2016 • edited Loading

mavenugo commented Sep 27, 2016

aboch commented Sep 27, 2016

anusha-ragunathan commented Sep 27, 2016

anusha-ragunathan commented Sep 27, 2016

aboch commented Sep 27, 2016 • 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

Choose a reason for hiding this comment

anusha-ragunathan commented Sep 27, 2016

aboch commented Sep 27, 2016

mavenugo commented Sep 28, 2016

mavenugo Sep 26, 2016 •

edited

Loading

anusha-ragunathan commented Sep 26, 2016 •

edited

Loading

aboch commented Sep 27, 2016 •

edited

Loading