Don't sort plugin mounts slice #36711

cpuguy83 · Mar 27, 2018

This was added as part of a53930a with
the intent to sort the mounts in the plugin config, but this was sorting
all the mounts from the default OCI spec which is problematic.

In reality we don't need to sort this because we are only adding a
self-binded mount to flag it as rshared.

We may want to look at sorting the plugin mounts before they are added
to the OCI spec in the future, but for now I think the existing behavior
is fine since the plugin author has control of the order (except for the
propagated mount).

Fixes #36698

anusha-ragunathan · Mar 27, 2018

integration/plugin/volumes/mounts_test.go

Just for my knowledge. How does the new test framework run on specific OSes? For example, how does it achieve the old DaemonIsLinux condition?

This is handled here: https://github.com/moby/moby/pull/36711/files#diff-ead2a878e85c5d12126b9757bff4cf69R24

And yes I totally added that after you said something :)

Also removed this check (which was a copy/paste from logging) since we don't need it here.

anusha-ragunathan · Mar 27, 2018

integration/plugin/volumes/mounts_test.go

assert.NilError

This is a copy-paste from the logging package... I don't know why it was changed to assert.Check... @dnephin ?

I guess assert.Check just returns ok if the passed in value is nil. If it's not nil there's some switch on the type, and error types would fail the test.

Yes, that's right. The automated migration converted testify/assert.NoError (which uses t.Fail()) to assert.Check() (which uses t.Fail()) because those have the equivalent behaviour. Checking errors with t.Fail() is generally going to be wrong and lead to a panic later in the test when something else is nil. So the original test was wrong to use testify/assert.NoError, and should have used testify/require.NoError.

gotestyourself/assert.NilError(t, err) is correct because it uses t.FailNow() and stops execution of the test.

I didn't fix every place that was using the wrong assertion, because there were many, but I tried to fix up a few.

codecov · Mar 27, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@b6a7d02). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36711   +/-   ##
=========================================
  Coverage          ?   34.96%           
=========================================
  Files             ?      613           
  Lines             ?    45556           
  Branches          ?        0           
=========================================
  Hits              ?    15928           
  Misses            ?    27541           
  Partials          ?     2087

anusha-ragunathan · Mar 27, 2018

integration/plugin/volumes/helpers_test.go

Would be nice to add a function comment about expectations:
"DummyPluginSrc at cmd/dummy
DummyPluginInstallPath at $GOPATH/bin/dummy"

Not in this PR, but we should have a common set of helpers for all plugins, instead of duplicating code.

anusha-ragunathan · Mar 27, 2018

integration/plugin/volumes/helpers_test.go

What is this lock protecting?

It makes sure that tests can run in parallel and that we only build a specific binary once.

anusha-ragunathan · Mar 27, 2018

plugin/v2/plugin_linux.go

For my clarification: So including the OCI defaults in the sort was overwriting the explicit /dev mounts from the plugin with the OCI defaults? Even so, how did it mess up the host /dev?

Because it's adding things like a custom /dev/pts for the plugin, but if the host /dev is mounted in first this causes issues.

anusha-ragunathan · Mar 27, 2018

integration-cli/fixtures/plugin/plugin.go

Janky panicked due to test case where m.Source is nil.

moby/integration-cli/docker_cli_plugins_test.go

Line 175 in 8aa694c

{Name: "pmount2", Settable: []string{"source"}, Type: "none"}, // Mount without source is invalid.

Please check for m.Source != nil

vdemeester · Mar 28, 2018

@cpuguy83 validation failures 😓

01:46:10 integration/plugin/volumes/helpers_test.go:25:15:warning: GetEnv not declared by package os (gosimple)
01:46:10 integration/plugin/volumes/helpers_test.go:26:3:warning: ineffectual assignment to goPath (ineffassign)

This was added as part of a53930a with the intent to sort the mounts in the plugin config, but this was sorting *all* the mounts from the default OCI spec which is problematic. In reality we don't need to sort this because we are only adding a self-binded mount to flag it as rshared. We may want to look at sorting the plugin mounts before they are added to the OCI spec in the future, but for now I think the existing behavior is fine since the plugin author has control of the order (except for the propagated mount). Signed-off-by: Brian Goff <cpuguy83@gmail.com>

anusha-ragunathan · Mar 28, 2018

LGTM

vdemeester

LGTM 🐯

cpuguy83 requested a review from vdemeester as a code owner March 27, 2018 18:14

GordonTheTurtle added the status/0-triage label Mar 27, 2018

cpuguy83 requested a review from anusha-ragunathan March 27, 2018 18:17

cpuguy83 mentioned this pull request Mar 27, 2018

Plugin starts in 17.03, but won't start in 18.03 (no such file or directory) #36698

Closed

cpuguy83 added status/2-code-review and removed status/0-triage labels Mar 27, 2018

anusha-ragunathan reviewed Mar 27, 2018

View reviewed changes

cpuguy83 force-pushed the plugin_mounts_sorting branch from 75ba560 to 63501cc Compare March 27, 2018 21:11

cpuguy83 force-pushed the plugin_mounts_sorting branch from 63501cc to 4c550ef Compare March 27, 2018 21:19

anusha-ragunathan reviewed Mar 27, 2018

View reviewed changes

cpuguy83 force-pushed the plugin_mounts_sorting branch from 4c550ef to a8b4ddd Compare March 28, 2018 01:36

cpuguy83 force-pushed the plugin_mounts_sorting branch 2 times, most recently from 39315c2 to fab5a51 Compare March 28, 2018 12:35

cpuguy83 force-pushed the plugin_mounts_sorting branch from fab5a51 to ec90839 Compare March 28, 2018 13:10

anusha-ragunathan added the impact/changelog label Mar 28, 2018

vdemeester approved these changes Mar 28, 2018

View reviewed changes

anusha-ragunathan merged commit 18d1688 into moby:master Mar 28, 2018

This was referenced Apr 4, 2018

[18.03] Don't sort plugin mounts slice docker-archive/docker-ce#490

Merged

[18.04] Don't sort plugin mounts slice docker-archive/docker-ce#491

Merged

cpuguy83 deleted the plugin_mounts_sorting branch April 4, 2018 20:15

This was referenced Apr 4, 2018

CRITICAL!!! docker remove /dev/* on the host docker/for-linux#272

Closed

[volume plugin] CRITICAL!!! docker remove /dev/* on the host #36784

Closed

Search code, repositories, users, issues, pull requests...

Don't sort plugin mounts slice #36711

Don't sort plugin mounts slice #36711

Uh oh!

Conversation

cpuguy83 commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Mar 28, 2018

Uh oh!

anusha-ragunathan commented Mar 28, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cpuguy83 commented Mar 27, 2018 •

edited

Loading

dnephin Mar 27, 2018 •

edited

Loading

codecov bot commented Mar 27, 2018 •

edited

Loading