Skip to content

Only filter on plugins if running with docker engine#2192

Merged
aaronlehmann merged 1 commit intomoby:masterfrom
ijc:no-plugin-filter-if-no-plugin-getter
May 25, 2017
Merged

Only filter on plugins if running with docker engine#2192
aaronlehmann merged 1 commit intomoby:masterfrom
ijc:no-plugin-filter-if-no-plugin-getter

Conversation

@ijc
Copy link
Copy Markdown
Contributor

@ijc ijc commented May 18, 2017 •
edited
Loading

If there is no engine description then this assumes that all drivers etc are built-in and thus always available, and when they are not proper errors will be generated and propagated.

This avoids the need in #1965 to populate a spurious api.NodeDescription.Engine field (spurious because there is no engine in this case).

Signed-off-by: Ian Campbell ian.campbell@docker.com

The api.NodeDescription.Engine field is https://github.com/ijc25/swarmkit/blob/42f853a3c262666e29d4c0bd3ea301d5e736bc82/agent/exec/containerd/executor.go#L93...L101 in the current PR (but I expect that link to break when I next force push there) it is the executor.Describe method and is just:

		// Possibly this should have a Containerd entry
		// instead. There is some code which
		// `desc.Engine.Plugins` to be non-nil see
		// `PluginFilter.Check` in
		// `manager/scheduler/filter.go`
		Engine: &api.EngineDescription{
			Labels:  map[string]string{},
			Plugins: []api.PluginDescription{},
		},

@codecov
Copy link
Copy Markdown

codecov bot commented May 18, 2017 •
edited
Loading

Codecov Report

Merging #2192 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2192      +/-   ##
==========================================
- Coverage   60.06%   59.99%   -0.08%     
==========================================
  Files         119      119              
  Lines       19847    19849       +2     
==========================================
- Hits        11921    11908      -13     
- Misses       6568     6583      +15     
  Partials     1358     1358

@aaronlehmann
Copy link
Copy Markdown
Collaborator

aaronlehmann commented May 18, 2017

As an alternative solution, I would suggest changing (*PluginFilter).Check to return true (bypassing the filter) if n.Description.Engine == nil. This would avoid manipulating the list of filters, and also avoid making the decision based on whether a PlugginGetter is available on the leader node. Would this work for you?

@ijc
Copy link
Copy Markdown
Contributor Author

ijc commented May 18, 2017

@aaronlehmann That sounds much better and I think ought to work just fine, I'll make the change.

@ijc ijc force-pushed the no-plugin-filter-if-no-plugin-getter branch from 5daf3be to 0c955be Compare May 18, 2017 18:24
@ijc ijc changed the title Only filter on plugins if a plugin getter is provided. Only filter on plugins if running with docker engine May 18, 2017
@ijc
Copy link
Copy Markdown
Contributor Author

ijc commented May 18, 2017

Force pushed (a much smaller change now!) and update title + head comment to reflect new commit message.

aaronlehmann
aaronlehmann reviewed May 18, 2017
View reviewed changes
manager/scheduler/filter.go Outdated
// Check returns true if the task can be scheduled into the given node.
// TODO(amitshukla): investigate storing Plugins as a map so it can be easily probed
func (f *PluginFilter) Check(n *NodeInfo) bool {
if n.Description.Engine == nil {
Copy link
Copy Markdown
Collaborator

@aaronlehmann aaronlehmann May 18, 2017

Choose a reason for hiding this comment

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

Let's change this to if n.Description == nil || n.Description.Engine == nil.

It looks like not checking for nil here was an oversight that would currently allow a misbehaving or malicious worker to crash the manager.

Copy link
Copy Markdown
Contributor Author

@ijc ijc May 18, 2017

Choose a reason for hiding this comment

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

Done.

@ijc ijc force-pushed the no-plugin-filter-if-no-plugin-getter branch from 0c955be to 38a9fca Compare May 18, 2017 18:33
aaronlehmann
aaronlehmann reviewed May 18, 2017
View reviewed changes
manager/scheduler/filter.go Outdated
if n.Description == nil || n.Description.Engine == nil {
// If we are not running on engine then there is no
// plugin getter and no plugins, assume everything
// must be built-in.
Copy link
Copy Markdown
Collaborator

@aaronlehmann aaronlehmann May 18, 2017

Choose a reason for hiding this comment

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

"If the node is not running Engine, plugins are not supported."

@aaronlehmann
Copy link
Copy Markdown
Collaborator

aaronlehmann commented May 18, 2017

Is it correct that this is temporary behavior to support #1965? I imagine that eventually we will want to validate the drivers for volumes and networks against some variant of Description which doesn't exist yet? If this is correct, it might be good to add a comment explaining what will change to make the plugin filter operate correctly once the information is available.

@ijc ijc force-pushed the no-plugin-filter-if-no-plugin-getter branch from 38a9fca to 9636223 Compare May 18, 2017 18:38
Only filter on plugins if running with docker engine.
58aab16 
If there is no engine description then thus assumes that all drivers etc are
built-in and this always available, and when they are not proper errors will be
generated and propagated.

This avoids the need in moby#1965 to
populate a spurious `api.NodeDescription.Engine` field (spurious because there
is no engine in this case).

Signed-off-by: Ian Campbell <ian.campbell@docker.com>
@ijc ijc force-pushed the no-plugin-filter-if-no-plugin-getter branch from 9636223 to 58aab16 Compare May 18, 2017 18:42
@ijc
Copy link
Copy Markdown
Contributor Author

ijc commented May 18, 2017

@aaronlehmann that's what I was trying to get at with "built-in and this always available, and when they are not proper errors will be generated and propagated." (oops, s/this/thus/) in the commit message.

Right now in my (unpublished) CNI POC based on the #1965 work the network plugin here is "cni" (at @al's suggestion), so there isn't much to check. The actual cni backend/plugin name is in the CNI JSON config blob, which for the most part of opaque to us, although the relevant field type is one of the ones I think it would valid for us to inspect. If we did that then we would be able to provide the listing of /opt/cni/bin on each node in the corresponding info and check against that (that wouldn't catch e.g. using an ipam plugin as a network plugin or vice versa, but would be better than nothing).

I haven't thought much about storage yet, but if we guess we might go the csi route I would expect something similar there.

I had vaguely considered adding a method to the executor interface (or a new interface which the executor could implement) and plumbing the executor through to here so it could be asked, but I guess that falls afoul of using the leader's executor to answer node specific questions.

@aaronlehmann
Copy link
Copy Markdown
Collaborator

aaronlehmann commented May 18, 2017

LGTM

1 similar comment
@dongluochen
Copy link
Copy Markdown
Contributor

dongluochen commented May 23, 2017

LGTM

@aaronlehmann aaronlehmann merged commit 4a52495 into moby:master May 25, 2017
@ijc ijc deleted the no-plugin-filter-if-no-plugin-getter branch May 26, 2017 09:07
@ijc ijc mentioned this pull request May 31, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaronlehmann aaronlehmann aaronlehmann 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.

3 participants

@ijc @aaronlehmann @dongluochen