Skip to content

Avoid running plugin filter when Log Driver does not have a Name#2438

Merged
dperny merged 1 commit intomoby:masterfrom
nishanttotla:log-driver-plugin-filter
Nov 10, 2017
Merged

Avoid running plugin filter when Log Driver does not have a Name#2438
dperny merged 1 commit intomoby:masterfrom
nishanttotla:log-driver-plugin-filter

Conversation

@nishanttotla
Copy link
Copy Markdown
Contributor

When a LogDriver is provided without a name, as part of the service spec, the plugin filter fails to find a suitable node. This PR fixes that behavior, by ignoring the filtering for log drivers when the name isn't provided.

Fix #2392

@nishanttotla
Copy link
Copy Markdown
Contributor Author

Ping @cpuguy83 @anshulpundir @dperny

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 10, 2017

Codecov Report

Merging #2438 into master will decrease coverage by 3.38%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2438      +/-   ##
=========================================
- Coverage   63.89%   60.5%   -3.39%     
=========================================
  Files          64     128      +64     
  Lines       11788   26407   +14619     
=========================================
+ Hits         7532   15978    +8446     
- Misses       3649    9019    +5370     
- Partials      607    1410     +803

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@nishanttotla nishanttotla force-pushed the log-driver-plugin-filter branch from 13e1c97 to 407dc40 Compare November 10, 2017 21:01
Copy link
Copy Markdown
Contributor

@anshulpundir anshulpundir left a comment

Choose a reason for hiding this comment

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

just a minor nit

}

if f.t.Spec.LogDriver != nil && f.t.Spec.LogDriver.Name != "none" {
if f.t.Spec.LogDriver != nil && (f.t.Spec.LogDriver.Name != "none" || f.t.Spec.LogDriver.Name != "") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a comment for this conditional

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

@nishanttotla nishanttotla force-pushed the log-driver-plugin-filter branch from 407dc40 to 61676ab Compare November 10, 2017 21:45
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@nishanttotla nishanttotla force-pushed the log-driver-plugin-filter branch from 61676ab to 05c8675 Compare November 10, 2017 21:46
@nishanttotla
Copy link
Copy Markdown
Contributor Author

@andrewhsu @thaJeztah is it too late to get this fix into 17.11? It should be a single line cherry pick.

Copy link
Copy Markdown
Collaborator

@dperny dperny left a comment

Choose a reason for hiding this comment

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

LGTM

@dperny dperny merged commit 6951306 into moby:master Nov 10, 2017
@nishanttotla nishanttotla deleted the log-driver-plugin-filter branch November 10, 2017 22:10
@thaJeztah
Copy link
Copy Markdown
Member

@nishanttotla would that be a bump of swarmkit to master, or do we have to cherry-pick into https://github.com/docker/swarmkit/tree/bump_v17.11?

If the latter; please prepare a cherry-pick, so that we can act fast if it's still possible; I opened an internal tracking issue (you should see it linked above)

@nishanttotla
Copy link
Copy Markdown
Contributor Author

@thaJeztah it would be a cherry-pick, not a bump. I'll open a cherry-pick right away regardless, even if we can't take it in.

@thaJeztah
Copy link
Copy Markdown
Member

Just discussed and it's too late for 17.11, but can be considered backporting to supported releases (if other more critical issues warrant a patch release)

@vide
Copy link
Copy Markdown

vide commented Nov 13, 2017

IMO this should be a backport for 17.09 stable because basically it makes impossible to use Swarm with syslog logging, and it's a regression from 17.06

@nishanttotla
Copy link
Copy Markdown
Contributor Author

@vide I'll create the cherry-pick for that.

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.

no suitable node (missing plugin on 3 nodes)

6 participants