-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[17.06] Make plugin removes more resilient to failure #91
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ping @cpuguy83 @tiborvass
ugh, actually looks like it needs to be rewritten for 17.06, or another PR cherry-picked;
|
never mind, I seemed to have looked in the wrong "component", looks like it's there, it's just an import missing |
Just added one more commit to this PR to get the import system line in. Looks like that import line was added by 8508f49 but don't want to take the entire commit. |
return errors.Wrap(err, "error performing atomic remove of plugin dir") | ||
} | ||
|
||
if err := system.EnsureRemoveAll(pluginDir); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:( I'm an idiot... EnsureRemoveAll
won't work here because we're using the old path. This won't get cleaned up until daemon restart.
We need pluginDir+"-removing"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpuguy83 is it required to be part of this patch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@andrewhsu I think you need to |
i've cherry-picked the git commit from moby/moby#33944
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ping @andrewhsu ready to go? |
ping @andrewhsu |
Before this patch, if the plugin's `config.json` is successfully removed but the main plugin state dir could not be removed for some reason (e.g. leaked mount), it will prevent the daemon from being able to be restarted. This patches changes this to atomically remove the plugin such that on daemon restart we can detect that there was an error and re-try. It also changes the logic so that it only logs errors on restore rather than erroring out the daemon. This also removes some code which is now duplicated elsewhere. Signed-off-by: Brian Goff <cpuguy83@gmail.com> (cherry picked from commit 11cf394) Signed-off-by: Andrew Hsu <andrewhsu@docker.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com> (cherry picked from commit 4bf263c) Signed-off-by: Andrew Hsu <andrewhsu@docker.com>
Enable new test versioning scheme Upstream-commit: 27d07d7 Component: packaging
Enable new test versioning scheme Upstream-commit: 27d07d7 Component: packaging
Enable new test versioning scheme
Backport fixes: