8000 Explicitly disallow network pluginv1 creation in swarm mode. by anusha-ragunathan · Pull Request #1894 · moby/swarmkit · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Explicitly disallow network pluginv1 creation in swarm mode. #1894

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 25, 2017
Merged

Explicitly disallow network pluginv1 creation in swarm mode. #1894

merged 1 commit into from
Jan 25, 2017

Conversation

anusha-ragunathan
Copy link
Contributor

In swarm mode, only network pluginv2 is supported; pluginv1 is not. When
the plugin support was re-enabled in 1.13.x, it inadvertently allowed
both pluginv1 and pluginv2. This commit fixes that.

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

@anusha-ragunathan
Copy link
Contributor Author

@mavenugo PTAL

@mavenugo
Copy link
Contributor

LGTM

@aaronlehmann can you PTAL and also re-trigger the CI ?

dongluochen
dongluochen reviewed Jan 24, 2017
View reviewed changes
manager/controlapi/common.go
@@ -87,5 +88,14 @@ func validateDriver(driver *api.Driver) error {
return grpc.Errorf(codes.InvalidArgument, "driver name: if driver is specified name is required")
}

p, err := pg.Get(driver.Name, pluginType, plugingetter.LOOKUP)
if err != nil {
return grpc.Errorf(codes.InvalidArgument, "error during plugin lookup")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better to provide driver.Name for debugging.

manager/controlapi/common.go
}

