Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use GetAllByCap to get list of network plugins in docker info #29877

Merged
merged 1 commit into from Jan 4, 2017

Conversation

mavenugo
Copy link
Contributor

@mavenugo mavenugo commented Jan 4, 2017

returned in "docker info". Currently info endpoint isnt using the
GetAllByCap, but relies on existing networks to get the plugin names.
This causes a basic issue when it comes to global network plugins which
swarm-mode relies on, wherein swarmkit will not be able to schedule the
network on the worker nodes due to the filtering logic.

In order to break this chicken & egg issue, we must start to use the
GetAllManagedPluginsByCap. We are unable to use GetAllByCap due to
various issues with Plugin-V1's lazy loading approach causing issues
especially during daemon restarts (which uses SystemInfo)

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

@mavenugo mavenugo added this to the 1.13.0 milestone Jan 4, 2017
@mavenugo
Copy link
Contributor Author

mavenugo commented Jan 4, 2017

@mavenugo
Copy link
Contributor Author

mavenugo commented Jan 4, 2017

@cpuguy83 idk how this PR causes the 2 failures (TestAuthZPluginBadManifestFailsDaemonStart and TestExternalVolumeDriverRetryNotImmediatelyExists). It seems call to getAllByCap still is not safe ?

returned in "docker info". Currently info endpoint isnt using the
GetAllByCap, but relies on existing networks to get the plugin names.
This causes a basic issue when it comes to global network plugins which
swarm-mode relies on, wherein swarmkit will not be able to schedule the
network on the worker nodes due to the filtering logic.

In order to break this chicken & egg issue, we must start to use the
GetAllManagedPluginsByCap. We are unable to use GetAllByCap due to
various issues with Plugin-V1's lazy loading approach causing issues
especially during daemon restarts (which uses SystemInfo)

Signed-off-by: Madhu Venugopal <madhu@docker.com>
@mavenugo
Copy link
Contributor Author

mavenugo commented Jan 4, 2017

@cpuguy83 as discussed, since GetAllByCap cannot be reliably used, I decided to make use of the V2 only GetAllManagedPluginsByCap. Since plugin support is being introduced in swarm-mode for the first time in 1.13, it is appropriate to use the plugin-V2 API instead of dealing with the plugin-v1 support for swarm-mode.

cpuguy83 added a commit to cpuguy83/docker that referenced this pull request Jan 4, 2017
This makes the test a bit more robust to change and is a bit cleaner.
As implemented before this commit, we have two named plugins pointing to
the same http service. If the daemon makes any unexpected calls to the
plugin (e.g. during startup) we'll get more counts on the event counter
than expected since the daemon sees 2 plugins.

Found this while working on moby#29877 which broke this test originally (but
is no longer using V1 plugins, so is this is no longer broken there) and
took some time to debug what was going on.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
Copy link
Member

cpuguy83 commented Jan 4, 2017

LGTM.
Fine with only supporting v2 for swarm mode.
I'm assuming here that we have sufficient test coverage with v1 network plugins to make sure this doesn't break anything?

@tiborvass
Copy link
Contributor

LGTM

vieux added a commit that referenced this pull request Jan 4, 2017
Cherry-pick #29877 : Use GetAllByCap to get list of network plugins in docker info
@tiborvass tiborvass merged commit 4617b66 into moby:master Jan 4, 2017
@anusha-ragunathan
Copy link
Contributor

I'm not sure if its a good idea to ignore v1 network plugins in swarm mode. moby/swarmkit#1808 enabled swarm filtering support for both v1 and v2. Did 1.12 ever support plugin filters? And if yes, wouldnt this PR cause a regression? cc @aaronlehmann @aluzzardi

@aaronlehmann
Copy link
Contributor

Did 1.12 ever support plugin filters?

No, it was disabled before release. The problem was that plugins were lazy-loaded, so the list of plugins wasn't useful for scheduling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants