[WIP] Re-enable plugin filter #1517

nishanttotla · 2016-09-09T14:39:18Z

Fix #1231. It was disabled in #1224, due to lazy loading by the engine.

Using the plugins API, it is possible to list installed plugins.

One issue is that we don't have a way of storing arbitrary plugins in the TaskSpec, from which to match them against the node on which PluginFilter is applied. For volumes and networks, this happens via a separate code path that allows for attaching to networks or mounting volumes. This is something we might want to build to support general plugins, and will require proto changes. This PR will work for volume and network v2 plugins.

Signed-off-by: Nishant Totla nishanttotla@gmail.com

codecov-io · 2016-09-09T14:45:46Z

Current coverage is 55.12% (diff: 0.00%)

Merging #1517 into master will decrease coverage by 0.01%

@@             master      #1517   diff @@
==========================================
  Files           102        102          
  Lines         16940      16945     +5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           9340       9341     +1   
+ Misses         6438       6434     -4   
- Partials       1162       1170     +8

Powered by Codecov. Last update 4e3982b...1651614

aaronlehmann · 2016-09-09T18:51:32Z

Isn't the issue that there's a catch-22? I'm not sure my memory is correct, but I thought requiring a certain plugin wouldn't schedule the task on a node until the plugin was loaded on that node, but the plugin might not get loaded until a task which uses it runs there.

nishanttotla · 2016-09-11T00:12:18Z

@aaronlehmann I wasn't aware of this catch-22 situation. I was under the impression that a plugin is loaded externally on a node (say by an operator) and has nothing to do with SwarmKit. I'll try to get this clarified.

nishanttotla · 2016-09-13T10:06:13Z

@stevvooe @mrjana what do you think about this change?

stevvooe · 2016-09-13T19:25:32Z

The preconditions for adding this back are in the TODO:

We can add this back when we can schedule plugins in the future.

I think the problem is that there are cases where the plugin isn't reported until it is actually in use.

What testing have we done to confirm this works?

aluzzardi · 2016-09-17T01:14:59Z

Welp @tiborvass @vieux

nishanttotla · 2016-09-20T00:23:18Z

After some discussion with @vieux, here's an update:
In the experimental part of the Docker API, we have a function called PluginList, which lists all installed plugins on the engine (loaded or not). This is what we want.

There are two caveats though

PluginList will only list v2 plugins, there is no way to get a list of v1 plugins that haven't been loaded. So as long as users aren't using v1 plugins, we're good.
The v2 plugin functionality is still experimental, so nodes will need to be running engine with the experimental flag. We could either wait for this to get out of experimental, or else tell users that if they want plugin filter to work, they will have to run engines with experimental flag.

WDYT @aluzzardi @stevvooe @aaronlehmann?

nishanttotla · 2016-09-20T15:42:18Z

cc @anusha-ragunathan

anusha-ragunathan · 2016-09-20T21:39:53Z

@nishanttotla : PluginsV2 can be queried using PluginList API endpoint. Also pluginsV2 will be stable in 1.13, so you dont need to worry about experimental support only.

nishanttotla · 2016-09-20T21:47:20Z

@anusha-ragunathan thanks! Is there a PR to track, that's moving PluginsV2 out of experimental? We can then update this PR and merge after that one is.

anusha-ragunathan · 2016-09-20T21:53:36Z

@nishanttotla : There's no PR yet, but an issue tracking it. moby/moby#26760

nishanttotla · 2016-10-13T21:14:41Z

Also related: #1545

nishanttotla · 2016-11-11T20:16:46Z

Picking this up again, after moby/moby#28226 was merged.

I will update this PR using PluginList to list plugins instead of relying on docker info. This will make installed plugins available. We will need to vendor in docker/docker for that though. Should I do it, or will it cause other issues? cc @aaronlehmann @LK4D4 @mrjana

anusha-ragunathan · 2016-11-14T16:16:55Z

In 1.13, we will support pluginv1 (where plugins are queried via docker info) as well as pluginv2 (where plugins are queried via docker plugin list). So the correct approach in swarm would be to query both, rather than use pluginv2 alone.

nishanttotla · 2016-11-14T17:24:43Z

@anusha-ragunathan thanks for the heads up. In the case of Swarm mode though, v1 plugins are tricky because they won't show up in docker info unless they're actually loaded/running, but the scheduler will not schedule until the plugin actually is running (which won't happen unless a task uses it).

So we might have to stick to v2 plugins for Swarm mode.

anusha-ragunathan · 2016-11-14T17:41:05Z

What about the case where the v1 plugin was loaded and running before the node joined the swarm? Node had some containers using the plugin. So plugin was loaded and running and docker info shows the plugin. Then node joins a swarm. For such a node, we should honor plugin v1.

nishanttotla · 2016-11-14T18:22:14Z

@anusha-ragunathan you're right, I hadn't considered that case. Fortunately, it won't be hard to support both, so I will update the PR accordingly.

nishanttotla · 2016-11-28T18:17:41Z

Are these test failures related? @LK4D4

aaronlehmann · 2016-11-28T19:11:06Z

Looks similar to #1662

It is unrelated.

aluzzardi · 2016-11-29T22:18:54Z

agent/exec/container/executor.go

 	addPlugins("Volume", info.Plugins.Volume)
 	// Add builtin driver "overlay" (the only builtin multi-host driver) to
 	// the plugin list by default.
 	addPlugins("Network", append([]string{"overlay"}, info.Plugins.Network...))
 	addPlugins("Authorization", info.Plugins.Authorization)

+	// retrieve v2 plugins


/cc @tiborvass @vieux @anusha-ragunathan

Could you please take a look at this logic?

aluzzardi · 2016-11-29T22:23:50Z

@nishanttotla This needs testing (at the very least manual). e.g. install a plugin on a machine and then deploy a service using that plugin (and make sure it gets deployed ONLY to that machine)

anusha-ragunathan · 2016-11-29T22:34:39Z

agent/exec/container/executor.go

+	for _, plgn := range v2plugins {
+		for _, typ := range plgn.Config.Interface.Types {
+			plugins[api.PluginDescription{
+				Type: typ.String(),


typ.String() will return something of the format docker/VolumeDriver:1.0. However, test code in scheduler_test.go: L1404 still sets the type to be Volume. Where are the changes in swarmkit to accomodate this new format?

Is this new microformat documented anywhere?

Do plugins no long have a type?

@stevvooe : Its documented @ https://github.com/docker/docker/blob/master/docs/extend/config.md#config-field-descriptions

nishanttotla · 2016-12-07T18:27:53Z

@tiborvass @stevvooe @anusha-ragunathan Updated the PR to save plugins based on driver type. PTAL.

tiborvass · 2016-12-07T18:42:27Z

agent/exec/container/executor.go

@@ -41,12 +41,35 @@ func (e *executor) Describe(ctx context.Context) (*api.NodeDescription, error) {
 		}
 	}

+	// add v1 plugins to 'plugins'
 	addPlugins("Volume", info.Plugins.Volume)


@anusha-ragunathan do we want to add v2 plugins to docker info or not? If we do, then info.Plugins.Volume will contain both built-in local driver, v1 plugins, and v2 plugins.

I dont feel strongly towards adding v2 plugins to docker info.

It feels weird not to add them imho. Why would only built-in drivers + v1 plugins appear there?

There's already a way to query them from docker using docker plugin ls and hence there's no reason to duplicate the info.

Right, but now there is a field called Plugins in docker info, that is essentially deprecated without being deprecated. Anyway, not sure what's the best solution.

The reason docker info historically had a Plugins section is because, there was no other way to query for that information through docker. Now that plugins are a first-class docker citizen, we should not continue down a legacy path of exposing that info.

We should document that Plugins in docker info lists only pluginsv1 and to query for pluginsv2 use docker plugin ls. When pluginsv1 is deprecated, we should consider removing Plugins from docker info.

tiborvass · 2016-12-07T22:06:21Z

agent/exec/container/executor.go

+	// add v2 plugins to 'plugins'
+	for _, plgn := range v2plugins {
+		for _, typ := range plgn.Config.Interface.Types {
+			plgnTyp := typ.Capability


you need to also verify that typ.Prefix == "docker"

tiborvass · 2016-12-07T22:09:28Z

agent/exec/container/executor.go

+		for _, typ := range plgn.Config.Interface.Types {
+			plgnTyp := typ.Capability
+			if typ.Capability == "volumedriver" {
+				plgnTyp = "Volume"


@nishanttotla what happens if we remove these ifs ? The capability is volumedriver. I actually wonder if it shouldn't even be the whole type: docker/volumedriver:1.0. cc @anusha-ragunathan

My understanding is that we want to rename it to Volume here because of this line that was shipped in 1.12: https://github.com/docker/swarmkit/pull/1517/files#diff-0c043559989dc54f88aaac9214b99260R45
which in turn was there because that's the nomenclature in docker info.

@tiborvass Not exactly. The way I understand, the reason is in filter.go. Notice the Check() function for PluginFilter

// Check returns true if the task can be scheduled into the given node. func (f *PluginFilter) Check(n *NodeInfo) bool { // Get list of plugins on the node nodePlugins := n.Description.Engine.Plugins // Check if all volume plugins required by task are installed on node container := f.t.Spec.GetContainer() if container != nil { for _, mount := range container.Mounts { if mount.VolumeOptions != nil && mount.VolumeOptions.DriverConfig != nil { if !f.pluginExistsOnNode("Volume", mount.VolumeOptions.DriverConfig.Name, nodePlugins) { return false } } } } // Check if all network plugins required by task are installed on node for _, tn := range f.t.Networks { if !f.pluginExistsOnNode("Network", tn.Network.DriverState.Name, nodePlugins) { return false } } return true }

In particular, I'm pointing out to the fact that volumes are stored as mounts, and networks stored separately, and we check if the corresponding Driver exists on that node. This is why I wanted to store with a simple "Volume" or "Network" key, instead of the longer form returned by type.String(). For other plugins, this PR is storing the long form version.

On that note, the issue that we don't have a way of storing arbitrary plugins in the TaskSpec, from which to match them against the node on which PluginFilter is applied. For volumes and networks, this happens via a separate code path that allows for attaching to networks or mounting volumes. This is something we might want to build to support general plugins, and will require proto changes. This PR will work for volume and network v2 plugins.

cc @stevvooe

stevvooe · 2016-12-07T23:51:33Z

I think there may be fundamental problems with the plugin data model that need to be worked out before going forward with this.

For example, the plugin list reported via NodeDescription is actually a list of drivers (we should probably rename this), but the new plugin model creates "capabilities". What are these capabilities? How do they work? I have seen a few examples in discussion but the results look foreign. Do we have a list of capabilities for a plugin?

For this to work properly, we really need a feedback loop of installation of a plugin to whatever the result is for a running plugin. This only lists the plugins and reports them, and doesn't provide any indication if they are the right state or the correct plugin.

anusha-ragunathan · 2016-12-08T01:16:03Z

agent/exec/container/executor.go

+
+	// add v2 plugins to 'plugins'
+	for _, plgn := range v2plugins {
+		for _, typ := range plgn.Config.Interface.Types {


We should be looking for enabled (aka running) plugins. For this, use plgn.Enabled.

This was addressed.

anusha-ragunathan · 2016-12-13T16:59:35Z

@nishanttotla @aluzzardi : Any updates on this? Are you blocked on something? Is there something I can help with. We need this PR for 1.13-rc5.

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

aaronlehmann · 2016-12-13T21:57:59Z

agent/exec/container/executor.go

+	v2plugins, err := e.client.PluginList(ctx)
+	if err != nil {
+		return nil, err
+	}


This should be changed not to make Describe error out if the error contains "plugins are not supported on this platform".

Actually, I'm going to make it ignore all errors. This will also fail against daemons that are too old to support this endpoint.

aaronlehmann · 2016-12-13T23:33:39Z

I am going to carry this PR. I've fixed some of the outstanding issues, but I've discovered a big problem that prevents this from being practically useful for volumes.

Creating a volume is supposed to be idempotent. Some parts of the engine code assume this, for example registerMountPoints. However, an inconsistency around tags causes problems here. If I try to create a volume using the driver vieux/sshfs, using the same parameters I used to create it before, it will fail because the volume is already associated with vieux/sshfs:latest. This is a big problem because it means I can't create a service using vieux/sshfs as a volume driver without including the latest tag.
...but including :latest doesn't work either. In this case, no suitable node will be found to run the service, because the vieux/sshfs plugin reported by the agent (based on the Name field) won't match vieux/sshfs:latest.

This is tricky to get right. We should be consistent in the way we refer to plugins. We shouldn't just strip the tag off, because that loses information. We also need to make sure that we handle other cases like name:tag@digest matching name@digest (since the tag is purely advisory in this case).

What we probably need as a starting point is a canonical function that compares plugin references, adding a default tag where necessary and handling digest cases properly. However, even this won't fix (1), since the volume code is where this comparison is happening, and it does a simple string comparison on the volume driver name, which already had the tag included. I'm not sure what the right solution to this is.

cc @anusha-ragunathan @vieux @dmcgowan

aaronlehmann · 2016-12-14T00:10:52Z

Discussed with @anusha-ragunathan and @tonistiigi and learned that digests will never be used in plugin naming. The canonical name of a plugin is name:tag. If no tag was specified at pull time, latest is implicit.

I will fix (1) in docker/docker by fixing the driver comparison in the volumes code.

I will fix (2) in this PR (or a carried version) by comparing tags in the plugin filter, and assuming latest if no tag is specified.

aaronlehmann · 2016-12-14T01:14:38Z

Carried in #1808

[Carry #1517] Support v2 plugins; re-enable scheduler plugin filter

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

nishanttotla force-pushed the enable-plugin-filter branch from 8bddc81 to fdce868 Compare September 16, 2016 18:52

nishanttotla force-pushed the enable-plugin-filter branch from fdce868 to 60c85b2 Compare September 19, 2016 21:00

nishanttotla added this to the 1.13.0 milestone Sep 20, 2016

nishanttotla force-pushed the enable-plugin-filter branch from 60c85b2 to 4b52408 Compare September 28, 2016 20:21

nishanttotla force-pushed the enable-plugin-filter branch from 4b52408 to 1525602 Compare October 18, 2016 21:43

aluzzardi added the priority/P1 label Oct 24, 2016

nishanttotla force-pushed the enable-plugin-filter branch from 1525602 to 95d591a Compare November 14, 2016 22:31

nishanttotla mentioned this pull request Nov 15, 2016

Updating vendoring for docker/docker #1752

Merged

nishanttotla force-pushed the enable-plugin-filter branch from 95d591a to c6abcf8 Compare November 16, 2016 21:45

nishanttotla force-pushed the enable-plugin-filter branch 2 times, most recently from e30d3b3 to b2ae871 Compare November 28, 2016 17:59

nishanttotla force-pushed the enable-plugin-filter branch from b2ae871 to 2ac4be6 Compare November 29, 2016 19:52

aluzzardi reviewed Nov 29, 2016

View reviewed changes

aluzzardi added the process/cherry-pick label Nov 29, 2016

aluzzardi removed the process/cherry-pick label Nov 29, 2016

anusha-ragunathan reviewed Nov 29, 2016

View reviewed changes

nishanttotla changed the title ~~Re-enable plugin filter~~ [WIP] Re-enable plugin filter Nov 29, 2016

nishanttotla force-pushed the enable-plugin-filter branch 4 times, most recently from b7d2cd2 to 9a5c348 Compare December 6, 2016 21:17

tiborvass reviewed Dec 7, 2016

View reviewed changes

nishanttotla force-pushed the enable-plugin-filter branch 2 times, most recently from 94a85f7 to 1651614 Compare December 7, 2016 23:10

anusha-ragunathan reviewed Dec 8, 2016

View reviewed changes

Re-enable plugin filter

d9fd705

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

nishanttotla force-pushed the enable-plugin-filter branch from 1651614 to d9fd705 Compare December 13, 2016 21:05

aaronlehmann reviewed Dec 13, 2016

View reviewed changes

aaronlehmann mentioned this pull request Dec 14, 2016

[Carry #1517] Support v2 plugins; re-enable scheduler plugin filter #1808

Merged

aaronlehmann closed this Dec 14, 2016

aaronlehmann added a commit that referenced this pull request Dec 14, 2016

Merge pull request #1808 from aaronlehmann/enable-plugin-filter

9e4bd71

[Carry #1517] Support v2 plugins; re-enable scheduler plugin filter

[WIP] Re-enable plugin filter #1517

[WIP] Re-enable plugin filter #1517

Conversation

nishanttotla commented Sep 9, 2016 • edited Loading

codecov-io commented Sep 9, 2016 • edited Loading

Current coverage is 55.12% (diff: 0.00%)

aaronlehmann commented Sep 9, 2016

nishanttotla commented Sep 11, 2016

nishanttotla commented Sep 13, 2016

stevvooe commented Sep 13, 2016

aluzzardi commented Sep 17, 2016

nishanttotla commented Sep 20, 2016 • edited Loading

nishanttotla commented Sep 20, 2016

anusha-ragunathan commented Sep 20, 2016

nishanttotla commented Sep 20, 2016

anusha-ragunathan commented Sep 20, 2016

nishanttotla commented Oct 13, 2016

nishanttotla commented Nov 11, 2016

anusha-ragunathan commented Nov 14, 2016

nishanttotla commented Nov 14, 2016

anusha-ragunathan commented Nov 14, 2016

nishanttotla commented Nov 14, 2016

nishanttotla commented Nov 28, 2016

aaronlehmann commented Nov 28, 2016

Choose a reason for hiding this comment

aluzzardi commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishanttotla commented Dec 7, 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

anusha-ragunathan Dec 7, 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

stevvooe commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusha-ragunathan commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Dec 13, 2016

aaronlehmann commented Dec 14, 2016

aaronlehmann commented Dec 14, 2016

nishanttotla commented Sep 9, 2016 •

edited

Loading

codecov-io commented Sep 9, 2016 •

edited

Loading

nishanttotla commented Sep 20, 2016 •

edited

Loading

anusha-ragunathan Dec 7, 2016 •

edited

Loading