Skip to content

Make sure plugin rootfs is unmounted on upgrade#32525

Merged
vdemeester merged 1 commit intomoby:masterfrom
cpuguy83:ensure_unmount_plugin
Apr 12, 2017
Merged

Make sure plugin rootfs is unmounted on upgrade#32525
vdemeester merged 1 commit intomoby:masterfrom
cpuguy83:ensure_unmount_plugin

Conversation

@cpuguy83
Copy link
Copy Markdown
Member

In some cases, if a user specifies -f when disabling a plugin mounts
can still exist on the plugin rootfs.
This can cause problems during upgrade where the rootfs is removed and
may cause data loss.

To resolve this, ensure the rootfs is unmounted
before performing an upgrade.

Fixes #32515

download

@cpuguy83 cpuguy83 changed the title Make sure plugin rootfs is unmounted on upgraded Make sure plugin rootfs is unmounted on upgrade Apr 11, 2017
Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

@anusha-ragunathan
Copy link
Copy Markdown
Contributor

@cpuguy83 : That is no cute animal.

@thaJeztah
Copy link
Copy Markdown
Member

That is no cute animal.

Ok, let's close

Comment thread plugin/manager_linux.go Outdated
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.

log the error and continue trying to unmount other mounts?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't want to do that because we really can't proceed if there is anything mounted.
This is what we do (sort of) when disabling the plugin, but for upgrade we really need to error out.

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.

Looks like recursiveUnmount can be used. Also, not related to this PR, but https://github.com/cpuguy83/docker/blob/564839a8d8958ecc06600c8f730955c89cc32626/plugin/backend_linux.go#L651 error message should not include while performing recursive unmount

Comment thread plugin/manager_linux.go Outdated
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.

typo: "If the anything" -> "If anything"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️ classic case of starting the setance one way and finishing it another :(

@cpuguy83 cpuguy83 force-pushed the ensure_unmount_plugin branch from 564839a to 59bffd2 Compare April 12, 2017 01:08
In some cases, if a user specifies `-f` when disabling a plugin mounts
can still exist on the plugin rootfs.
This can cause problems during upgrade where the rootfs is removed and
may cause data loss.

To resolve this, ensure the rootfs is unmounted
before performing an upgrade.

Signed-off-by: Brian Goff <[email protected]>
@cpuguy83 cpuguy83 force-pushed the ensure_unmount_plugin branch from 59bffd2 to 83f44d2 Compare April 12, 2017 01:09
@cpuguy83
Copy link
Copy Markdown
Member Author

Updated

@anusha-ragunathan
Copy link
Copy Markdown
Contributor

DockerSwarmSuite.TestAPISwarmLeaderProxy is failing on powerpc. Stacktrace at https://gist.github.com/anusha-ragunathan/81d151b187e1ec4266e08b8a699ed4ed

@anusha-ragunathan
Copy link
Copy Markdown
Contributor

LGTM (CI failures are unrelated.)

@vdemeester vdemeester merged commit aa92df7 into moby:master Apr 12, 2017
@cpuguy83 cpuguy83 deleted the ensure_unmount_plugin branch April 12, 2017 16:08
@thaJeztah thaJeztah added this to the 17.06 milestone Apr 14, 2017
@thaJeztah
Copy link
Copy Markdown
Member

@cpuguy83 @anusha-ragunathan should this be cherry-picked into 17.05?

@anusha-ragunathan
Copy link
Copy Markdown
Contributor

anusha-ragunathan commented Apr 17, 2017

@thaJeztah : If its not too late, that would be nice.

@thaJeztah thaJeztah modified the milestones: 17.05.0, 17.06 Apr 17, 2017
@thaJeztah
Copy link
Copy Markdown
Member

ping @vieux FYI ^^

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.

The data lost when do docker plugin upgrade action if the container is running

6 participants