Skip to content
This repository has been archived by the owner on Oct 13, 2023. It is now read-only.

[17.06] Make plugin removes more resilient to failure #91

Merged
merged 3 commits into from
Jul 12, 2017
Merged

[17.06] Make plugin removes more resilient to failure #91

merged 3 commits into from
Jul 12, 2017

Conversation

andrewhsu
Copy link
Contributor

@andrewhsu andrewhsu commented Jul 4, 2017 •
edited
Loading

Backport fixes:

@andrewhsu andrewhsu changed the title Make plugin removes more resilient to failure [17.06] Make plugin removes more resilient to failure Jul 4, 2017
thaJeztah
thaJeztah approved these changes Jul 4, 2017
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a 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

@thaJeztah
Copy link
Member

thaJeztah commented Jul 4, 2017 •
edited
Loading

ugh, actually looks like it needs to be rewritten for 17.06, or another PR cherry-picked;

03:41:11 [init] Building: bundles/17.06.0-ce/binary-daemon/dockerd-17.06.0-ce
03:41:58 [init] # github.com/docker/docker/plugin
03:41:58 [init] plugin/manager.go:194: undefined: system in system.EnsureRemoveAll
03:42:10 [init] $ ssh-agent -k

@thaJeztah
Copy link
Member

thaJeztah commented Jul 4, 2017 •
edited
Loading

This depends on moby/moby#31012, which was on the 17.06 milestone, but actually missing in the branch / release

never mind, I seemed to have looked in the wrong "component", looks like it's there, it's just an import missing

@andrewhsu
Copy link
Contributor Author

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.

cpuguy83
cpuguy83 reviewed Jul 4, 2017
View reviewed changes
components/engine/plugin/backend_linux.go Outdated
return errors.Wrap(err, "error performing atomic remove of plugin dir")
}

if err := system.EnsureRemoveAll(pluginDir); err != nil {
Copy link
Contributor

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"

Copy link
Contributor

Choose a reason for hiding this comment

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

moby/moby#33944

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@rogaha
Copy link
Contributor

rogaha commented Jul 6, 2017

@andrewhsu I think you need to include moby/moby@4bf263c here (see #91 (comment)).

@andrewhsu
Copy link
Contributor Author

i've cherry-picked the git commit from moby/moby#33944

$ git cherry-pick -s -x -Xsubtree=components/engine 4bf263c

cpuguy83
cpuguy83 approved these changes Jul 6, 2017
View reviewed changes
Copy link
Contributor

@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

rogaha
rogaha approved these changes Jul 7, 2017
View reviewed changes
Copy link
Contributor

@rogaha rogaha left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

ping @andrewhsu ready to go?

@andrewhsu andrewhsu mentioned this pull request Jul 11, 2017
Open
@rogaha
Copy link
Contributor

rogaha commented Jul 11, 2017

ping @andrewhsu

cpuguy83 and others added 3 commits July 11, 2017 22:51
@cpuguy83 @andrewhsu
Make plugin removes more resilient to failure
73b8fa7 
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>
@andrewhsu
import system for plugin/manager.go
3218b57 
To get the cherry-pick ba42966 to merge smoothly,
expecting import of system. The import of system was added by
8508f49 but not taking
entire commit.

Signed-off-by: Andrew Hsu <andrewhsu@docker.com>
@cpuguy83 @andrewhsu
Fix plugin remove dir name after rename.
371fed7 
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit 4bf263c)
Signed-off-by: Andrew Hsu <andrewhsu@docker.com>
@andrewhsu andrewhsu merged commit b2eb133 into docker-archive:17.06 Jul 12, 2017
@andrewhsu andrewhsu deleted the fix-plugin branch July 12, 2017 01:37
@andrewhsu andrewhsu modified the milestone: 17.06.1 Jul 12, 2017
@thaJeztah thaJeztah mentioned this pull request Jul 24, 2017
Merged
docker-jenkins pushed a commit that referenced this pull request Mar 17, 2018
@seemethere
Merge pull request #91 from seemethere/new_ver_scheme
e4c18f1 
Enable new test versioning scheme
Upstream-commit: 27d07d7
Component: packaging
silvin-lubecki pushed a commit to silvin-lubecki/docker-ce that referenced this pull request Jan 30, 2020
@seemethere
Merge pull request docker-archive#91 from seemethere/new_ver_scheme
05becc6 
Enable new test versioning scheme
Upstream-commit: 27d07d7
Component: packaging
silvin-lubecki pushed a commit to silvin-lubecki/docker-ce that referenced this pull request Jan 30, 2020
@seemethere
Merge pull request docker-archive#91 from seemethere/new_ver_scheme
27d07d7 
Enable new test versioning scheme
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@rogaha rogaha rogaha approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
17.06.1
Development

Successfully merging this pull request may close these issues.
4 participants
@andrewhsu @thaJeztah @rogaha @cpuguy83