if p.IsV1() {
return grpc.Errorf(codes.InvalidArgument, "legacy plugins of type %s are not supported in swarm mode", pluginType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide driver.Name in error message.

@codecov-io
Copy link

Current coverage is 55.33% (diff: 40.00%)

Merging #1894 into bump_v1.13.1 will decrease coverage by 2.92%

@@           bump_v1.13.1      #1894   diff @@
==============================================
  Files                58        102     +44   
  Lines              8927      17061   +8134   
  Methods               0          0           
  Messages              0          0           
  Branches              0          0           
==============================================
+ Hits               5201       9441   +4240   
- Misses             3160       6475   +3315   
- Partials            566       1145    +579

Sunburst

Powered by Codecov. Last update 0692326...5b73846

@aaronlehmann
Copy link
Collaborator

Why aren't v1 plugins supported? Is it just because the scheduler's plugin filter won't work correctly for these, or is there another reason? Does it apply to volume plugins as well?

nishanttotla
nishanttotla reviewed Jan 24, 2017
View reviewed changes
Copy link
Contributor
@nishanttotla nishanttotla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A node could have a v1 plugin running before it joins the Swarm, and it will also pass through the plugin filter IIUC. Is this not the case?

@anusha-ragunathan
Copy link
Contributor Author
anusha-ragunathan commented Jan 24, 2017 •
edited
Loading

@aaronlehmann : Its specific to network plugins. moby/moby#29878 and moby/moby#30332 (comment) has some details. @mavenugo made the decision and I'll let him explain.

@mavenugo
Copy link
Contributor
mavenugo commented Jan 24, 2017 •
edited
Loading

@aaronlehmann there are a few technical reasons and a few maintenance reasons.
There are a few pre-req details of the current design/situation in order to undestand the issue.

  1. Plugin-v1 loads lazily. Only when the first network or volume is actually created on the daemon, the plugin is loaded.
  2. As of 1.13.0, Swarmkit supports only Global-scoped network drivers and Local-scoped volume drivers.
  3. Plugin filtering logic in swarmkit during service creation will filter out the nodes that doesnt have a particular plugin loaded.

The combination of the above 3 causes the technical issue at hand. When docker network create is done using a global-scoped network driver/plugin, it is handled just by the swarm manager at swarmkit and it retains the information in the raftstore. Only when the service is created with the said network, the network is scheduled to be created on a node where the task is scheduled. But since plugin-v1 is lazy loading, the plugin is not loaded on any node until the network is created. And since the filtering logic doesnt choose a node without the said plugin loaded, we end up in this dead heat. In order to solve this deadlock, we tried to use GetAllByCap() in SystemInfo path of daemon. But this function doesnt work well in this code path due to the way plugin-v1 is implemented and integrated with daemon and it failed on a few IT cases. Hence we decided not to use plugin-v1 to be pulled in SystemInfo.

From a maintenance standpoint, plugin-v1 had many daemon issues in the past and the major one was the fact that plugins can be loaded later than the actual container being used and it caused major headaches. Also, the plugins can be out of the box and can cause more issues. So, plugin-v2 is a welcome change.

Since 1.13.1 is the first time we are planning on supporting plugins in swarmkit, we feel that there is no point carrying an implementation that must be deprecated asap. Also, plugin-v2 is the recommended approach. Since it doesnt break any backward compatibility (being the first release of plugin support), I vote to not support plugin-v1 with swarmkit.

@aaronlehmann
Copy link
Collaborator

Thanks for the explanation. It makes technical sense to me now why v1 network plugins don't really work with swarmkit, but v1 volume plugins may work in some cases (if you happen to precreate the volumes on the nodes, or do something else to get the plugin to load).

However, based on this:

Since 1.13.1 is the first time we are planning on supporting plugins in swarmkit, we feel that there is no point carrying an implementation that must be deprecated asap. Also, plugin-v2 is the recommended approach. Since it doesnt break any backward compatibility (being the first release of plugin support), I vote to not support plugin-v1 with swarmkit.

...I wonder if it makes sense to disallow v1 volume plugins as well. It might be confusing that services that use v1 volume plugins only get scheduled to nodes that have the plugin loaded because of some external action, and it also might be confusing that v1 volume plugins are "supported" but v1 network plugins are not.

Then again, v1 volume plugins technically work in 1.13.0 (if you work around the plugin filter issues), so maybe removing that support in a patch release is not reasonable.

@anusha-ragunathan
Copy link
Contributor Author

Yes, it is inconsistent (volume pluginv1 and network pluginv1 in swarm mode), but we should leave volume pluginv1 in swarm mode untouched in 1.13.x.

@anusha-ragunathan
Explicitly disallow network pluginv1 creation in swarm mode.
0b0e2a4 
In swarm mode, only network pluginv2 is supported; pluginv1 is not. When
the plugin support was re-enabled in 1.13.x, it inadvertently allowed
both pluginv1 and pluginv2. This commit fixes that.

Signed-off-by: Anusha Ragunathan <anusha.ragunathan@docker.com>
@aaronlehmann
Copy link
Collaborator

LGTM

1 similar comment
@dongluochen
Copy link
Contributor

LGTM

@dongluochen dongluochen merged commit b9f07cb into moby:bump_v1.13.1 Jan 25, 2017
@mavenugo mavenugo mentioned this pull request Jan 25, 2017
Merged
@anusha-ragunathan anusha-ragunathan deleted the disallow_nw_pv1 branch January 25, 2017 23:56
yongtang added a commit to yongtang/docker that referenced this pull request Jan 29, 2017
@yongtang
Update SwarmKit to 78ae345
849816d 
This fix updates SwarmKit to 78ae345
(from 037b491)

The following issues in docker are related
- Can not update service in host publish mode (moby#30199) (fixed)
- Add `ReadonlyRootfs` in ContainerSpec for `--read-only` (moby#29972) (needed)
- Explicitly disallow network pluginv1 creation in swarm mode
  (See discussion in moby/swarmkit/pull/1899, moby/swarmkit/pull/1894,
  and moby/pull/30332#issuecomment-274277948)

This fix fixes moby#30199

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
This was referenced Jan 29, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nishanttotla nishanttotla nishanttotla left review comments

@dongluochen dongluochen dongluochen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@anusha-ragunathan @mavenugo @codecov-io @aaronlehmann @dongluochen @nishanttotla
0