Honor Network and IPAM plugin #1607

mavenugo · 2016-10-10T17:58:25Z

In Docker 1.12, swarmkit is hard-coded only to accept overlay network driver and builtin ipam driver. That practically eliminates all other global network and IPAM plugins.

This PR enables the network and ipam plugins be used with swarmkit as well.

mavenugo · 2016-10-10T17:59:22Z

ping @mrjana

aaronlehmann · 2016-10-10T18:27:09Z

@mavenugo: There is a unit test failure:

--- FAIL: TestAllocateRemoteDriver (30.01s)
    Error Trace:    networkallocator_test.go:163
            networkallocator_test.go:41
            networkallocator_test.go:176
    Error:      Received unexpected error "mkdir /etc/docker: permission denied"

mavenugo · 2016-10-10T18:36:24Z

@aaronlehmann the test fails due to privileged operation ( creating /etc/docker directory). But in order to test the plugin integration, we need that.

I added the failing test in this PR. If we cannot do privileged operation in the tests, then I would have to remove the test.

aaronlehmann · 2016-10-10T18:39:30Z

Would it be possible to rewrite this test as a Docker Engine integration test?

It's important that unit tests can run without root privileges, or touching files in /etc. I think it's really bad if running a unit test has side effects for your Docker installation.

The other thing I can think of is parameterizing this path in libnetwork, so that it doesn't have to be /etc/docker.

mavenugo · 2016-10-10T18:41:13Z

its not a libnetwork requirement. Its the plugin infrastructure requirement.

I could move this IT out of swarmkit and move it into docker engine. That will make it simpler.

codecov-io · 2016-10-10T18:58:44Z

Current coverage is 54.89% (diff: 64.70%)

Merging #1607 into master will increase coverage by 10.07%

@@             master      #1607   diff @@
==========================================
  Files            33         86     +53   
  Lines          4520      14195   +9675   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
+ Hits           2026       7793   +5767   
- Misses         2208       5375   +3167   
- Partials        286       1027    +741

Powered by Codecov. Last update dc3dec0...2176817

mavenugo · 2016-10-10T18:59:29Z

@aaronlehmann okay. that did it.

mrjana · 2016-10-11T15:18:32Z

manager/allocator/networkallocator/drivers_linux.go

+	"github.com/docker/libnetwork/drivers/remote"
+)
+
+func getInitializers() []initializer {


We need a definition of this for other non-linux OSes

mrjana · 2016-10-11T15:23:28Z

manager/allocator/networkallocator/drivers_ipam.go

+	remoteIpam "github.com/docker/libnetwork/ipams/remote"
+)
+
+func initIPAMDrivers(r *drvregistry.DrvRegistry, lDs, gDs interface{}) error {


We will never have the datastores passed to IPAM from swarmkit so it is better to not even have these args. Just pass nil to ipam Init functions

mrjana · 2016-10-11T15:25:44Z

manager/allocator/networkallocator/networkallocator.go

@@ -633,12 +626,26 @@ func (na *NetworkAllocator) resolveDriver(n *api.Network) (driverapi.Driver, str

 	d, _ := na.drvRegistry.Driver(dName)


We need to ignore all local scope drivers here since a plugin may be local scope and a user might mistakenly ask for it. I know it won't even come here because of the logic in docker/docker but it is better not to assume that and just ignore local scope drivers here.

mavenugo · 2016-10-11T16:01:55Z

@mrjana PR updated with your comments addressed.

mrjana · 2016-10-11T16:25:37Z

manager/allocator/networkallocator/networkallocator.go

@@ -633,12 +627,31 @@ func (na *NetworkAllocator) resolveDriver(n *api.Network) (driverapi.Driver, str

 	d, _ := na.drvRegistry.Driver(dName)


We need to check even in the case where the driver is already in driver registry because the current logic will only check the first time when the plugin is not loaded. Subsequently when the same driver name is used it will not check for the scope since it is already there in driver registry. I'd suggest moving the scope checking logic out of the d == nil { clause below and do it afterward so that it applies for both first time plugin init and subsequent references.

good catch. fixed it. PTAL

Signed-off-by: Madhu Venugopal <madhu@docker.com>

mrjana

LGTM

mavenugo · 2016-10-11T23:47:45Z

ping @anusha-ragunathan Can you please review this as well ?

EDIT : BTW, Based on your comment in my docker/docker changes, I get we should not be using github.com/docker/docker/pkg/plugins package anymore. But swarmkit doesnt seem to be enabled for plugin-v2 yet (similar to your work in libnetwork) ?

mavenugo · 2016-10-12T15:18:27Z

I checked with @anusha-ragunathan and she confirmed that plugin-v2 support is not included in swarmkit yet. So, I guess we can go with the current changes and we can update the import along with the plugin-v2 support.

aaronlehmann · 2016-10-12T18:05:24Z

This is causing TestAllocator to fail for me on master:

=== RUN   TestAllocator
WARN[0000] Unable to locate plugin: overlay, retrying in 1s
WARN[0001] Unable to locate plugin: overlay, retrying in 2s
WARN[0003] Unable to locate plugin: overlay, retrying in 4s
--- FAIL: TestAllocator (5.00s)
        allocator_test.go:485: timed out before watchNetwork found expected network state
FAIL
exit status 1
FAIL    github.com/docker/swarmkit/manager/allocator    5.017s

As of moby/swarmkit#1607, swarmkit honors global network plugins while allocating network resources. This IT covers the e2e integration between libnetwork, swarmkit and docker engine to support global network-plugins for swarm-mode Signed-off-by: Madhu Venugopal <madhu@docker.com>

GordonTheTurtle added the status/0-triage label Oct 10, 2016

mavenugo force-pushed the pluginv2-sk3 branch from 166fded to f5cbf61 Compare October 10, 2016 18:51

mavenugo mentioned this pull request Oct 11, 2016

Allow multiple handlers to support network plugins in swarm-mode moby/moby#27287

Merged

mrjana suggested changes Oct 11, 2016

View reviewed changes

mavenugo force-pushed the pluginv2-sk3 branch from f5cbf61 to e110509 Compare October 11, 2016 16:00

mrjana suggested changes Oct 11, 2016

View reviewed changes

mavenugo added 2 commits October 11, 2016 15:02

Honor Network and IPAM plugin in "network create"

5da65cd

Signed-off-by: Madhu Venugopal <madhu@docker.com>

Vendoring libnetwork to bring in dependent changes

2176817

Signed-off-by: Madhu Venugopal <madhu@docker.com>

mavenugo force-pushed the pluginv2-sk3 branch from e110509 to 2176817 Compare October 11, 2016 22:02

mrjana approved these changes Oct 11, 2016

View reviewed changes

mrjana merged commit 1fed8d2 into moby:master Oct 12, 2016

mavenugo mentioned this pull request Oct 22, 2016

Network Drivers support #1330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Honor Network and IPAM plugin #1607

Honor Network and IPAM plugin #1607

mavenugo commented Oct 10, 2016

mavenugo commented Oct 10, 2016

aaronlehmann commented Oct 10, 2016

mavenugo commented Oct 10, 2016

aaronlehmann commented Oct 10, 2016

mavenugo commented Oct 10, 2016

codecov-io commented Oct 10, 2016 •

edited

Loading

mavenugo commented Oct 10, 2016

mrjana Oct 11, 2016

mavenugo Oct 11, 2016

mrjana Oct 11, 2016

mavenugo Oct 11, 2016

mrjana Oct 11, 2016

mavenugo Oct 11, 2016

mavenugo commented Oct 11, 2016

mrjana Oct 11, 2016

mavenugo Oct 11, 2016

mrjana left a comment

mavenugo commented Oct 11, 2016 •

edited

Loading

mavenugo commented Oct 12, 2016

aaronlehmann commented Oct 12, 2016

		@@ -633,12 +626,26 @@ func (na NetworkAllocator) resolveDriver(n api.Network) (driverapi.Driver, str

		d, _ := na.drvRegistry.Driver(dName)

		@@ -633,12 +627,31 @@ func (na NetworkAllocator) resolveDriver(n api.Network) (driverapi.Driver, str

		d, _ := na.drvRegistry.Driver(dName)

Honor Network and IPAM plugin #1607

Honor Network and IPAM plugin #1607

Conversation

mavenugo commented Oct 10, 2016

mavenugo commented Oct 10, 2016

aaronlehmann commented Oct 10, 2016

mavenugo commented Oct 10, 2016

aaronlehmann commented Oct 10, 2016

mavenugo commented Oct 10, 2016

codecov-io commented Oct 10, 2016 • edited Loading

Current coverage is 54.89% (diff: 64.70%)

mavenugo commented Oct 10, 2016

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 Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrjana left a comment

Choose a reason for hiding this comment

mavenugo commented Oct 11, 2016 • edited Loading

mavenugo commented Oct 12, 2016

aaronlehmann commented Oct 12, 2016

codecov-io commented Oct 10, 2016 •

edited

Loading

mavenugo commented Oct 11, 2016 •

edited

Loading