Allow multiple handlers to support network plugins in swarm-mode #27287
Allow multiple handlers to support network plugins in swarm-mode #27287aaronlehmann merged 3 commits intomoby:masterfrom
Conversation
daemon/network.go
Outdated
There was a problem hiding this comment.
We should stop importing pkg/plugins since that's pluginv1 only. Use of pkg/plugingetter with an object that implements the PluginGetter interface such as daemon.pluginStore is the right way to get plugins v2 and fallback to v1.
There was a problem hiding this comment.
I'll look into how this import rule can be enforced at build time.
There was a problem hiding this comment.
Got it. thanks @anusha-ragunathan . That also answers @mrjana's question on the expectations.
daemon/network.go
Outdated
There was a problem hiding this comment.
daemon.pluginStore implements plugingetter. Use GetAllByCap()
pkg/plugins/plugins.go
Outdated
There was a problem hiding this comment.
The pluginv2 equivalent is in the plugin enable() -> CallHandler() path. https://github.com/docker/docker/blob/master/plugin/store/store_experimental.go. This along with the handler definition at https://github.com/docker/docker/blob/master/plugin/store/defs.go#L18 needs to be updated.
mavenugo
force-pushed
the
pluginv2-sk2
branch
from
b7427ae to
d4abb6d
Compare
There's additional activation call to Plugin due to the added GetAll() in Handle and GetAllByCap() in GetNetworkDriverList. |
|
TestDockerNetworkConnectDisconnectToStoppedContainer panic: |
|
goroutine hangs when the newly added GetAll() in Handle waits on activation. I was able to repro the TestDockerNetworkConnectDisconnectToStoppedContainer panic. Relevant backtrace (got from kill USR1 , not the jenkins log) |
|
@mavenugo : TestDockerNetworkConnectDisconnectToStoppedContainer hang is because GetAll() activates the plugin, but there's no Broadcast() to wake up the goroutine. Handle can simply access the already populated storage.plugins array instead. |
|
GetNetworkDriverList still suffers from this problem due to call to GetAllByCap. We can potentially have a method that simply accesses |
mavenugo
force-pushed
the
pluginv2-sk2
branch
from
d4abb6d to
89701a3
Compare
Signed-off-by: Madhu Venugopal <[email protected]>
|
@anusha-ragunathan thanks for the pointers. I managed to resolve the issue without having to use the GetAll API. But I couldn't narrow down on exactly what is going on with GetAll & activate. Either we need a different API as you suggested : #27287 (comment) or resolve the funkiness around lazy loading. I think we should handle that independently. Now that moby/swarmkit#1607 is merged, I realized that it will be good to bring in swarmkit along with this PR and add IT to validate the behavior introduced by this PR. |
mavenugo
force-pushed
the
pluginv2-sk2
branch
from
89701a3 to
bc2d809
Compare
mavenugo
changed the title
mavenugo
changed the title
mavenugo
changed the title
|
@mrjana @aaronlehmann can u PTAL @ the swarmkit vendoring and the IT for e2e global network plugin support in swarm-mode ? @anusha-ragunathan as discussed, other failures are resolved now. I opened #27411 to add Plugin-v2 specific IT (which is unrelated to this PR). |
|
@mavenugo : What about resetting activation as discussed before? https://github.com/docker/docker/pull/24172/files#r69201630 |
Currently the plugins pkg allows a single handler. This assumption breaks down if there are mutiple listeners to a plugin of a certain Manifest such as NetworkDriver or IpamDriver when swarm-mode is enabled. Signed-off-by: Madhu Venugopal <[email protected]>
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 <[email protected]>
mavenugo
force-pushed
the
pluginv2-sk2
branch
from
bc2d809 to
af185a3
Compare
|
@anusha-ragunathan yes. that is still applicable. I missed that case while updating the patch. It is fixed now. PTAL. BTW, I did observe some funkiness with GetAllByCap() & GetAll() APIs. I will work with you offline to understand the intended behavior. |
|
LGTM |
|
ping @vieux PTAL |
|
also @dongluochen fyi. this completes the global-scoped network plugin (v1) support for swarm-mode. |
|
LGTM ping @aaronlehmann @aluzzardi no know issue with this swarmkit vendoring ? |
| handlers = append(handlers, fn) | ||
| extpointHandlers[iface] = handlers | ||
| for _, p := range storage.plugins { | ||
| p.activated = false |
There was a problem hiding this comment.
Question: why all the plugins should be deactivated here?
There was a problem hiding this comment.
There was a problem hiding this comment.
That makes sense. Thanks.
|
sgtm (not a maintainer) |
|
LGTM Windows failure looks unrelated. Merging. |
7 participants
Currently the plugins pkg allows a single handler. This assumption
breaks down if there are mutiple listeners to a plugin of a certain
Manifest such as NetworkDriver or IpamDriver when swarm-mode is enabled.
Now that we have libnetwork remote APIs supporting AllocateNetwork & FreeNetwork APIs which is required for swarm-mode support and moby/swarmkit#1607 is merged, this PR is a revisit of #24172.
Also vendoring swarmkit to bring in moby/swarmkit#1607 to support global network-plugin in swarm-mode
Signed-off-by: Madhu Venugopal [email protected]