Skip to content

Enforce zero plugin refcount during disable, not remove.#29599

Merged
anusha-ragunathan merged 1 commit intomoby:masterfrom
anusha-ragunathan:refcount
Dec 22, 2016
Merged

Enforce zero plugin refcount during disable, not remove.#29599
anusha-ragunathan merged 1 commit intomoby:masterfrom
anusha-ragunathan:refcount

Conversation

@anusha-ragunathan
Copy link
Contributor

@anusha-ragunathan anusha-ragunathan commented Dec 20, 2016

When plugins have a positive refcount, they were not allowed to be
removed. However, plugins could still be disabled when volumes
referenced it and containers using them were running. This change fixes
that by enforcing plugin refcount during disable and not remove.

Signed-off-by: Anusha Ragunathan anusha@docker.com

Part of fix for #29447

@anusha-ragunathan anusha-ragunathan added this to the 1.13.0 milestone Dec 20, 2016
@anusha-ragunathan
Copy link
Contributor Author

anusha-ragunathan commented Dec 20, 2016

cc @mavenugo @tiborvass

@anusha-ragunathan anusha-ragunathan mentioned this pull request Dec 20, 2016
Merged
@tiborvass
Copy link
Contributor

tiborvass commented Dec 20, 2016

LGTM

@cpuguy83
Copy link
Member

cpuguy83 commented Dec 20, 2016

I'm not sure this is right.
Having a plugin enabled does not necessarily mean it's resources are not available and I may want to disable a plugin if it's causing problems, or to test something, etc.

@cpuguy83
Copy link
Member

cpuguy83 commented Dec 20, 2016

For instance, in the docker plugin upgrade PR (which is of course not accepted yet, so grain of salt here...) I require that the plugin be disabled to call upgrade.

@anusha-ragunathan
Copy link
Contributor Author

anusha-ragunathan commented Dec 20, 2016

If the plugin is buggy, what good is it to have a reference to it?

Not having an enabled plugin, when there are resources (volumes, networks, etc) referencing them, is not a good model.

@cpuguy83
Copy link
Member

cpuguy83 commented Dec 20, 2016

@anusha-ragunathan I'm remembering my sysadmin days, monitoring the memory usage of a particularly leaky daemon that I'd have something automatically restart once it hit a certain limit while waiting for a patch to come in.

@anusha-ragunathan
Copy link
Contributor Author

anusha-ragunathan commented Dec 20, 2016

@cpuguy83 : Does it make sense to have a disable --force which ignores refcounts? Would that handle your use cases?

@cpuguy83
Copy link
Member

cpuguy83 commented Dec 20, 2016

@anusha-ragunathan I suppose it would, given that docker plugin rm would still fail without the force as well.

cpuguy83
cpuguy83 reviewed Dec 22, 2016
View reviewed changes
plugin/backend_linux.go Outdated
Copy link
Member

@cpuguy83 cpuguy83 Dec 22, 2016

Choose a reason for hiding this comment

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

Can we keep this refcount check?

Copy link
Contributor Author

@anusha-ragunathan anusha-ragunathan Dec 22, 2016

Choose a reason for hiding this comment

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

Done

@anusha-ragunathan
Enforce zero plugin refcount during disable.
8cb2229 
When plugins have a positive refcount, they were not allowed to be
removed. However, plugins could still be disabled when volumes
referenced it and containers using them were running.

This change fixes that by enforcing plugin refcount during disable.
A "force" disable option is also added to ignore reference refcounting.

Signed-off-by: Anusha Ragunathan <anusha@docker.com>
cpuguy83
cpuguy83 approved these changes Dec 22, 2016
View reviewed changes
Copy link
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

@anusha-ragunathan
Copy link
Contributor Author

anusha-ragunathan commented Dec 22, 2016

Experimental failure is due to Hub issues. windows failure is unrelated.

@anusha-ragunathan anusha-ragunathan merged commit d1dfc1a into moby:master Dec 22, 2016
@tonistiigi tonistiigi mentioned this pull request Dec 27, 2016
Merged
@anusha-ragunathan anusha-ragunathan deleted the refcount branch January 13, 2017 21:52
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
@anusha-ragunathan
Merge pull request moby#29599 from anusha-ragunathan/refcount
4b933cc 
Enforce zero plugin refcount during disable, not remove.
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
@anusha-ragunathan
Merge pull request moby#29599 from anusha-ragunathan/refcount
ae76b43 
Enforce zero plugin refcount during disable, not remove.
@thaJeztah thaJeztah mentioned this pull request Aug 2, 2022
Merged
@thaJeztah thaJeztah mentioned this pull request Jul 2, 2023
Merged
@thaJeztah thaJeztah mentioned this pull request Jul 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees

No one assigned

Labels

area/plugins process/cherry-picked status/2-code-review

Projects

None yet

Milestone

1.13.0

Development

Successfully merging this pull request may close these issues.

6 participants

@anusha-ragunathan @tiborvass @cpuguy83 @tonistiigi @vieux @GordonTheTurtle